Open bluetech opened 1 year ago
Hi @bluetech,
Thanks for writing this up.
I agree option 2 is more natural and makes more sense: I expect that a class-scoped fixture to have a class-scope, regardless if it is nested or not, while also expecting it to match the collection tree.
I think this is even more obvious if we expand the example a bit to:
import pytest
@pytest.fixture(scope="class")
def fix(request):
print("SETUP", request.node)
yield
print("TEARDOWN", request.node)
class TestTop:
def test_top(self, fix):
print("top")
class TestNested1:
def test_nested1(self, fix):
print("nested1")
class TestNested2:
def test_nested2(self, fix):
print("nested2")
def test_bottom(self, fix):
print("bottom")
I would expect this to agree with option 2, with this output:
SETUP <Class TestTop>
top
SETUP <Class TestNested1>
nested1
TEARDOWN <Class TestNested1>
SETUP <Class TestNested2>
nested2
TEARDOWN <Class TestNested2>
bottom
TEARDOWN <Class TestTop>
So 👍 from me for option 2.
I also like option 2
I do see some pain points wrt fixture definition place vs scopes in practice
It may be necessary to help fixture definitions to pick the scope/nesting in more detail
I Hope I can find some time later for a example
Hi,
I am interested in taking on this issue, because I have a class where my team is tasked to contribute to an open source project.
I wanted to ask if there is any advice on where to look/ how my team can start taking on this issue.
Thanks!
We have two collection nodes (and fixture scopes),
Package
andClass
, which may be nested. I'll use classes for example because it's easier to discuss (single file) but it's the same for packages.Consider the following:
The collection tree is:
What should be printed?
(I'm adding some indentation to the outputs to make them clearer).
Option 1 - Current behavior
The current behavior is the following.
Technical details why it happens
Each fixture definition is pytest becomes a `FixtureDef` instance. In the example we have one `FixtureDef`, for `fix`. In the current implementation, the `FixtureDef` itself holds a fixture's value (`cached_value`). When a fixture value is requested (`SubRequest.getfixturevalue()`), if the `FixtureDef`'s `cached_value` is not already set, it calculates it by running the fixture function. Then it schedules a finalizer on the fixture request's node (`request.node`) to clear the `cached_value`. In the example: - `test_top` requests `fix` for the first time, which runs the fixture and schedules to clear `fix`'s `cached_result` when `TestTop` is torn down. - `test_nested1` requests `fix`. At this point the `fix` `FixtureDef` has `cached_value` set (since `TestTop`'s finalizer hasn't run yet), so it just uses it without rerunning the fixture. Then it schedules a finalizer to clear `fix` when `TestNested1` is torn down. - `TestNested1` is torn down which clears `fix`. - `test_nested2` requests `fix`. The `cached_value` is cleared so it reruns the fixture. - `TestNested2` is torn down, clears `fix`. - `TestTop` is torn down, wants to clear `cached_value`, but it is already cleared, goes :shrug: and continues.Option 2 - Nested scope
Nested classes/packages get their own scope, nested within the parent class/package scope.
Option 3 - Independent scope
Nested classes/packages get their own scope, independent of the parent class/package.
Option 4 - Shared scope
Nested classes do not have a scope of their own; same scope as the top class/package.
My opinion
Option 1 seems broken to me, I don't think there's any case to be made for it.
Option 2 is my preference, I think it's the most coherent with the collection tree, lexical/filesystem layout and is pretty intuitive, for both packages and classes.
Option 3 I think is not intuitive, i.e. I think most would not expect fixtures to behave this way. It imposes some restrictions on tests ordering between the class and nested. Note that currently due to #7777, Packages work like this in practice.
Option 4 would be my second choice, it does make sense logically (that what if there is no
test_top
?), but I think it's less intuitive than Option 2.I can write more but don't want to make this too long.