TestCase.setUpClass is not called in some cases in pytest>=6.2.0

[Bobronium]

• a detailed description of the bug or problem you are having
• output of pip list from the virtual environment you are using
• pytest and operating system versions: pytest>=6.2.0,<=6.2.2
• minimal example if possible

python version: 3.8.7 (v3.8.7:6503f05dd5, Dec 21 2020, 12:45:15)  [Clang 6.0 (clang-600.0.57)]
platform: macOS-10.16-x86_64-i386-64bit

The problem

pytest doesn't call setUpClass, if test method has / in its name. Test method itself is executed.

In example below setUpClass is executed only after running first test method. If there wasn't another test method without / in its name, setUpClass wouldn't be called at all.

from unittest import TestCase

class BaseTest(TestCase):

    def __init_subclass__(cls, **kwargs):
        def test_method(self):
            self.assertEqual('foo', self.setup_class_attr)

        # / symbol on the next line causes the problem
        setattr(cls, "test_setup_class_attr['/foo']", test_method)
        setattr(cls, "test_setup_class_attr['foo']", test_method)

    @classmethod
    def setUpClass(cls):
        cls.setup_class_attr = 'foo'

class Test(BaseTest):
    ...

Running with unittests (gives expected results):

Ran 2 tests in 0.005s

OK

Running with pytest<6.2.0 (gives expected results):

$ pytest
====================================== test session starts ======================================
platform darwin -- Python 3.8.7, pytest-6.1.2, py-1.10.0, pluggy-0.13.1
rootdir: /Users/rocky/py/test
collected 2 items                                                                               

test_setup_cls.py ..                                                                      [100%]

======================================= 2 passed in 0.04s =======================================

Running with pytest >=6.2.0:

$ pytest
====================================== test session starts =======================================
platform darwin -- Python 3.8.7, pytest-6.2.2, py-1.10.0, pluggy-0.13.1
rootdir: /Users/rocky/py/test
collected 2 items                                                                                

test_setup_cls.py F.                                                                       [100%]

============================================ FAILURES ============================================
_______________________________ Test.test_setup_class_attr['/foo'] _______________________________

self = <test_setup_cls.Test testMethod=test_setup_class_attr['/foo']>

    def test_method(self):
>       self.assertEqual('foo', self.setup_class_attr)
E       AttributeError: 'Test' object has no attribute 'setup_class_attr'

test_setup_cls.py:7: AttributeError
==================================== short test summary info =====================================
FAILED test_setup_cls.py::Test::test_setup_class_attr['/foo'] - AttributeError: 'Test' object h...
================================== 1 failed, 1 passed in 0.20s ===================================

$ pip list          
Package    Version
---------- -------
attrs      20.3.0
iniconfig  1.1.1
packaging  20.9
pip        21.0.1
pluggy     0.13.1
py         1.10.0
pyparsing  2.4.7
pytest     6.2.2
setuptools 54.2.0
toml       0.10.2

[parthdpa]

Hello, I am new to contributing to open source projects and I am in my final year for my CS degree. Is it possible I can work on this issue?

[RonnyPfannschmidt]

while you certainly can work on it, currently its not clear whats the cause

[RonnyPfannschmidt]

#8033 is potentially related
#7990 and its precursor as well

[parthdpa]

I think #8157 may be related

[parthdpa]

Did someone find a cause for this bug?

[nicoddemus]

Managed to bisect to aa0e2d6:

aa0e2d654fb0c8ac18747fe1cdf54d8f29bcd24a is the first bad commit
commit aa0e2d654fb0c8ac18747fe1cdf54d8f29bcd24a
Author: Ran Benita <ran@unusedvar.com>
Date:   Sat Oct 24 02:09:28 2020 +0300

    fixtures: use a faster replacement for ischildnode

    ischildnode can be quite hot in some cases involving many fixtures.
    However it is always used in a way that the nodeid is constant and the
    baseid is iterated. So we can save work by pre-computing the parents of
    the nodeid and use a simple containment test.

    The `_getautousenames` function has the same stuff open-coded, so change
    it to use the new function as well.

 src/_pytest/fixtures.py | 11 +++-----
 src/_pytest/nodes.py    | 68 ++++++++++++++++++++++---------------------------
 testing/test_nodes.py   | 29 ++++++++++++---------
 3 files changed, 51 insertions(+), 57 deletions(-)
bisect run success

Which explains why the / in the name makes a difference now. 😅 cc @bluetech

[bluetech]

Method with / in the name, I guess in pytest you do see everything 😀

Thanks for the bisect @nicoddemus, I'll take a look soon.

[RonnyPfannschmidt]

@bluetech a possible initial quick-fix could be to split them out of parameter names (via '[')

[parthdpa]

@bluetech I was wondering if I can possibly contribute to this issue with guidance from you as a first time contributor as part of a final project for my class? Thank you

[bluetech]

@parthdpa Sure! I'll try to write some things to get you started.

Nodeids

The pytest collection phase creates a tree of nodes. If you have a test suite, you can see the tree by running pytest --collect-only.

Each node has a parent (exception Session, the root node) and nodeid. The nodeid of some test might look like this:

testing/code/test_excinfo.py::TestFormattedExcinfo::test_repr_source

This is the nodeid of test test_repr_source in test class TestFormattedExcinfo in file testing/code/test_excinfo.py.

The parent of this node is the node with nodeid

testing/code/test_excinfo.py::TestFormattedExcinfo

which represents the test class, whose parent in turn is testing/code/test_excinfo.py, whose parent is testing/code and so on, until we reach Session whose nodeid is the empty string "" (without the quotes).

Generally the path parts are separated by / and the further parts are separated by ::.

Nodeid parents

Some code wants to know whether one nodeid is the parent of another nodeid. For example, if testing/code defines an autouse=True fixture, then it should affect its children nodes, but not others.

Previously this was done with an ischildnode method, but this turned out to be slow in some cases, so commit aa0e2d6 changed it such that a iterparentnodeids function computes all of the parent nodeids of a given node, which is then faster to check for containment.

New code

The code for iterparentnodeids uses the following rule: "Note that :: parts are only considered at the last / component.". I.e.

testing/code/test_excinfo.py::TestFormattedExcinfo
            ^ last /

so in the case of

testing::foo/code/test_excinfo.py::TestFormattedExcinfo

The path part is still testing::foo/code/test_excinfo.py.

Problem

The problem is that it broke @MrMrRobat case:

x.py::Test::test_setup_class_attr[/foo]

The path part here is taken according to the rule above as x.py::Test::test_setup_class_attr[/ (up to the first /). But what was really intended is for the path part to be just x.py.

Solution

Well that's where you come in :)

Here are some possible solutions from the top of my head:

1. Keep the current behavior. This is necessary if we want to allow :: in paths.
2. Change the rule to / parts are only considered at the first :: component, i.e. give precedence to ::. This will break :: in paths which may be fine.
3. Use a rule which considers [] brackets in some special way, as @RonnyPfannschmidt suggests.
4. Use some dynamic rule which takes in consideration the actual existing nodes instead of just the static nodeid format.

Personally I wouldn't want to do 4, I think static meaning is very good to have.

I would try 2 and see if it breaks anything, maybe that would be enough.

[RonnyPfannschmidt]

Ultimately I would like to see something that collects this information in actual nodeid objects similar to path objects

The string parsing is a hack

[bluetech]

Ultimately I would like to see something that collects this information in actual nodeid objects similar to path objects

I think that would be good but I think it's mostly orthogonal to the issue here -- IMO we want nodeids (as plain strings -- what the user sees) to be unambiguous, and that means deciding how it's parsed exactly.

[parthdpa]

Thank you for the detailed explanation and information. I will attempt to implement changes suggested that will best fix this issue.

[parthdpa]

@bluetech or anyone who knows,
Is it possible to the path to ever end with something that is not ".py"? So is it possible for a path to ever be a file that doesn't have the python extension?

Edit:

Actually is it possible to have a path like this a/b/c::D/d::e from the test case where "::" comes between 2 "/"?

[parthdpa]

Hello @bluetech @RonnyPfannschmidt,

I may have fixed the bug, I am describing it here:

So when a / is found say:

testing/code/test_excinfo.py::TestFormattedExcinfo
            ^ last /

I return the path only if there is :: after or there are no :: before.
So in @MrMrRobat case:

x.py::Test::test_setup_class_attr[/foo]

now x.py::Test::test_setup_class_attr[/ is not returned incorrectly. All the existing tests still pass and fixes the issue. Is this good or are there cases I am not considering, please let me know. I will be opening a pull request for this fix soon.