-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Avoiding looking upwards for parameter argnames when generating fixtu… #5254
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
Avoiding looking upwards for parameter argnames when generating fixtu… #5254
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5254 +/- ##
==========================================
+ Coverage 91.21% 93.21% +1.99%
==========================================
Files 115 115
Lines 26282 26303 +21
Branches 2589 2592 +3
==========================================
+ Hits 23974 24519 +545
+ Misses 1983 1462 -521
+ Partials 325 322 -3
Continue to review full report at Codecov.
|
Basically the two failing tests are problems with the CI themselves it seems. Anyways, I think this is functional enough so someone can just review it? |
Closing and reopening to trigger CI |
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.
Excellent work @Sup3rGeo, really sorry about the delay on this. 👍
Please take a look at my suggestions and let me know what you think.
Also please squash your commits and rebase on |
Thanks for this! |
(wouldn't be a problem if we could do it from the UI (#4361)) |
@Sup3rGeo would you mind squashing/rebasing one last time? Feel free to merge it yourself once CI is green again. 😁 |
Sure! I was waiting to do this the very last thing, after checking if CI passed, which it did so I will do it now |
505ee0a
to
5bf88e9
Compare
Looks like you've rebased it on features? |
Looks like it. Problem is that now all commits have been rewritten 😬 |
Well, it should go to master, no? (wouldn't be a problem if we could do it from the UI (#4361)) - so maybe @asottile / @RonnyPfannschmidt should take over.. ;) |
5bf88e9
to
eb5c797
Compare
Hahaha sorry guys looks like I screwed big time, thanks @asottile for fixing it up. |
Found a mistake in a test, I will commit the fix and squash (properly this time) |
eb5c797
to
65bd1b8
Compare
Ok hope I did right this time (sorry for this mess). If no one objects, I will do as @nicoddemus said and merge this if CI becomes green. |
Looks good! Please go ahead with the merge once it's green. 👍 |
…reinfo.
Closes #5036
Basically even though there is code to check if a fixture is a parameter name:
pytest/src/_pytest/fixtures.py
Lines 1218 to 1231 in 6a43c8c
The problem is that any other higher level dependant fixtures would also be included in the
metafunc.fixturenames
for the function, and because of course the name of this higher fixture does not match the parametrization parameter, the function would be erroneously additionally parametrized.///
changelog
folder, with a name like<ISSUE NUMBER>.<TYPE>.rst
. See changelog/README.rst for details.master
branch for bug fixes, documentation updates and trivial changes.features
branch for new features and removals/deprecations.Unless your change is trivial or a small documentation fix (e.g., a typo or reword of a small section) please:
AUTHORS
in alphabetical order;