[Django] #37072: assertWarnsMessage() also ignores entire warning category

[Django]

#37072: assertWarnsMessage() also ignores entire warning category
-------------------------------------+-------------------------------------
Reporter: Mike Edmunds | Type: Bug
Status: new | Component: Testing
| framework
Version: 6.0 | 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
-------------------------------------+-------------------------------------
`assertWarnsMessage(category, message)` implicitly does the equivalent of
`ignore_warnings(category)`. As a result, tests for particular deprecation
warnings can overlook other, unrelated deprecation warnings coming from
the code being tested.

You would expect the second test case below to cause an error for
`RemovedInDjango70Warning: Unrelated deprecation`, but it actually passes:

{{{#!python
class SomeTests(SimpleTestCase):
def test_this_errors_as_expected(self):
# ERROR: RemovedInDjango70Warning: Unrelated deprecation
warnings.warn("Unrelated deprecation", RemovedInDjango70Warning)

def test_so_this_should_also_error_but_does_not(self):
with self.assertWarnsMessage(RemovedInDjango70Warning, "Expected
deprecation"):
warnings.warn("Expected deprecation",
RemovedInDjango70Warning)
# This gets ignored.
warnings.warn("Unrelated deprecation",
RemovedInDjango70Warning)

def test_duplicate_warnings_should_maybe_also_error(self):
with self.assertWarnsMessage(RemovedInDjango70Warning, "Expected
deprecation"):
warnings.warn("Expected deprecation",
RemovedInDjango70Warning)
# This also gets ignored. Maybe it shouldn't?
warnings.warn("Expected deprecation",
RemovedInDjango70Warning)
}}}

The problem is `assertWarnsMessage()` and `assertRaisesMessage()` try to
share `SimpleTestCase._assertFooMessage()`, which wraps `assertWarns()`
and `assertRaises()` respectively. But those methods aren't really
parallel: there can be only one error raised, but execution can continue
after a warning. `assertWarns(category)` captures ''all'' warnings in the
category.
--
Ticket URL: <https://code.djangoproject.com/ticket/37072>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#37072: assertWarnsMessage() also ignores entire warning category

-----------------------------------+--------------------------------------
Reporter: Mike Edmunds | Owner: (none)

Type: Bug | Status: new
Component: Testing framework | Version: 6.0

Severity: Normal | Resolution:

Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0

-----------------------------------+--------------------------------------
Comment (by Mike Edmunds):

Here's a test case that could be added to tests/test_utils/tests.py to
verify a fix:

{{{#!python
class AssertWarnsMessageTests(SimpleTestCase):
...

def test_does_not_ignore_entire_category(self):
with (
self.assertRaisesMessage(RemovedInNextVersionWarning,
"Unexpected"),
self.assertWarnsMessage(RemovedInNextVersionWarning,
"Expected"),
):
warnings.warn("Expected", RemovedInNextVersionWarning)
warnings.warn("Unexpected", RemovedInNextVersionWarning)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/37072#comment:1>

[Django]

#37072: assertWarnsMessage() also ignores entire warning category
-----------------------------------+------------------------------------

Reporter: Mike Edmunds | Owner: (none)
Type: Bug | Status: new
Component: Testing framework | Version: 6.0
Severity: Normal | Resolution:

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 Tim Graham):

* stage: Unreviewed => Accepted

--
Ticket URL: <https://code.djangoproject.com/ticket/37072#comment:2>

[Django]

#37072: assertWarnsMessage() also ignores entire warning category
-----------------------------------+------------------------------------
Reporter: Mike Edmunds | Owner: (none)
Type: Bug | Status: new
Component: Testing framework | Version: 6.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+------------------------------------

Comment (by Mike Edmunds):

See also #37076.

I'm thinking maybe `assertWarnsMessage()` should do something like:
- Capture all warnings while running the tested code
- Check that at least one captured warning matches the expected category
and message
- ''Optionally'' check that ''exactly'' one (or exactly a specified
`expected_count`) matches
- Re-emit any non-matching warnings that were captured, to be handled by
the original warning filters
--
Ticket URL: <https://code.djangoproject.com/ticket/37072#comment:3>

[Django]

#37072: assertWarnsMessage() also ignores entire warning category

-------------------------------------+-------------------------------------
Reporter: Mike Edmunds | Owner: Artyom
| Kotovskiy
Type: Bug | Status: assigned

Component: Testing framework | Version: 6.0
Severity: Normal | Resolution:
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 Artyom Kotovskiy):

* owner: (none) => Artyom Kotovskiy
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/37072#comment:4>

[Django]

#37072: assertWarnsMessage() also ignores entire warning category
-------------------------------------+-------------------------------------
Reporter: Mike Edmunds | Owner: Artyom
| Kotovskiy
Type: Bug | Status: assigned
Component: Testing framework | Version: 6.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Mike Edmunds):

Python unittest's `assertWarnsRegex()` has the same (problematic)
behavior: https://github.com/python/cpython/issues/143231. And a comment
in that issue proposes a similar fix to the one I described above for
Django's `assertWarnsMessage()`.

In some local experimentation, there are a handful of existing Django
tests that unintentionally depend on the current behavior and would need
to be updated (e.g., by adding appropriate `ignore_warnings()` for the
non-matching deprecation messages those tests don't care about).

There also seem to be a handful of existing Django tests that
''intentionally'' depend on `assertWarnsMessage()` capturing ''multiple''
instances of the same message. (So the answer to my question in
`test_duplicate_warnings_should_maybe_also_error()` above is: duplicate
warnings should ''not'' error, at least not by default.)
--
Ticket URL: <https://code.djangoproject.com/ticket/37072#comment:5>

[Django]

#37072: assertWarnsMessage() also ignores entire warning category
-------------------------------------+-------------------------------------
Reporter: Mike Edmunds | Owner: Artyom
| Kotovskiy
Type: Bug | Status: assigned
Component: Testing framework | Version: 6.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Artyom Kotovskiy):

Replying to [comment:5 Mike Edmunds]:

> Python unittest's `assertWarnsRegex()` has the same (problematic)
behavior: https://github.com/python/cpython/issues/143231. And a comment
in that issue proposes a similar fix to the one I described above for
Django's `assertWarnsMessage()`.
>
> In some local experimentation, there are a handful of existing Django
tests that unintentionally depend on the current behavior and would need
to be updated (e.g., by adding appropriate `ignore_warnings()` for the
non-matching deprecation messages those tests don't care about).
>
> There also seem to be a handful of existing Django tests that
''intentionally'' depend on `assertWarnsMessage()` capturing ''multiple''
instances of the same message. (So the answer to my question in
`test_duplicate_warnings_should_maybe_also_error()` above is: duplicate
warnings should ''not'' error, at least not by default.)
>

Thanks for the suggestions and analysis. I’m taking my time going through
the test case code since I’m new to contributing.
--
Ticket URL: <https://code.djangoproject.com/ticket/37072#comment:6>

[Django]

Am I missing something or those #37072 and #37076 are kind of
contradicting each other?

We expect this to pass

{{{
def test_deprecated_in_my_app(self):
with self.assertWarnsMessage(DeprecationWarning, "Deprecated in
MyApp"):
warnings.warn("Deprecated in Django",
RemovedInNextVersionWarning)
warnings.warn("Deprecated in MyApp", DeprecationWarning)
}}}

And this to fail.

def test_so_this_should_also_error_but_does_not(self):
with self.assertWarnsMessage(RemovedInDjango70Warning, "Expected
deprecation"):
warnings.warn("Expected deprecation",
RemovedInDjango70Warning)
warnings.warn("Unrelated deprecation",
RemovedInDjango70Warning)

RemovedInNextVersionWarning is a subclass of DeprecationWarning. So as I
see it they both have same category, different message — but one should
pass and one should fail.
--
Ticket URL: <https://code.djangoproject.com/ticket/37072#comment:7>

[Django]

#37072: assertWarnsMessage() also ignores entire warning category
-------------------------------------+-------------------------------------
Reporter: Mike Edmunds | Owner: Artyom
| Kotovskiy
Type: Bug | Status: assigned
Component: Testing framework | Version: 6.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Mike Edmunds):

Replying to [comment:7 Artyom Kotovskiy]:

> Am I missing something or those #37072 and #37076 are kind of
contradicting each other?
>

> We expect this to pass in #37076

>
> {{{
> def test_deprecated_in_my_app(self):
> with self.assertWarnsMessage(DeprecationWarning, "Deprecated in
MyApp"):
> warnings.warn("Deprecated in Django",
RemovedInNextVersionWarning)
> warnings.warn("Deprecated in MyApp", DeprecationWarning)
> }}}
>

> And this to fail in #37072.
>
>
>
> {{{

> def test_so_this_should_also_error_but_does_not(self):
> with self.assertWarnsMessage(RemovedInDjango70Warning, "Expected
deprecation"):
> warnings.warn("Expected deprecation",
RemovedInDjango70Warning)
> warnings.warn("Unrelated deprecation",
RemovedInDjango70Warning)
> }}}
>
>
> RemovedInNextVersionWarning is a subclass of DeprecationWarning. So as I
see it they both have same category, different message — but one should
pass and one should fail.

Django's runtests.py
[https://github.com/django/django/blob/9f790ef1a0f356cf6342b5d57bbaeac35aed0d9f/tests/runtests.py#L45-L50
converts some warnings to errors] within the tests. In particular,
RemovedInDjango70Warning is currently converted to an error, but the
current superclass PendingDeprecationWarning is not, and
RemovedInNextVersion warning is not. (There is no RemovedInDjango62Warning
due to a quirk of Django's deprecation cycles. If we did have that, it
would be equivalent to RemovedInNextVersionWarning, and runtests.py
''would'' convert RemovedInDjango62Warning to an error.)

So in Django's own tests, if both issues are fixed we'd expect:
- `test_so_this_should_also_error_but_does_not()` would ''error'' due to
the non-matching "Unrelated deprecation" message (converted to an error by
the runtests.py warning filters).
- `test_deprecated_in_my_app()` would pass (because of the matching
"Deprecated in MyApp") ''and'' might print "RemovedInNextVersionWarning:
Deprecated in Django" to the console, because runtests.py doesn't
currently convert RemovedInNextVersionWarning to an error. (But that will
change after 6.1 is released and runtests.py is updated for 6.2.)

The examples in the bug reports aren't good tests to add to Django itself:
they're a little ambiguous depending on how the testing environment is set
up, and they make assumptions about which Django warnings are currently
defined.

comment:1 suggests a `test_does_not_ignore_entire_category()` for #37072.
It uses an outer `assertRaisesMessage()` to verify "Unrelated deprecation"
makes it out of `assertWarnsMessage()` and is converted to an error by
runtests.py. (It also has a problem that I will fix in an edit.)

We'll need to craft a better test case for #37076 that takes all this into
account. You can rely on runtests.py always treating
RemovedAfterNextVersionWarning as an error (in all releases), and never
treating the base-class DeprecationWarning or PendingDeprecationWarning as
errors.
--
Ticket URL: <https://code.djangoproject.com/ticket/37072#comment:8>

[Django]

#37072: assertWarnsMessage() also ignores entire warning category
-------------------------------------+-------------------------------------
Reporter: Mike Edmunds | Owner: Artyom
| Kotovskiy
Type: Bug | Status: assigned
Component: Testing framework | Version: 6.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted

Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Artyom Kotovskiy):

* has_patch: 0 => 1

Comment:

Pr -> [https://github.com/django/django/pull/21228]
--
Ticket URL: <https://code.djangoproject.com/ticket/37072#comment:9>

[Django]

#37072: assertWarnsMessage() also ignores entire warning category
-------------------------------------+-------------------------------------
Reporter: Mike Edmunds | Owner: Artyom
| Kotovskiy
Type: Bug | Status: assigned
Component: Testing framework | Version: 6.0
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Mike Edmunds):

* needs_better_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/37072#comment:10>