Opened 3 years ago

Closed 3 years ago

#33264 closed Bug (fixed)

DiscoverRunner doesn't counts unexpectedSuccess as an error

Reported by: Baptiste Mispelon Owned by: Baptiste Mispelon
Component: Testing framework Version: 3.2
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I noticed today that my CI test suite was green even though its console output printed:

FAILED (expected failures=13, unexpected successes=1)

It turns out that Django's default DiscoverRunner does not count "unexpected successes" as errors when deciding the return code of its test management command. [1]

So even though it prints "FAILED", the command returns with an exit code of 0 which my CI (correctly) interprets as a success.

The fix seems quite simple:

  • django/test/runner.py

    diff --git a/django/test/runner.py b/django/test/runner.py
    index 09ac4e142a..2e36514922 100644
    a b class DiscoverRunner:  
    875875        teardown_test_environment()
    876876
    877877    def suite_result(self, suite, result, **kwargs):
    878         return len(result.failures) + len(result.errors)
     878        return len(result.failures) + len(result.errors) + len(result.unexpectedSuccesses)
    879879
    880880    def _get_databases(self, suite):
    881881        databases = {}

But I wonder if that would be considered backwards incompatible.
I also think this is a pretty serious issue and I wonder if backports would be considered for this.

[1] https://github.com/django/django/blob/60503cc747eeda7c61bab02b71f8f55a733a6eea/django/test/runner.py#L877-L878

Change History (6)

comment:1 by Mariusz Felisiak, 3 years ago

Triage Stage: UnreviewedAccepted

Thanks for the report! As far as I'm aware this doesn't qualify for a backport and yes it's backward incompatible. This behavior is in Django from the very beginning (e.g. we take errors into account from 2007, see c1a73d80da65694319d803160b5e400e11318213). Python's TestResult treats unexpected successes as a tests failure since version 3.4.

I'd change this in Django 4.1 without a deprecation period.

comment:2 by Baptiste Mispelon, 3 years ago

Owner: changed from nobody to Baptiste Mispelon
Status: newassigned

Thanks for the quick triage and the git history research.

I'll get started on a PR today.

Meanwhile, if anyone finds this ticket looking for a workaround, it's easy enough to write a custom runner and point to it in settings.TEST_RUNNER:

class CustomTestRunner(DiscoverRunner):
    """
    A custom test runner to get around Django bug
    https://code.djangoproject.com/ticket/33264
    """
    def suite_result(self, suite, result, **kwargs):
        return len(result.failures) + len(result.errors) + len(result.unexpectedSuccesses)

comment:3 by Baptiste Mispelon, 3 years ago

Has patch: set

comment:4 by Tim Graham, 3 years ago

I'll accept this change as correct, but I'll also mention that I've used expectedFailure to prevent tests that sometimes fail (e.g. https://github.com/cockroachdb/django-cockroachdb/commit/97aaf0bd3abff19b1405215d377b5c91e00cfc84) from causing a test suite to fail. I guess I'll have to instead skip these tests.

comment:5 by Mariusz Felisiak, 3 years ago

Triage Stage: AcceptedReady for checkin

comment:6 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

Resolution: fixed
Status: assignedclosed

In 91acfc3:

Fixed #33264 -- Made test runner return non-zero error code for unexpected successes.

Note: See TracTickets for help on using tickets.
Back to Top