Opened 9 years ago

Closed 2 months ago

#24721 closed New feature (duplicate)

Add "test extensions" (e.g. something similar to mail.outbox for messages)

Reported by: Marc Tamlyn Owned by: nobody
Component: Testing framework Version: dev
Severity: Normal Keywords:
Cc: alasdair@… Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

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.

Change History (10)

comment:1 by Carl Meyer, 9 years ago

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.

comment:2 by Marc Tamlyn, 9 years ago

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

comment:3 by Carl Meyer, 9 years ago

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 :/

comment:4 by Marc Tamlyn, 9 years ago

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

comment:5 by Carl Meyer, 9 years ago

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.

comment:6 by Alasdair Nicol, 9 years ago

Cc: alasdair@… added

comment:7 by Tim Graham, 9 years ago

Triage Stage: UnreviewedAccepted

comment:9 by Tim Graham, 9 years ago

Component: contrib.messagesTesting framework
Has patch: set
Needs documentation: set
Patch needs improvement: set
Summary: Introduce something similar to mail.outbox for messagesAdd "test extensions" (e.g. something similar to mail.outbox for messages)

comment:10 by Mariusz Felisiak, 2 months ago

Resolution: duplicate
Status: newclosed

Duplicate of #34730, fixed by adding assertMessages().

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