lint and refactor integration tests

charm/src/charm.py

@@ -86,7 +86,7 @@ def _on_redeliver_failed_webhooks_action(self, event: ops.charm.ActionEvent) ->
             logger.warning("Webhook redelivery failed, script reported: %s", exc.stderr)
             event.fail("Webhooks redelivery failed. Look at the juju logs for more information.")
 
-    def _get_github_auth_env(self, event: ActionEvent) -> dict[str, str]:
+    def _get_github_auth_env(self, event: ActionEvent) -> dict[str, str | None]:
         """Get the GitHub auth environment variables from the action parameters.
 
         Args:

tests/integration/conftest.py

@@ -90,14 +90,14 @@ def github_app_private_key_fixture(pytestconfig: pytest.Config) -> str | None:
 
 
 @pytest.fixture(
-    name="github_app_auth",
+    name="github_auth",
     scope="module",
     params=[
         pytest.param(True, id="use github token"),
         pytest.param(False, id="use github app auth"),
     ],
 )
-def github_app_auth_fixture(
+def github_auth_fixture(
     request: pytest.FixtureRequest,
     github_token: str | None,
     github_app_client_id: str | None,

tests/integration/test_app.py

@@ -30,11 +30,12 @@
 
 
 @pytest_asyncio.fixture(name="app", scope="module")
-async def app(
+async def app_fixture(
     router: Application,
     mongodb: Application,
     deploy_config: dict[str, Any],
 ) -> Application:
+    """Relate the router with mongodb and return the router application."""
     if not deploy_config["use-existing-app"]:
         await router.model.relate(f"{router.name}:mongodb", f"{mongodb.name}:database")
     await router.model.wait_for_idle(apps=[router.name, mongodb.name], status="active")

tests/integration/test_webhook_redelivery.py

@@ -1,20 +1,22 @@
 #  Copyright 2024 Canonical Ltd.
 #  See LICENSE file for licensing details.
+"""Integration tests for the webhook redelivery script."""
+
 import secrets
 from asyncio import sleep
 from datetime import datetime, timezone
-from typing import Iterator
+from typing import Callable, Iterator
 from uuid import uuid4
 
 import pytest
 from github import Github
 from github.Auth import Token
 from github.Hook import Hook
+from github.HookDelivery import HookDeliverySummary
 from github.Repository import Repository
 from github.Workflow import Workflow
 from juju.action import Action
 from juju.application import Application
-from juju.model import Model
 from juju.unit import Unit
 
 from tests.integration.conftest import GithubAuthenticationMethodParams
@@ -33,14 +35,19 @@
 
 @pytest.fixture(name="repo", scope="module")
 def repo_fixture(github_token: str, test_repo: str) -> Repository:
+    """Create a repository object for the test repo."""
     github = Github(auth=Token(github_token))
     repo = github.get_repo(test_repo)
 
     return repo
 
 
 @pytest.fixture(name="hook")
-def hook_fixture(github_token: str, repo: Repository) -> Iterator["Hook"]:
+def hook_fixture(repo: Repository) -> Iterator["Hook"]:
+    """Create a webhook for the test repo.
+
+    The hook gets deleted after the test.
+    """
     # we need a unique url to distinguish this webhook from others
     # the ip is internal and the webhook delivery is expected to fail
     unique_url = f"http://192.168.0.1:8080/{uuid4().hex}"
@@ -57,6 +64,7 @@ def hook_fixture(github_token: str, repo: Repository) -> Iterator["Hook"]:
 
 @pytest.fixture(name="test_workflow", scope="module")
 def test_workflow_fixture(repo: Repository) -> Iterator[Workflow]:
+    """Create a workflow for the test repo."""
     start_time = datetime.now(timezone.utc)
     workflow = repo.get_workflow(TEST_WORKFLOW_DISPATCH_FILE)
     yield workflow
@@ -67,66 +75,58 @@ def test_workflow_fixture(repo: Repository) -> Iterator[Workflow]:
 
 async def test_webhook_redelivery(
     router: Application,
-    github_app_auth: GithubAuthenticationMethodParams,
+    github_auth: GithubAuthenticationMethodParams,
     repo: Repository,
     hook: Hook,
     test_workflow: Workflow,
 ) -> None:
-    unit: Unit = router.units[0]
-    # create secret with github token
-    model: Model = router.model
-    secret_name = f"github-auth-{uuid4().hex}"  # use uuid in case same model is reused
+    """
+    arrange: a hook with a failed delivery
+    act: call the action to redeliver the webhook
+    assert: the failed delivery has been redelivered
+    """
+    unit = router.units[0]
 
     action_parms = {"webhook-id": hook.id, "since": 600, "github-path": repo.full_name}
-    if github_app_auth.token:
-        secret = await model.add_secret(secret_name, [f"token={github_app_auth.token}"])
-        secret_id = secret.split(":")[-1]
+    secret_id = await _create_secret_for_github_auth(router, github_auth)
+    if github_auth.token:
         action_parms["github-token-secret-id"] = secret_id
     else:
-        secret = await model.add_secret(
-            secret_name, [f"private-key={github_app_auth.private_key}"]
-        )
-        secret_id = secret.split(":")[-1]
         action_parms |= {
             GITHUB_APP_PRIVATE_KEY_SECRET_ID_PARAM_NAME: secret_id,
-            GITHUB_APP_CLIENT_ID_PARAM_NAME: github_app_auth.client_id,
-            GITHUB_APP_INSTALLATION_ID_PARAM_NAME: github_app_auth.installation_id,
+            GITHUB_APP_CLIENT_ID_PARAM_NAME: github_auth.client_id,
+            GITHUB_APP_INSTALLATION_ID_PARAM_NAME: github_auth.installation_id,
         }
-    await model.grant_secret(secret_name, router.name)
-
+    # we need to dispatch a job in order to have a webhook delivery with event "workflow_job"
     assert test_workflow.create_dispatch(ref="main")
 
-    # confirm webhook delivery failed
-    async def _wait_for_webhook_delivery(event: str) -> None:
-        """Wait for the webhook to be delivered."""
+    async def _wait_for_delivery_condition(
+        condition: Callable[[HookDeliverySummary], bool], condition_title: str
+    ) -> None:
+        """Wait to find a certain delivery with the condition."""
         for _ in range(10):
             deliveries = repo.get_hook_deliveries(hook.id)
-            for d in deliveries:
-                if d.event == event:
+            for delivery in deliveries:
+                if condition(delivery):
                     return
             await sleep(1)
-        assert False, f"Did not receive a webhook with event {event}"
+        assert False, f"Did not receive a webhook who fits the condition '{condition_title}'"
+
+    await _wait_for_delivery_condition(
+        lambda d: d.event == "workflow_job", "event is workflow_job"
+    )
 
-    await _wait_for_webhook_delivery("workflow_job")
-    # call redliver webhook action
     action: Action = await unit.run_action("redeliver-failed-webhooks", **action_parms)
+
     await action.wait()
     assert action.status == "completed", f"action failed with f{action.data['message']}"
     assert (
         action.results.get("redelivered") == "1"
     ), f"redelivered not matching in {action.results}"
-
-    async def _wait_for_webhook_redelivered(event: str) -> None:
-        """Wait for the webhook to be redelivered."""
-        for _ in range(10):
-            deliveries = repo.get_hook_deliveries(hook.id)
-            for d in deliveries:
-                if d.event == event and d.redelivery:
-                    return
-            await sleep(1)
-        assert False, f"Did not receive a redelivered webhook with event {event}"
-
-    await _wait_for_webhook_redelivered("workflow_job")
+    await _wait_for_delivery_condition(
+        lambda d: d.event == "workflow_job" and bool(d.redelivery),
+        "delivery with event workflow_job has been redelivered",
+    )
 
 
 @pytest.mark.parametrize(
@@ -139,7 +139,8 @@ async def _wait_for_webhook_redelivered(event: str) -> None:
             446,
             "private",
             "token",
-            "Invalid action parameters passed: Provided github app auth parameters and github token, "
+            "Invalid action parameters passed: "
+            "Provided github app auth parameters and github token, "
             "only one of them should be provided",
             id="github app config and github token secret",
         ),
@@ -161,7 +162,8 @@ async def _wait_for_webhook_redelivered(event: str) -> None:
         ),
     ],
 )  # we use a lot of arguments, but it seems not worth to introduce a capsulating object for this
-async def test_action_github_auth_param_error(  # pylint: disable=too-many-arguments
+async def test_action_github_auth_param_error(
+    # pylint: disable=too-many-arguments,too-many-positional-arguments
     github_app_client_id: str | None,
     github_app_installation_id: int | None,
     github_app_private_key_secret: str | None,
@@ -171,23 +173,19 @@ async def test_action_github_auth_param_error(  # pylint: disable=too-many-argum
 ):
     """
     arrange: Given a mocked environment with invalid github auth configuration.
-    act: Call github_client.get.
-    assert: ConfigurationError is raised.
+    act: Call the action.
+    assert: The action fails with the expected message.
     """
     unit: Unit = router.units[0]
-    # create secret with github token
-    model: Model = router.model
-    secret_name = f"github-creds-{uuid4().hex}"  # use uuid in case same model is reused
+
     secret_data = []
     action_parms = {"webhook-id": FAKE_HOOK_ID, "since": 600, "github-path": FAKE_REPO}
     if github_app_private_key_secret:
         secret_data.append(f"private-key={github_app_private_key_secret}")
     if github_token_secret:
         secret_data.append(f"token={github_token_secret}")
     if secret_data:
-        secret = await model.add_secret(secret_name, secret_data)
-        secret_id = secret.split(":")[-1]
-        await model.grant_secret(secret_name, router.name)
+        secret_id = await _create_secret(router, secret_data)
         if github_app_private_key_secret:
             action_parms[GITHUB_APP_PRIVATE_KEY_SECRET_ID_PARAM_NAME] = secret_id
         if github_token_secret:
@@ -199,6 +197,7 @@ async def test_action_github_auth_param_error(  # pylint: disable=too-many-argum
 
     action: Action = await unit.run_action("redeliver-failed-webhooks", **action_parms)
     await action.wait()
+
     assert action.status == "failed"
     assert expected_message in action.data["message"]
 
@@ -216,9 +215,12 @@ async def test_action_github_auth_param_error(  # pylint: disable=too-many-argum
         ),
     ],
 )
-async def test_secret_missing(
-    use_app_auth: bool, router: Application
-) -> None:
+async def test_secret_missing(use_app_auth: bool, router: Application) -> None:
+    """
+    arrange: Given auth parameters with a secret which is missing.
+    act: Call the action.
+    assert: The action fails with the expected message.
+    """
     unit: Unit = router.units[0]
 
     action_parms = {"webhook-id": FAKE_HOOK_ID, "since": 600, "github-path": FAKE_REPO}
@@ -232,11 +234,13 @@ async def test_secret_missing(
         action_parms["github-token-secret-id"] = secrets.token_hex(16)
 
     action: Action = await unit.run_action("redeliver-failed-webhooks", **action_parms)
+
     await action.wait()
     assert action.status == "failed"
     assert "Invalid action parameters passed" in (action_msg := action.data["message"])
     assert "Could not access/find secret" in action_msg
 
+
 @pytest.mark.parametrize(
     "use_app_auth, expected_message",
     [
@@ -255,19 +259,19 @@ async def test_secret_missing(
 async def test_secret_key_missing(
     use_app_auth: bool, expected_message: str, router: Application
 ) -> None:
-    unit: Unit = router.units[0]
-    # create secret with github token
-    model: Model = router.model
-    secret_name = f"github-creds-{uuid4().hex}"  # use uuid in case same model is reused
+    """
+    arrange: Given auth parameters with a secret which is missing a key.
+    act: Call the action.
+    assert: The action fails with the expected message.
+    """
+    unit = router.units[0]
 
     action_parms = {"webhook-id": FAKE_HOOK_ID, "since": 600, "github-path": FAKE_REPO}
     secret_data = [
         f"private-key-invalid={secrets.token_hex(16)}",
         f"token-invalid={secrets.token_hex(16)}",
     ]
-    secret = await model.add_secret(secret_name, secret_data)
-    secret_id = secret.split(":")[-1]
-    await model.grant_secret(secret_name, router.name)
+    secret_id = await _create_secret(router, secret_data)
     if use_app_auth:
         action_parms |= {
             GITHUB_APP_PRIVATE_KEY_SECRET_ID_PARAM_NAME: secret_id,
@@ -279,6 +283,30 @@ async def test_secret_key_missing(
 
     action: Action = await unit.run_action("redeliver-failed-webhooks", **action_parms)
     await action.wait()
+
     assert action.status == "failed"
     assert "Invalid action parameters passed" in (action_msg := action.data["message"])
     assert expected_message in action_msg
+
+
+async def _create_secret_for_github_auth(
+    app: Application, github_auth: GithubAuthenticationMethodParams
+) -> str:
+    """Create a secret with appropriate key depending on the Github auth type."""
+    if github_auth.token:
+        secret_data = [f"token={github_auth.token}"]
+    else:
+        secret_data = [f"private-key={github_auth.private_key}"]
+
+    return await _create_secret(app, secret_data)
+
+
+async def _create_secret(app: Application, secret_data: list[str]) -> str:
+    """Create a secret with the given data."""
+    secret_name = f"secret-{uuid4().hex}"
+    secret = await app.model.add_secret(secret_name, secret_data)
+    secret_id = secret.split(":")[-1]
+
+    await app.model.grant_secret(secret_name, app.name)
+
+    return secret_id

tests/unit/test_webhook_redelivery.py

@@ -84,6 +84,11 @@ def test_redeliver(
     expected_redelivered: set[int],
     webhook_address: WebhookAddress,
 ):
+    """
+    arrange: A mocked github client and different combinations of deliveries.
+    act: Call the script.
+    assert: The expected redeliveries are made.
+    """
     github_client = MagicMock(spec=github.Github)
     monkeypatch.setattr("webhook_redelivery.Github", MagicMock(return_value=github_client))
     now = datetime.now(tz=timezone.utc)
@@ -143,6 +148,11 @@ def test_redelivery_github_errors(
     expected_msg: str,
     webhook_address: WebhookAddress,
 ):
+    """
+    arrange: A mocked github client and a github exception.
+    act: Call the script.
+    assert: The expected error is raised.
+    """
     github_client = MagicMock(spec=github.Github)
     monkeypatch.setattr("webhook_redelivery.Github", MagicMock(return_value=github_client))
 
@@ -176,6 +186,11 @@ def test_redelivery_ignores_non_queued_or_non_workflow_job(
     action: str,
     event: str,
 ):
+    """
+    arrange: A mocked github client and different combinations of non-routable actions and events.
+    act: Call the script.
+    assert: No redeliveries are made.
+    """
     github_client = MagicMock(spec=github.Github)
     monkeypatch.setattr("webhook_redelivery.Github", MagicMock(return_value=github_client))
     now = datetime.now(tz=timezone.utc)

tox.ini

@@ -7,11 +7,10 @@ skip_missing_interpreters = True
 envlist = lint, unit, static, coverage-report
 
 [vars]
-charm_path = {toxinidir}/charm/src
 app_path = {toxinidir}/webhook_router/
 tst_path = {toxinidir}/tests/
 redelivery_script_path = {toxinidir}/webhook_redelivery.py
-all_path = {[vars]app_path} {[vars]tst_path} {[vars]charm_path} {[vars]redelivery_script_path}
+all_path = {[vars]app_path} {[vars]tst_path} {[vars]redelivery_script_path}
 
 [testenv]
 setenv =

webhook_redelivery.py

@@ -155,6 +155,7 @@ def _arg_parsing() -> _ParsedArgs:
     github_app_private_key = os.getenv(GITHUB_APP_PRIVATE_KEY_ENV_NAME)
     github_token = os.getenv(GITHUB_TOKEN_ENV_NAME)
 
+    github_auth_details: GithubAuthDetails
     if github_token:
         github_auth_details = github_token
     elif github_app_client_id and github_app_installation_id and github_app_private_key: