[Django] #34730: Add an assertMessages assertion

[Django]

#34730: Add an assertMessages assertion
---------------------------------------------+------------------------
Reporter: François Freitag | Owner: nobody
Type: New feature | Status: new
Component: Testing framework | Version: dev
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 1
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
---------------------------------------------+------------------------
Most projects I’ve worked on used the messages framework, and many of them
were repeating some boilerplate in the form described in
https://stackoverflow.com/questions/2897609/how-can-i-unit-test-django-
messages.

{{{#!python
from django.contrib.messages import get_messages

messages = list(get_messages(response.wsgi_request))
self.assertEqual(len(messages), 1)
self.assertEqual(str(messages[0]), 'my message')
}}}

I see several small pain points in this code:
- accessing `response.wsgi_request`, because get_messages needs a
`request` but all we have is a response.
- `get_messages` returns an iterator, that must be consumed to perform
assertions, hence the casting to a list
- the expectation lacks precision (missing the level) and clarity
- it’s boilerplate-ish

Maybe adding an assertion to facilitate this test would benefit the other
projects.

The code is small enough that it won’t be worth pulling down as a 3rd
party package, especially since it requires changing the base test class
over the entire test suite.

--
Ticket URL: <https://code.djangoproject.com/ticket/34730>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#34730: Add an assertMessages assertion

-----------------------------------+--------------------------------------

Reporter: François Freitag | Owner: nobody
Type: New feature | Status: new
Component: Testing framework | Version: dev

Severity: Normal | Resolution:

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

-----------------------------------+--------------------------------------

Comment (by François Freitag):

PR : https://github.com/django/django/pull/17101

--
Ticket URL: <https://code.djangoproject.com/ticket/34730#comment:1>

[Django]

#34730: Add an assertMessages assertion
-----------------------------------+--------------------------------------
Reporter: François Freitag | Owner: nobody
Type: New feature | Status: new
Component: Testing framework | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+--------------------------------------

Description changed by François Freitag:

Old description:

> Most projects I’ve worked on used the messages framework, and many of
> them were repeating some boilerplate in the form described in
> https://stackoverflow.com/questions/2897609/how-can-i-unit-test-django-
> messages.
>
> {{{#!python
> from django.contrib.messages import get_messages
>
> messages = list(get_messages(response.wsgi_request))
> self.assertEqual(len(messages), 1)
> self.assertEqual(str(messages[0]), 'my message')
> }}}
>

> I see several small pain points in this code:
> - accessing `response.wsgi_request`, because get_messages needs a
> `request` but all we have is a response.
> - `get_messages` returns an iterator, that must be consumed to perform
> assertions, hence the casting to a list
> - the expectation lacks precision (missing the level) and clarity
> - it’s boilerplate-ish
>

> Maybe adding an assertion to facilitate this test would benefit the other
> projects.
>
> The code is small enough that it won’t be worth pulling down as a 3rd
> party package, especially since it requires changing the base test class
> over the entire test suite.

New description:

Most projects I’ve worked on used the messages framework, and many of them
were repeating some boilerplate in the form described in
https://stackoverflow.com/questions/2897609/how-can-i-unit-test-django-
messages.

{{{#!python
from django.contrib.messages import get_messages

messages = list(get_messages(response.wsgi_request))
self.assertEqual(len(messages), 1)
self.assertEqual(str(messages[0]), 'my message')
}}}

I see several small pain points in this code:
- accessing `response.wsgi_request`, because get_messages needs a
`request` but all we have is a response.
- `get_messages` returns an iterator, that must be consumed to perform
assertions, hence the casting to a list

- the expectation lacks precision (missing the level) and clarity (in
intent, and when failures are reported)
- it’s boilerplate-ish

Maybe adding an assertion to facilitate this test would benefit the other
projects.

The code is small enough that it won’t be worth pulling down as a 3rd
party package, especially since it requires changing the base test class
over the entire test suite.

--

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

[Django]

#34730: Add an assertMessages assertion
-----------------------------------+--------------------------------------
Reporter: François Freitag | Owner: nobody
Type: New feature | Status: new
Component: Testing framework | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+--------------------------------------

Comment (by Mariusz Felisiak):

Thanks for the ticket, however, I'm torn. Introducing app-specific logic
in the Django's test classes doesn't sounds right to me. What do you think
about instead adding a helper function to the
`django.contrib.messages.utils` which would return messages for a
response? e.g. `get_messages_from_test_client_response()`

--
Ticket URL: <https://code.djangoproject.com/ticket/34730#comment:3>

[Django]

#34730: Add an assertMessages assertion
-----------------------------------+--------------------------------------
Reporter: François Freitag | Owner: nobody
Type: New feature | Status: new
Component: Testing framework | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+--------------------------------------

Comment (by François Freitag):

Thanks for your input!
Your suggestion addresses the two first pain points (retrieving messages
from a response, and storing in a `list` for assertions).

I see additional benefits with the suggested helper, because it is easier
to define the expected result (using tuples instead of
`django.contrib.messages.storage.base.Message` instances), and a failure
has a readable output. Without such helper, one would at best get
`'<django.contrib.messages.storage.base.Message object at some_address>'`
not being equal to `'<django.contrib.messages.storage.base.Message object
at another_address>'`.

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

[Django]

#34730: Add an assertMessages assertion
-----------------------------------+--------------------------------------
Reporter: François Freitag | Owner: nobody
Type: New feature | Status: new
Component: Testing framework | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+--------------------------------------

Comment (by François Freitag):

https://github.com/django/django/pull/17101/files#diff-
801f0c08dc288b2efa15c535b8156c30e4d49d39aa24557ab389203e719f3556R2248
gives an example of failure output.

--
Ticket URL: <https://code.djangoproject.com/ticket/34730#comment:5>

[Django]

#34730: Add an assertMessages assertion
-----------------------------------+--------------------------------------
Reporter: François Freitag | Owner: nobody
Type: New feature | Status: new
Component: Testing framework | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+--------------------------------------

Comment (by Tim Graham):

I agree that adding `SimpleTestCase` assertions for contrib packages is
giving them inappropriate special privileges that third-party packages
wouldn't get. Perhaps the assertion could live in a test case mixin in a
module like `django.contrib.messages.tests` (matching the helpers in
`contrib.admin.tests`).

--
Ticket URL: <https://code.djangoproject.com/ticket/34730#comment:6>

[Django]

#34730: Add an assertMessages assertion
-----------------------------------+--------------------------------------
Reporter: François Freitag | Owner: nobody
Type: New feature | Status: new
Component: Testing framework | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+--------------------------------------

Comment (by François Freitag):

I agree, importing an optional, contrib app in the core of `django.test`
isn’t great. The reason I chose to do it was to make the assertion more
visible and easier to use.
Note that the import from `django.contrib.messages` happens when the
assertion is called, so the dependency only exists when the assertion is
called. I thought it was an acceptable trade-off, but a contrib app
assertion would still be available to projects who don’t use
`django.contrib.messages`.

Projects I worked on often added assertions or setUp/tearDown to
`SimpleTestCase`, I assume that’s fairly common and shouldn’t be a big
deal for others to add the mixin.

If we’re all in agreement with Tim’s proposal, I can move the helper to
`django.contrib.messages.test`.

--
Ticket URL: <https://code.djangoproject.com/ticket/34730#comment:7>

[Django]

#34730: Add an assertMessages assertion

-------------------------------------+-------------------------------------
Reporter: François Freitag | Owner: François
| Freitag
Type: New feature | Status: assigned
Component: contrib.messages | Version: dev
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 Mariusz Felisiak):

* owner: nobody => François Freitag
* needs_better_patch: 0 => 1
* status: new => assigned
* component: Testing framework => contrib.messages
* stage: Unreviewed => Accepted

Comment:

Replying to [comment:7 François Freitag]:

> If we’re all in agreement with Tim’s proposal, I can move the helper to
`django.contrib.messages.test`.

Sounds good, tentatively accepted.

--
Ticket URL: <https://code.djangoproject.com/ticket/34730#comment:8>

[Django]

#34730: Add an assertMessages assertion
-------------------------------------+-------------------------------------
Reporter: François Freitag | Owner: François
| Freitag
Type: New feature | Status: assigned
Component: contrib.messages | Version: dev
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 François Freitag):

* needs_better_patch: 1 => 0

--
Ticket URL: <https://code.djangoproject.com/ticket/34730#comment:9>

[Django]

#34730: Add an assertMessages assertion
-------------------------------------+-------------------------------------
Reporter: François Freitag | Owner: François
| Freitag
Type: New feature | Status: assigned
Component: contrib.messages | Version: dev
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 Mariusz Felisiak):

* needs_better_patch: 0 => 1

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

[Django]

#34730: Add an assertMessages assertion
-------------------------------------+-------------------------------------
Reporter: François Freitag | Owner: François
| Freitag
Type: New feature | Status: assigned
Component: contrib.messages | Version: dev
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
-------------------------------------+-------------------------------------

Comment (by Mariusz Felisiak <felisiak.mariusz@…>):

In [changeset:"b7fe36ad37fb18c4bc7932c0aec6ae4f299b9622" b7fe36ad]:
{{{
#!CommitTicketReference repository=""
revision="b7fe36ad37fb18c4bc7932c0aec6ae4f299b9622"
Refs #34730 -- Made Message importable from django.contrib.messages.
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/34730#comment:11>

[Django]

#34730: Add an assertMessages assertion
-------------------------------------+-------------------------------------
Reporter: François Freitag | Owner: François
| Freitag
Type: New feature | Status: assigned
Component: contrib.messages | Version: dev
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 François Freitag):

* needs_better_patch: 1 => 0

--
Ticket URL: <https://code.djangoproject.com/ticket/34730#comment:12>

[Django]

#34730: Add an assertMessages assertion
-------------------------------------+-------------------------------------
Reporter: François Freitag | Owner: François
| Freitag
Type: New feature | Status: assigned
Component: contrib.messages | Version: dev
Severity: Normal | Resolution:

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/34730#comment:13>

[Django]

#34730: Add an assertMessages assertion
-------------------------------------+-------------------------------------
Reporter: François Freitag | Owner: François
| Freitag

Type: New feature | Status: closed
Component: contrib.messages | Version: dev
Severity: Normal | Resolution: fixed

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 <felisiak.mariusz@…>):

* status: assigned => closed
* resolution: => fixed

Comment:

In [changeset:"cafe7266ee69f7e017ddbc0d440084ace559b04b" cafe726]:
{{{
#!CommitTicketReference repository=""
revision="cafe7266ee69f7e017ddbc0d440084ace559b04b"
Fixed #34730 -- Added
django.contrib.messages.test.MessagesTestMixin.assertMessages().
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/34730#comment:14>