Adding mailoutbox fixture, and removing internal _django_clear_outbox

[peterlauri]

pytest-django doesn't provide any mechanism to get the mail outbox, however it is clearing the mailbox, that effects external fixtures that might want to interact with the mail.outbox. This fixture stores previous outbox (if any), creates a new list for usage in fixture, and then restores the state to what it was previously...

This PR is initiated to add a mailoutbox fixture, and if interested of getting this released, I would enhance it with documentation and possible tests...

PR #407 would not be needed if this is chosen as the path to go...

[pelme]

This change would break existing test suites which depend on having a clean mail.outbox for each test. _django_clear_outbox is an autouse fixture.

I'd prefer to recommend from django.core import mail and check for mail.outbox in tests. If one uses the outbox heavily in a project and wants to avoid having to import it, you can add a fixture like

from django.core import mail

@pytest.fixture
def outbox():
    return mail.outbox

Closing this PR, please leave a comment and reopen it if there is another angle of this PR that I missed!

[peterlauri]

@pelme Are you refering to test_mail and test_mail_again in the test suite?

I see this as an hidden functionality, as it is an "private" autouse fixture that is not documented.

Wouldn't it be more clear for the user, if pytest-django provides the "outbox" fixture it self, and that fixture handles whatever "magic" needed to clear the mail.outbox? And of course documented :)

[blueyed]

_django_clear_outbox to be there as a autouse fixture seems a bit strange to me, too.

[peterlauri]

@pelme, any comments?

[pelme]

Agreed, it is cleaner and probably the correct approach.

We just need to think about the backward compatibility. (I have a bunch of tests that depend on this myself (maybe it's only me since I wrote that fixture :)):

grep -r mail.outbox tests | wc -l
      24

)

I'm fine with getting rid of the old way as long as we provide some kind of deprecation warning and not just break test suites that depend on the old behavior.

[peterlauri]

@pelme I'll fix it...

[peterlauri]

@pelme , in my branch:

grep -r mail.outbox tests | wc -l
       1

[peterlauri]

Related to deprecation warning, we should just issue one warning, so we could do that somewhere in setup of the plugin. I don't know where it is best practise to issue DeprecationWarnining in pytest plugins :/

[pelme]

The grep command was run from my current project that uses pytest-django, not pytest-django itself. I have about 24 places that would be broken if we merge this change as is. I'm personally fine fixing those, however a bunch of test suites probably use the same kind of pattern (even though this is not currently documented). I don't want to break their test suites (or even worse, make tests pass without failures because of emails that suddenly "exists" because they were not cleared :)).

Maybe we could replace mail.outbox during tests with a special object that issue warnings when anything other than .append() happens to it?
(.append() is called when new emails are added https://github.com/django/django/blob/004ba05bcaab9133bc2b7f943f6c3198da38dbc0/django/core/mail/backends/locmem.py#L27)

i.e., in a test

mail.outbox[0], mail.outbox[1] etc => trigger warning with instructions to use the fixture
len(mail.outbox) => trigger warning with instructions to use the fixture
list(mail.outbox) => trigger warning with instructions to use the fixture
bool(mail.outbox) => trigger warning with instructions to use the fixture
mail.outbox.append(...) => do not trigger warning since this is used by Django to insert messages

To emit a warning, pytest's request.config.warn() can be used.

[peterlauri]

We could as an intermediate step introduce a flag --clear-django-mailbox that enables the autouse fixture, with an FutureDepcreationWarning that it will be removed in future release of pytest-django.

This means, that projects that relies on this will just need to add --clear-django-mailbox to their pytest config, and they should be fine... And we drop the functionality in later release... (or never) :)

[pelme]

def test_important_email():
    send_important_email()
    assert mail.outbox

def test_another_important_email():
    send_another_important_email()
    assert mail.outbox

If suddenly there is a bug in send_another_important_email() the assertion that checks mail.outbox will still pass since the first test filled the outbox.

I don't think opt-in via a flag like --clear-django-mailbox is enough for this change, I think we need to be very clear that this changed in a way that cannot be overlooked.

[peterlauri]

I get your point with that those tests, however it isn't that pytestic to do it that way. The way how the mailoutbox fixture works is very much like the db fixtures/functions, that provides a transactional behavior that will restore the mail.outbox to the way it was before requesting the fixture.

I have updated changelog to highlight the change as an important change. And I take back my suggestion to have the --clear-django-mailbox as it would encourage, in my opinion, bad practise to rely on automatic cleaning of the mail.outbox.

[pelme]

I am in favor of having the mailoutbox fixture and think it is a good idea.

You are correct: It is more pytestic (nice word! :)) to explicitly ask for fixtures that setup/teardown stuff you care about.

I agree that those tests in my example are bad that I and should not be encouraged/used.

However, those tests works today and are valid. They exists in real tests suites (my current project included). People rely on them working as the did (even though it is not pytestic). There may be test suites of 5000 tests where some of these depend on a clear mailbox to be valid. Maybe the person who wrote those test left those companies and someone else is upgrading pytest-django this year?

My point is that we can avoid all of that with a small effort with a safeguard to mail.outbox that raises explicit warnings or errors. We use the same pattern with the database by raising an error when you did not request the database. In code this means roughly:

class _OutboxError(object):
    def _raise_assertion(*args, **kwargs):
        raise AssertionError('Use the mailoutbox fixture')
    __len__ = __getitem__ = __nonzero__ = __bool__ = append = _raise_assertion

@pytest.fixture(autouse=True)
def _error_on_mailbox_access():
    from django.core import mail
    old = mail.outbox
    mail.outbox = _OutboxError()
    yield
    mail.outbox = old

[peterlauri]

Sounds fair, but can it be guaranteed that an autouse fixture will be executed before all normal non-autouse fixtures? I hope you have deeper insight in that, as you are way more involved with pytest development than I am :)

Because if this cannot be guaranteed, mailoutbox fixture might will not function if _error_on_mailbox_access fixture will be applied after mailoutbox fixture...

[pelme]

It can be listed as a dependency for mailoutbox which will ensure it is run before/teared down after:

@pytest.fixture
def mailoutbox(_error_on_mailbox_access):
    ....

[peterlauri]

Of course, thanks :) Implementation coming...

[pelme]

This will be awesome ✨ 👍

Btw, we need to make sure Django TestCases still works without errors, _error_on_mailbox_access probably needs to check for that and avoid make sure mail.outbox works as before in those cases (Django TestCase should take care of it but we just need to not mess it up).

[pelme]

Please add test_two that does the same thing and checks to make sure the clearing works :)

[peterlauri]

done

[ionelmc]

Shouldn't this be scope="session"? If it's session-scoped then the "protection" would be on for other fixtures too.

[ionelmc]

If the outbox was actually None (and not missing) now it would be removed. Is this acceptable behavior?

[peterlauri]

mail.outbox will never be None, mail.outbox is only setup when locmem.EmailBackend is used, and setup either in constructor of locmem.EmailBackend or in django.test.util.setup_test_environment

[peterlauri]

Just pushed last update of changelog, I think this is ready for final review...

[pelme]

Thanks a lot for this feature, it will be great! ✨

[peterlauri]

Your welcome. Thanks for an awesome pytest plugin.