Fixes issue with mail.outbox #1187
Conversation
|
@bluetech thoughts? |
There was a problem hiding this comment.
Thanks for tackling this @kingbuzzman. Please see my comments.
tests/conftest.py
Outdated
| @@ -30,11 +30,13 @@ def _marker_apifun( | |||
| extra_settings: str = "", | |||
| create_manage_py: bool = False, | |||
| project_root: str | None = None, | |||
| has_settings: bool = True, | |||
There was a problem hiding this comment.
I'd call this create_settings, read a bit better and matches create_manage_py.
| @@ -599,7 +599,8 @@ def _dj_autoclear_mailbox() -> None: | |||
|
|
|||
| from django.core import mail | |||
|
|
|||
| del mail.outbox[:] | |||
| if hasattr(mail, "outbox"): | |||
There was a problem hiding this comment.
This change LGTM. This is an autouse fixture and should just do nothing if locmem backend is not used, not error.
| @@ -608,12 +609,13 @@ def mailoutbox( | |||
| _dj_autoclear_mailbox: None, | |||
| ) -> list[django.core.mail.EmailMessage] | None: | |||
| """A clean email outbox to which Django-generated emails are sent.""" | |||
| if not django_settings_is_configured(): | |||
There was a problem hiding this comment.
This change LGTM. If someone explicitly asked for this fixture and Django is not configured, either it is not meant to run and skip is good, or there's some misconfiguration and then None is not really expected.
pytest_django/plugin.py
Outdated
| return mail.outbox # type: ignore[no-any-return] | ||
| if hasattr(mail, "outbox"): | ||
| return mail.outbox # type: ignore[no-any-return] | ||
| return None |
There was a problem hiding this comment.
I'm not sure about returning None. The situation is that someone explicitly used the mailoutbox fixture, but didn't properly configure Django to actually fill it. Do they expect None? I think probably not.
So I think we should error in this case, with a proper message telling the user what they need to do to get the mailoutbox fixture to work (configure email backend to locmem), or if an error is too harsh, then a warning and return [], this way we at least get rid of the pesky None that no one expects. I'd go with the error.
There was a problem hiding this comment.
I was on the fence about this one.. I've made the change
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Closes: #993
Replaces: #1033