tests: make Ninja APIs more testable (bug 2004871) r=zeid

 * tests: move Ninja Test Client to conftests
 * tests: make user fixture more flexible wrt. permissions

As there was two `api_client` in use in various tests, the were renamed `ninja_api_client` and `github_api_client` as appropriate.

Pull request: #767

Author: shtrom

src/lando/conftest.py

@@ -16,6 +16,8 @@
 from django.conf import settings
 from django.contrib.auth.models import Permission, User
 from django.contrib.contenttypes.models import ContentType
+from ninja import NinjaAPI
+from ninja.testing import TestClient
 from requests.models import HTTPError
 
 from lando.api.legacy.stacks import (
@@ -928,16 +930,26 @@ def _create_hg_commit(clone_path: Path) -> str:
 
 
 @pytest.fixture
-def conduit_permissions():
+def to_profile_permissions() -> Callable:
+    """Convert a list of un-namespaced permissions strings to a list of profile Permissions."""
+
+    def _to_profile_permissions(permissions: list[str]) -> list[Permission]:
+        all_perms = Profile.get_all_scm_permissions()
+
+        return [all_perms[p] for p in permissions]
+
+    return _to_profile_permissions
+
+
+@pytest.fixture
+def conduit_permissions(to_profile_permissions: Callable) -> list[Permission]:
     permissions = (
         "scm_level_1",
         "scm_level_2",
         "scm_level_3",
         "scm_conduit",
     )
-    all_perms = Profile.get_all_scm_permissions()
-
-    return [all_perms[p] for p in permissions]
+    return to_profile_permissions(permissions)
 
 
 @pytest.fixture
@@ -956,23 +968,42 @@ def _instance(scm, **kwargs) -> Worker:
 
 
 @pytest.fixture
-def user_phab_api_key():
-    return "api-123456789012345678901234567x"
+def scm_user() -> Callable:
+    def scm_user(perms: list[Permission], password: str = "password") -> User:
+        """Return a user with the selected Permissions and password."""
+        user = User.objects.create_user(
+            username="test_user",
+            password=password,
+            email="testuser@example.org",
+        )
 
+        user.profile = Profile(user=user, userinfo={"name": "test user"})
 
-@pytest.fixture
-def user(user_plaintext_password, conduit_permissions, user_phab_api_key):
-    user = User.objects.create_user(
-        username="test_user",
-        password=user_plaintext_password,
-        email="testuser@example.org",
-    )
+        for permission in perms:
+            user.user_permissions.add(permission)
 
-    user.profile = Profile(user=user, userinfo={"name": "test user"})
+        user.save()
+        user.profile.save()
 
-    for permission in conduit_permissions:
-        user.user_permissions.add(permission)
+        return user
 
+    return scm_user
+
+
+@pytest.fixture
+def user_phab_api_key():
+    return "api-123456789012345678901234567x"
+
+
+@pytest.fixture
+def user(
+    scm_user: Callable,
+    user_plaintext_password: str,
+    conduit_permissions: list[str],
+    user_phab_api_key: str,
+) -> User:
+    """A User with all SCM permissions levels."""
+    user = scm_user(conduit_permissions, user_plaintext_password)
     user.profile.save_phabricator_api_key(user_phab_api_key)
 
     user.save()
@@ -1340,3 +1371,24 @@ def _mock_response(
         )
 
     return _mock_response
+
+
+@pytest.fixture()
+def ninja_api_client() -> Callable:
+    """Fixture to create a test client for the API."""
+
+    # XXX If we pass the API directly, we get an error if we want to use this client
+    # more than once (regardless of the scope of the fixture), as follows:
+    #
+    #    Looks like you created multiple NinjaAPIs or TestClients
+    #    To let ninja distinguish them you need to set either unique version or urls_namespace
+    #     - NinjaAPI(..., version='2.0.0')
+    #     - NinjaAPI(..., urls_namespace='otherapi')
+    #
+    # Passing the pre-existing router to the TestClient instead, works. However, getting the
+    # router is not golden-path.
+    #
+    def _ninja_api_client(api: NinjaAPI) -> TestClient:
+        return TestClient(api.default_router)
+
+    return _ninja_api_client

src/lando/try_api/tests/test_authentication.py

@@ -1,39 +1,22 @@
+from typing import Callable
 from unittest.mock import MagicMock, patch
 
 import pytest
 from django.contrib.auth.models import User
 from django.test import Client, override_settings
-from ninja.testing import TestClient
 
 from lando.environments import Environment
 from lando.try_api.api import api
 
 
-@pytest.fixture()
-def api_client():
-    """Fixture to create a test client for the API."""
-    # XXX If we pass the API directly, we get an error if we want to use this client
-    # more than once (regardless of the scope of the fixture), as follows:
-    #
-    #    Looks like you created multiple NinjaAPIs or TestClients
-    #    To let ninja distinguish them you need to set either unique version or urls_namespace
-    #     - NinjaAPI(..., version='2.0.0')
-    #     - NinjaAPI(..., urls_namespace='otherapi')
-    #
-    # Passing the pre-existing router to the TestClient instead, works. However, getting the
-    # router is not golden-path.
-    #
-    return TestClient(api._routers[0][1])
-
-
 @pytest.mark.django_db()
 @patch("lando.try_api.api.AccessTokenAuth.authenticate")
 def test_authentication_valid_token(
-    mock_authenticate: MagicMock, api_client: TestClient
+    mock_authenticate: MagicMock, ninja_api_client: Callable
 ):
     mock_authenticate.return_value = User(username="testuser", is_active=True)
 
-    response = api_client.get(
+    response = ninja_api_client(api).get(
         "/__userinfo__",
         # The value of the token doesn't actually matter, as the output is controlled by
         # the authenticator function, which we mock to return a User.
@@ -45,19 +28,19 @@ def test_authentication_valid_token(
 
 
 @pytest.mark.django_db()
-def test_authentication_no_token(client: Client, api_client: TestClient):
-    response = api_client.get("/__userinfo__")
+def test_authentication_no_token(client: Client, ninja_api_client: Callable):
+    response = ninja_api_client(api).get("/__userinfo__")
     assert response.status_code == 401, "Missing token should result in 401"
 
 
 @pytest.mark.django_db()
 @patch("lando.try_api.api.AccessTokenAuth.authenticate")
 def test_authentication_invalid_token(
-    mock_authenticate: MagicMock, api_client: TestClient
+    mock_authenticate: MagicMock, ninja_api_client: Callable
 ):
     mock_authenticate.return_value = None
 
-    response = api_client.get(
+    response = ninja_api_client(api).get(
         "/__userinfo__",
         # The value of the token doesn't actually matter, as the output is controlled by
         # the authenticator function, which we mock to return None.
@@ -71,9 +54,9 @@ def test_authentication_invalid_token(
 @pytest.mark.django_db()
 @override_settings(ENVIRONMENT=Environment("production"))
 @patch("lando.try_api.api.AccessTokenAuth.authenticate")
-def test_userinfo_not_in_prod(mock_authenticate: MagicMock, api_client: TestClient):
+def test_userinfo_not_in_prod(mock_authenticate: MagicMock, ninja_api_client: Callable):
     mock_authenticate.return_value = User(username="testuser", is_active=True)
-    response = api_client.get(
+    response = ninja_api_client(api).get(
         "/__userinfo__", headers={"AuThOrIzAtIoN": "bEaReR valid_token"}
     )
 

src/lando/utils/tests/test_github.py

@@ -158,15 +158,15 @@ def test_github_authenticated_url_no_token(
 
 
 def test_github_api_init(mock_github_fetch_token: mock.Mock):
-    api_client = GitHubAPI("https://github.com/o/r")
+    github_api_client = GitHubAPI("https://github.com/o/r")
 
-    assert api_client.session.headers.get("Authorization") == "Bearer mock_token"
+    assert github_api_client.session.headers.get("Authorization") == "Bearer mock_token"
 
 
 def test_github_api_client_init(mock_github_fetch_token: mock.Mock):
-    api_client = GitHubAPIClient("https://github.com/o/r")
+    github_api_client = GitHubAPIClient("https://github.com/o/r")
 
-    assert api_client.repo_base_url == "repos/o/r"
+    assert github_api_client.repo_base_url == "repos/o/r"
 
 
 @pytest.fixture
@@ -378,29 +378,29 @@ def test_api_client_build_pr(
     github_pr_diff: str,
     github_pr_patch: str,
 ):
-    api_client = GitHubAPIClient("https://github.com/mozilla-conduit/test-repo")
+    github_api_client = GitHubAPIClient("https://github.com/mozilla-conduit/test-repo")
 
-    api_client.get_pull_request = mock.MagicMock()
-    api_client.get_pull_request.return_value = json.loads(github_pr_response)
+    github_api_client.get_pull_request = mock.MagicMock()
+    github_api_client.get_pull_request.return_value = json.loads(github_pr_response)
 
-    api_client.get_diff = mock.MagicMock()
-    api_client.get_diff.return_value = github_pr_diff
+    github_api_client.get_diff = mock.MagicMock()
+    github_api_client.get_diff.return_value = github_pr_diff
 
-    api_client.get_patch = mock.MagicMock()
-    api_client.get_patch.return_value = github_pr_patch
+    github_api_client.get_patch = mock.MagicMock()
+    github_api_client.get_patch.return_value = github_pr_patch
 
-    pr = api_client.build_pull_request(1)
+    pr = github_api_client.build_pull_request(1)
 
-    assert api_client.get_pull_request.call_count == 1
+    assert github_api_client.get_pull_request.call_count == 1
     assert pr.number == 1
 
     assert pr.diff == github_pr_diff
-    assert api_client.get_diff.call_count == 1
-    assert api_client.get_diff.call_args.args == (1,)
+    assert github_api_client.get_diff.call_count == 1
+    assert github_api_client.get_diff.call_args.args == (1,)
 
     assert pr.patch == github_pr_patch
-    assert api_client.get_patch.call_count == 1
-    assert api_client.get_patch.call_args.args == (1,)
+    assert github_api_client.get_patch.call_count == 1
+    assert github_api_client.get_patch.call_args.args == (1,)
 
 
 @pytest.fixture