Skip to content

Comments

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

Closed
blueyed wants to merge 7 commits intopytest-dev:masterfrom
blueyed:fixture-stack
Closed

[WIP/RFC] fixtures: register finalizers with all previous fixtures in the stack (take 2)#6551
blueyed wants to merge 7 commits intopytest-dev:masterfrom
blueyed:fixture-stack

Conversation

@blueyed
Copy link
Contributor

@blueyed blueyed commented Jan 23, 2020 •
edited
Loading

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
TEST: blueyed/pytest@fixture-stack
f0122e6 
Ref: pytest-dev/pytest#6551
This was referenced Jan 23, 2020
Closed
Closed
@SalmonMode SalmonMode mentioned this pull request Jan 23, 2020
Open
@blueyed blueyed mentioned this pull request Jan 24, 2020
Open
@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 blueyed mentioned this pull request Jan 24, 2020
Merged
blueyed added a commit to blueyed/pytest that referenced this pull request Jan 24, 2020
@blueyed
tests: add test_fixture_arg_ordering
22a3b14 
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

SalmonMode commented Jan 24, 2020

@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
@blueyed @nicoddemus
tests: add test_fixture_arg_ordering
192d3ad 
This is a regression test for part of
pytest-dev#6492, testing one of the
fixes in pytest-dev#6551.
@SalmonMode
Copy link
Contributor

SalmonMode commented Jan 24, 2020

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

Sign in to view
@blueyed

This comment has been minimized.

Sign in to view
@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 •
edited
Loading

@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
@SalmonMode @blueyed
fixtures register finalizers with all fixtures before them in the stack
163b03c
@blueyed
_dependee_fixture_argnames: return tuple
b84d70b 
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.
@SalmonMode @blueyed
avoid exponential recursion by checking if already added
cfee6e7
@SalmonMode @blueyed
cleaned up, DAMPified, and added tests to ensure fixtures only add th…
a950653 
…er finalizer to a dependee fixture once
@SalmonMode @blueyed
add type annotations
b1e1b73
@blueyed
fixup! cleaned up, DAMPified, and added tests to ensure fixtures only…
4e07783 
… add ther finalizer to a dependee fixture once
blueyed added a commit to blueyed/pytest-django that referenced this pull request Mar 31, 2020
@blueyed
TEST: blueyed/pytest@fixture-stack
0803b92 
Ref: pytest-dev/pytest#6551
@SalmonMode
Copy link
Contributor

SalmonMode commented Apr 11, 2020 •
edited
Loading

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 SalmonMode mentioned this pull request Apr 11, 2020
Open
@SalmonMode
Copy link
Contributor

SalmonMode commented May 12, 2020

@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

SalmonMode commented May 17, 2020

@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 •
edited
Loading

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

SalmonMode commented May 17, 2020

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

@SalmonMode SalmonMode mentioned this pull request May 17, 2020
Closed
@nicoddemus
Copy link
Member

nicoddemus commented May 18, 2020

@RonnyPfannschmidt could you help reviewing this one as well?

@RonnyPfannschmidt
Copy link
Member

RonnyPfannschmidt commented May 18, 2020

at best by the weekend

@blueyed blueyed mentioned this pull request Jun 30, 2020
Closed
@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

SalmonMode commented Jul 17, 2020

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!

@SalmonMode SalmonMode mentioned this pull request Jul 17, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@blueyed @SalmonMode @timc13 @SHxKM @nicoddemus @RonnyPfannschmidt