pytest.mark in classes marks methods defined in the class body only

[nicoddemus]

Fix #725

[nicoddemus]

Hmm didn't realize it but this change is in direct conflict with the use case for #265:

The test which fails is this:

import pytest

class TestGeneric(object):
    def test_nothing(self):
        """Tests the API of the implementation (for generic and specialized)."""

@pytest.mark.skipif("True", reason="Skip tests to check if teardown is skipped as well.")
class TestSkipTeardown(TestGeneric):

    def setup(self):
        """Sets up my specialized implementation for $COOL_PLATFORM."""
        raise Exception("should not call setup for skipped tests")

    def teardown(self):
        """Undoes the setup."""
        raise Exception("should not call teardown for skipped tests")

Because this PR precisely changes the semantics of the markers applied to classes using decorators, test_nothing now gets executed normally.

A workaround would be:

class TestSkipTeardown(TestGeneric):

    def setup(self):
        """Sets up my specialized implementation for $COOL_PLATFORM."""
        if feature_not_available('$COOL_PLATFORM'):
            pytest.skip('not available')
        raise Exception("should not call setup for skipped tests")

I feel the behavior implemented in this PR is less surprising as #725 demonstrates, but it will certainly break test suites which rely on the previous behavior.

@hpk42 and @RonnyPfannschmidt, any thoughts?

[nicoddemus]

Btw, I just realized the original poster's example doesn't make sense, because if a subclass decided to skip all its tests because $COOL_PLATFORM is not available, then all tests for all other subclasses would be skipped as well.

[RonnyPfannschmidt]

the fix makes the breakage worse

the problem is that markers are applied to a function object instead of being collected on the item

its absolutely correct that class scoped markers of a subclass get applied to inherited methods
however its absolutely incorrect that those get applied to the method thats located on the base class

[nicoddemus]

its absolutely correct that class scoped markers of a subclass get applied to inherited methods
however its absolutely incorrect that those get applied to the method thats located on the base class

Hmm but that's what the PR does, it only applies the markers to the functions defined in the class dict (which does not take in account superclasses).

    # transfer markers from class decorators only if the method is defined
    # in the class itself
    if cls is not None and funcobj.__name__ in cls.__dict__:
        transfer(cls)

Or am I missing something?

[RonnyPfannschmidt]

Inherited methods are collected on subclasses, those items must receive the markers even tho they are defined in subclasses

[RonnyPfannschmidt]

perhaps we could put a descriptor into the subclasses that will carry along the mark decorators and help invoking the original method

[nicoddemus]

I'm not sure I understand your points... 😅

To clarify things, let's consider:

class A:
    def test_A(): pass

@pytest.mark.M
class B(A):
    # inherits A.test_A
    def test_B1(): pass
    def test_B2(): pass

class C(B):
    # inherits A.test_A
    def test_B1(): pass # overrides B.test_B1
    # inherits B.test_B2

Current behavior

The following functions end up having M applied to them:

A
  .test_A

B
  .test_A
  .test_B1
  .test_B2

C
  .test_A
  .test_B2

So class markers apply the marks to all the functions found in a class during class creation, including functions defined in upperclasses.

Behavior implemented in this PR

The following functions end up having M applied to them:

B
  .test_B1
  .test_B2

C
  .test_B2

So class markers apply marks to all functions found in the class body at the time of class creation only. Subclasses which don't explicitly override functions will inherit the same markers (that's why C.test_B1 doesn't have a mark, it is overwritten in C).

I find the behavior implemented in this PR much less surprising, as it follows usual python inheritance rules. Nobody expects that a change in a function in a subclass to affect functions in the upperclass, but it's natural for changes in superclasses to affect subclasses, don't you agree?

Or do you have something else in mind, like review how markers in general are handled? If that's the case, either way IMO the new behavior is less surprising anyway and a major overhaul of how makers are handled are the scope of another PR.

[RonnyPfannschmidt]

expected behavior should be:

B
  .test_A
  .test_B1
  .test_B2

C
  .test_A
  .test_B1
  .test_B2

[nicoddemus]

I see what you mean, thanks.

Do you think this should be tackled in this PR, or should be a separate task? I prefer the former, because I think as it is the PR is an improvement already.

[RonnyPfannschmidt]

i think it should happen in this pr, however the current state of this pr is bound to break things for users

[nicoddemus]

As discussed, a better solution is proposed in #891.