TestResult.stopTest called when test is skipped, despite TestResult.startTest not being called

[yvan-the-barbarian]

Bug report

Bug description:

Following commit 551aa6ab9419109a80ad53900ad930e9b7f2e40d a bug seems to have been introduced in the TestCase.run method.

Because the result.startTest(self) call was moved inside the try/finally block after the check to see if the test is skipped, the result.stopTest(self) call will be made even if the test is skipped and startTest is never called.

While this does not cause issues with the standard TestResult or TextTestResult classes, it can cause issues with other classes which subclass those and expect every call to stopTest to be preceded by a call from startTest.

Most notably for me, the unittest-xml-reporting package, whose stopTest method uses data initialized in startTest.

Note: Further thinking about this problem, it seems like changing the flow of methods like this was ill thought solution to the problem in the first place, as this might be an unexpected change of behaviour for different libraries. I'm still leaving what I think could be the solution in case I'm wrong on that.

It looks like the fix would seem to be moving the call to stopTest in yet another try/finally block right after the call to startTest, emulating the same behaviour as previous code while keeping the call to startTest after the skip check. The call to stopTestRun would need to remain where it is as startTestRun has already been called at this point.

So using the code as found in that commit, something like:

    def run(self, result=None):
        if result is None:
            result = self.defaultTestResult()
            startTestRun = getattr(result, 'startTestRun', None)
            stopTestRun = getattr(result, 'stopTestRun', None)
            if startTestRun is not None:
                startTestRun()
        else:
            stopTestRun = None

        try:
            testMethod = getattr(self, self._testMethodName)
            if (getattr(self.__class__, "__unittest_skip__", False) or
                getattr(testMethod, "__unittest_skip__", False)):
                # If the class or method was skipped.
                skip_why = (getattr(self.__class__, '__unittest_skip_why__', '')
                            or getattr(testMethod, '__unittest_skip_why__', ''))
                _addSkip(result, self, skip_why)
                return result

            # Increase the number of tests only if it hasn't been skipped
            result.startTest(self)
            try:
                expecting_failure = (
                    getattr(self, "__unittest_expecting_failure__", False) or
                    getattr(testMethod, "__unittest_expecting_failure__", False)
                )
                outcome = _Outcome(result)
                start_time = time.perf_counter()
                try:
                    self._outcome = outcome

                    with outcome.testPartExecutor(self):
                        self._callSetUp()
                    if outcome.success:
                        outcome.expecting_failure = expecting_failure
                        with outcome.testPartExecutor(self):
                            self._callTestMethod(testMethod)
                        outcome.expecting_failure = False
                        with outcome.testPartExecutor(self):
                            self._callTearDown()
                    self.doCleanups()
                    self._addDuration(result, (time.perf_counter() - start_time))

                    if outcome.success:
                        if expecting_failure:
                            if outcome.expectedFailure:
                                self._addExpectedFailure(result, outcome.expectedFailure)
                            else:
                                self._addUnexpectedSuccess(result)
                        else:
                            result.addSuccess(self)
                    return result
                finally:
                    # explicitly break reference cycle:
                    # outcome.expectedFailure -> frame -> outcome -> outcome.expectedFailure
                    outcome.expectedFailure = None
                    outcome = None

                    # clear the outcome, no more needed
                    self._outcome = None
                    
            finally:
                result.stopTest(self)
            
        finally:
            if stopTestRun is not None:
                stopTestRun()

CPython versions tested on:

3.12

Operating systems tested on:

Linux, Windows

Linked PRs

• gh-113267: Revert "gh-106584: Fix exit code for unittest in Python 3.12 (#106588)" #114470
• [3.12] gh-113267: Revert "gh-106584: Fix exit code for unittest in Python 3.12 (GH-106588)" (GH-114470) #114994

[yvan-the-barbarian]

I am now also seeing failures in PyCharm's unit testing because of this issue.

This is starting to be a huge issue.

[serhiy-storchaka]

I think that it was a wrong change, and not only because it makes startTest() and stopTest() calls unbalanced, but because it did not handle all cases and changed the behavior in wrong direction. #113661 was the proper fix of the original problem.

[sodul]

Thanks, the changes for 3.12.2 seem to fix CleanCut/green#277