-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Fix a tiny bug in python.py::_find_parametrized_scope
#11277
Merged
bluetech
merged 7 commits into
pytest-dev:main
from
sadra-barikbin:Fix-a-typo-in-find-parametrized-scope
Aug 6, 2023
Merged
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
68361d9
Fix the typo and update test
sadra-barikbin 05283a4
Add changelog entry and a test
sadra-barikbin 4fdd3d3
Fix a tiny typo
sadra-barikbin 6ca11f7
python: fix scope assignment for indirect parameter sets
sadra-barikbin 02cba9c
Merge remote-tracking branch 'upstream/Fix-a-typo-in-find-parametrize…
sadra-barikbin aec2ee3
Apply the comment
sadra-barikbin 92db454
Delete 11234.bugfix.rst
bluetech File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
Fixed a bug that when there are multiple fixtures for an indirect parameter, | ||
the scope of the highest-scope fixture is picked for the parameter set, instead of that of the one with the narrowest scope. |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I think this is important for testing this particular code path:
This change might make test3 to be reordered before test2, in order to share to module-scoped
fixture
-- I'm not sure (maybe it will only happen once we reorder based on param value and not param index).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.
I'll add it but it does not change the order since the only produced fixture key of
test_3
isFixtureArgKey('fixture', 0, 'test_module', 'Test')
and the only produced fixture key oftest_1
isFixtureArgKey('fixture', 0, 'test_module', None)
. Basing reordering on param value does not change it either as the module-scopedfixture
is shadowed behind the class-scoped one fortest_3
. In other words we only pickfixturedefs[-1]
to produce a key.If you think we should consider shadowed-but-used fixtures into reordering as well, one way to that end is to check if
fixturedefs[-1]
itself usesfixture
or not so that we could produce a key for it as well. A general solution to considering shadowed-but-used fixtures was among the items in #11234 (Considering used shadowed fixture dependencies for reordering as well. This was done by changing fixturemanager::getfixtureclosure algo from BFS to DFS.
) but I removed it as I thought it would not get accepted by the team. We could move forward with that as well.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.
Right, I forgot about the
item_cls
.Regarding reordering based on shadowed-but-used fixtures, naively it makes sense to me -- they are used after all so why should they be ignored?
Perhaps after the current batch of changes we can look into it and think more deeply about it.