Skip to content

Fix module fixtures with --doctest-modules#14540

Merged
bluetech merged 3 commits into
pytest-dev:mainfrom
minbang930:fix-14533-doctest-module-fixtures
Jun 8, 2026
Merged

Fix module fixtures with --doctest-modules#14540
bluetech merged 3 commits into
pytest-dev:mainfrom
minbang930:fix-14533-doctest-module-fixtures

Conversation

@minbang930

@minbang930 minbang930 commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Fix fixture registration when --doctest-modules collects a Python module before the normal module collector.
  • Key parsed fixture holders by their visibility anchor, so normal tests get module-scoped FixtureDefs without relaxing node-based scoping.
  • Add a regression test, changelog entry, and AUTHORS entry.

Closes #14533.

Test plan

  • PYTHONPATH=src python /tmp/pytest-14533-repro/run_checkout_pytest.py -q testing/test_doctest.py::TestDoctests::test_module_fixture_available_to_normal_test_with_doctestmodules testing/test_doctest.py::TestDoctests::test_doctestmodule_with_fixtures testing/test_doctest.py::TestDoctests::test_doctestmodule_three_tests testing/test_doctest.py::TestDoctestAutoUseFixtures37 passed in 0.75s
  • PYTHONPATH=src python /tmp/pytest-14533-repro/run_checkout_pytest.py -q testing/test_conftest.py::test_conftest_fixture_scoping_with_testpaths_outside_rootdir testing/test_conftest.py::test_conftest_fixture_from_ancestor_above_rootdir testing/deprecated_test.py::TestFixtureNodeidDeprecations6 passed in 0.14s
  • PYTHONPATH=src python /tmp/pytest-14533-repro/run_checkout_pytest.py -q testing/python/fixtures.py::TestFixtureManagerParseFactories::test_parsefactories_conftest testing/python/fixtures.py::TestFixtureManagerParseFactories::test_parsefactories_conftest_and_module_and_class testing/python/fixtures.py::TestFixtureManagerParseFactories::test_parsefactories_relative_node_ids3 passed in 0.10s
  • git diff --check
  • python -m compileall -q src/_pytest/fixtures.py testing/test_doctest.py
  • python -m ruff check src/_pytest/fixtures.py testing/test_doctest.py
  • python -m ruff format --check src/_pytest/fixtures.py testing/test_doctest.py

AI assistance

OpenAI Codex helped reproduce the bug, draft the regression test, and prepare the initial patch. I reviewed the diff, checked the fixture scoping risk, added the changelog and AUTHORS entries, reran the targeted tests, and take responsibility for the change.

  • Include new tests or update existing tests when applicable.
  • Allow maintainers to push and squash when merging my commits.
  • Add closes #14533 to the PR description and commit message.
  • Credit the AI agent in a Co-authored-by commit trailer.
  • Create a changelog file in changelog/.
  • Add myself to AUTHORS in alphabetical order.

@psf-chronographer psf-chronographer Bot added the bot:chronographer:provided (automation) changelog entry is part of PR label Jun 1, 2026

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a good start and a fix

please expand the docs on the test

@bluetech a later major followup could make dedicated fs nodes part of the collection tree and specific nodes could decide if they ancor fixtures in the file node or the specific node

this first idea needs more iteration

Comment thread testing/test_doctest.py
reprec = pytester.inline_run(p, "--doctest-modules")
reprec.assertoutcome(passed=1)

def test_module_fixture_available_to_normal_test_with_doctestmodules(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add a mark that links the issue and add a note on the regression on the docstring

@minbang930 minbang930 force-pushed the fix-14533-doctest-module-fixtures branch from eeed18e to 0ae2a23 Compare June 2, 2026 07:40
@minbang930

Copy link
Copy Markdown
Contributor Author

Thanks, updated the regression test docs for the DoctestModule/Module fixture case. CI is green now.

@bluetech

bluetech commented Jun 2, 2026

Copy link
Copy Markdown
Member

If we key _holderobjseen by (obj, node), under which conditions will it be effective? I don't think parsefactories needs to be idempotent. So if this is the solution we go with, I prefer to just remove _holderobjseen.

The PR should document in the changelog the fact that higher-scoped autouse fixtures will now execute twice when using --doctest-modules, and how to avoid it if that's a problem (move said fixtures to conftest.py). We should also add a test for this.

If we impart to users the mental model that --doctest-modules effectively means that each python file is processed twice, I think the fixture behavior would make sense to them.

@bluetech a later major followup could make dedicated fs nodes part of the collection tree and specific nodes could decide if they ancor fixtures in the file node or the specific node

Interesting idea, but regarding the "specific nodes could decide if they ancor fixtures in the file node or the specific node" part:

  • If e.g. Module and DoctestModule decide differently, then fixtures will still be registered twice, so no good.
  • If they both choose to anchor at the FS node, it seems sort of ugly to me for the first one to "take over" the FS node and rely on a _holderobjseen-like mechanism to avoid duplication (ugly because it breaks encapsulation, which is why I'd really like to get rid of _holderobjseen).
  • So presumably the way would be for the FS node itself to do the parsefactories, and remove it from Module and DoctestModule. But parsefactories requires an obj which I think is not appropriate for a pure "FS" node to know about.

@minbang930 minbang930 force-pushed the fix-14533-doctest-module-fixtures branch 2 times, most recently from c70b02a to 0526d9b Compare June 2, 2026 11:14
@minbang930

Copy link
Copy Markdown
Contributor Author

Updated per feedback: removed _holderobjseen, documented the higher-scoped autouse fixture caveat, and added a regression test for it.

CI is green now.

@RonnyPfannschmidt

Copy link
Copy Markdown
Member

@bluetech hence the need for iteration - theres need for certain cooperation to make things work nice

technically i would actually go as far as considering doctestmodules a collector that intently is actually a child of the module, that it is at the same level as the module its "contained in" is the real issue for me

@bluetech

bluetech commented Jun 3, 2026

Copy link
Copy Markdown
Member

technically i would actually go as far as considering doctestmodules a collector that intently is actually a child of the module, that it is at the same level as the module its "contained in" is the real issue for me

One problem with this is that it makes the DoctestModule dependent on the Module, but I think it's a nice property that they're independent, e.g. I can run just the doctests without running the python tests.

Another problem is that module is not the only relevant scope, there is also class scope. I think if module-level fixtures are shared then class-level fixtures are expected to be shared as well, and Module -> DoctestModule -> DoctestItem wouldn't work for that. Scratch that, doctest currently doesn't support class scope so this wouldn't be a regression.

@bluetech bluetech force-pushed the fix-14533-doctest-module-fixtures branch from 0526d9b to e8877d6 Compare June 8, 2026 07:56
@bluetech

bluetech commented Jun 8, 2026

Copy link
Copy Markdown
Member

Updated the PR:

  • Reworked commit message and changelog entry
  • Updated docs a tiny bit
  • Improved the tests a bit
  • Added a couple of semi-related small commits

minbang930 and others added 3 commits June 8, 2026 12:21
`parsefactories` is the function which discovers fixtures on a given
holder object (module, class, etc). Previously it was made idempotent
using a `holderobjseen` map, which made it only process a given
holderobj once.

However, a single obj can be legitimately processed multiple times for
different nodes. In particular, in pytest core this happens with
`--doctest-modules` -- a python module is processed once as regular
Python and once as doctest, which both support fixtures.

Before 46478fa we were using `nodeid`
for fixture visibility, which ended up working "accidentally" since both
`Module` and `DoctestModule` happen to have the same `nodeid`. But now
we use `Node` visibility, and the nodes are different, so the fixtures
defined in the module only end up being visible to the `DoctestModule`
(which runs `parsefactories` first).

Fix this by removing the `parsefactories`, remove the idempotency and
allowing multiple plugins to collect fixtures from the same obj.

This particularly means that e.g. `module`-scoped autouse fixtures will
now run *twice* with `--doctest-modules`.

See discussion in issue and PR for other options we've considered.

Closes pytest-dev#14533.

Co-authored-by: OpenAI Codex <codex@openai.com>
Co-authored-by: Ran Benita <ran@unusedvar.com>
This allows running `pytest --doctest-modules`.

`tox -e plugins` still works.
@bluetech bluetech force-pushed the fix-14533-doctest-module-fixtures branch from e8877d6 to ef55156 Compare June 8, 2026 09:21
@bluetech bluetech merged commit 2ce9c6d into pytest-dev:main Jun 8, 2026
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot:chronographer:provided (automation) changelog entry is part of PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

enabling doctest-modules causes fixture to not be found

3 participants