Skip to content

pytester: test for _makefile joining an absolute path #6579

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 28, 2020
Merged

pytester: test for _makefile joining an absolute path #6579

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions testing/test_pytester.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -710,3 +710,13 @@ def test_error2(bad_fixture):
result.assert_outcomes(error=2)

assert result.parseoutcomes() == {"error": 2}


def test_makefile_joins_absolute_path(testdir: Testdir) -> None:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From #6578 (comment) I gather that we should not support absolute paths in _makefile, and I agree at first, I don't see a good use case to allow that and might open up for unwanted writes to system wide files.

I see two options here:

  1. Merge as is and document that this just tests the current behavior, but we want to change it in the future and point to here.
  2. While at it, we might disallow abspaths to _makefile and raise an error, or warning if we want to be conservative.

@RonnyPfannschmidt can correct me if I'm misinterpreting him.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nicoddemus
This is only a test for the actual behavior, yes.
See #6603 for a saner helper that also allows for this.

Merge as is and document that this just tests the current behavior, but we want to change it in the future and point to here.

How should it be documented? Since it is internal that would be with the test's docstring then? Note that the commit has a reference already.

While at it, we might disallow abspaths to _makefile and raise an error, or warning if we want to be conservative.

Yeah, somebody could do it on top of this, but if nobody wants to do it now I think it is better to test the given behavior.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Always failing is preferable to surprises, however this needs to be done starting with a warning

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nicoddemus how to proceed here then? I'd say to just merge it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How should it be documented? Since it is internal that would be with the test's docstring then?

Definitely, sorry I wasn't clear.

Note that the commit has a reference already.

That's good, but I prefer to also include it into the docstring: we often move code around so it is harder to find stuff with blame.

But OK, let's merge it now and decide on the warning/error/etc on the other PR (I don't think we will lose sight of this and this comes back to bite us later).

absfile = testdir.tmpdir / "absfile"
if sys.platform == "win32":
with pytest.raises(OSError):
testdir.makepyfile(**{str(absfile): ""})
else:
p1 = testdir.makepyfile(**{str(absfile): ""})
assert str(p1) == (testdir.tmpdir / absfile) + ".py"