ParallelTestSuite reports error occurred in arbitrary test instead of setUpClass

[David Winiecki]

When running tests in parallel, if an error occurs in a test suite (TestCase) before the first test method runs, for example in setUpClass, then the shell output incorrectly states that the error occurred in an arbitrary test method, rather than, for example, setUpClass.

This is a problem because a user reading the test output may not notice the real error location (for example, setUpClass) in the traceback and may not run the other tests in the test suite.

This is especially problematic if automation parses the error location to automatically re-run tests (to address flaky, intermittently failing tests), because the other tests in the suite may never be run.

Repro:

1. Create a Django project for testing:

Instructions to create the project yourself are below. Or you can use this existing project in the bugdemo/ directory at ​https://github.com/dcki/django/blob/demo_report_test_error_bug/bugdemo/README.md

To create the project yourself:

1. Install Python.
2. Create a virtualenv and activate it: mkdir my_project && cd my_project && python3 -m venv .venv && . .venv/bin/activate
3. Install Django and create a Django project: pip install django && django-admin startproject mysite .
4. Create a tests directory and an __init__.py file inside: mkdir mysite/tests && touch mysite/tests/__init__.py
5. Install tblib: pip install tblib
6. Add these tests to the project:

mysite/tests/test_things.py

from django.test import SimpleTestCase


class AlwaysFailTest(SimpleTestCase):
    @classmethod
    def setUpClass(cls) -> None:
        super().setUpClass()
        try:
            raise Exception('Intentional error')
        except:
            super().tearDownClass()
            raise

    def test_should_pass_a(self) -> None:
        pass

    def test_should_pass_b(self) -> None:
        pass


# Exists so that, when an attempt is made to run tests in parallel, and at
# least two TestCases are specified, then tests actually run in parallel.
# (As of this writing, Django runs tests serially if there is only one test
# suite to run, even if `--parallel=2` is specified.)
class AlwaysPassTest(SimpleTestCase):
    def test_should_pass_a(self) -> None:
        pass

    def test_should_pass_b(self) -> None:
        pass

2. Run the tests: python3 manage.py test --parallel=2

Expected output:

Found 4 test(s).
System check identified no issues (0 silenced).
E..
======================================================================
ERROR: setUpClass (mysite.tests.test_things.AlwaysFailTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/dwiniecki/.pyenv/versions/3.11.9/lib/python3.11/unittest/suite.py", line 166, in _handleClassSetUp
    setUpClass()
  File "/Users/dwiniecki/src/other/dcki-django/bugdemo2/mysite/tests/test_things.py", line 9, in setUpClass
    raise Exception('Intentional error')
    ^^^^^^^^^^^^^^^^^
Exception: Intentional error

----------------------------------------------------------------------
Ran 2 tests in 0.214s

FAILED (errors=1)

Actual output:

Found 4 test(s).
System check identified no issues (0 silenced).
E..
======================================================================
ERROR: test_should_pass_b (mysite.tests.test_things.AlwaysFailTest.test_should_pass_b)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/dwiniecki/.pyenv/versions/3.11.9/lib/python3.11/unittest/suite.py", line 166, in _handleClassSetUp
    setUpClass()
  File "/Users/dwiniecki/src/other/dcki-django/bugdemo/mysite/tests/test_things.py", line 9, in setUpClass
    raise Exception('Intentional error')
    ^^^^^^^^^^^^^^^^^
Exception: Intentional error

----------------------------------------------------------------------
Ran 2 tests in 0.221s

FAILED (errors=1)

[David Winiecki]

I wrote a proposed fix and regression test for this bug in a fork of the django GitHub project:

​https://github.com/dcki/django/tree/ticket_35849

I want to help fix this but I'm not certain what the next step is. I read all the contributing documentation I could find.

tox passes. (Except tox -e docs due to a problem with pyenchant on my specific computer, but pip install pyenchant==3.3.0rc1 && cd docs && make spelling passes.)

[David Winiecki]

Some of this work was done personally and some of it was done for my employer ​https://www.techsmart.codes/

We don't believe we've submitted a "Corporate Contributor License Agreement" and I haven't submitted an "Individual Contributor License Agreement". Another team member at TechSmart intends to submit a CCLA in a few days or so.

[David Winiecki]

I think this bug has existed since ParallelTestSuite was first written. I've confirmed that it exists at least as far back as 4.2. (But personally I don't feel like it's important enough to backport to earlier versions of Django. EDIT: Earlier than dev or 5.1 for example, that is.)

As far as I've seen there is no related documentation to update.

[David Winiecki]

I'm tempted to check the "has patch" box but I'm not sure if it's too early to do that. :)

[Natalia Bidart]

Hello David, thank you for your detailed report. I have reproduced the issue as you indicated.

I find the distinction between this being an issue or not somewhat ambiguous. In my view, the first test run "is" the one that fails... but what leads me to accept this ticket is the fact that the error reporting differs depending on whether parallel=1 is set or not.

I will include a few folks in the ticket to see what they recommend, I took an initial look at your PR but I think we need more expert eyes on it.

[David Winiecki]

Thank you!

[David Winiecki]

I created [a pull request from the fork to itself (not to upstream)](​https://github.com/dcki/django/pull/1), just to have a place for discussion in the context of code. If this is not useful then I'll close the pull request. I may delete this fork later so if having historical context is important, then maybe using this pull request is not a good idea.

My impression is that I shouldn't create a PR to upstream until the proposed git branch is almost ready to merge, but maybe that's not true.

[Jacob Walls]

It makes sense to me to clean up the discrepancy between parallel=1 and parallel=n, if we can do it easily enough. The single process runner uses TextTestRunner, but the parallel one does not, so it makes sense that it's "on us" to mimic what TextTestRunner does to get consistent test labels in this edge case. We might want to ensure unexpected success is covered just like test errors.

My impression is that I shouldn't create a PR to upstream until the proposed git branch is almost ready to merge, but maybe that's not true.

Appreciate you being careful, but I think it's totally appropriate to open a PR against upstream once the ticket's been accepted.

[David Winiecki]

Okay awesome! I opened a real PR to upstream: ​https://github.com/django/django/pull/18695

We might want to ensure unexpected success is covered just like test errors.

I think I have an idea what you mean. If you have a chance to elaborate please do. I'll try to do more here soon.

Thank you all!

[Jacob Walls]

Sure, I just was musing that since unexpected successes are a flavor of error, we will want to be sure that their reporting doesn't change--since we still want failures to be labeled with the test name, not anything else--but that seems unlikely to have changed by your patch, so no worries.

[David Winiecki]

I filled in the "branch description" section in the PR description.

but that seems unlikely to have changed by your patch

I think I understand. I'll try to learn more soon about unexpected successes and how they might apply to this.

[David Winiecki]

Regarding unexpected successes: It looks to me like unittest.TestCase.run() is the only thing that calls result.addUnexpectedSuccess(), and when I read the definition of run(), it looks to me like it should not interact with the change proposed in the PR.

[David Winiecki]

I'm not aware of any open questions currently, so I think it's ready for review when someone has time.

[Natalia Bidart]

Thank you David, a reviewer will review it soon, but please note that Django moves at a slow pace, so it may be weeks.

[David Winiecki]

Thank you Natalia, sounds good.

[David Winiecki]

I think I responded to all current comments in the ​pull request and applied changes where decisions were made, so I don't have other changes pending currently. I can make more changes when we make more decisions.

I'm unchecking "patch needs improvement" for now. Please let me know if I shouldn't do that.

Thank you!

[Sarah Boyce <42296566+sarahboyce@…>]

In 41da8a4:

Refs #35849 -- Added a handle_event hook to ParallelTestSuite.

[Sarah Boyce <42296566+sarahboyce@…>]

In 661dfdd:

Fixed #35849 -- Made ParallelTestSuite report correct error location.

[David Winiecki]

Thank you!

[David Winiecki]

My manager told me that he submitted a "Corporate Contributor License Agreement" Monday November 4th.

[Sarah Boyce <42296566+sarahboyce@…>]

In 398cec4:

Refs #35849 -- Skipped ParallelTestSuiteTest.test_handle_add_error_before_first_test() without tblib.

Follow up to 661dfdd59809f4abd5077f7a2529735d07b98ba4.