Fix fixture discovery to use definition order instead of alphabetical#14169
Fix fixture discovery to use definition order instead of alphabetical#14169veeceey wants to merge 6 commits intopytest-dev:mainfrom
Conversation
|
Hi maintainers, friendly ping on this PR. It's been open for about 10 days. I see the merge state is showing a conflict -- I'll rebase to resolve that. In the meantime, would appreciate any feedback on the approach (using |
Fixtures are now discovered in their definition order (as they appear in source code) rather than alphabetical order. This change resolves issues where fixtures with the same name at different scopes would be processed in unexpected order due to dir() sorting. The fix changes parsefactories() to use __dict__ iteration (which preserves insertion order in Python 3.7+) instead of dir() which sorts. This makes fixture behavior predictable based on source code order. Fixes pytest-dev#11281 Related to pytest-dev#12952
for more information, see https://pre-commit.ci
The previous implementation only looked at holderobj.__dict__ which is empty for new instances (e.g. unittest TestCase instances passed from unittest.py). This caused autouse fixtures defined on the class to not be discovered. Now properly handle three cases: - Modules: use module.__dict__ directly - Classes: walk __mro__ to get class and base class attributes - Instances: walk type(instance).__mro__ for class hierarchy Also add assert isinstance(holderobj, type) to satisfy mypy since safe_isclass() doesn't use TypeGuard.
- Add explicit type annotation `list[Mapping[str, Any]]` for dicts variable to fix mypy error about MappingProxyType vs dict mismatch (cls.__dict__ returns MappingProxyType for classes) - Fix test_autouse_fixtures_definition_order_preserved to use distinct fixture names and correct expectations (module + class fixtures both run, not just module)
8601e0f to
8fad347
Compare
The elif safe_isclass(holderobj) branch was unreachable in practice since parsefactories() is never called with a class directly (only modules and instances). Merged it with the instance branch by using holderobj_tp, which already resolves to the correct type for both classes and instances (set on line 1866-1869).
nicoddemus
left a comment
There was a problem hiding this comment.
Thanks @veeceey, please see my comments.
src/_pytest/fixtures.py
Outdated
| # Use __dict__ to preserve definition order instead of dir() which sorts. | ||
| # This ensures fixtures are processed in their definition order, which is | ||
| # important for autouse fixtures and fixture overriding (#11281, #12952). | ||
| # | ||
| # For modules: module.__dict__ contains all module-level names. | ||
| # For classes/instances: walk the MRO to get all class and base class | ||
| # attributes (using holderobj_tp which resolves to the class in both cases). | ||
| dicts: list[Mapping[str, Any]] | ||
| if isinstance(holderobj, types.ModuleType): | ||
| dicts = [holderobj.__dict__] | ||
| else: | ||
| # For classes and instances: walk the MRO to get all attributes. | ||
| # For classes, holderobj_tp is the class itself; for instances, | ||
| # holderobj_tp is type(holderobj) (set above on line 1867). | ||
| # In both cases holderobj_tp is a type with __mro__. | ||
| assert isinstance(holderobj_tp, type) | ||
| dicts = [cls.__dict__ for cls in holderobj_tp.__mro__] | ||
|
|
||
| seen: set[str] = set() | ||
| for dic in dicts: | ||
| for name in dic: | ||
| if name in seen: | ||
| continue | ||
| seen.add(name) |
There was a problem hiding this comment.
Could you please extract this logic into its own function? That would make it easier to understand and allow to add a unit test to ensure the desired behavior in isolation.
There was a problem hiding this comment.
done -- the logic is now in its own method _ensure_autouse_super_fixtures() on FixtureRequest. it's called from _get_active_fixturedef when we detect an autouse fixture that shadows another autouse fixture without explicitly requesting its super.
testing/python/fixtures.py
Outdated
| result.assert_outcomes(passed=1) | ||
|
|
||
|
|
||
| def test_autouse_fixtures_definition_order_preserved(pytester: Pytester) -> None: |
There was a problem hiding this comment.
These 2 tests pass for me on main, so they are not really testing the underlying issue. Could you please review them?
There was a problem hiding this comment.
yep, you were right. the old tests used different fixture names (module_setup vs class_setup) so they always passed -- they weren't actually hitting the bug. rewrote them to use the same name "setup" at both module and class scope, which is the actual trigger for the issue. they now fail on main and pass with the fix.
| holderobj_tp = holderobj | ||
|
|
||
| self._holderobjseen.add(holderobj) | ||
| for name in dir(holderobj): |
There was a problem hiding this comment.
I'm not sure the fix is correct: when discovering the fixtures for a module, we holderobj will be a Module. We then find a test class, we will collect it in a later call, where holderobj will be the class itself.
So I don't think the order returned by dir is not related to the original issue.
There was a problem hiding this comment.
you were totally right about this -- the dir() ordering in parsefactories wasn't the root cause at all. I've reverted all the parsefactories changes.
the actual issue is in the fixture activation at runtime. when _get_active_fixturedef("setup") is called for a class test, it finds fixturedefs = [module_setup, class_setup] and picks the closest one (class_setup at index -1). since class_setup doesn't explicitly request "setup" in its argnames, the module-level autouse fixture never gets activated.
the fix is in _get_active_fixturedef: before executing the closest autouse fixture, we now check if there are broader-scoped autouse fixtures in the chain that were shadowed, and activate those first. this preserves the documented "higher-scoped fixtures execute first" behavior.
|
thanks for the review @nicoddemus, these are really good points. re: extracting the logic — totally makes sense, I'll pull it into its own function so it's easier to test in isolation. re: the tests passing on main — yeah you're right, that's a problem. I think I was testing the wrong scenario. I'll rework them to actually reproduce the ordering issue. re: the fix itself — that's a fair concern. I need to dig deeper into how holderobj works across modules vs classes. if dir() order isn't actually the root cause then the fix is wrong and I need to find the real one. let me investigate and come back with an updated approach. |
|
hey @nicoddemus, I've been digging into this more and I think you're right — the dir() ordering isn't the actual root cause here. The real issue seems to be how fixture name resolution works when same-named fixtures exist at different scopes (module vs class). When both are named I need to rethink the approach entirely. Going to look at how |
…ting When a closer-scoped autouse fixture (e.g., class-level) shadows a broader-scoped autouse fixture (e.g., module-level) with the same name, only the closer one would run. The broader-scoped autouse fixture was silently skipped because the override chain wasn't walked for fixtures that don't explicitly request their super. The fix adds _ensure_autouse_super_fixtures() which activates any broader-scoped autouse fixtures in the chain before the closer one executes. This preserves the documented behavior that higher-scoped fixtures execute first. Also rewrote tests to actually reproduce the issue (same-named fixtures at different scopes) -- old tests used different names and passed on main. Fixes pytest-dev#11281
Summary
Fixes #11281
This PR changes fixture discovery to preserve definition order instead of using alphabetical sorting from
dir().Problem
Previously,
parsefactories()useddir()to iterate over object attributes, which sorts names alphabetically. This caused unexpected behavior when fixtures with the same name were defined at different scopes (e.g., module vs class level), because the order of discovery depended on alphabetical sorting rather than source code order.Solution
Changed
parsefactories()to use__dict__iteration (similar to howPyCollector.collect()works), which preserves insertion/definition order in Python 3.7+. This ensures fixtures are processed in the order they appear in source code, making behavior predictable and intuitive.Changes
FixtureManager.parsefactories()to iterate__dict__instead ofdir()PyCollector)Test plan