-
-
Notifications
You must be signed in to change notification settings - Fork 3k
fix #14004 - connect conftests to nodeids/nodes instead of matching string prefixes - ensure we connect in the collect phase in case of directory confusion #14098
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
base: main
Are you sure you want to change the base?
Conversation
…ootdir When testpaths in config points to a directory outside the rootdir using a relative path like '../tests/sdk', conftest fixture scoping was broken. Fixtures from nested conftest.py files would leak to sibling directories because conftests outside rootpath were assigned an empty string baseid, making them global. The fix computes proper nodeids for conftests outside rootpath by finding them relative to the initial paths (from config.args/testpaths), similar to how FSCollector computes nodeids for test files outside rootpath. Fixes pytest-dev#14004. Co-authored-by: Cursor AI <[email protected]> Co-authored-by: Anthropic Claude Opus 4 <[email protected]>
Fixes pytest-dev#14004 - conftest fixtures now properly scoped when testpaths points outside rootdir. Instead of computing fixture nodeids during plugin registration (which required complex path resolution for conftests outside rootpath), conftest fixtures are now deferred until their Directory is collected. This ensures: - Conftest fixtures use the Directory's actual nodeid from the collection tree - Proper fixture scoping regardless of conftest location relative to rootpath - Simpler, more robust implementation Changes: - Add _pending_conftests dict to store conftest modules by directory path - Defer conftest fixture parsing via pytest_make_collect_report hook - Add parsefactories(holder=, node=) as preferred keyword-only API - Remove _get_nodeid_for_path_outside_rootpath (no longer needed) Co-authored-by: Cursor AI <[email protected]> Co-authored-by: Anthropic Claude Opus 4 <[email protected]>
Add node parameter to FixtureDef and use node-based matching instead of relying solely on nodeid string prefix matching. Changes: - Add node parameter to FixtureDef to store the defining node - Derive baseid from node.nodeid when node is available - Add node parameter to _register_fixture - Update parsefactories to track and pass effective_node - Update _matchfactories to use node identity for matching when available - Fall back to string-based matching for legacy/plugins This enables more robust fixture matching by using node identity comparison instead of string prefix matching, while maintaining backward compatibility. Co-authored-by: Cursor AI <[email protected]> Co-authored-by: Anthropic Claude Opus 4 <[email protected]>
…ration Update all internal call sites to use node= parameter instead of nodeid= string for fixture registration. This enables node-based matching. Changes: - python.py: Module and Class xunit fixtures now use node=self - python.py: Class.collect parsefactories uses holder=..., node=self - unittest.py: UnitTestCase fixtures now use node=self - unittest.py: UnitTestCase.collect parsefactories uses holder=..., node=self This completes Phase 1 of migrating from string-based to node-based fixture scoping. Co-authored-by: Cursor AI <[email protected]> Co-authored-by: Anthropic Claude Opus 4 <[email protected]>
…meters Add deprecation warnings for using string-based fixture scoping: - FixtureDef baseid parameter: use node parameter instead - _register_fixture nodeid parameter: use node parameter instead - parsefactories nodeid string: use holder/node API instead The warnings only trigger when a non-empty nodeid string is passed without a node. Global plugins (nodeid=None) and synthetic fixtures (baseid='') do not trigger warnings. These will be removed in pytest 10, completing the migration to node-based fixture scoping. Co-authored-by: Cursor AI <[email protected]> Co-authored-by: Anthropic Claude Opus 4 <[email protected]>
Co-authored-by: Cursor AI <[email protected]> Co-authored-by: Anthropic Claude Opus 4 <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes conftest fixture scoping when testpaths points outside rootdir using relative paths (issue #14004). The fix migrates from fragile string prefix matching to robust node-based matching for fixture scoping. Conftest fixtures are now parsed during Directory collection, using the Directory node's nodeid for proper scoping instead of calculating string-based nodeids during plugin registration.
- Implements deferred conftest parsing tied to Directory collection
- Adds node-based fixture matching alongside string-based fallback for compatibility
- Deprecates
baseid/nodeidstring parameters in favor ofnodeparameter
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| testing/test_conftest.py | Adds comprehensive regression test for fixture scoping with testpaths outside rootdir |
| src/_pytest/fixtures.py | Implements deferred conftest parsing, node-based matching, and deprecates string-based APIs |
| src/_pytest/unittest.py | Updates fixture registration calls to use new node parameter |
| src/_pytest/python.py | Updates fixture registration calls to use new node parameter |
| changelog/14004.deprecation.rst | Documents deprecation of baseid/nodeid string parameters |
| changelog/14004.bugfix.rst | Documents the fixture scoping fix |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Interesting! The "tests outside of the rootdir" scenario is ill-advised but I guess we need to support it. Regarding the first commit, I'm looking at the test <Dir tests> '::tests'
<Dir sdk> ''
<Dir inner> 'inner'
<Module test_inner.py> 'inner/test_inner.py'
<Function test_inner> 'inner/test_inner.py::test_inner'
<Module test_outer.py> 'test_outer.py'
<Function test_outer> 'test_outer.py::test_outer'That seems odd and broken to me, there's obviously a confusion going on here. Generally the intention is for nodeid's to be relative to the rootdir, so what happens when the collection path is not under the rootdir? I've already written about this issue in #11245. The commit "fix #14004 - connect conftests to nodeids/nodes instead of matching string prefixes - ensure we connect in the collect phase in case of directory confusion" works around the issue using some "fixup" code in fixtures.py. I think it's a bit confusing, particularly the function name "_get_nodeid_for_path_outside_rootpath", it basically invents a "fake" nodeid because the existing nodeid is broken, but it's not an actual ID of a node... I think that if we fix the underlying issue #11245, that would be a better way to go? In that issue we had a couple of suggestions. Also regarding the implementation itself, I fear that the loop over the Regarding the "fix: assign conftest fixtures to Directory nodes during collection" idea and subsequent changes, I think I like the direction, I definitely think we should to try to integrate conftests with the collection tree better, and get away from the current path matching. Thanks for taking a look at this. I need to look into it more, but can you describe the interaction with initial conftests? Are they associated with a Directory or are they "global"? The Lines 383 to 399 in 0e9db5f
|
|
the main confusion thats happening is that initial conftest files are loaded before we have a node, so we cant parse factories until we see them again in collection the case of tets outside of a rootdir is not as ill advised as one might think the moment one ships integration testsuites as installed python packages and suddenly many tests collect as part of site-packages instead of the cwd |
- Add compute_nodeid_prefix_for_path() for better nodeid computation: - Paths in site-packages use site:// prefix - Nearby paths (≤2 levels up) use relative paths with .. - Far-away paths use absolute paths - Add _path_in_site_packages() with optional site_packages parameter for testing - Fix _getautousenames() to skip duplicate nodeids (Session and root Dir both have nodeid='') - Add unit tests for nodeid prefix computation Co-authored-by: Cursor AI <[email protected]> Co-authored-by: Anthropic Claude <[email protected]>
- Add norm_sep() to convert path separators to forward slashes - Simplifies handling of Windows paths and cross-platform data - Use norm_sep in compute_nodeid_prefix_for_path and FSCollector - Fix test_compute_nodeid_far_away_absolute for Windows compatibility Co-authored-by: Cursor AI <[email protected]> Co-authored-by: Anthropic Claude <[email protected]>
|
The "add: nodes.norm_sep helper for nodeid path separator normalization" change looks sensible, you can submit and merge it to main with my review if you'd like. As an internal refactoring it doesn't need a changelog entry though. Can you submit "improve: compute meaningful nodeids for paths outside rootdir" in its own PR? I have some thoughts but it's a substantial change so will be better considered separately before this PR. |
But do all initial conftests will have a node? Conftests are looked up the filesystem tree (up to confcutdir) while nodes are not. So I figure it's possible to have a conftest plugin without a corresponding Directory, though I haven't verified this so might be wrong about that. |
|
As far as I understand any conftest not having a attached node isn't a valid fixture source |
|
You may be right but that would surprise me! I'd imagine these fixtures would be "global" (empty/None baseid, don't remember which one of them). Are you able to verify this? |
fixes #14004
the key issue was that
nodeid/baseidfor fixtures of suchconftest.pywould be""instead of something fittingths pr is a chain of changes