zulip/zulip

Simplify signup tests for clarity and flexibility

Open

#7,564 创建于 2017年11月29日

在 GitHub 查看
 (13 评论) (0 反应) (0 负责人)Python (19,672 star) (7,339 fork)batch import
area: authenticationarea: refactoringarea: testing-infrastructurehelp wanted

描述

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.:
    • realm would default to get_realm("zulip").
    • The next test in the file, test_signup_invalid_name, passes a funny full_name value to submit_reg_form_for_user, which in fact is the point of that test -- so signup would have an optional argument full_name, which that test might be the only user of.
  • Optional arguments to expect failure. E.g., test_signup_invalid_name expects the flow to stop after submit_reg_form_for_user. So signup would have an optional argument like fail_reg_form. To allow the test to do its own inspection of the details of the failure, maybe signup would return result at 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.

贡献者指南