描述
One of our largest test files is zerver/tests/test_signup.py, at 2490 lines. It's appropriate to have lots of tests there -- it's really important that signup work right, it has a lot of different cases, and it's an area that few members of the Zulip community regularly encounter (because our daily use of Zulip is on accounts that we've already signed up). But a lot of that line count is basically repetition and boilerplate, which makes it hard to understand what's being tested and makes it likely that there are things we aren't testing which we should be.
A typical example:
def test_signup_existing_email(self) -> None:
"""
Check if signing up with an email used in another realm succeeds.
"""
email = self.example_email('hamlet')
password = "newpassword"
realm = get_realm('lear')
result = self.client_post('/accounts/home/', {'email': email}, subdomain="lear")
self.assertEqual(result.status_code, 302)
result = self.client_get(result["Location"], subdomain="lear")
confirmation_url = self.get_confirmation_url_from_outbox(email)
result = self.client_get(confirmation_url, subdomain="lear")
self.assertEqual(result.status_code, 200)
result = self.submit_reg_form_for_user(email, password, subdomain="lear")
self.assertEqual(result.status_code, 302)
get_user(email, realm)
self.assertEqual(UserProfile.objects.filter(email=email).count(), 2)
Ideally this test would look more like:
def test_signup_existing_email(self) -> None:
self.signup(email=self.example_email('hamlet'),
realm=get_realm("lear"))
self.assertEqual(UserProfile.objects.filter(email=email).count(), 2)
All the steps of going through the signup flow, and checking that things look good at each stage, are common to lots of tests and they should be in a common function like my hypothetical self.signup. That would (a) make many tests much shorter, and as a bonus (b) help us do all those checks every time, for added assurance that the tests aren't missing something.
Key to writing a function like this self.signup are a couple of principles:
- Lots of optional arguments. E.g.:
realmwould default toget_realm("zulip").- The next test in the file,
test_signup_invalid_name, passes a funnyfull_namevalue tosubmit_reg_form_for_user, which in fact is the point of that test -- sosignupwould have an optional argumentfull_name, which that test might be the only user of.
- Optional arguments to expect failure. E.g.,
test_signup_invalid_nameexpects the flow to stop aftersubmit_reg_form_for_user. Sosignupwould have an optional argument likefail_reg_form. To allow the test to do its own inspection of the details of the failure, maybesignupwouldreturn resultat that stage -- so the test would look like
def test_signup_invalid_name(self) -> None:
result = self.signup(full_name="<invalid>",
stop_after='reg_form')
self.assert_in_success_response(
["Invalid characters in name!", 'id_password', 'id_full_name'], result)
These example functions shrink 4-5x, to 20-25% of their length. That doesn't account for the helpers (like signup) themselves, and some test functions already have less boilerplate or may be harder to compress. Overall I think it should be feasible to reduce this file from about 2500 lines now to less than half that, like 1000-1200 lines, without removing any test functionality (and in fact adding some because the helpers will do the same scrupulous step-by-step checks every time.)
@showell has written a number of test helpers along these lines, like check_sent_emails in this file, and may have thoughts to add. @hackerkid, mentioning you because you've also written much of this file, including some helpers on these lines like check_user_able_to_register.