[Django] #24721: Introduce something similar to mail.outbox for messages
Django
--------------------------------------------+------------------------
Reporter: mjtamlyn | Owner: nobody
Type: New feature | Status: new
Component: contrib.messages | Version: master
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 |
--------------------------------------------+------------------------
'''TL;DR'''
- Testing messages is tricky, and depends on the choice of storage used
- Testing email has been abstracted nicely, let's copy that
- Doing so from a contrib app is hard, let's leverage app configs as a
place for hooks for contrib apps to inject their own testing needs into
Django's normal test environment
'''The long version'''
When testing emails with Django, it automatically changes your email
backend to a clever in memory one, and exposes {{{mail.outbox}}} which
will contain any emails sent during that test and tidy itself up
afterwards for you.
With messages we have no such machinery. If you want to check a message
has been sent on a GET this is quite straightforwards as you can simply
check {{{response.context['messages']}}} for it. However with POST
requests it is rather more complex, especially if you use
{{{assertRedirects}}} in it's normal form - the check of the destination
of the redirect will remove the message!
As an example, consider this test, which is based on an existing test
({{{messages.tests.test_mixins.SuccessMessageMixinTests}}}):
{{{
from django.core.urlresolvers import reverse
from django.test import TestCase, override_settings
@override_settings(ROOT_URLCONF='messages_tests.urls')
class TestRedirectionMessage(TestCase):
def test_simple(self):
author = {'name': 'John Doe',
'slug': 'success-msg'}
add_url = reverse('add_success_msg')
response = self.client.post(add_url, author)
# response.context['messages'] does not exist as response is a 302
self.assertRedirects(response, reverse('show_message'))
# Message is now gone, even if I were to make another GET request.
}}}
In this case, the original test inspects {{{response.cookies}}} for the
302 response, which is fairly easy, but it's more complex with other
message storages. If you use the default {{{FallbackStorage}}}, you would
have to change your test if the message got too long to inspect the
session storage instead of the cookie...
Here's the version of that test I would like:
{{{
def test_simple(self):
author = {'name': 'John Doe',
'slug': 'success-msg'}
add_url = reverse('add_success_msg')
response = self.client.post(add_url, author)
self.assertRedirects(response, reverse('show_message'))
self.assertEqual(len(messages.outbox), 1)
self.assertEqual(messages.outbox[0].message, 'John Doe was created
successfully')
self.assertEqual(messages.outbox[0].level, messages.SUCCESS)
}}}
The use of the name {{{outbox}}} here is somewhat strange I admit, but as
yet I haven't found an alternative.
The actual implementation detail is fairly straightforwards in some sense
- create a new {{{InMemoryStorage}}} with the same basic infrastructure as
the corresponding email backend. The difficult part is that due to the
fact this is a contrib app. The engineering to make the email backend work
takes place in two distinct locations - first in the
{{{setup_test_environment}}} to initialise it, and then in
{{{TestCase._pre_setup}}} to reset before each test.
The best proposal I have for designing this is to add some optional
methods to the {{{AppConfig}}} class which are hooks called by both of
these methods. This would also be extremely useful for other packages
which interact with external resources and/or have alternate backends. For
example, django-redis could use these hooks to switch to another redis
database and automatically clear it before each test (like we do with
other data stores), or django-celery could automatically switch to using
an in-memory broker and provide a good api for checking that the task has
been queued, then flushing the queue later if desired, if not then reset
the queue after each test. This is likely to be more expressive and
performant than {{{CELERY_ALWAYS_EAGER = True}}}.
Backwards compatibility with existing tests which expect the request to
have been modified with messages is an issue, I suspect this may need to
be phased in, or set as non default. This could be done via an alternative
app config if this is the route we choose to go.
--
Ticket URL: <https://code.djangoproject.com/ticket/24721>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: mjtamlyn | Owner: nobody
Type: New feature | Status: new
Component: contrib.messages | Version: master
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 carljm):
I think an in-memory backend for `contrib.messages`, for testing purposes,
is a great idea.
I'm less sold on doing more automatic monkeypatching for tests, or adding
a hook for third-party apps to automatically monkeypatch themselves when
under test. I'd really like to see such changes be explicit somehow,
rather than automatic. Personally I just use a test-specific settings file
with a few settings overrides, but there may be other options.
--
Ticket URL: <https://code.djangoproject.com/ticket/24721#comment:1>
Django
Reporter: mjtamlyn | Owner: nobody
Type: New feature | Status: new
Component: contrib.messages | Version: master
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 mjtamlyn):
An overridden test runner works very well for changing the test
environment (changing settings for example), but it doesn't allow for
wrapping tests. I personally am not a huge fan of the following pattern:
{{{
MyTestMixins(object):
def _pre_setup(self):
redis.clear()
queue.clear()
messages.outbox = []
...
MyTestCase(MyTestMixins, TestCase)
MyTransactionTestCase(MyTestMixins, TestCase)
MyLiveServerTestCase(MyTestMixins, MyLiveServerTestCase)
...
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24721#comment:2>
Django
Reporter: mjtamlyn | Owner: nobody
Type: New feature | Status: new
Component: contrib.messages | Version: master
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 carljm):
Yeah, I use `pytest-django` and pytest fixtures for that, which I think is
a much better pattern, but not one we can enforce for Django. So it looks
like this is another case where we'll end up reinventing a Django-specific
solution to a problem that's already better-solved elsewhere, because we
don't want to take on the better solution as a dependency :/
--
Ticket URL: <https://code.djangoproject.com/ticket/24721#comment:3>
Django
Reporter: mjtamlyn | Owner: nobody
Type: New feature | Status: new
Component: contrib.messages | Version: master
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 mjtamlyn):
A more explicit design possibility which doesn't require so much end user
code and provides a more explicit (and more optional) hook:
{{{
# settings.py
TEST_RUNNER = 'myproject.test_runner.TestRunner'
# test_runner.py
class TestRunner(DiscoverRunner):
test_extensions = [messages.test.MessagesExtension,
django_redis.test.TestExtension]
}}}
Each extension class would have hooks for test environment setup and
teardown, before/after each test case and individual test.
At the moment I find most third party apps are very bad at making it easy
to use them in a suitable manner for tests - I hope by introducing an
official API and making it easy for users they will consider more
providing utilities for testing.
I'd love to find an elegant way for these extensions to provide custom
assertion methods as well, for example {{{self.assertMessageSent('you
failed', level=messages.ERROR)}}}, but this would require something more
magical involving getattrs, deliberate monkeypatching on of methods or
something equally ugly.
At this point it does start looking a lot like stuff pytest has done
better. Personally I've not had the chance to use it a huge amount yet,
but it does look good. I think that pytest is now big enough and
established enough that we could consider adding a
{{{django.test.pytest}}} module with utilities along the lines of those in
{{{pytest-django}}}.
Mind you, if we are talking about magical code in test utilities, pytest
takes it to a whole new level ;)
--
Ticket URL: <https://code.djangoproject.com/ticket/24721#comment:4>
Django
Reporter: mjtamlyn | Owner: nobody
Type: New feature | Status: new
Component: contrib.messages | Version: master
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 carljm):
I like the "test extensions" idea much better than automatically-run hooks
on `AppConfig`. It's still a bit of an "NIH" partial re-implementation of
pytest fixtures, but at least it is fully explicit (and these extension
classes could be structured in such a way that they could be easily reused
as a pytest fixture, which would be awesome). I wouldn't even mind if said
"test extensions" were configured via their own setting rather than as an
attribute of a custom `TestRunner`, to make them easier to use (I'm not
allergic to new settings).
Re the idea of `django.test.pytest`, I think there's some merit to the
idea but it's getting OT for this ticket, so we can discuss it elsewhere
:-)
And it's not "magic" per se that concerns me here - it's implicitly and
automatically monkeypatching things only when under test. It's important
that developers be aware of how their system under test differs from their
system not under test, which is why I don't think such changes should be
made automatically, in most cases.
--
Ticket URL: <https://code.djangoproject.com/ticket/24721#comment:5>
Django
Reporter: mjtamlyn | Owner: nobody
Type: New feature | Status: new
Component: contrib.messages | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by alasdairnicol):
* cc: alasdair@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/24721#comment:6>
Django
Reporter: mjtamlyn | Owner: nobody
Type: New feature | Status: new
Component: contrib.messages | Version: master
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 timgraham):
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/24721#comment:7>
Django
Reporter: mjtamlyn | Owner: nobody
Type: New feature | Status: new
Component: contrib.messages | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by mjtamlyn):
Proof of concept at https://github.com/django/django/pull/4926
--
Ticket URL: <https://code.djangoproject.com/ticket/24721#comment:8>
Django
-----------------------------------+------------------------------------
Reporter: mjtamlyn | Owner: nobody
Type: New feature | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
* component: contrib.messages => Testing framework
* needs_docs: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/24721#comment:9>
Django
-----------------------------------+-------------------------------------
Reporter: Marc Tamlyn | Owner: nobody
Type: New feature | Status: closed
Component: Testing framework | Version: dev
Severity: Normal | Resolution: duplicate
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* status: new => closed
* resolution: => duplicate
Comment:
Duplicate of #34730, fixed by adding `assertMessages()`.
--
Ticket URL: <https://code.djangoproject.com/ticket/24721#comment:10>