Skip to content
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

[WIP/RFC] fixtures: register finalizers with all previous fixtures in the stack (take 2) #6551

Closed
wants to merge 7 commits into from

Conversation

blueyed
Copy link
Contributor

@blueyed blueyed commented Jan 23, 2020

Picking up #6438 again.

TODO:

/cc @SalmonMode

NOTES:

blueyed added a commit to blueyed/pytest-django that referenced this pull request Jan 23, 2020
@blueyed blueyed changed the title fixtures: register finalizers with all previous fixtures in the stack (take 2) [WIP/RFC] fixtures: register finalizers with all previous fixtures in the stack (take 2) Jan 24, 2020
blueyed added a commit to blueyed/pytest that referenced this pull request Jan 24, 2020
This is a regression test for part of
pytest-dev#6492, testing one of the
fixes in pytest-dev#6551.
@blueyed
Copy link
Contributor Author

blueyed commented Jan 24, 2020

@SalmonMode any idea/plan to test #6504 (83349d7)?

@SalmonMode
Copy link
Contributor

@blueyed I have an idea. I was working on a test for #6492 / #6551 but I just saw you beat me to the punch 😂

I'll put together the test for #6504 now

nicoddemus pushed a commit to blueyed/pytest that referenced this pull request Jan 24, 2020
This is a regression test for part of
pytest-dev#6492, testing one of the
fixes in pytest-dev#6551.
@SalmonMode
Copy link
Contributor

@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.

@SalmonMode

This comment has been minimized.

@blueyed

This comment has been minimized.

@timc13
Copy link

timc13 commented Feb 3, 2020

Not sure if this is related, but I'm seeing that indirect fixtures are not getting cleaned up when being stacked.

e.g.

import pytest


@pytest.fixture
def x(): pass


@pytest.fixture
def y(): pass


@pytest.mark.parametrize("x", [0, 1], indirect=['x'])
@pytest.mark.parametrize("y", [2, 3], indirect=['y'])
def test_foo(x, y):
    pass

@blueyed
Copy link
Contributor Author

blueyed commented Feb 3, 2020

@timc13 with this branch I assume?

@timc13
Copy link

timc13 commented Feb 3, 2020

no, sorry - this is in 5.3.5. I was wondering if this PR is related. I can also create another ticket otherwise

@blueyed
Copy link
Contributor Author

blueyed commented Feb 3, 2020

@timc13

I was wondering if this PR is related.

Test it? :)

I can also create another ticket otherwise

Yes, but information if this here helps would be useful there then also. (have not looked at the issue itself myself)

SalmonMode and others added 6 commits March 9, 2020 13:00
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.
… add ther finalizer to a dependee fixture once
blueyed added a commit to blueyed/pytest-django that referenced this pull request Mar 31, 2020
@SalmonMode
Copy link
Contributor

SalmonMode commented Apr 11, 2020

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?

@SalmonMode
Copy link
Contributor

@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.

@SHxKM
Copy link

SHxKM commented May 17, 2020

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?

@SalmonMode
Copy link
Contributor

@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.

@SHxKM
Copy link

SHxKM commented May 17, 2020

Edit: FWIW my issue had nothing to do with this.

@SalmonMode - Thanks for responding. So I ran:

pip install git+https://github.com/blueyed/pytest@fixture-stack
pip install git+https://github.com/SalmonMode/pytest-django@cnejame-fix-pytest-5-3-3-compatibility

I hope that's what you meant. The test still fails when running together with the whole suite :/

@SalmonMode
Copy link
Contributor

Hmmm are you able to provide a minimal set of tests to recreate it?

@nicoddemus
Copy link
Member

@RonnyPfannschmidt could you help reviewing this one as well?

@RonnyPfannschmidt
Copy link
Member

at best by the weekend

@blueyed
Copy link
Contributor Author

blueyed commented Jul 17, 2020

Closing as per blueyed#365 (comment).

@blueyed blueyed closed this Jul 17, 2020
@blueyed blueyed deleted the fixture-stack branch July 17, 2020 20:07
@SalmonMode
Copy link
Contributor

As mentioned here, I'll be remaking this branch under my repo again to continue the work.

@blueyed
Copy link
Contributor Author

blueyed commented Jul 17, 2020

@SalmonMode Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants