-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
[WIP/RFC] fixtures: register finalizers with all previous fixtures in the stack (take 2) #6551
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
Conversation
This is a regression test for part of pytest-dev#6492, testing one of the fixes in pytest-dev#6551.
@SalmonMode any idea/plan to test #6504 (83349d7)? |
This is a regression test for part of pytest-dev#6492, testing one of the fixes in pytest-dev#6551.
@blueyed added the test case and also cleaned up the logic a bit by putting it into its own function so the code reads a little easier. |
79ed509
to
6db4724
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Not sure if this is related, but I'm seeing that e.g.
|
@timc13 with this branch I assume? |
no, sorry - this is in 5.3.5. I was wondering if this PR is related. I can also create another ticket otherwise |
Test it? :)
Yes, but information if this here helps would be useful there then also. (have not looked at the issue itself myself) |
Note: black cannot parse `return *active_fixture_argnames, *self.argnames` yet (fixed in master, psf/black#1121). Tested manually using: ```python @pytest.fixture(scope="session") def xdist_suffix(request): print("\nxdist_suffix") suffixes.append("xdist") @pytest.fixture(scope="session") def parallel_suffix(tox_suffix, xdist_suffix): pass def test_suffix(parallel_suffix): assert suffixes == ["tox", "xdist"] ``` When using a set there the order is not deterministic, i.e. the test is flaky.
…er finalizer to a dependee fixture once
… add ther finalizer to a dependee fixture once
any update on this? I also wanted to link this related fix for pytest-django pytest-dev/pytest-django#807 just for later reference @timc13 were you able to run your tests using this branch to see if it solved your issue? |
@RonnyPfannschmidt @nicoddemus any thoughts on moving this forward? If I get the chance, I'll resolve the merge conflict later today (and I'm guessing that's the cause of the failed build). But I think it's a pretty solid fix at this point. |
I'm pretty sure this is affecting me in Inconsistent results for a test when running in-suite. Is there at least a temporary workaround that once can use? |
@SHxKM would you be able to try this branch in combination with the branch for pytest-django here? I believe both changes will provide the solution, because the original fix I made for this caused some issues to be revealed in pytest-django that line up with what you described in the issue you linked. |
Edit: FWIW my issue had nothing to do with this. @SalmonMode - Thanks for responding. So I ran:
I hope that's what you meant. The test still fails when running together with the whole suite :/ |
Hmmm are you able to provide a minimal set of tests to recreate it? |
@RonnyPfannschmidt could you help reviewing this one as well? |
at best by the weekend |
Closing as per blueyed#365 (comment). |
As mentioned here, I'll be remaking this branch under my repo again to continue the work. |
@SalmonMode Thanks! |
Picking up #6438 again.
TODO:
/cc @SalmonMode
NOTES: