Opened 10 months ago

Closed 9 months ago

#34730 closed New feature (fixed)

Add an assertMessages assertion

Reported by: François Freitag Owned by: François Freitag
Component: contrib.messages Version: dev
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 (last modified by François Freitag)

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.

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.

Change History (14)

comment:2 by François Freitag, 10 months ago

Description: modified (diff)

comment:3 by Mariusz Felisiak, 10 months ago

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()

comment:4 by François Freitag, 10 months ago

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>'.

comment:6 by Tim Graham, 10 months ago

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).

comment:7 by François Freitag, 10 months ago

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.

in reply to:  7 comment:8 by Mariusz Felisiak, 10 months ago

Component: Testing frameworkcontrib.messages
Owner: changed from nobody to François Freitag
Patch needs improvement: set
Status: newassigned
Triage Stage: UnreviewedAccepted

Replying to 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.

comment:9 by François Freitag, 10 months ago

Patch needs improvement: unset

comment:10 by Mariusz Felisiak, 10 months ago

Patch needs improvement: set

comment:11 by Mariusz Felisiak <felisiak.mariusz@…>, 9 months ago

In b7fe36ad:

Refs #34730 -- Made Message importable from django.contrib.messages.

comment:12 by François Freitag, 9 months ago

Patch needs improvement: unset

comment:13 by Mariusz Felisiak, 9 months ago

Triage Stage: AcceptedReady for checkin

comment:14 by Mariusz Felisiak <felisiak.mariusz@…>, 9 months ago

Resolution: fixed
Status: assignedclosed

In cafe726:

Fixed #34730 -- Added django.contrib.messages.test.MessagesTestMixin.assertMessages().

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