Fix issue where fixtures would lose the decorated functionality

[nicoddemus]

Fix #3774

[coveralls]

Coverage increased (+0.06%) to 92.527% when pulling 67106f0 on nicoddemus:mock-integration-fix into 5d3c512 on pytest-dev:master.

[RonnyPfannschmidt]

based on the still pending order based incorrectness we should after merging this plan for a rather soonish breaking release where we make fixture definitions truly un-callable

[nicoddemus]

I don't know how we can make fixtures definitions un-callable, because we will hit the exact same problem we have today with issuing the warning: we still need to wrap the function to raise an error instead of a warning, which will bring the same class of problems. 😕

[RonnyPfannschmidt]

@nicoddemus by returning a non-callable object and by raising hard errors if a fixture definition is wrapped with anything else

[nicoddemus]

@nicoddemus by returning a non-callable object and by raising hard errors if a fixture definition is wrapped with anything else

Not sure I follow, won't that bring the exact same problems that we are facing now? I mean, if @pytest.fixture returns a non-callable object, the following will blow up:

@classmethod
@pytest.fixture
def setup(cls): ... 

Because @classmethod will be applied to the object, not to setup.

[RonnyPfannschmidt]

@nicoddemus as far as i can tell its a unreasonable amount of work to "correctly combine the wrappers before and after a fuxture definition markers, with a gernally tremendous chance to be incorrect
so the goal for the breakage is to make any decoration applied to the marker incorrect- the class-method on a fixture definition should blow up

[nicoddemus]

Hi @RonnyPfannschmidt sorry for the delay on this.

@nicoddemus as far as i can tell its a unreasonable amount of work to "correctly combine the wrappers before and after a fuxture definition markers, with a gernally tremendous chance to be incorrect

I agree it seems hard to implement.

so the goal for the breakage is to make any decoration applied to the marker incorrect- the class-method on a fixture definition should blow up

But how would that work for class-scoped fixtures? They by definition need to be @classmethods, no? At least that's how it always worked so far.

import pytest

class Test:

    #@classmethod
    @pytest.fixture(scope='class', autouse=True)
    def setup(self):
        self.x = []

    def test1(self):
        assert self.x == []
        self.x.append(1)

    def test2(self):
        assert self.x == [1]
        self.x.append(2)

    def test3(self):
        assert self.x == [1, 2]

Without @classmethod this never worked as far as I can see, even in previous versions.

[nicoddemus]

Unless you mean that we should apply the @classmethod decorator ourselves when applying @pytest.fixture(scope="class")?

[RonnyPfannschmidt]

applying class-method should be done in the correct order - aka

import pytest

class Test:

   
    @pytest.fixture(scope='class', autouse=True)
    @classmethod
    def setup(cls):
        cls.x = []

    def test1(self):
        assert self.x == []
        self.x.append(1)

    def test2(self):
        assert self.x == [1]
        self.x.append(2)

    def test3(self):
        assert self.x == [1, 2]

we should take a look at warning if lassmethod is not applied
we shouldn't apply ourselfes

[nicoddemus]

applying class-method should be done in the correct order - aka

Oh I see. We should then advertise this more as the correct way to do the decoration.

Also I wonder if we might conflict with some other library which also has special requirements? But then again, decorators around fixtures are somewhat rare, and few libraries will return a non-callable object, so I might be overthinking here. 😛

we should take a look at warning if lassmethod is not applied
we shouldn't apply ourselfes

Sounds fair, but do you see a possible problem with us applying it ourselves? I'm not suggesting we do, I think the warning or even an error is a good solution, just curious if you are already foreseeing problems with pytest applying the @classmethod decorator automatically.

[RonnyPfannschmidt]

@nicoddemus its already tricky as is - it would be even worse with multi scoped-fixtures should they ever land - in addition class scope is possibly declared outside - so from my pov there is a massive amount of special cases for next to no gain - warning people to make this right is a lot more easy than correctly setting up things

[nicoddemus]

it would be even worse with multi scoped-fixtures should they ever land

Oh yeah!

in addition class scope is possibly declared outside - so from my pov there is a massive amount of special cases for next to no gain - warning people to make this right is a lot more easy than correctly setting up things

Sure sounds reasonable. Thanks for the patience to discuss all the points. 👍

Should we merge this then? It seems like it is an improvement regardless.

[RonnyPfannschmidt]

overall i want to see this merged because it makes things better

[RonnyPfannschmidt]

i just noticed this special case - we shouldn't have it - because in practice this means we blow up on evil objects there

[nicoddemus]

Good catch. Used a custom holder class so we can be sure we are unwrapping an object set by us.