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.

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

py.test on pytest fails when not limiting to the testing folder #3245

Open
RonnyPfannschmidt opened this Issue Feb 21, 2018 · 10 comments

Comments

Projects
None yet
3 participants
@RonnyPfannschmidt
Member

RonnyPfannschmidt commented Feb 21, 2018

it seems our tests fail if we include the documentation folder in a test run

precisely - it makes about 7 tests fail due to path issues

@jeffreyrack

This comment has been minimized.

Member

jeffreyrack commented Mar 3, 2018

I just looked into this a bit, and the test failure is occuring on tests that use tempdirs named "sub1" and/or "sub2".

The root issue appears to be from doc/en/example/costlysetup.
A quicker way to reproduce the same test failures in metafunc.py for example would be to run:
pytest doc/en/example/costlysetup/sub1/test_quick.py testing/python/metafunc.py

For some reason, this will then cause all test functions that then go on to call testdir.mkpydir("sub1") to raise an ImportError: No module named conftest if that testdir is used in testdir.runpytest.

I tried poking around in _pytest/config.py to find out why this exception was being raised, but couldn't figure out why it's only raised after the costlysetup test is run.

@RonnyPfannschmidt

This comment has been minimized.

Member

RonnyPfannschmidt commented Mar 3, 2018

@jeffreyrack great job with the investigation, basically costlysetup gets added to pythonpath, and later tests are unable to find the module in any sane way since theirs looks the same, if there is also conftests around, then this also mismatches

this is in part a shortcoming of pytester and in part of our own setup layout

@jeffreyrack

This comment has been minimized.

Member

jeffreyrack commented Mar 4, 2018

Interestingly enough, the issue seems to be that py.path is trying to import sub1.conftest, which it's unable to find in the path.

For example, in this test that currently fails in metafunc.py, if I add logic to remove init.py, then it will pass:

    def test_generate_tests_only_done_in_subdir(self, testdir):
        sub1 = testdir.mkpydir("sub1")
        sub2 = testdir.mkpydir("sub2")
        sub1.join("conftest.py").write(_pytest._code.Source("""
            def pytest_generate_tests(metafunc):
                assert metafunc.function.__name__ == "test_1"
        """))
+        sub1.join('__init__.py').remove()
        sub2.join("conftest.py").write(_pytest._code.Source("""
            def pytest_generate_tests(metafunc):
                assert metafunc.function.__name__ == "test_2"
        """))
+        sub2.join('__init__.py').remove()
        sub1.join("test_in_sub1.py").write("def test_1(): pass")
        sub2.join("test_in_sub2.py").write("def test_2(): pass")
        result = testdir.runpytest("--keep-duplicates", "-v", "-s", sub1, sub2, sub1)
        result.assert_outcomes(passed=3)

The reason is that after costlysetup is run, py.path will try to do: __import__('sub1.conftest').
By removing init.py, it will instead just try to do __import__('conftest').

It's still not clear to me why costlysetup messes with this at all, as I don't see anything in costlysetup that messes with the path.

@RonnyPfannschmidt

This comment has been minimized.

Member

RonnyPfannschmidt commented Mar 4, 2018

the main issue is python path overlaps and sys.modules, to me its not clear what other sideeffects the patch you posted wil lhave

@jeffreyrack

This comment has been minimized.

Member

jeffreyrack commented Mar 5, 2018

The issue is that when pytest initially loads the tests, it imports the doc/en/example/costlysetup/sub1 module as sub1.

Then, when the tests later on get ran that also create a sub1 module, import sub-modules fails because the import (in this case above, it ends up calling __import__('sub1.conftest')) fails because it's searching the costlysetup sub1 module (Although we're expecting it to use our temp sub1 directory that we added to the path).

Given the above, the reason that removing __init__.py works is because then pyimport won't perceive the conftest.py file as being part of a python package. So, instead we essentially do __import__('conftest'), which works because we end up finding our conftest.py from the temp directory first in the path.
That said, I see what you're saying about potential side-effects from this patch.

I think the best way to solve this would be to fix the directory layout issue, and have the costlysetup example use sub-directory names other than sub1 and sub2 as those package names are assumed to be unused by our automated tests.

Maybe renaming them to sub_a and sub_b?
By renaming the subdirectories in the example, when the automated tests try to import conftest.py from sub1, they will correctly find sub1 within the path, rather than using the stored sub1 in sys.modules.

@RonnyPfannschmidt

This comment has been minimized.

Member

RonnyPfannschmidt commented Mar 5, 2018

i believe in order to keep things safe in general we should have unique folders for all automated tests in some way

@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Mar 5, 2018

it seems our tests fail if we include the documentation folder in a test run

I believe this happened by error rather than someone wanting to run tests in the docs folder. If this is the case, I think we can just add docs to be ignored by default by adding --ignore=docs to our pytest.ini.

@RonnyPfannschmidt

This comment has been minimized.

Member

RonnyPfannschmidt commented Mar 6, 2018

i#d prefer if we includes the documentation folder and actually tested it and the examples there

@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Mar 6, 2018

Sure, makes sense.

@nicoddemus

This comment has been minimized.

Member

nicoddemus commented Mar 6, 2018

#3283 added doc to norecursedirs, but the underlying issue still remains.

@RonnyPfannschmidt RonnyPfannschmidt changed the title from py.test on pytest fails when nor limiting to the testing folder to py.test on pytest fails when not limiting to the testing folder Mar 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment