pytest-dev / pytest

The pytest framework makes it easy to write small tests, yet scales to support complex functional testing
https://pytest.org
MIT License
11.93k stars 2.65k forks source link

Regression in 7.1 ? Windows CI started to fail with "assert mod not in mods" since 7.1 #9765

Closed Carreau closed 8 months ago

Carreau commented 2 years ago

Now sorry I know this is not a minimal reproducer, but I just want to log this here, and will investigate later:

You can see one Failure here

ERROR collecting test session 
C:\hostedtoolcache\windows\Python\3.9.10\x64\lib\site-packages\_pytest\runner.py:338: in from_call
    result: Optional[TResult] = func()
C:\hostedtoolcache\windows\Python\3.9.10\x64\lib\site-packages\_pytest\runner.py:369: in <lambda>
    call = CallInfo.from_call(lambda: list(collector.collect()), "collect")
C:\hostedtoolcache\windows\Python\3.9.10\x64\lib\site-packages\_pytest\main.py:7[23](https://github.com/ipython/ipython/runs/5535021197?check_suite_focus=true#step:8:23): in collect
    for x in self._collectfile(pkginit):
C:\hostedtoolcache\windows\Python\3.9.10\x64\lib\site-packages\_pytest\main.py:575: in _collectfile
    ihook = self.gethookproxy(fspath)
C:\hostedtoolcache\windows\Python\3.9.10\x64\lib\site-packages\_pytest\main.py:539: in gethookproxy
    my_conftestmodules = pm._getconftestmodules(
C:\hostedtoolcache\windows\Python\3.9.10\x64\lib\site-packages\_pytest\config\__init__.py:579: in _getconftestmodules
    mod = self._importconftest(conftestpath, importmode, rootpath)
C:\hostedtoolcache\windows\Python\3.9.10\x64\lib\site-packages\_pytest\config\__init__.py:6[24](https://github.com/ipython/ipython/runs/5535021197?check_suite_focus=true#step:8:24): in _importconftest
    assert mod not in mods
E   AssertionError

Now I'm not sure what this does, but my quick understanding is that it checks whether conftest.py itself is not in the list of collected modules ? We might definitely do something wrong on IPython side, but it's strange as other non-windows CI are passing and regardless of whether this is something I did wrong, can the assert get an explanation messages as second argument ?

Downgrading to <7.1 fixes the issue.

Again I'll try to get a MVP/small example, but it may take me some time, and I guessed maybe one of you will immediately know why this is happening.

jgb commented 9 months ago

The exception tells what to do, the debugger you use is not pytest aware so you need to disable io capture

@RonnyPfannschmidt @bluetech

ok I did that (pytest -s) so I'm in the debugger, what would you like me to do there? :)

bluetech commented 9 months ago

Do bt (backtrace). You can travel up and down the stackframes, print the local variables and list the code at each point. In this case I wanted to see what imports the conftest for the first time.

jgb commented 9 months ago

@bluetech so when I'm on the first level with the debugger, I see these local variables:

{'__name__': 'vidicenter.conftest', '__doc__': None, '__package__': 'vidicenter', '__loader__': <_pytest.assertion.rewrite.AssertionRewritingHook object at 0x7f637ff23690>, '__spec__': ModuleSpec(name='vidicenter.conftest', loader=<_pytest.assertion.rewrite.AssertionRewritingHook object at 0x7f637ff23690>, origin='/srv/vidicenter/../vidicenter/conftest.py'), '__file__': '/srv/vidicenter/../vidicenter/conftest.py', '__cached__': '/srv/vidicenter/../vidicenter/__pycache__/conftest.cpython-311.pyc',

When I travel up further and further all the way to the original frame, I don't get any wiser, I see code from either pytest or pluggy.

pllim commented 9 months ago

Hello. I started to see this error in pytest 8.0rc . Example log: https://github.com/spacetelescope/jdaviz/actions/runs/7383949691/job/20085994919

Was not a problem using pytest 7.4.3 . Example log: https://github.com/spacetelescope/jdaviz/actions/runs/7302736038/job/19901924971

lesteve commented 9 months ago

@pllim could you show the value of str(conftestpath) and mod.__file__ as mentioned in https://github.com/pytest-dev/pytest/issues/9765#issuecomment-1860082090? This would maybe allow to see if this is a similar issue as the ones that have already been listed, path case normalization i.e. os.path.normcase (issue on Windows), path normalization i.e. os.path.normpath (issue on Linux), or something different.

Wild-guess: it looks like yet another variant of the issue since this does not happen with pytest 7.4.3 ...

pllim commented 9 months ago

@lesteve , I am not able to get this error locally so I will have to do something that Actions can run and display. Do you mean I write a new test to print out just the values you asked for? Do you have a working example? Thanks!

lesteve commented 9 months ago

@pllim is it easily doable in your CI to install pytest from my branch:

pip install git+https://github.com/lesteve/pytest@debug-pytest-issue-9765

and post the output?

I added a quick and dirty commit that should add a bit more information to troubleshoot.

pllim commented 8 months ago

@lesteve , any chance you can apply your patch to the pytest 8.0.x series instead of pytest 8.1.x ? I am trying over at https://github.com/spacetelescope/jdaviz/pull/2654 but I am running into the problem of https://github.com/pytest-dev/pytest/issues/11779#issuecomment-1879387034 that is a completely separate can of worms.

bluetech commented 8 months ago

While we still don't have a proper reproduction for all cases, I believe we have a pretty good grasp of the issue and how to fix it (see https://github.com/pytest-dev/pytest/issues/9765#issuecomment-1869103766). Since @fcharras presumably hasn't had time to update PR #11708, I'll try to update it myself in the next few days so that we can backport it to pytest 8.0 release.

h-vetinari commented 8 months ago

We're hitting this in scipy with 8.0.0rc1.

We are not using --no-use-pep517 ~and neither --no-build-isolation~ (edit, the PR where this fails added --no-isolation); we are calling pytest programmatically using pytest.main(args), in case that plays a role.

It's also worth noting that the failure is specific to python 3.9 & 3.10, i.e. it does not appear for 3.11 & 3.12.

tylerjereddy commented 8 months ago

@h-vetinari no, that's not right, the CI job that disables isolation is passing

tylerjereddy commented 8 months ago

I believe the wheels builds do disable build isolation though, because we pull in NumPy pre-release in anticipation of NumPy 2.0.0. Just not that specific CI job that uses Python 3.11 in our regular CI.

lesteve commented 8 months ago

@pllim can you try with (cherry-picked the commit but using the 0.8.x branch):

pip install git+https://github.com/lesteve/pytest@debug-pytest-8.0-issue-9765

and post the output?

I guess @tylerjereddy you can do this as well?

lesteve commented 8 months ago

While we still don't have a proper reproduction for all cases, I believe we have a pretty good grasp of the issue and how to fix it (see #9765 (comment)).

I agree with this, thanks for looking into it and your very clear summary! I realised only now there was a question at the end of your summary post :sweat_smile:. As far as I am concerned, 1. seems like the first straightforward thing to do. Without being super familiar with the pytest code, it is a bit hard to guess if 3. needs to be done but I feel this can be done in a second step.

Since @fcharras presumably hasn't had time to update PR #11708, I'll try to update it myself in the next few days so that we can backport it to pytest 8.0 release.

With @fcharras we'll try to work on this PR on Monday. I recently have worked on turning my reproducer into a pytest test so I think we should be close to having something mergeable.

pllim commented 8 months ago

@lesteve , thanks for the updated branch! I got a log but I don't understand how this error is related to "assert mod not in mods". I have not seen this error before as far as pytest 8.0rc is concerned.

https://github.com/spacetelescope/jdaviz/actions/runs/7439536734/job/20239554633?pr=2654

lesteve commented 8 months ago

@pllim not sure what is going on, ideally you would cherry-pick my simple debug commit on top of whichever version you were seeing the assert mod not in mods error before.

lesteve commented 8 months ago

@tylerjereddy looking closely at the Scipy CI logs, it could potentially be fixed by #11708, this is a os.path.normcase issue.

conftestpath has a capital L Lib, and mod.file has a small l lib. No idea why this would happen, and even less idea why only on Python 3.9 and Python 3.10 ...

Here is one relevant excerpt from your Python 3.10 log. You need to look at conftestpath vs mod.

  E   AssertionError
          conftestpath = WindowsPath('C:/Users/runneradmin/AppData/Local/Temp/cibw-run-flppgo17/cp310-win_amd64/venv-test/Lib/site-packages/scipy/conftest.py')
          dirpath    = WindowsPath('C:/Users/runneradmin/AppData/Local/Temp/cibw-run-flppgo17/cp310-win_amd64/venv-test/Lib/site-packages/scipy')
          existing   = None
          importmode = 'prepend'
          mod        = <module 'scipy.conftest' from 'C:\\Users\\runneradmin\\AppData\\Local\\Temp\\cibw-run-flppgo17\\cp310-win_amd64\\venv-test\\lib\\site-packages\\scipy\\conftest.py'>
          mods       = [<module 'scipy.conftest' from 'C:\\Users\\runneradmin\\AppData\\Local\\Temp\\cibw-run-flppgo17\\cp310-win_amd64\\venv-test\\lib\\site-packages\\scipy\\conftest.py'>]
          path       = WindowsPath('C:/Users/runneradmin/AppData/Local/Temp/cibw-run-flppgo17/cp310-win_amd64/venv-test/Lib/site-packages/scipy')
          pkgpath    = WindowsPath('C:/Users/runneradmin/AppData/Local/Temp/cibw-run-flppgo17/cp310-win_amd64/venv-test/Lib/site-packages/scipy')
          rootpath   = WindowsPath('C:/Users/runneradmin/AppData/Local/Temp/cibw-run-flppgo17/cp310-win_amd64/test_cwd')
          self       = <_pytest.config.PytestPluginManager object at 0x0000023B663B3D00>
pllim commented 8 months ago

@lesteve , I ended up forking 8.0rc1 on my fork and then cherry-pick your commit directly on top of that. I think this is the log you are requesting? Does this mean anything to you? Thanks! https://github.com/spacetelescope/jdaviz/actions/runs/7450389588/job/20269180187?pr=2654

lesteve commented 8 months ago

@pllim this seems very similar to the Scipy one, os.path.normcase issue, capital L Lib (in mod.__file__) vs small l lib (in conftestpath):

E   RuntimeError: mod=<module 'jdaviz.conftest' from 'D:\\a\\jdaviz\\jdaviz\\.tox\\py310-test-predeps\\lib\\site-packages\\jdaviz\\conftest.py'> has already been loaded
E   
E   str(conftestpath)='D:\\a\\jdaviz\\jdaviz\\.tox\\py310-test-predeps\\Lib\\site-packages\\jdaviz\\conftest.py'

I think this could be fixed by https://github.com/pytest-dev/pytest/pull/11708. Hopefully we manage to push the PR forward tomorrow.

pllim commented 8 months ago

Thanks, @lesteve ! I will be happy to test that PR when it is ready.

lesteve commented 8 months ago

@pllim and maybe @h-vetinari or @tylerjereddy, can you try installing the code from #11708 and see whether that fixes your issue :crossed_fingers:?

pip install git+https://github.com/fcharras/pytest@FIX/crash_during_conftest_collection
pllim commented 8 months ago

I'll give it a go but if I run into other problems I am seeing with 8.1.dev (your current main), I can try to again cherry-pick that onto the 8.0rc1. 🤞

Update: Nope, definitely cannot use that branch as-is but I cherry-picked the fix to git+https://github.com/pllim/pytest.git@debug-8.0rc1

pllim commented 8 months ago

@lesteve , I applied the patch from fcharras to 8.0rc1 directly at https://github.com/pllim/pytest/tree/debug-8.0rc1 (https://github.com/pllim/pytest/commit/22949f2de7e7cb689f2dada5e0c536ad60e60e3d). It went further this time but errored out because it cannot find test fixtures. We have a root level root/conftest.py and a root/package/conftest.py. The first one is so that tox picks up the pytest header stuff. The second one is where all the fixtures are and we need it at the package level so we can run package.test() command for any installed package (this uses the astropy test runner machinery).

Log: https://github.com/spacetelescope/jdaviz/actions/runs/7465954413/job/20316247352?pr=2654

For completeness, I can confirm that this problem only still happens on Windows. OSX passed and Linux failed with some transient remote data access problem that is unrelated to pytest itself.

lesteve commented 8 months ago

OK, so at least you avoid the assert mod not in mods and it goes a bit further, so I would count that as a win for the PR :wink: !

Your debug branch looks fine to me. Right now, I don't really understand how such a change could cause some fixtures to not be registered.

It could potentially be another pytest 8.0rc1 regression, hard to tell since I am by no means a pytest expert ...

pllim commented 8 months ago

Yes, I agree that the "assert mod not in mods" problem is solved in https://github.com/pytest-dev/pytest/pull/11708

pllim commented 8 months ago

I also just found out that this "mod not in mods" problem on Windows is not a problem at all if I switch from Python 3.10 to Python 3.11 🤪 🤷‍♀️

woutdenolf commented 8 months ago

For reference, in case this needs to be reproduced (Windows 11, python 3.8.10):

python -m pip install "ewokscore[test]==0.7.1" "pytest==8.0.0rc1"
python -m pytest -v --pyargs ewokscore.tests

This works fine (well there is an issue with the papermill dependency but that's not related to pytest)

python -m pip install "ewokscore[test]==0.7.1" "pytest==7.4.4"
python -m pytest -v --pyargs ewokscore.tests
lesteve commented 8 months ago

@woutdenolf this is similar to scipy and jdaviz see https://github.com/pytest-dev/pytest/issues/9765#issuecomment-1880768441, lib (small l) vs Lib (capital L).

I think this is fixed in main (since Saturday January 13 ~9PM Grenoble time) and in has been back-ported in the 8.0.x branch.

I double-checked and I don't get the assert mod not in mods on Windows and Python 3.8, when doing this:

python -m pip install "ewokscore[test]==0.7.1" git+https://github.com/pytest-dev/pytest.git@main
python -m pytest -v --pyargs ewokscore.tests
bluetech commented 8 months ago

For reference, the simple "Solution 3" normalize-as-early-as-possible approach (implemented here) does not work -- the problem is that, while Foo/Mod.py and foo/mod.py are equivalent paths in a case-insensitive filesystem, Foo.Mod and foo.mod are not equivalent Python module names.

woutdenolf commented 8 months ago

For reference, the simple "Solution 3" normalize-as-early-as-possible approach (implemented here) does not work -- the problem is that, while Foo/Mod.py and foo/mod.py are equivalent paths in a case-insensitive filesystem, Foo.Mod and foo.mod are not equivalent Python module names.

The link is broken. What do you mean by "normalize"? Normalize what and how?

I don't see the problem. You cannot import Foo/Mod.py by doing import foo.mod. Could you create an example?

I'm asking because I made a proposal that normalizes anymodule.__file__ but preserves symlinks: https://github.com/pytest-dev/pytest/pull/11821. The tests pass so if there is a problem with this, it is not covered by the tests.

lesteve commented 8 months ago

The correct link for the commit is https://github.com/pytest-dev/pytest/commit/203889b2ba67af85e3c3adff5fda2042c5da7a97