Test assertions does not work properly when specifying python_files setting

[nip3o]

The user-friendly error messages when test assertion fails in seems to be broken in certain situations. I have created a minimal example project to illustrate this: https://github.com/nip3o/pytest-assert-rewrite-example

When python_files setting is not set, or when the full path to the test file is specified, assertion rewrite works as expected

> pytest tests/test_addition.py
...
    def test_kaka_b():
>       assert 1 + 2 == 4
E       assert (1 + 2) == 4

tests/test_addition.py:6: AssertionError

When adding a python_files setting to pytest.ini, and not specifying the full path to the test file, assertion rewrite does not work.

> pytest
...
    def test_kaka_b():
>       assert 1 + 2 == 4
E       AssertionError

tests/test_addition.py:6: AssertionError

Tested in multiple projects with pytest 3.0.3 as well as 3.0.5. I have included the full output from test runs with debug mode in the repository above.

[nicoddemus]

Thanks @nip3o for the report and taking the time to produce a reproducible example repository.

[zapaan]

I had the same problem, files matched from a directory do not get their errors rewritten.
The workaround I used is, of course, to just name my files starting with test_ (or just tests.py since i'm using Django) and match that instead, though it could be potentially slower on big projects.

I'm no expert, but I think it has to do with what is matched when searching for files and catching their exceptions.

[pelme]

Thanks to @nip3o example repository I've created a test case that shows this bug in PR #2140.

When AssertionRewritingHook is installed, the config object seems not to be initialized properly. config.getini("python_files") always returns ['test_*.py', '*_test.py', 'testing/*/*.py'] instead of the actual value specified in pytest.ini.

Would it be possible to delay the fetching of python_files during the assertion rewrite or somehow parse the ini files earlier?

@flub Do you have any idea on this? :)

[nicoddemus]

Thanks @pelme! Looking at https://github.com/pytest-dev/pytest/blob/master/_pytest/assertion/rewrite.py#L51, it seems we could just initialize it with None and obtain that value the first time it is requested in _should_rewrite and cache it to reuse afterwards.

I can take a look when I have some time (perhaps this weekend), but feel free to try it yourself in #2140 if you find the time. 🙇

[pelme]

Good suggestion Bruno! :) However, calling self.config.getini('python_files') directly from _should_rewrite also returns the default and not the correct value unfortunately.

[nicoddemus]

Uh oh, then I'm afraid the fix is more complicated than I was expecting, because _should_rewrite is the method which actually needs to know the value of the python_files option... 😞

[pelme]

I did a bit of digging, here is what I found:

• I think self.fnpats is actually correctly set (but not 100% sure since this is tricky for me to debug) and is not the issue. I was initially confused from the pytest test run itself (hello inception)
• This issue was introduced in 6711b1d
• It looks like the line if fnmatch(fn_pypath.basename, pat): is the problem, before the above commit it read if fn_pypath.fnmatch(pat):. There is some slight difference between the py LocalPath.fnmatch and stdlib fnmatch.fnmatch. It is not super trivial just to just use py .fnmatch since it causes import loops (or something like that).

I'll continue to and see if I can work around the issue and use pys fnmatch somehow. :)

[pelme]

I've tracked down the root cause:

fnmatch(fn_pypath.basename, pat) does not give the correct/same result as fn_pypath.fnmatch(pat), this is the main problem that causes this bug.

However, the issue with just doing fn_pypath.fnmatch(pat) is that FNMatcher in py uses py.std which triggers an import. Since this code is called from the rewrite hook, this import have the nasty side effect of triggering endless recursion.

Here is the relevant line in py:
https://github.com/pytest-dev/py/blob/9db8a57a8beb7af86c88146b424896956928ed7c/py/_path/common.py#L439

Here is a PR to py to avoid using py.std:
pytest-dev/py#99

With that PR, we can just use .fnmatch() and this bug will be gone. ✨