Improve add-member API functests #9110

seanh · 2024-11-22T12:57:03Z

The functional tests for the add-member API are a mess: they're in kind
of an old-fashioned coding style, but more importantly there are some
missing test cases and there are several tests that aren't actually
testing what they're supposed to be testing because the test setup is
wrong, and at least one of these tests was passing when there was
actually a crash in the code.

A difficulty is that the h API doesn't return specific error responses.
Whenever anything goes wrong with an add-member API request h returns a
404 with the same fixed body that doesn't say specifically what was
wrong with the request. This means that tests can't assert that they've
triggered the error that they intended to trigger. They can only assert
that the response is a 404. Several of add-member's functests were
actually triggering a different 404 than what the test was intending
to trigger because the test setup was wrong, but the tests were still
passing.

For example this test doesn't actually test what it's intending to test:

def test_it_returns_404_if_authority_mismatch_on_user(
    self, app, factories, auth_client
):
    group = factories.Group()
    user = factories.User(authority="somewhere-else.org")

    res = app.post_json(
        "/api/groups/{pubid}/members/{userid}".format(
            pubid=group.pubid, userid=user.userid
        ),
        headers=self.authorization_header(auth_client),
        expect_errors=True,
    )

    assert res.status_code == 404

The group that the test has created has an authority that doesn't
match that of the auth_client so the request will 404 for that reason
before the code ever gets to the user's authority. In addition the
db_session hasn't been commited so neither group or user have been
written to the DB, which will also cause the request to 404. The test
needs to create a group with the right authority, a user with the wrong
authority, and add both to the DB.

In this commit I've adopted an approach where class-level test fixtures
set up everything that's needed for the API request to succeed. The
test for the happy-path doesn't need to do any setup beyond what the
fixtures have already done. Tests for error cases then change one
particular thing before calling the API. This means the test can be sure
that the 404 was triggered by the one thing it changed rather than by
anything else. For example:

class TestAddMember:
    def test_it(self, do_request, group, user):
        do_request()

        assert user in group.members

    def test_it_errors_if_the_request_isnt_authenticated(self, do_request, headers):
        del headers["Authorization"]

        do_request(status=404)

    ...

    @pytest.fixture
    def authclient(self):
        """Return a group with an authority matching authclient's."""

    @pytest.fixture
    def headers(self, authclient):
        """Return an `Authorization` header for authclient."""

    @pytest.fixture
    def group(self, authclient):
        """Return a group with an authority matching authclient's."""

    @pytest.fixture
    def user(self, authclient):
        """Return a user with an authority matching authclient's."""

    @pytest.fixture
    def do_request(self, db_session, group, user, headers):
        def do_request(...):
            """Commit the db_session then make an API request with group, user and headers."""
        return do_request

The happy path test (test_it()) verifies that, with everything as set
up by the test fixtures, the API request succeeds and adds the user to
the group.

The error test (test_it_errors_if_the_request_isnt_authenticated())
changes one thing about the fixtures set up (it deletes the
Authorization header) and asserts that the request 404s.

This approach makes mistakes in tests much less likely.

I have also gone through breaking specific parts of the context,
security, view and service code and testing that the expected test
fails. I did this for every functest in the class.

This commit also fixes one bug in the code: if the add-member API is called
with an invalid userid (one that isn't in acct:{username}@{authority}
format) the app crashes. The previous functests actually had a test for
this case, but the test was wrong:

def test_it_returns_404_if_malformed_userid(
    self, app, factories, auth_client, db_session
):
    group = factories.Group()
    db_session.commit()

    res = app.post_json(
        "/api/groups/{pubid}/members/{userid}".format(
            pubid=group.pubid, userid="[email protected]"
        ),
        headers=self.authorization_header(auth_client),
        expect_errors=True,
    )

    assert res.status_code == 404

This test creates a group whose authority doesn't match that of the
auth_client and the request 404s for that reason before the code ever
gets to parsing the malformed userid. So the test passes. In fact if the
test had set up the group's authority correctly the app would crash on
the malformed userid. This commit replaces this test with a correct one.

seanh · 2024-11-22T12:58:32Z