[Django] #33264: DiscoverRunner doesn't counts unexpectedSuccess as an error
Django
---------------------------------------------+------------------------
Reporter: Baptiste Mispelon | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 3.2
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
---------------------------------------------+------------------------
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:
{{{#!diff
diff --git a/django/test/runner.py b/django/test/runner.py
index 09ac4e142a..2e36514922 100644
--- a/django/test/runner.py
+++ b/django/test/runner.py
@@ -875,7 +875,7 @@ class DiscoverRunner:
teardown_test_environment()
def suite_result(self, suite, result, **kwargs):
- return len(result.failures) + len(result.errors)
+ return len(result.failures) + len(result.errors) +
len(result.unexpectedSuccesses)
def _get_databases(self, suite):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/33264>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Baptiste Mispelon | Owner: nobody
Type: Bug | Status: new
Component: Testing framework | Version: 3.2
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* stage: Unreviewed => Accepted
Comment:
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
[https://docs.python.org/3/library/unittest.html?highlight=unexpectedsuccesses#unittest.TestResult.wasSuccessful
3.4].
I'd change this in Django 4.1 without a deprecation period.
--
Ticket URL: <https://code.djangoproject.com/ticket/33264#comment:1>
Django
Reporter: Baptiste Mispelon | Owner: Baptiste
| Mispelon
Type: Bug | Status: assigned
Component: Testing framework | Version: 3.2
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Baptiste Mispelon):
* owner: nobody => Baptiste Mispelon
* status: new => assigned
Comment:
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`:
{{{#!py
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)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33264#comment:2>
Django
Reporter: Baptiste Mispelon | Owner: Baptiste
| Mispelon
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Baptiste Mispelon):
* has_patch: 0 => 1
Comment:
Here is the PR: https://github.com/django/django/pull/15060 ✨
--
Ticket URL: <https://code.djangoproject.com/ticket/33264#comment:3>
Django
Reporter: Baptiste Mispelon | Owner: Baptiste
| Mispelon
Type: Bug | Status: assigned
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/33264#comment:4>
Django
Reporter: Baptiste Mispelon | Owner: Baptiste
| Mispelon
Type: Bug | Status: assigned
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/33264#comment:5>
Django
Reporter: Baptiste Mispelon | Owner: Baptiste
| Mispelon
Component: Testing framework | Version: 3.2
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"91acfc3514280370535f1dd6bebee57760bac7b9" 91acfc3]:
{{{
#!CommitTicketReference repository=""
revision="91acfc3514280370535f1dd6bebee57760bac7b9"
Fixed #33264 -- Made test runner return non-zero error code for unexpected
successes.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33264#comment:6>