Skip to content
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

pytest 8.0.1 regression: setup_module is not run with --doctest-modules #12011

Closed
lesteve opened this issue Feb 19, 2024 · 7 comments
Closed
Labels
plugin: doctests related to the doctests builtin plugin type: bug problem that needs to be addressed type: regression indicates a problem that was introduced in a release which was working previously

Comments

@lesteve
Copy link
Contributor

lesteve commented Feb 19, 2024

It seems like setup_module is not being run in 8.0.1 when using --doctest-modules. This works fine with 8.0.0.

This was seen in scikit-learn/scikit-learn#28461.

To reproduce:

# test.py
CONSTANT = 0

def setup_module():
    global CONSTANT
    CONSTANT = 1


def test():
    assert CONSTANT == 1
pytest --doctest-modules test.py

Expected output: no failures

Actual output: failure

❯ pytest --doctest-modules test.py
======================================== test session starts ========================================
platform linux -- Python 3.12.2, pytest-8.0.1, pluggy-1.4.0
rootdir: /tmp/test
collected 1 item                                                                                    

test.py F                                                                                     [100%]

============================================= FAILURES ==============================================
_______________________________________________ test ________________________________________________

    def test():
>       assert CONSTANT == 1
E       assert 0 == 1

test.py:9: AssertionError
====================================== short test summary info ======================================
FAILED test.py::test - assert 0 == 1
========================================= 1 failed in 0.01s =========================================

❯ pip list              
Package    Version
---------- -------
iniconfig  2.0.0
packaging  23.2
pip        24.0
pluggy     1.4.0
pytest     8.0.1
setuptools 69.1.0
wheel      0.42.0
@bluetech bluetech added type: bug problem that needs to be addressed plugin: doctests related to the doctests builtin plugin type: regression indicates a problem that was introduced in a release which was working previously labels Feb 19, 2024
@bluetech
Copy link
Member

Thanks, I can reproduce. Bisected to a6851e3, which is unexpected, but I confirmed that it's the culprit. Interestingly, it does not reproduce in main. I will check what's going on.

@bluetech
Copy link
Member

Analysis

The reason this happens:

  • doctest: fix autouse fixtures possibly not getting picked up #11941 made doctest start doing its own parsefactories on the module object.
  • parsefactories returns early if it had already seen an object.
  • The python Module has the same module object as the DoctestModule.
  • The python Module is collected after the DoctestModule.
  • The python Module patches the module object with fixtures, but now the patching is not effective -- parsefactories already ran on the object before it was patched.

Why a6851e3 revealed the issue? Because that bug caused the python Module to (wrongly) be collected before the DoctestModule.

Why it doesn't happen in main? Because c8792bd replaced the patching with directly registering the fixtures, making this a non-issue.

Possible solutions

  1. Backport c8792bd to 8.0.x. Should be fine but a bit risky for a backport.

  2. Reorder the python and doctest plugins such that pytest hooks run before doctest hooks. Should be fine (IMO an improvement) but risky - will cause python tests to be executed before doctests (in the same file), not sure which other effects it might have.

  3. More localized version of the previous solution - add trylast=True to doctest's pytest_collect_file hookimpl. Same risk.

  4. Since this is fixed in main, just release 8.1.0 in lieu of a pytest 8.0.2.

I am leaning towards solution 1 but happy to hear other opinions or ideas.

@nicoddemus
Copy link
Member

Thanks @bluetech for the quick analysis.

IMHO it is OK to hold this a bit and release 8.1.0 soonish, something like:

  1. Release 8.0.2 in 2 weeks or so.
  2. Release 8.1.0 about 1 week or so later.

I understand the frustration of having something working in 8.0.0, then 8.0.1 (a bugfix), breaks that use case for you in 8.0.1, and then maintainers telling you that you have to wait for 8.1.0 to get back what was working in 8.0.0.

I'm not so keen of implementing quick solutions in 8.0.x, specially if they have the potential of introducing other subtle problems.

If the above is not good enough, I would go with 4) then and release 8.1.0 next.

@lesteve
Copy link
Contributor Author

lesteve commented Feb 20, 2024

Personally, I think this is completely fine to wait for 8.1.0.

This may also be an occasion for us to replace the few instances of setup_module we still have by module-level fixtures.

@bluetech
Copy link
Member

OK, let's wait for 8.1.0 roughly by @nicoddemus's suggested schedule.

I'll submit a regression test against main, and add a changelog that this bug is fixed, which will be included in the 8.1.0 release notes.

@bluetech
Copy link
Member

bluetech commented Mar 3, 2024

pytest 8.1.0 is now released.

@bluetech bluetech closed this as completed Mar 3, 2024
@lesteve
Copy link
Contributor Author

lesteve commented Mar 4, 2024

Thanks for the follow-up! I confirm that this has been fixed in 8.1.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plugin: doctests related to the doctests builtin plugin type: bug problem that needs to be addressed type: regression indicates a problem that was introduced in a release which was working previously
Projects
None yet
Development

No branches or pull requests

3 participants