Fixtures now register finalizers with all fixtures before them in the stack #6438
Conversation
|
I'm not sure why it's failing. Could it be because I pushed a fix for the string formatting issue before the build was completely done? |
|
@SalmonMode |
|
Thanks for catching those! All fixed. Also thanks for the heads up about pytest-CI 😁 |
There was a problem hiding this comment.
Thanks @SalmonMode, awesome contribution!
This is one of the hairest parts of the code base, congratulations in pulling this off.
I used the debugger and things look right to me; also the fact that you didn't need to change any other tests at all further suggests the patch is solid.
Could you please apply my last suggestion and squash/rebase on master (let me know if you would like me to do that for you)?
I would also appreciate if @RonnyPfannschmidt could take a look at this.
|
You got it! All set! 😁 |
|
Thanks @SalmonMode, but there's a bunch of commits which shouldn't be there... can you recheck how you rebased/squashed please? |
SalmonMode
force-pushed
the
consistent-fixture-stack
branch
2 times, most recently
from
fcf2770 to
34f58d9
Compare
|
Ah, right you are. I pulled and merged my branch after rebasing without thinking haha. Looks like it's good now. I also made sure to get the very latest master and rebased on that again (since some stuff got committed there since yesterday). |
|
The rebase now looks correct, thanks! Could you please also squash the commits into one? The history in that case is not really relevant. 😁 (Let me know if you want me to do it, I will be glad to; our policy is to always ask before push-forcing into forks) |
SalmonMode
force-pushed
the
consistent-fixture-stack
branch
from
34f58d9 to
9918093
Compare
|
Sure thing! All set 😁 I forgot it could be done on the branch itself instead of squashing on merge |
|
Thanks @SalmonMode appreciate the patience! |
|
No worries, @nicoddemus! Sorry for the poke. I just downloaded the new GitHub app and was messing around :P |
|
Not at all 😁 |
Closes #6436
Turns out the root cause was because fixtures were only considering the fixtures requested in their argnames when trying to find out where to register their finalizers at execute time. They weren't considering autouse fixtures, so when this was combined with parameterization, this resulted in fixtures not being torn down when a parameterized, autouse fixture was being torn down to go from one param set to the next.
This also resulted in fixtures being torn down out of order in extreme cases. You can see an example of this in the test that I added (if you run it without this fix).
The solution ultimately required me to have fixtures register their finalizers with all fixtures that came before them in the stack. This list of fixtures came from using
request.fixturenamesin combination with the parent requests (when relevant) and the fixture's requested argnames to determine a good slicing point. Since this logic could be summed up on its own, I moved it into its own private method with a beefy docstring to explain it.