-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
[regression][8.2.2] KeyError
sometimes crashes test collection on PyPy while reordering fixtures
#13312
Comments
This patch temporarily restricts pytest version below 8.2.2 under PyPy due to a discovered regression that it introduced [[1]]. The regression has been observed on at least `pypy3.9-7.3.16`, `pypy3.10-7.3.19` and `pypy3.11-7.3.19`. It can be triggered by running the following in affected runtimes: pytest --collect-only --no-cov tests/test_abc.py tests/test_copy.py tests/test_incorrect_args.py tests/test_multidict.py tests/test_mypy.py tests/test_pickle.py tests/test_types.py tests/test_update.py tests/test_version.py [1]: pytest-dev/pytest#13312 [2]: pytest-dev/pytest#12414 [3]: pytest-dev/pytest#12409
KeyError
sometimes crashes test collection on PyPy 3.9 while reordering fixturesKeyError
sometimes crashes test collection on PyPy while reordering fixtures
i believe we may need to report this to pypy as well |
Oh, I forgot to notify PyPy 🤦♂️. Thanks for tagging them! |
i suspect the key differerenve is somewhere in https://github.com/pypy/pypy/blob/branches/release-pypy3.10-v7.x/pypy/module/_collections/app_odict.py |
Unfortunately, debugging RPython is above my pay grade. It would be interesting to find out when duplicate keys get introduced, but at least at a first glance, nothing stands out. Most of the code is calling into |
it may be possible the exact behavior is dependent on the dictionary strategy in pypy |
The above was the minimum I could come up with. At some point, deselecting one more test was making it work. So I gave up for the time being... I thought that maybe it's PyPy's GC mechanism that influences this again. But I also don't know how to troubleshooting that. |
My guess would be that the key to reproducing it would be figuring out how did it manage to end up with a duplicate key. |
a fine hack to validate would be if using a dict plus |
Yep, that's exactly what I can't wrap my head around. And staring at the PyPy source code has given me exactly zero clarity 🤷♂️ |
So @bdraco was adding new tests in aio-libs/multidict#1072. And it worked at first. But they had readability problems, and I added a few refactoring commits on top. And that's when things went south.
The first version of the tests made use of
@pytest.mark.parametrize
which I later converted into a parametrized fixture. That specific fixture depends onrequest: pytest.FixtureRequest
, but doesn't depend on anything else we have declared inconftest.py
.The params set uses a trick that @The-Compiler has shown in his EuroPython 2023 workshop — wrapping them in dataclasses featuring
__str__()
interface. Not sure if that's relevant, though.So the traceback I'm talking about is only happening on PyPy and looks like this:
I was able to reproduce it locally on my Gentoo Linux laptop using pypy3.9-7.3.16. But I wasn't really able to make the repro smaller. I learned that this traceback is first seen in
pytest == 8.2.2
. Andpytest == 8.2.1
does not have this problem.So the current STR is something like:
pyenv install pypy3.9-7.3.16
pyenv virtualenv pypy3.9-7.3.16 multidict-pyenv-pypy3.9-7.3.16
pyenv shell multidict-pyenv-pypy3.9-7.3.16
pip install -r requirements/pytest.txt
pip install -e .
SETUPTOOLS_SCM_PRETEND_VERSION_FOR_PYTEST='8.2.2.dev0+first-broken-due-to-pr12414-regression' pip install 'pytest @ https://github.com/pytest-dev/pytest/archive/214d098fcce88940f5ce9353786b3cc8f0bd3938.tar.gz'
pytest --collect-only --no-cov tests/test_abc.py tests/test_copy.py tests/test_incorrect_args.py tests/test_multidict.py tests/test_mypy.py tests/test_pickle.py tests/test_types.py tests/test_update.py tests/test_version.py
These aren't all the test modules of the project. Removing them from CLI args or removing some of the tests inside makes it so that it doesn't traceback anymore. That's why I wasn't able to come up with a single-snippet reproducer. It feels like it only happens when a certain amount of tests is hit or something.
I also ran it with
--pdb
which allowed me to see the value ofitem
at that point whenOrderedDict.move_to_end()
is called: https://github.com/pytest-dev/pytest/blame/2b40981/src/_pytest/fixtures.py#L278C29-L280C30.It kinda suggests that the PyPy implementation of
OrderedDict
is weird:🤔 Why does this dict have duplicate keys? 😱
Anyway.. I confirmed that doing
SETUPTOOLS_SCM_PRETEND_VERSION_FOR_PYTEST='8.2.2.dev0+last-working-right-before-pr12414-regression' pip install 'pytest @ https://github.com/pytest-dev/pytest/archive/b41d5a52bbb808780ab310456d71e5ce509fd402.tar.gz'
makes the problem go away (which is expected because it does not yet have theOrderedDict.move_to_end()
call).And so this makes #12414 (#12409) responsible for the regression.
cc @bluetech I hope you'll have at least some idea of where to look because I've spent enough time staring at this w/o any clue...
UPD: I also forgot to mention that removing our own
pytest_collection_modifyitems
inconftest.py
does not help in any way, it still reproduces. Plus, changing the fixture scopes to match what we have inconftest.py
has no effect either.UPD2: pypy3.11-7.3.19 shows this behavior too.
The text was updated successfully, but these errors were encountered: