Opened 98 minutes ago
#36953 assigned Cleanup/optimization
Refactor Django mail tests
| Reported by: | Mike Edmunds | Owned by: | Mike Edmunds |
|---|---|---|---|
| Component: | Core (Mail) | Version: | 6.0 |
| Severity: | Normal | Keywords: | tests |
| Cc: | Triage Stage: | Unreviewed | |
| Has patch: | no | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | no |
| Easy pickings: | no | UI/UX: | no |
Description
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:
- 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.
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.)
- 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: test_wrong_admins_managers()). Failures in those tests are noisy (repeated 5x) and confusingly attributed to different backend configurations.
All BaseEmailBackendTests cases that aren't covering backend-dependent behavior should be moved to one of the test classes created by item 1.
- There are several compound test methods (example: test_backend_arg()). These should use subTest() to isolate independent cases (or be split into separate test methods).
- [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.)
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