Regression in testdir fixture

[Dreamsorcerer]

You can see further details in our Dependabot PR: https://github.com/aio-libs/aiohttp/pull/8704

Basically, upgrading from 8.1.1 seems to have caused one of our tests to fail (I think there was another regression in 8.2 which has been fixed, so not sure if this was introduced in 8.2 or 8.3). The code is:

def test_testcase_no_app(
    testdir: pytest.Testdir, loop: asyncio.AbstractEventLoop
) -> None:
    testdir.makepyfile(
        """
        from aiohttp.test_utils import AioHTTPTestCase


        class InvalidTestCase(AioHTTPTestCase):
            def test_noop(self) -> None:
                pass
        """
    )
    result = testdir.runpytest()
    result.stdout.fnmatch_lines(["*TypeError*"])

The logged output from pytest says that it's collected 0 items. My first guess is that the default filename may have changed, thus causing pytest to not find any test files? According to the documentation, I believe the default filename should be test_testcase_no_app.py.

[RonnyPfannschmidt]

For quick verification simply add a top level test function and see if that gets collected

I recall a change in unittest collection that might be a offender

I'm unable to verify this week

[Dreamsorcerer]

I just tested it locally, and it does seem to collect a test on my computer, just to complicate things...

[RonnyPfannschmidt]

https://github.com/pytest-dev/pytest/blob/main/src%2F_pytest%2Funittest.py#L65 may be related, your test is abstract,its missing get application

We should indicate thisbetter

[Dreamsorcerer]

OK, I assume that's what the test is actually trying to test, which is a bit awkward...

[RonnyPfannschmidt]

The ignore for abstract got added due to a change in instantiation position

I believe the test tries to check for the type error on that

[RonnyPfannschmidt]

@bluetech @nicoddemus I believe we should report a skip there, or trigger some kind of warning

[Dreamsorcerer]

Maybe this is why it collects when I run locally, but it doesn't report as abstract when I run on my computer:

>>> from aiohttp.test_utils import AioHTTPTestCase
>>> class InvalidTestCase(AioHTTPTestCase):
...     def test_noop(self) -> None:
...         pass
... 
>>> InvalidTestCase()
<__main__.InvalidTestCase testMethod=runTest>
>>> import inspect
>>> inspect.isabstract(InvalidTestCase)
False

But, I did test adding a top-level test as you suggested and it did find that test, so I think you must still be right..

[Dreamsorcerer]

Forget that, I was just looking at an old version by mistake. It is definitely abstract.

[bluetech]

I believe we should report a skip there, or trigger some kind of warning

TLDR: +1 on a warning, but for unittest only.

Looking at unittest, it doesn't have any handling of ABC so it just tries to execute the test and fails because it can't be instantiated. This is compared to pytest which silently ignores it.

I can think of 3 scenarios of abstract unittest TestCases:

1. Abstract base unittest.TestCase defined in non-test file. Example: AioHTTPTestCase.
2. Abstract base unittest.TestCase provided by test file. Example: user defines a base class for convenience in a specific test file for other tests in that file.
3. Accidentally abstract unittest.TestCase. Example: the InvalidTestCase from the issue.

I don't think we should use a skip -- it is either intentional (2) or accidental (3) and a skip is not appropriate for either. So let's consider a warning.

A warning would not trigger for 1 (good), will trigger for 3 (good), and will trigger for 2. Arguably it's good that it triggers for 2, since as mentioned above unittest would error on it. But it currently works in pytest. If a warning is triggered, the user needs to fix in one of two ways:

• Move the base class to a non-test_*.py file and import it.
• Remove unittest.TestCase from the base class, instead add it to each concrete sub-class.

Both are kind of annoying. But we're talking about compat with unittest, so I guess it's alright by me.

Just to be clear, this analysis is only about unittest, not about non-unittest pytest test classes. For pytest classes I think silently ignoring is OK.

[RonnyPfannschmidt]

We should report a skip if the abc is defined and not used, that May be more trouble than worth

[bluetech]

We should report a skip if the abc is defined and not used, that May be more trouble than worth

Right I don't think this is realistic, it would have to be done as a phase after all files have been collected which goes and double checks all ABCs.