[Django] #35118: Mention test-suggestions when specific email-backend is used
Django
------------------------------------------------+------------------------
Reporter: jecarr | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: 5.0
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 1
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------------+------------------------
- The [https://docs.djangoproject.com/en/5.0/topics/testing/tools/#email-
services 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
--
Ticket URL: <https://code.djangoproject.com/ticket/35118>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: jecarr | Owner: nobody
Type: Cleanup/optimization | Status: new
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Baptiste Mispelon):
* needs_better_patch: 0 => 1
* component: Documentation => Core (Mail)
* stage: Unreviewed => Accepted
Comment:
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`.
--
Ticket URL: <https://code.djangoproject.com/ticket/35118#comment:1>
Django
Reporter: jecarr | Owner: nobody
Type: | Status: closed
Cleanup/optimization |
Component: Core (Mail) | Version: 5.0
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Natalia Bidart):
* status: new => closed
* resolution: => invalid
* stage: Accepted => Unreviewed
Comment:
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
[https://github.com/django/django/blob/main/django/test/utils.py#L121
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!
--
Ticket URL: <https://code.djangoproject.com/ticket/35118#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: jecarr | Owner: nobody
Type: | Status: closed
Cleanup/optimization |
Component: Core (Mail) | Version: 5.0
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Baptiste Mispelon):
Replying to [comment:2 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:
{{{#!python
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.",
"fr...@example.com",
["t...@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.
--
Ticket URL: <https://code.djangoproject.com/ticket/35118#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: jecarr | Owner: nobody
Type: | Status: closed
Cleanup/optimization |
Component: Core (Mail) | Version: 5.0
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Natalia Bidart):
Replying to [comment:3 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
[https://docs.djangoproject.com/en/5.0/topics/email/#defining-a-custom-
email-backend 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 [https://github.com/django/django/pull/13728 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/35118#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: jecarr | Owner: nobody
Cleanup/optimization |
Component: Core (Mail) | Version: 5.0
Severity: Normal | Resolution:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: closed => new
* resolution: invalid =>
Comment:
Replying to [comment:4 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
[https://docs.djangoproject.com/en/5.0/topics/email/#defining-a-custom-
email-backend 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
[https://docs.djangoproject.com/en/5.0/topics/email/#smtp-backend 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.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.",
"fr...@example.com",
["t...@example.com"],
fail_silently=False,
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
[https://github.com/django/django/pull/17741 the PR] when I opened my
ticket but it seems to be unlinked at time of writing.
Replying to [comment:1 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!).
--
Ticket URL: <https://code.djangoproject.com/ticket/35118#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: jecarr | Owner: nobody
Cleanup/optimization |
Component: Core (Mail) | Version: 5.0
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => invalid
Comment:
Replying to [comment:5 jecarr]:
> I believed I was following the docs anyway: I didn't change the
`EMAIL_BACKEND` setting in my project. I just used an
[https://docs.djangoproject.com/en/5.0/topics/email/#smtp-backend 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 [comment:1 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/35118#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: jecarr | Owner: nobody
Type: | Status: closed
Cleanup/optimization |
Component: Core (Mail) | Version: 5.0
Severity: Normal | Resolution: invalid
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by jecarr):
Replying to [comment:6 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/35118#comment:7>