#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 , 10 months ago
Component: | Documentation → Core (Mail) |
---|---|
Patch needs improvement: | set |
Triage Stage: | Unreviewed → Accepted |
follow-up: 3 comment:2 by , 10 months ago
Resolution: | → invalid |
---|---|
Status: | new → closed |
Triage Stage: | Accepted → Unreviewed |
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!
follow-up: 4 comment:3 by , 10 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.
follow-up: 5 comment:4 by , 10 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.
follow-up: 6 comment:5 by , 10 months ago
Resolution: | invalid |
---|---|
Status: | closed → new |
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 linkedsetup_test_environment
would do the right thing by overriding theEMAIL_BACKEND
setting to belocmem
.
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.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.", "from@example.com", ["to@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 the PR when I opened my ticket but it seems to be unlinked at time of writing.
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!).
follow-up: 7 comment:6 by , 10 months ago
Resolution: | → invalid |
---|---|
Status: | new → closed |
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 myconnection
parameter when creating anEmailMessage
or to callconnection.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.
comment:7 by , 10 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 databaseusing
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.
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 customconnection
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
.