MPP-3352: Expand, refactor tests #4434
Conversation
This exposes an issue that the DomainAddress.address (the local part of a custom mask) is allowed to change with a PATCH. This field should be writable at creation and read-only later. Tracked in MPP-3753.
This pattern is used as the signal that the address was generated on a website, and will be a distinct interaction event in the future.
Previously, the shared ProfileTestCase.upgrade_to_premium() could randomly select a VPN bundle plan. It now only selects a unlimited email protection plan. Addition helpers .upgrade_to_phone and .upgrade_to_vpn_bundle can upgrade the test user to those plan levels. This exposed some issues with the Django model cache around User and Profile. The make_free_test_user and make_premium_test_user functions now update the profile through user.profile, making the cache consistent. make_premium_test_user can still select a VPN bundle for a user.
Rename unlimited_subscription to premium_subscription, and only return unlimited email plans. Use in upgrade_test_user_to_premium, so that these users are only ones with the unlimited email plans, not the VPN bundle.
Split SNSNotificationTest into three parts: - SNSNotificationTestBase - shared setup and helper functions - SNSNotificationIncomingTest - tests for emails to Relay users - SNSNotificationRepliesTest - test for email replies from Relay users
Test more failure modes for incoming emails and Relay user replies. Check log messages and emitted metrics.
| ("num_forwarded", -1), | ||
| ("num_blocked", -1), | ||
| ("num_spam", -1), | ||
| ("num_level_one_trackers_blocked", -1), | ||
| ("num_replied", -1), |
There was a problem hiding this comment.
question (non-blocking): Why all these negative (invalid?) values? Could this be accidentally testing the validation of these fields and not testing the read-only aspect of them?
There was a problem hiding this comment.
I didn't want to accidentally use the original value, so I picked a value it definitely could not be. I thought the 200 response was sufficient for saying it was not a validation error.
I could first do a GET and then write the next value for integers, if the invalid value worries you.
There was a problem hiding this comment.
suggestion (non-blocking): if it's not too much work, let's try to GET and then write the next value.
| ("num_forwarded", -1), | ||
| ("num_blocked", -1), | ||
| ("num_spam", -1), | ||
| ("num_level_one_trackers_blocked", -1), | ||
| ("num_replied", -1), |
There was a problem hiding this comment.
question (non-blocking): same question as above: should we use -1 values here?
There was a problem hiding this comment.
I'm going to remove some duplicate code around premium subscriptions.
There is a potential bigger change around assert_email_equals that could be it's own PR. Let me know if you think this should be done in this PR.
I've answered the other questions, and I think there are no code changes for those.
| ("num_forwarded", -1), | ||
| ("num_blocked", -1), | ||
| ("num_spam", -1), | ||
| ("num_level_one_trackers_blocked", -1), | ||
| ("num_replied", -1), |
There was a problem hiding this comment.
I didn't want to accidentally use the original value, so I picked a value it definitely could not be. I thought the 200 response was sufficient for saying it was not a validation error.
I could first do a GET and then write the next value for integers, if the invalid value worries you.
|
I've got some time, I'm planning to change |
Replace SNSNotificationTestBase.get_details_from_mock_send_raw_email() with SNSNotificationTestBase.assert_email_equals() which extracts the sent email from mock_send_raw_email and calls the helper assert_email_equals(). This implicitly checks headers and content, so remove redundant checks. Optionally check the source and destination values passed to send_raw_email() as well.
|
The further changes:
|
Rename the top-level function to assert_email_equals_fixture, and the test case method to check_sent_email_matches_fixture. Rearrange and rename the parameters for consistency.
In preparation for adding Glean metrics in MPP-3352, this PR expands and refactors the tests. This will reduce the size and complexity of the near-future Glean PR.
There is one code change, to remove a redundant error log. The rest are test changes, so running the tests should be sufficient to validate the PR.
Each commit is roughly one refactor:
version_json_pathfixturePATCHandDELETEtests for address APIs. This exposes an issue that theDomainAddress.address(the local part of a custom mask) is allowed to change with aPATCH. This field should be writable at creation and read-only later. Tracked in MPP-3753.RandomAddresswithgenerated_for. This pattern is used as the signal that the address was generated on a website, and will be a distinct interaction event in the future.log_recordtest utilityTYPE_CHECKINGlines in coverage. This code is only evaluated by static type checkers, so it is impossible to get code coverage for them.ProfileTestCase.upgrade_to_premium() could randomly select a VPN bundle plan. It now only selects a unlimited email protection plan. Addition helpers.upgrade_to_phoneand.upgrade_to_vpn_bundlecan upgrade the test user to those plan levels. This exposed some issues with the Django model cache aroundUserandProfile. Themake_free_test_userandmake_premium_test_userfunctions now update the profile throughuser.profile, making the cache consistent.unlimited_subscriptiontopremium_subscription, and only return unlimited email plans. Use inupgrade_test_user_to_premium, so that these users are only ones with the unlimited email plans, not the VPN bundle.SNSNotificationTestinto three parts:SNSNotificationTestBase- shared setup and helper functions-
SNSNotificationIncomingTest- tests for emails to Relay usersSNSNotificationRepliesTest- test for email replies from Relay usersassertLogsinstead of mock loggerl10n changes have been submitted to the l10n repository, if any.I've added or updated relevant docs in the docs/ directory.All UI revisions follow the coding standards, and use Protocol tokens where applicable (see/frontend/src/styles/tokens.scss).