#36953: Refactor Django mail tests
-------------------------------------+-------------------------------------
Reporter: Mike | Owner: Mike Edmunds
Edmunds |
Type: | Status: assigned
Cleanup/optimization |
Component: Core | Version: 6.0
(Mail) |
Severity: Normal | Keywords: tests
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
= Refactor Django mail tests =
A bit of cleanup in tests/mail/tests.py would make it easier to edit and
review…
* That file is currently >3200 lines long—the ninth largest in Django's
~280 test files. (And it's about to get bigger.) It's hard to load into a
''human's'' context window (at least mine), let alone an AI's. GitHub's PR
viewer often just cries "too large" and gives up.
* Parts of the file are confusingly organized, making it difficult for
contributors to create or update relevant tests and for reviewers to spot
duplicates and discrepancies.
Specifically:
1. The catchall MailTests class has over 80 individual test cases covering
the EmailMessage classes, the function-based mail APIs, plus a grab bag of
other things. Related cases are ''mostly'' grouped near each other, but
it's not always consistent.
[[br]][[br]]
MailTests should be split up into smaller, more focused classes: one
just for EmailMessage and EmailMultiAlternatives, plus a few others
grouping related functional APIs. (A detailed refactoring plan is below.)
2. The BaseEmailBackendTests class defines a set of common cases that need
to be tested individually against each EmailBackend, via subclasses. But
it also has accumulated several tests that are ''not'' backend related and
''don't'' need to be repeated (example:
[https://github.com/django/django/blob/08b4dfc5734f5d2fce685eabcd65385a6656db2f/tests/mail/tests.py#L2428-L2457
test_wrong_admins_managers()]). Failures in those tests are noisy
(repeated 5x) and confusingly attributed to different backend
configurations.
[[br]][[br]]
All BaseEmailBackendTests cases that ''aren't'' covering backend-
dependent behavior should be moved to one of the test classes created by
item 1.
3. There are several compound test methods (example:
[https://github.com/django/django/blob/08b4dfc5734f5d2fce685eabcd65385a6656db2f/tests/mail/tests.py#L1295-L1327
test_backend_arg()]). These should use subTest() to isolate independent
cases (or be split into separate test methods).
4. [optional] The file currently includes tests for both the public
django.core.mail APIs plus the individual EmailBackend classes. This
accounts for its length, and co-locating the two types of tests encourages
the problem described in item 2. (Compare to tests for the newer tasks
feature, which uses separate files in tests/tasks/ for the public APIs and
individual task backends.)
[[br]][[br]]
The backend tests should be moved from mail/tests.py to a new
mail/test_backends.py, leaving mail/tests.py covering the public APIs and
a few internals that are tested separately. (We could also split
individual backends into separate test files: smtp, locmem, etc., but
given the shared BaseEmailBackendTests it seems reasonable to keep them
together.)
I realize splitting a file complicates reviewing its history and bisecting
future issues. There's value in the first three cleanup items even if we
keep all the tests in a single large file, so I've marked the fourth
"optional." But splitting is the best way to avoid more instances of item
2 in the future, and the only way to make the file a more manageable size.
These changes are mostly mechanical, rearranging existing test cases
without altering them. (Item 2 would change test content—though retain the
current intent.) But there will be a huge diff. Each of the items above
can (and should) be a separate commit.
'''Timing:''' This cleanup needs to be carefully coordinated with #35514.
It could simplify that work (by better organizing the large number of
tests that will need updating for that ticket) or complicate it (by
creating merge conflicts with in-progress work).
== Proposed refactoring ==
Here is an outline of mail/tests.py, ignoring utilities and helpers. "→"
indicates recommended refactorings. "+subTest" identifies compound tests
that should be subdivided. "+" before a class name marks new classes that
would be created in the refactoring.
(Incidentally, this list also exposes the fact that we're missing basic
functional tests for send_mass_mail(). That could be addressed as a
separate ticket, later.)
* MailTests → rename to EmailMessageTests; move non-
EmailMessage/EmailMultiAlternatives cases to other (new) test classes
* test_ascii
* test_nonascii_as_string_with_ascii_charset (RemovedInDjango70) →
DeprecatedInternalsTests
* test_multiple_recipients
* test_header_omitted_for_no_to_recipients
* test_recipients_with_empty_strings
* test_cc +subTest
* test_cc_headers
* test_cc_in_headers_only
* test_bcc_not_in_headers
* test_reply_to +subTest
* test_recipients_as_tuple
* test_recipients_as_string +subTest
* test_header_injection
* test_folding_white_space
* test_message_header_overrides
* test_datetime_in_date_header
* test_from_header
* test_to_header +subTest
* test_to_in_headers_only
* test_reply_to_header
* test_reply_to_in_headers_only
* test_lazy_headers
* test_multiple_message_call
* test_unicode_address_header +subTest
* test_unicode_headers
* test_non_utf8_headers_multipart
* test_multipart_with_attachments
* test_alternatives
* test_alternatives_constructor
* test_alternatives_constructor_from_tuple
* test_alternative_alternatives
* test_alternatives_and_attachment_serializable
* test_none_body
* test_non_ascii_dns_non_unicode_email
* test_encoding
* test_encoding_alternatives
* test_attachments
* test_attachments_constructor
* test_attachments_constructor_from_tuple
* test_attachments_constructor_omit_mimetype
* test_attachments_with_alternative_parts
* test_decoded_attachment_text_MIMEPart
* test_non_ascii_attachment_filename
* test_attach_file
* test_attach_text_as_bytes
* test_attach_text_as_bytes_using_property
* test_attach_utf8_text_as_bytes
* test_attach_non_utf8_text_as_bytes
* test_attach_8bit_rfc822_message_non_ascii
* test_attach_mime_image (RemovedInDjango70)
* test_attach_mime_part
* test_attach_mime_image_in_constructor (RemovedInDjango70)
* test_attach_mime_part_in_constructor
* test_attach_rfc822_message
* test_attach_mimepart_prohibits_other_params +subTest
* test_attach_content_is_required
* test_dummy_backend → LocmemBackendTests
* test_arbitrary_keyword → GetConnectionTests
* test_custom_backend → GetConnectionTests
* test_backend_arg → GetConnectionTests+subTest
* test_connection_arg_send_mail → SendMailTests
* test_connection_arg_send_mass_mail → SendMassMailTests
* test_connection_arg_mail_admins → MailAdminsAndManagersTests
* test_connection_arg_mail_managers → MailAdminsAndManagersTests
* test_dont_mangle_from_in_body
* test_body_content_transfer_encoding +subTest
* test_sanitize_address (RemovedInDjango70) → DeprecatedInternalsTests
* test_sanitize_address_invalid (RemovedInDjango70) →
DeprecatedInternalsTests
* test_sanitize_address_header_injection (RemovedInDjango70) →
DeprecatedInternalsTests
* test_address_header_handling
* test_address_header_injection
* test_localpart_only_address
* test_email_multi_alternatives_content_mimetype_none +subTest
* test_mime_structure
* test_body_contains
* test_body_contains_alternative_non_text
* test_all_params_optional
* test_positional_arguments_order
* test_all_params_can_be_set_before_send
* test_message_is_python_email_message
* test_message_policy_smtputf8
* test_message_policy_cte_7bit
* test_message_policy_compat32
* +SendMailTests: new class for send_mail() test cases (moved from
elsewhere in the file)
* +SendMassMailTests: new class for send_mass_mail() test cases (moved
from elsewhere in the file)
* +MailAdminsAndManagersTests: new class for mail_admins() and
mail_managers() test cases (moved from elsewhere in the file; these two
functions are almost always tested in tandem)
* +GetConnectionTests: new class for get_connection() test cases (moved
from elsewhere in the file)
* +DeprecatedInternalsTests (RemovedInDjango70): new class for internals
test cases (moved from elsewhere in the file)
* MailDeprecatedPositionalArgsTests (RemovedInDjango70) → no changes
* test_get_connection
* test_send_mail
* test_send_mass_mail
* test_mail_admins
* test_mail_managers
* test_email_message_init
* test_email_multi_alternatives_init
* MailTimeZoneTests → rename to EmailMessageTimeZoneTests or move cases
into EmailMessageTests
* test_date_header_utc
* test_date_header_localtime
* PythonGlobalState (RemovedInDjango70) → no changes
* test_utf8
* test_7bit
* test_8bit_latin
* test_8bit_non_latin
* BaseEmailBackendTests → move this and all subclasses to
mail/test_backends.py
* test_send (the str/unicode distinction went away in Python 3, so this
and the next test are now redundant; could clean up in a later ticket)
* test_send_unicode
* test_send_long_lines
* test_send_many
* test_send_verbose_name → EmailMessageTests
* test_plaintext_send_mail → SendMailTests
* test_html_send_mail → SendMailTests
* test_mail_admins_and_managers → MailAdminsAndManagersTests
* test_html_mail_managers → MailAdminsAndManagersTests
* test_html_mail_admins → MailAdminsAndManagersTests
* test_manager_and_admin_mail_prefix → MailAdminsAndManagersTests
* test_empty_admins → MailAdminsAndManagersTests
* test_deprecated_admins_managers_tuples (RemovedInDjango70) →
MailAdminsAndManagersTests
* test_wrong_admins_managers → MailAdminsAndManagersTests
* test_message_cc_header → EmailMessageTests
* test_idn_send
* test_recipient_without_domain
* test_lazy_addresses
* test_close_connection
* test_use_as_contextmanager
* LocmemBackendTests(BaseEmailBackendTests) → no changes
* (all shared tests from BaseEmailBackendTests)
* test_locmem_shared_messages
* test_validate_multiline_headers (ticket*18861—not a duplicate of
MailTests.test_header_injection)
* test_outbox_not_mutated_after_send
* FileBackendTests(BaseEmailBackendTests) → no changes
* (all shared tests from BaseEmailBackendTests)
* test_file_sessions
* FileBackendPathLibTests(FileBackendTests) → no changes
* (repeats FileBackendTests with EMAIL_FILE_PATH=Path rather than str)
* ConsoleBackendTests(BaseEmailBackendTests) → no changes
* (all shared tests from BaseEmailBackendTests)
* test_console_stream_kwarg
* SMTPBackendTests(BaseEmailBackendTests) → no changes (but maybe consider
splitting into settings/config/init vs behavior tests later)
* (all shared tests from BaseEmailBackendTests)
* test_email_authentication_use_settings
* test_email_authentication_override_settings
* test_email_disabled_authentication
* test_auth_attempted
* test_server_open
* test_reopen_connection
* test_email_tls_use_settings
* test_email_tls_override_settings
* test_email_tls_default_disabled
* test_ssl_tls_mutually_exclusive
* test_email_ssl_use_settings
* test_email_ssl_override_settings
* test_email_ssl_default_disabled
* test_email_ssl_certfile_use_settings
* test_email_ssl_certfile_override_settings
* test_email_ssl_certfile_default_disabled
* test_email_ssl_keyfile_use_settings
* test_email_ssl_keyfile_override_settings
* test_email_ssl_keyfile_default_disabled
* test_email_tls_attempts_starttls
* test_email_ssl_attempts_ssl_connection
* test_connection_timeout_default
* test_connection_timeout_custom
* test_email_timeout_override_settings
* test_email_msg_uses_crlf
* test_send_messages_after_open_failed
* test_send_messages_empty_list
* test_send_messages_zero_sent
* test_avoids_sending_to_invalid_addresses
* test_encodes_idna_in_smtp_commands
* test_does_not_reencode_idna
* test_rejects_non_ascii_local_part
* test_prep_address_without_force_ascii
* SMTPBackendStoppedServerTests → move to test_backends.py along with
SMTPBackendTests; otherwise no changes (tests that require a modified mock
SMTP server, so can't be included in SMTPBackendTests)
* test_server_stopped
* test_fail_silently_on_connection_error
* LegacyAPINotUsedTests → no changes
* test_no_legacy_apis_imported
--
Ticket URL: <https://code.djangoproject.com/ticket/36953>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To view this discussion visit
https://groups.google.com/d/msgid/django-updates/0107019c9710d3db-4473c998-c483-407a-8596-27864ba985bd-000000%40eu-central-1.amazonses.com.
