-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Make test parametrization override indirect fixtures #3629
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
Make test parametrization override indirect fixtures #3629
Conversation
We don't deploy anything on tags with AppVeyor, we use Travis instead, so we might as well save resources
Skip AppVeyor builds on tag pushes
src/_pytest/fixtures.py
Outdated
working_set.update(self.name2fixturedefs[argname][-1].argnames) | ||
|
||
self.names_closure[:] = sorted( | ||
closure, key=lambda name: self.names_closure.index(name) |
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.
the key could just be self.names_closure.index
since this one corrects a rather strange misbehavior, i wonder if its better to target the feature branch just to avoid certain kinds of disruption in a patch release CC @nicoddemus |
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.
nice addition, do the type definitions do any good wrt tooling?
i believe this is good to go after we decide the target branch/disruption and add a changelog entry
@RonnyPfannschmidt I added those type definitions as a form of documentation, just to make it clear what the fields of FuncFixtureInfo are. Not sure what you mean by tooling. |
I haven't had a chance to review the PR yet, but I agree that even if this changes a somewhat wrong behavior, we better be safe and add this to the next feature release. |
@egnartsms aka mypy or something like that (because if its only acting as comments i consider it a liability) |
we will have to do our own homework first and merge master to features before this one can apply as intended |
#3634 will sort the conflict out then i will try again, |
@egnartsms oh, i missed it before, can you please add a news fragment about the bugfix once thats complete its good to merge 👍 |
@RonnyPfannschmidt do you mean to create a file in the changelog directory? Or something else? That's my first contribution to pytest. |
@edisongustavo correct a file in the changelog directory |
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 @egnartsms, thanks a lot!
A fix for #2220.