Pytest: Regression in `testdir.makefile()`, from `pytester` rewrite

[Zac-HD]

def test_regression_demo(testdir):
    testdir.makefile("tests.py", "# python file contents\n")

This works under pytest 6.1.2, but fails under 6.2.x - i.e. following #7854 - with

____________________________ test_regression_demo _____________________________
Traceback (most recent call last):
  File "t.py", line 2, in test_regression_demo
    testdir.makefile("tests.py", "# python file contents/n")
  File "site-packages/_pytest/pytester.py", line 1547, in makefile
    return py.path.local(str(self._pytester.makefile(ext, *args, **kwargs)))
  File "site-packages/_pytest/pytester.py", line 788, in makefile
    return self._makefile(ext, args, kwargs)
  File "site-packages/_pytest/pytester.py", line 756, in _makefile
    p = self.path.joinpath(basename).with_suffix(ext)
  File "lib/pathlib.py", line 889, in with_suffix
    raise ValueError("Invalid suffix %r" % (suffix))
ValueError: Invalid suffix 'tests.py'

I think the new behaviour for pytester is fine, but the compatibility layer seems to need a little work.

[bluetech]

(Changed the title from tmpdir to testdir)

Can you explain what you intended

testdir.makefile("tests.py", "# python file contents\n")

to do? The first argument is an extension/suffix, I'm not sure if you intended it this way, or as the actual file name.

[Zac-HD]

(thanks, my bad 😅)

The first argument is an extension/suffix, I'm not sure if you intended it this way, or as the actual file name.

I honestly don't know what our contributor thought it would do; but in any case my report is simply that it produced a file without error before 6.2 and raises an error afterwards. We've therefore changed to testdir.makepyfile("# python file contents\n") because we don't actually care about the name.

So the distinction or regression is precisely that this argument used to be treated as an arbitrary suffix of the filename, but must now be an extension (presumably a .-prefixed string). It's not like my example code is exactly an intended usage, but if we're keeping the testdir fixture around as a migration pathway we should probably avoid changing the behaviour 😕

[bluetech]

The docs say (also before pytester) for the makefile's first argument:

The extension the file(s) should use, including the dot, e.g. .py.

But seems that py.path is lax about the dot:

>>> p = py.path.local('/foo/bar/baz/file.txt')
>>> p.new(ext='test.py')
local('/foo/bar/baz/file.test.py')
>>> p.new(ext='.test.py')
local('/foo/bar/baz/file.test.py')

while pathlib requires it.

So I think to keep bug-compatibility with py.path in testdir, we can add a . prefix to the argument (if it's not present) before passing it over to pytester.

[RonnyPfannschmidt]

We should warn on the mistake however

im fairly confident that people mistook ext for filename and operate on wrong assumptions

[Zac-HD]

I'm fairly confident that people mistook ext for filename and operate on wrong assumptions

Yep, I think that's exactly what happened here.