Opened 6 months ago

Closed 6 months ago

Last modified 6 months ago

#35118 closed Cleanup/optimization (invalid)

Mention test-suggestions when specific email-backend is used

Reported by: jecarr Owned by: nobody
Component: Core (Mail) Version: 5.0
Severity: Normal Keywords:
Cc: Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

  • The email-services docs state emails are sent to a dummy outbox if the locmem email-backend is used
  • I needed to specify an email-backend (connection object) in my application and saw my tests were sending emails (not to the dummy outbox because locmem was now not used)
  • This ticket is to suggest in the docs what to do if locmem is not used except for only in tests

Change History (7)

comment:1 by Baptiste Mispelon, 6 months ago

Component: DocumentationCore (Mail)
Patch needs improvement: set
Triage Stage: UnreviewedAccepted

Hi and thanks for the report!

Your proposed addition looks quite good, though I personally would like to see a minimal code example, or at least link to Python's documentation for mocking since the term might not be familiar to everyone.

But in general I think your report point to a bigger issue that might require more than a documentation fix. The fact that "real" emails can be sent during tests if one uses a custom connection in their code was quite surprising to me.
When sending emails using the default connection it's easy enough to point your test settings to the locmem backend to prevent sending actual emails. But when using a custom connection there's no setting that can help. Instead one must remember to do the mocking you're suggesting. That seems very error-prone and the penalty for forgetting that can be quite high.

I can't see how to fix that problem easily. Maybe the multiple email backend idea floated in ticket:6989#comment:23 could be the way? Do you have other ideas?

I'll mark this ticket as accepted on that basis. I think we should try to find a systematic solution to the problem but if that's too hard we should at least document quite loudly that one must pay attention when using a custom connection.

comment:2 by Natalia Bidart, 6 months ago

Resolution: invalid
Status: newclosed
Triage Stage: AcceptedUnreviewed

Hello jecarr, thank you for your report.

Could you please provide more details and/or a reproducer? I've been using Django for a long time with various email backends and tests would always use the dummy mail.outbox as described in the docs, with no need of special overriding nor monkeypatching. Also, I have just tried locally with a test project with the email backend configured to django.core.mail.backends.console.EmailBackend and when running the example test case from the docs, I do not get emails sent to the console backend.

Furthermore, you can see how setup_test_environment overrides the settings for the whole test run to use django.core.mail.backends.locmem.EmailBackend. Of course that the docs and this response assumes that you are using the Django's TestCase.

On the other hand, if your code dynamically builds connections to various mail backends at runtime, the tests in your code should also account for that. This is because there is no way to cover that level of dynamicity in Django core for all possible combinations. But from the PR linked to this ticket, it seems that the issue is only about the configured email backend via settings? Baptiste, were you able to reproduce? Could you share a sample test project?

I'll be closing as invalid for now, but if I'm missing something, please provide as much details as possible so we can re-triage. Thank you!

in reply to:  2 ; comment:3 by Baptiste Mispelon, 6 months ago

Replying to Natalia Bidart:

[...] Baptiste, were you able to reproduce? Could you share a sample test project?

I'll be closing as invalid for now, but if I'm missing something, please provide as much details as possible so we can re-triage. Thank you!

Sorry if I accepted the ticket a bit eagerly, I haven't done triaging in a while. Here's a testcase that I think reproduces the issue being reported by jecarr:

from django.core import mail
from django.core.mail.backends.base import BaseEmailBackend
from django.test import SimpleTestCase


class BrokenEmailBackend(BaseEmailBackend):
    """
    A custom email backend that throws an exception when sending a message
    """
    def send_messages(self, messages):
        raise Exception("no sending allowed")


# in reality, this function would be burried deep somewhere in user code
def send_message():
    mail.send_mail(
        "Subject here",
        "Here is the message.",
        "from@example.com",
        ["to@example.com"],
        fail_silently=False,
        connection=BrokenEmailBackend()
    )


class ReproductionTestCase(SimpleTestCase):
    def test_custom_backend_bypass_locmem(self):
        send_message()
        self.assertEqual(len(mail.outbox), 1)  # fails with exception being raised from backend

You do have a point that a user who manually instantiates an email backend should also have the responsibility to mock it in their tests, but I feel that this is still a surprising effect (it surprised me for what it's worth).
Manually instantiating a backend is the documented way (and as far as i know, the only one) to use a different email backend for specific messages, so it should either be made safe or at least the documentation should be clearer that test should be careful and mock accordingly.
My preferred solution would be the named backend settings I linked to in my earlier comment, but I understand that's a completely different beast, and I don't want to stand in the way of the improvements suggested here.

in reply to:  3 ; comment:4 by Natalia Bidart, 6 months ago

Replying to Baptiste Mispelon:

You do have a point that a user who manually instantiates an email backend should also have the responsibility to mock it in their tests, but I feel that this is still a surprising effect (it surprised me for what it's worth).
Manually instantiating a backend is the documented way (and as far as i know, the only one) to use a different email backend for specific messages, so it should either be made safe or at least the documentation should be clearer that test should be careful and mock accordingly.

Thank you for these details, they definitely help to clarify the original report. I do think that if a project customizes the email backend on a per-email manner, it's the project's tests responsibility to provide a mock for it (even part of the test case should include asserting that the right backend is used in the right place, I guess).

My preferred solution would be the named backend settings I linked to in my earlier comment, but I understand that's a completely different beast, and I don't want to stand in the way of the improvements suggested here.

If a custom but single backend is needed across the project, the recommendation would be to define the custom backend class and point the EMAIL_BACKEND setting to it (as per these docs). In this case, the previously linked setup_test_environment would do the right thing by overriding the EMAIL_BACKEND setting to be locmem.

We may consider adding a note in the docs, but the clarification should be quite explicit about *when* a backend override is needed in tests, since the proposed PR could be read as that the tests always need an override (when, in practice, they almost never need one).

Lastly, I agree with you Baptiste that in a way this could be a duplicate of the not-yet-existing ticket for multiple email backends. I have pinged the author of the related PR but they do not have time at the moment to push the feature further. In order for that to progress, we would need someone to create the ticket and hopefully take ownership of the feature to progress it.

in reply to:  4 ; comment:5 by jecarr, 6 months ago

Resolution: invalid
Status: closednew

Replying to Natalia Bidart:

Thanks for the discussion on this.

If a custom but single backend is needed across the project, the recommendation would be to define the custom backend class and point the EMAIL_BACKEND setting to it (as per these docs). In this case, the previously linked setup_test_environment would do the right thing by overriding the EMAIL_BACKEND setting to be locmem.

I believed I was following the docs anyway: I didn't change the EMAIL_BACKEND setting in my project. I just used an SMTP EmailBackend instance as my connection parameter when creating an EmailMessage or to call connection.send_messages().

This then mirrors Baptiste Mispelon's code example on how my test cases sent actual emails:

from django.conf import settings
from django.core import mail
from django.core.mail.backends.smtp import EmailBackend
from django.test import SimpleTestCase

def send_message():
    mail.send_mail(
        "Subject here",
        "Here is the message.",
        settings.EMAIL_HOST_USER,
        [settings.EMAIL_HOST_USER],
        fail_silently=False,

        # Assumes email-related settings - user/password/host/port - are defined in settings
        connection=EmailBackend()  # comment out to pass test
    )


class ReproductionTestCase(SimpleTestCase):
    def test_custom_backend_bypass_locmem(self):
        send_message()
        self.assertEqual(len(mail.outbox), 1)

We may consider adding a note in the docs, but the clarification should be quite explicit about *when* a backend override is needed in tests, since the proposed PR could be read as that the tests always need an override (when, in practice, they almost never need one).

I thought I was explicit about when the override is needed? My PR begins with "If your application defines a different email backend to be used [other than locmem]". This was applicable to my scenario above because I was defining an SMTP backend. I did link the PR when I opened my ticket but it seems to be unlinked at time of writing.

edit - I see what you mean, I meant if a connection parameter is used as opposed to if a different email backend is used.

Replying to Baptiste Mispelon:

I think we should try to find a systematic solution to the problem but if that's too hard we should at least document quite loudly that one must pay attention when using a custom connection.

I therefore think this point still stands rather than this being about multiple email backends (do point out if I'm misunderstanding how that links!).

Last edited 6 months ago by jecarr (previous) (diff)

in reply to:  5 ; comment:6 by Natalia Bidart, 6 months ago

Resolution: invalid
Status: newclosed

Replying to jecarr:

I believed I was following the docs anyway: I didn't change the EMAIL_BACKEND setting in my project. I just used an SMTP EmailBackend instance as my connection parameter when creating an EmailMessage or to call connection.send_messages().

The usual pattern and recommended approach to change the email backend is to change the EMAIL_BACKEND setting. In fact, the docs you linked say:

The SMTP backend is the default configuration inherited by Django. If you want to specify it explicitly, put the following in your settings: EMAIL_BACKEND = "django.core.mail.backends.smtp.EmailBackend"

This then mirrors Baptiste Mispelon's code example on how my test cases sent actual emails:

I do understand your use case now, thank you and thanks Baptiste for the clarification. Still, building a backend instance and passing it explicitly to send_email, while supported, is not the common case. Passing a connection is usually done when a different/specific backend is needed for a specific email message. And to be clear, I'm not saying that your code is incorrect, I'm saying that the usual way of using the SMTP backend (or any other backend across a project) is to override the setting, for which the Django setup_test_environment has support.

We may consider adding a note in the docs, but the clarification should be quite explicit about *when* a backend override is needed in tests, since the proposed PR could be read as that the tests always need an override (when, in practice, they almost never need one).

I thought I was explicit about when the override is needed? My PR begins with "If your application defines a different email backend to be used [other than locmem]".

Is not explicit in that the most common approach associated with "your application defines a different email backend" is to override the EMAIL_BACKEND setting. If we were to add a clarification to the docs (I'm not convinced yet), it should clearly state that the test override is needed only in those less common cases where a connection is passed explicitly in the code.

Something along the lines:

.. admonition:: Testing email sending with custom connections

   If your email sending code explicitly passes a `connection` object, ensure
   that tests appropriately handle the connection to prevent the actual sending
   of emails during test runs. In such cases, consider using mocking techniques
   to simulate the behavior of the connection without triggering actual email
   delivery.

edit - I see what you mean, I meant if a connection parameter is used as opposed to if a different email backend is used.

Yes, exactly!

Replying to Baptiste Mispelon:

I think we should try to find a systematic solution to the problem but if that's too hard we should at least document quite loudly that one must pay attention when using a custom connection.

I therefore think this point still stands rather than this being about multiple email backends (do point out if I'm misunderstanding how that links!).

The thing is that, IMHO, when multiple email backends gets implemented, the connection param will likely be deprecated in favor of an email backend name, similar to how the database using parameter works. So, considering both this goal and the fact that passing custom email connection is not the common case, I really don't think is worth investing time and resources into automagically mocking custom email connections.

in reply to:  6 comment:7 by jecarr, 6 months ago

Replying to Natalia Bidart:

Thanks Natalia, I understand this better.

The thing is that, IMHO, when multiple email backends gets implemented, the connection param will likely be deprecated in favor of an email backend name, similar to how the database using parameter works. So, considering both this goal and the fact that passing custom email connection is not the common case, I really don't think is worth investing time and resources into automagically mocking custom email connections.

I understand the resolution, I'll rewrite my code to avoid using the connection parameter anyway.

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