pytester: testdir: add makefiles helper

[blueyed]

This is a sane method to create a set of files, allowing for absolute
paths.

TODO:

• changelog
• doc: mention that absolute paths work?

Ref: #6578
Ref: #6579

[RonnyPfannschmidt]

Why should this be allowed to accidentially allow writes outside?

From My pov this should fail on escape unless explicitly requested,

addressed

[nicoddemus]

(Did not notice this PR when replying to #6579).

I'm also interested in the use case to allow writing to absolute paths... I mean the user can just as easily write the files themselves (just missing the automatic dedent of course).

[blueyed]

I'm also interested in the use case to allow writing to absolute paths... I mean the user can just as easily write the files themselves (just missing the automatic dedent of course).

This could look much nicer with it:

pytest/testing/python/fixtures.py

Lines 3628 to 3654 in 712ac26

	def test_non_relative_path(self, testdir):
	tests_dir = testdir.mkdir("tests")
	fixdir = testdir.mkdir("fixtures")
	fixfile = fixdir.join("fix.py")
	fixfile.write(
	textwrap.dedent(
	"""\
	import pytest
	@pytest.fixture(params=[0, 1, 2])
	def fix_with_param(request):
	return request.param
	"""
	)
	)
	testfile = tests_dir.join("test_foos.py")
	testfile.write(
	textwrap.dedent(
	"""\
	from fix import fix_with_param
	def test_foo(request):
	request.getfixturevalue('fix_with_param')
	"""
	)
	)

The helper however is also about getting sane behavior in general, i.e. with regard to extensions and the return value.

[nicoddemus]

The helper however is also about getting sane behavior in general, i.e. with regard to extensions and the return value.

Sorry if I'm missing something, but you would still be writing to files inside the tmpdir, no?

[blueyed]

Sorry if I'm missing something, but you would still be writing to files inside the tmpdir, no?

What do you mean?

The example above is writing to files in tmpdir, yes, but would pass absolute paths (inside of tmpdir), which currently get concatenated with makepyfile etc.

[RonnyPfannschmidt]

As proposed and explained the implementation is a no go

There are ways to do this that don't add a safety off footgun

[blueyed]

@RonnyPfannschmidt
This only creates arbitrary files when enabled via kwarg.. and otherwise just does allow for using abspaths below tmpdir. What's the "safety off footgun" there?

[RonnyPfannschmidt]

i missed the force push on my mobile earlier today

[RonnyPfannschmidt]

this still doesn't answer however why to do the dance for allowing it, what's the use-case

[nicoddemus]

Sorry if I'm missing something, but you would still be writing to files inside the tmpdir, no?

What do you mean?

The example above is writing to files in tmpdir, yes, but would pass absolute paths (inside of tmpdir), which currently get concatenated with makepyfile etc.

You got my point correctly: you would still be writing files inside the tmpdir, but using absolute paths, and I fail to see why that would be beneficial. TBH I can't think of a good use case either.

In the lack of a good use case, this only seems to add a feature that nobody should ever use, I believe this is also @RonnyPfannschmidt's concerns.

[blueyed]

Ok, might remove the allow_outside_tmpdir flag then, although I can imagine it being useful, and cannot see why it should be disallowed, if you can do it anyway manually.
(the check for it being below tmpdir would be there still anyway)
Feel free to close it if you do not want it altogether.

[nicoddemus]

Ok, might remove the allow_outside_tmpdir flag then

I don't think the problem is a detail of implementation, but the feature in general.

although I can imagine it being useful

Can you provide one or more use cases where this feature would be beneficial? The only one you provided would still write inside tmpdir, and you didn't show how it would be improved in anyway by using absolute paths.

and cannot see why it should be disallowed, if you can do it anyway manually.

As I see it, adding an explicit option to it or even to allow users to do it is to encourage a bad practice: users might be end up writing outside testdir by mistake. The testdir fixture is intended to create tests files and execute them in a controlled, temporary dir.

Recently we even added a check against this type of usage in mktemp for the same reasons.

[blueyed]

@nicoddemus the main problem is that makepyfile etc join an given absolute path, therefore not allowing "/tmpdir/foo", but using "/tmpdir/tmpdir/foo" then (and failing on Windows).
That's the initial motivation: just allow for passing in an absolute tmpdir path.
Sure, you can make it relative before etc, but also having only helpers that force (single) extensions etc is not very sane.

Can you provide one or more use cases where this feature would be beneficial?

E.g. changing "/etc/hosts" with dockerized/sandboxed tests?
But I agree that this is not really part of the testdir fixture

users might be end up writing outside testdir by mistake.

Yeah, I can remove the flag (as said already). But it is not really "by mistake" if you explicitly enable it, is it?

The only one you provided would still write inside tmpdir, and you didn't show how it would be improved in anyway by using absolute paths.

It should not be necessary to make them relative before however, no?

This could also be addressed with the existing makefile, and allowing for no default extension there (have not checked if that would work already), but then it is also a behavior change (although failing on Windows anyway).
I've just figured back then that a new method could be used to make this "more sane".

[nicoddemus]

That's the initial motivation: just allow for passing in an absolute tmpdir path.

Sorry, unless there's a good use case, I don't see this as a good thing. testdir is a "temporary test directory for running isolated tests", it is not its job to be an utility to create files anywhere in the file system IMO.

E.g. changing "/etc/hosts" with dockerized/sandboxed tests?

This is a very convoluted example IMO, the user should be mocking this somehow and passing a temporary etc/hosts, not messing with the file's container. Also this can be easily accomplished with Path("/etc/hosts").write_text(dedent(...)).

So this is a bad example IMO, because it is a bad written test. Users should be able to do it? Sure, it is their code, but that's not testdir's place to explicitly add an API for that.

A bad practice should not be encouraged by the API, on the contrary.

But I agree that this is not really part of the testdir fixture

Good, on that we agree. 😉

The only one you provided would still write inside tmpdir, and you didn't show how it would be improved in anyway by using absolute paths.
It should not be necessary to make them relative before however, no?

They are relative already and it would be more work to make them absolute and still safe. Can you rewrite the test and post it here (it doesn't even need to pass) to showcase what you see as an improvement?

In summary, I don't like this idea because:

• No real good use cases.
• Encourages a bad practice: using a relative path when writing files with testdir is safer and what you should be doing.
• Outside of testdir's fixture job, which is writing files into a secure location for testing.
• Goes into the opposite direction of tmpdir.mkdir, which explicitly forbids absolute paths.

I'm curious though if I'm missing a good case for this or if I am thinking too hard about it, so would like to hear other's opinions. @bluetech @asottile @The-Compiler, what's your take on this?

[asottile]

I agree with @nicoddemus on all fronts, well stated

[blueyed]

That's the initial motivation: just allow for passing in an absolute tmpdir path.

Sorry, unless there's a good use case, I don't see this as a good thing. testdir is a "temporary test directory for running isolated tests", it is not its job to be an utility to create files anywhere in the file system IMO.

?? "tmpdir" is not "anywhere in the file system".

Do you see that testdir.makepyfile(**{fixfile.relto(testdir.tmpdir): ""}) is necessary currently, and that testdir.makepyfile(**{fixfile: ""}) would be easier?

E.g. changing "/etc/hosts" with dockerized/sandboxed tests?

This is a very convoluted example IMO, the user should be mocking this somehow and passing a temporary etc/hosts, not messing with the file's container. Also this can be easily accomplished with Path("/etc/hosts").write_text(dedent(...)).

Yeah. Since it can be done anyway, why not allow it via a flag then?
Imagine you have a set of files, and then for some of them have to use a "manual" method.
Anyway, I've accepted long ago that the flag is not wanted here.

The only one you provided would still write inside tmpdir, and you didn't show how it would be improved in anyway by using absolute paths.
It should not be necessary to make them relative before however, no?

They are relative already and it would be more work to make them absolute and still safe.

They are not relative. Passing in testdir.tmpdir.join("foo") is not the same as passing in "foo" to makefile.

Can you rewrite the test and post it here (it doesn't even need to pass) to showcase what you see as an improvement?

Currently the test needs to look like this to be "improved":

        tests_dir = testdir.mkdir("tests")
        fixdir = testdir.mkdir("fixtures")
        fixfile = fixdir.join("fix.py")
        testfile = tests_dir.join("test_foos.py")
        testdir.makepyfile(
            **{
                fixfile.relto(
                    testdir.tmpdir
                ): """
                …
                """,
                testfile.relto(
                    testdir.tmpdir
                ): """
                …
                """,
            }
        )

(Note that this only works because .new(ext=".py") does not append .py again.)

With makefiles:

        testdir.makefiles(
            {
                fixfile: """
                …
                """,
                testfile: """
                …
                """,
            }
        )

In summary, I don't like this idea because:

• No real good use cases.

See above.

• Encourages a bad practice: using a relative path when writing files with testdir is safer and what you should be doing.

But having to make a path below "tmpdir" explicitly relative should not be necessary, if it is an absolute path below it.

[blueyed]

The real problem is with p = self.tmpdir.join(basename).new(ext=ext) in _makefile:

1. it joins basename if absolute, even if below tmpdir (changing that behavior was rejected)
2. it forces an "ext" always, so you cannot create a set of different files in one go, and not even _makefile(ext="", **{"foo.py": "", "text.txt": ""}) would work (creating just "foo" and "text").

[nicoddemus]

But having to make a path below "tmpdir" explicitly relative should not be necessary, if it is an absolute path below it.

This I'm not against: ensure all paths given (relative or absolute) are below tmpdir. 👍

The confusing part about this conversation (to me at least) that you kept saying you wanted to pass absolute paths, implying that you wanted to write files outside tmpdir, even gave an example in that direction with "/etc/hosts" use case.

So you can see why I was against the proposal: all lead me to believe that you wanted to write outside the tmpdir, even your example.

[blueyed]

@asottile

I agree with @nicoddemus on all fronts

JFI: this is not a war.

well stated

You might have wanted to elaborate, since it turned out @nicoddemus has missed the point several times, and got deadlocked on the "this can allow for arbitrary files outside tmpdir" (which was something I agreed on to remove already several times).
And apparently you just chimed in on it without getting a grasp of the issue itself then.

[blueyed]

Just for reference: I will likely take this into my fork then - keeping allow_outside_tmpdir just because...

Since apart from all that I still think that testdir should have a better function "to just create files", which this was all about in the first place.

[blueyed]

@nicoddemus
you might want to reply to my comments inline also - I've repeately made clear that allow_outside_tmpdir could be removed, and only have given examples for its use on request.

You might all might want to read up from the beginning before replying.

But since I've closed this (also against out of frustration), you can also just ignore my comments (or rather just don't reply).

[nicoddemus]

You might all might want to read up from the beginning before replying.

@blueyed, I did follow the thread from the beginning.

The absolute first question raised was from @RonnyPfannschmidt:

Why should this be allowed to accidentially allow writes outside?

When I asked for a use case, then you wrote:

This could look much nicer with it:

And went on showing an example which did not use absolute paths at all, nor was obvious why using absolute paths would make it better.

Later on I said:

you would still be writing files inside the tmpdir, but using absolute paths, and I fail to see why that would be beneficial.

Instead of providing an example why that would be beneficial, you went on again about writing outside tmpdir, even mentioning this doesn't look like a problem:

although I can imagine it being useful, and cannot see why it should be disallowed

And when asked for an example, you again mentioned writing outside tmpdir:

E.g. changing "/etc/hosts" with dockerized/sandboxed tests?

My main concern through the entire thread was not about "absolute paths" per-se, but writing outside the tmpdir. Instead of adressing that explicitly and with examples, you kept side tracking with use cases and rationale that "it is possible to write outside tmpdir already, why shouldn't we allow it?".

When you finally explicitly showed the use case you had in mind (in #6603 (comment)), then things got clear: it was never about writing outside tmpdir, but to use absolute paths created by the other testdir methods. But your patch allowed to write outside the tmpdir, which is the whole point of content in the discussion.

@blueyed if I was the only one on this thread "not reading it carefully" as you imply and the only one who kept getting it "wrong", I would definitely be apologizing.

But you really should take a step back and realize that other 2 people also misunderstood what you were saying, so perhaps the problem is not me, but how you present your ideas?

And finally, when things were clear that allowing passing absolute paths was fine if we didn't allow absolute paths outside tmpdir (#6603 (comment)), you went on to close the issue and port it to your "fork".

Spending all the time and effort in this discussion, to see it going unresolved even after we have reached a suitable conclusion, is very frunstrating, insatisfying, and frankly draining.

[blueyed]

@nicoddemus

The absolute first question raised was from @RonnyPfannschmidt:

Why should this be allowed to accidentially allow writes outside?

Which was legitimate, and addressed right away (#6603 (comment)).

When I asked for a use case, then you wrote:

This could look much nicer with it:

And went on showing an example which did not use absolute paths at all, nor
was obvious why using absolute paths would make it better.

That's actually wrong.. it uses absolute paths. And that is the point you are missing..!
That was the beginning, and you even came from #6579 (which added a test for this with the existing helpers, which this PR was about to
address.)

Therefore I think the rest of your comment is based on having missed the point from there on already. I will reply to the rest via email.

[nicoddemus]

Never got that email, btw.