Support usage of other fixtures in xunit style methods: setup() and setup_class()

[rsyring]

I've been following #517 for some time and I've recently looked at #4091 in the hope that it would help me figure out how I can get better support for xunit style class fixtures. Here is what I want to be able to do:

@pytest.fixture(scope="session")
def db():
    return database.Database()

class TestIt(object):
    def setup_class(cls, db):
        # This method fails with: TypeError: setup_class() takes exactly 2 arguments (1 given)
        cls.foo = db.build_expensive_foo()

    def setup(self, db):
        # fails with same error as setup_class
        cls.bar = db.build_expensive_bar()

    @pytest.fixture(autouse=True, scope='class')
    def prep_class(self, db):
       # db is available here, but self is an instance, not the class itself, so this
       # doesn't work as intended.  I could use self.__class__, but feel like I
       # shouldn't need to.
        print('prepare', self, db)
        self.foo = 'bar'

    def test_foo_bar(self):
        assert self.foo
        assert self.bar

I seem to have two, maybe distinct, problems:

1. Despite the work done in Use fixtures to invoke xunit-style fixtures #4091, setup_class and setup do not seem to have access to fixtures (db in this example). I'm not sure if that is intended, an oversight, but the above "feels" like it should work to me.

2. Using an autouse fixture at the class level gives access to the db fixture but the fact that an instance instead of the class is being passed in makes it non-intuitive on how to store state at the class level.

Suggestion

I'd really like to see setup_class() and setup() get support for using fixtures defined elsewhere. We have a lot of tests that use this pattern to cache test data at the class level and it would be helpful to give those methods access to other fixtures.

I understand from the discussions in the issues and PR that pytest is trying to keep a distinction between xunit style and newer fixture style tests, but I'm not sure why that needs to be the case. Unless there is a technical limitation that makes this hard to support, the UX described above feels well balanced IMO and would be ideal for our code bases that are heavily using setup_class and setup extensively.

Being able to use fixtures as proposed also allows a more gradual transition from one style to another.

2. Maybe consider passing the class object instead of the instance object to class level methods using a fixture

Version Info

Using Arch Linux. Versions:

(pytest-xunit)rsyring@nanite:~/tmp$ pytest --version
This is pytest version 4.5.0, imported from /home/rsyring/tmp/virtualenvs/pytest-xunit/lib/python3.7/site-packages/pytest.py

(pytest-xunit)rsyring@nanite:~/tmp$ pip freeze
atomicwrites==1.3.0
attrs==19.1.0
more-itertools==7.0.0
pluggy==0.11.0
py==1.8.0
pytest==4.5.0
six==1.12.0
wcwidth==0.1.7

(pytest-xunit)rsyring@nanite:~/tmp$ python --version
Python 3.7.3

Edit: tested with clean virtualenv and Python 3

[gryznar]

I'll bump the topic. It would be great if fixtures were supported in setup_class, setup_method, teardown_class and teardown_method.

[RonnyPfannschmidt]

in "xunit" style one can jsut declare those a fixture and it works, fixtures will not be supported as args in normal xunit style methods

[rsyring]

in "xunit" style one can jsut declare those a fixture and it works,

As I stated in the initial description, this becomes problematic for setup_class and autouse:

Using an autouse fixture at the class level gives access to the db fixture but the fact that an instance instead of the class is being passed in makes it non-intuitive on how to store state at the class level.

To be specific, if one uses def setup_class(cls, ...) in that method cls is the class itself. When making it a fixture and using autouse, cls is instead an instance of cls, not cls itself. It can be worked around, as stated in my original post, but I think the DX on that is problematic.

fixtures will not be supported as args in normal xunit style methods

Why? Is this just a time issue, i.e. not a priority for the maintainers? Or, is there some technical reason why this would create a lot of additional complexity? FWIW, I personally find the need to add @fixture everywhere a bit busy. I prefer xunit style and try to avoid @fixture if possible. I'd even do the work on this if someone could point me in the right direction. I just don't want to waste my time if it's not going to get merged due to philosophical differences.

[bluetech]

@rsyring Does

    @pytest.fixture(autouse=True, scope='class')
    @classmethod
    def prep_class(cls, db):

not work?

[nicoddemus]

Or, is there some technical reason why this would create a lot of additional complexity?

Mostly this: we run the xunit tests using their own TestCase execution (and TestResult), which don't support parameters/fixtures. I suspect we would need to duplicate some work, increasing the plugin's complexity, with the risk of introducing bugs.

[gryznar]

Moreover using xunit style is much simplier and much more explicit than defining it via fixtures. So support for fixtures in it is very reasonable.

[gryznar]

Or, is there some technical reason why this would create a lot of additional complexity?

Mostly this: we run the xunit tests using their own TestCase execution (and TestResult), which don't support parameters/fixtures. I suspect we would need to duplicate some work, increasing the plugin's complexity, with the risk of introducing bugs.

How about rewritting it and use fixture with autouse and proper scope instead? So for example setup_class will be a fixture with scope 'class' and autouse set to True beneath, forced to run in proper order.

[rsyring]

@bluetech I can't check now, but I will in a bit. It could be that it didn't work b/c I wasn't including @classmethod. I think I cobbled the example together from actual tests, so not sure if that is a mistake in the example or an actual mistake in my testing of this issue.

Mostly this: we run the xunit tests using their own TestCase execution (and TestResult), which don't support parameters/fixtures. I suspect we would need to duplicate some work, increasing the plugin's complexity, with the risk of introducing bugs.

I can appreciate this. If it's just a matter of extracting the shared functionality and using it in both places, and the tests of this functionality are good, is it really that big of a concern? I don't want to downplay the complexity of the work if it is indeed complex, but I don't want to give up on this if it's just a timing/capacity issue as I'm willing to put in the time to do the work and test thoroughly. Are we really talking about unreasonable risk to pytest's stability at this point? If so, so be it. If not, then I'm motivated to get this resolved.

I realize as a maintainer you are not just approving the PR but "owning" the future of that functionality as well. I've been in the Python ecosystem for the last 15 years and I plan to be around in the future just as long. I'm willing to "own" this functionality for the foreseeable future if issues arise that need to be resolved because of it. I don't do a lot of work in visibly public repos, but I did help get Flask-SQLAlchemy unstuck a couple years go. My code is solid and I'm reasonable to work with. :)

Moreover using xunit style is much simpler and much more explicit than defining it via fixtures.

I agree with this from a DX perspective. Having to make those methods fixtures, just to get access to other fixtures, when I'm never going to use them as fixtures themselves feels like a...cludge.

[nicoddemus]

Are we really talking about unreasonable risk to pytest's stability at this point? If so, so be it. If not, then I'm motivated to get this resolved.

I'm sure given the time and effort, this could be made to work, and depending on how it is implemented, it might be low risk for the rest of pytest's functionality.

I realize as a maintainer you are not just approving the PR but "owning" the future of that functionality as well. I've been in the Python ecosystem for the last 15 years and I plan to be around in the future just as long. I'm willing to "own" this functionality for the foreseeable future if issues arise that need to be resolved because of it.

I appreciate that. This makes me much more willing to accept this kind of change.

Given this perspective, I'm personally OK with accepting that change, but not sure if other maintainers feel the same.

Also note that even if we are not willing to add this to the core, I'm pretty sure it is possible to implement this as an external plugin, if you would like to experiment with that first and see how it fares in the wild.

[RonnyPfannschmidt]

we have 2 compatibility concerns to consider

1. some those methods have optional parameters and multiple potential names
2. structurally all we would do is consider them "fixtures"

based on that im inclined to prefer just marking/declaring them as fixture instead of making the hack even more magical

however if the hack could actually be simplified as a outcome of this, then im happy to +1 it

personally i treat xunit style setup methods as a legacy anti-patter that prevents both good naming of resources one needs and good integration

[gryznar]

@RonnyPfannschmidt if we could treat

setup_class like:

@classmethod
@pytest.fixture(scope='class', autouse=True)
def _setup_class(cls, *args):
    ....

teardown_class like:

@classmethod
@pytest.fixture(scope='class', autouse=True)
def _teardown_class(cls, *args):
    yield
    ....

setup_method like:

@pytest.fixture(scope='function', autouse=True)
def _setup_method(self, method=None, *args):
    ....

and teardown_method like:

@pytest.fixture(scope='function', autouse=True)
def _teardown_method(self, method=None, *args):
    yield
    ...

(and so on for module and function scope), where are args are possible fixtures and force them to run in order in which current xunit style setup currently runs, so all the compatibility issues could be solved and you could get rid off custom implementation. Am I right?

Finally xunit style fixtures could be just a reference to normal fixtures, but written much simpler.

[RonnyPfannschmidt]

@gryznar in the current implementation we only add those fixtures when needed for xunit setup (if we had them for everything, it would be too expensive and slow down non-xunit tests, alternative approaches to remove the cost have not yet been investigated )

additionally we have a indirection in there to deal with teardown and with the fact that xunit setup/teardown methods have optional parameters that are NOT like the request for fixtures

[gryznar]

@RonnyPfannschmidt I see, but you can agree with me for sure, that they are much more readable and obvious for end user, so maybe we can really consider upgrading / refactioring / rewriting them?