mozilla / fx-private-relay

Keep your email safe from hackers and trackers. Make an email alias with 1 click, and keep your address to yourself.
https://relay.firefox.com
Other
1.44k stars 168 forks source link

MPP-3827: Switch from `RequestsClient` to `APIClient`, remove transaction tests #4801

Closed jwhitlock closed 1 week ago

jwhitlock commented 1 week ago

The RequestsClient docs have this detail:

The RequestsClient class is useful if you want to write tests that solely interact with the service interface. This is a little stricter than using the standard Django test client, as it means that all interactions should be via the API.

If you're using RequestsClient you'll want to ensure that test setup, and results assertions are performed as regular API calls, rather than interacting with the database models directly. For example, rather than checking that Customer.objects.count() == 3 you would list the customers endpoint, and ensure that it contains three records.

The existing IQ tests mixed API requests with direct database interactions, such as test_iq_endpoint_disabled_number(), which created a disabled phone using the Django ORM. I could not see a way to do the test setup with the API, so RequestsClient can't be used for all the tests.

Once the tests are converted to use APIClient, the transaction test case is no longer needed. This is good because pytest-django uses TransactionalTestCase, but with allow_cascade=False (Django code). This means test cleanup in transaction tests will run TRUNCATE TABLE emails_profile, emails_relayaddress..., but without CASCADE. This makes it very hard to pass the CircleCI migration tests when adding new models with relations to existing models. @say-yawn saw this issue in PR #4688, but we didn't tackle it. Removing all the transaction=true puts it off for another day, and also speeds up tests.