pytester: test for _makefile joining an absolute path #6579
Conversation
blueyed
force-pushed
the
pytester-makefile-joins-abspath
branch
from
1953a46 to
0fa4448
Compare
blueyed
force-pushed
the
pytester-makefile-joins-abspath
branch
from
0fa4448 to
c2980eb
Compare
This is a sane method to create a set of files, allowing for absolute paths. Ref: pytest-dev#6578 Ref: pytest-dev#6579
This is a sane method to create a set of files, allowing for absolute paths. Ref: pytest-dev#6578 Ref: pytest-dev#6579
This is a sane method to create a set of files, allowing for absolute paths. Ref: pytest-dev#6578 Ref: pytest-dev#6579
| @@ -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: | |||
There was a problem hiding this comment.
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:
- 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.
- While at it, we might disallow abspaths to
_makefileand raise an error, or warning if we want to be conservative.
@RonnyPfannschmidt can correct me if I'm misinterpreting him.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
Always failing is preferable to surprises, however this needs to be done starting with a warning
There was a problem hiding this comment.
@nicoddemus how to proceed here then? I'd say to just merge it.
There was a problem hiding this comment.
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).
This is a sane method to create a set of files, allowing for absolute paths. Ref: pytest-dev#6578 Ref: pytest-dev#6579
This is a sane method to create a set of files, allowing for absolute paths. Ref: pytest-dev#6578 Ref: pytest-dev#6579
This is a sane method to create a set of files, allowing for absolute paths. Ref: pytest-dev#6578 Ref: pytest-dev#6579
This is a sane method to create a set of files, allowing for absolute paths. Ref: pytest-dev#6578 Ref: pytest-dev#6579
This is a simple, straight-forward method to create a set of files with different extensions, and handling absolute paths (inside of the tmpdir). Ref: pytest-dev#6578 Ref: pytest-dev#6579
This is a simple, straight-forward method to create a set of files with different extensions, and handling absolute paths (inside of the tmpdir). Ref: pytest-dev#6578 Ref: pytest-dev#6579
This is a simple, straight-forward method to create a set of files with different extensions, and handling absolute paths (inside of the tmpdir). Ref: pytest-dev#6578 Ref: pytest-dev#6579
Ref: #6578 (comment)