improvements on pagination after code review

py-cov-action · Aug 5, 2024 · ac3108e · ac3108e
1 parent 0951563
commit ac3108e
Show file tree

Hide file tree

Showing 5 changed files with 91 additions and 60 deletions.
diff --git a/.gitignore b/.gitignore
@@ -3,6 +3,4 @@
 .coverage
 dev-env-vars
 .coverage*
-.python-version
 __pycache__
-.vscode
diff --git a/Makefile b/Makefile
@@ -1,21 +1,12 @@
-.DEFAULT_GOAL := help
-SHELL = bash
-
-
-.PHONY: help
-help: ## show this help
-	@grep -E '^[a-zA-Z_-]+:.*?## .*$$' $(MAKEFILE_LIST) | awk 'BEGIN {FS = ":.*?## "}; {printf "\033[36m%-30s\033[0m %s\n", $$1, $$2}'
-
 .PHONY: install
 install: ## install dependencies
 	poetry install --with dev
-	poetry run pre-commit install
+	pre-commit install
 
 .PHONY: lint
 lint: ## lint code
-	poetry run ruff check --fix coverage_comment tests
-	poetry run ruff format coverage_comment tests
+	pre-commit
 
 .PHONY: test
 test: ## run all tests
-	poetry run pytest tests -vv --cov-report term-missing --cov=coverage_comment
+	poetry run pytest tests
diff --git a/coverage_comment/github.py b/coverage_comment/github.py
@@ -54,22 +54,15 @@ def download_artifact(
     filename: pathlib.Path,
 ) -> str:
     repo_path = github.repos(repository)
-    page = 1
-    artifacts = []
-    while True:
-        result = repo_path.actions.runs(run_id).artifacts.get(page=str(page))
-        if not result:
-            break
-        artifacts.extend(result.artifacts)
-        page += 1
+
     try:
         artifact = next(
-            iter(artifact for artifact in artifacts if artifact.name == artifact_name),
+            artifact
+            for artifact in _fetch_artifacts(repo_path, run_id)
+            if artifact.name == artifact_name
         )
     except StopIteration:
-        raise NoArtifact(
-            f"Not artifact found with name {artifact_name} in run {run_id}"
-        )
+        raise NoArtifact(f"No artifact found with name {artifact_name} in run {run_id}")
 
     zip_bytes = io.BytesIO(repo_path.actions.artifacts(artifact.id).zip.get(bytes=True))
     zipf = zipfile.ZipFile(zip_bytes)
@@ -80,6 +73,24 @@ def download_artifact(
         raise NoArtifact(f"File named {filename} not found in artifact {artifact_name}")
 
 
+def _fetch_artifacts(repo_path, run_id):
+    page = 1
+    total_fetched = 0
+
+    while True:
+        result = repo_path.actions.runs(run_id).artifacts.get(page=str(page))
+        if not result or not result.artifacts:
+            break
+
+        yield from result.artifacts
+
+        total_fetched += len(result.artifacts)
+        if total_fetched >= result.total_count:
+            break
+
+        page += 1
+
+
 def get_branch_from_workflow_run(
     github: github_client.GitHub, repository: str, run_id: int
 ) -> tuple[str, str]:

diff --git a/tests/integration/test_github.py b/tests/integration/test_github.py
@@ -52,13 +52,8 @@ def test_download_artifact(gh, session, zip_bytes):
         {"name": "foo", "id": 789},
     ]
     session.register("GET", "/repos/foo/bar/actions/runs/123/artifacts")(
-        json={"artifacts": artifacts}
+        json={"artifacts": artifacts, "total_count": 2}
     )
-    session.register(
-        "GET",
-        "/repos/foo/bar/actions/runs/123/artifacts",
-        params={"page": "2"},
-    )(json={})
 
     session.register("GET", "/repos/foo/bar/actions/artifacts/789/zip")(
         content=zip_bytes(filename="foo.txt", content="bar")
@@ -84,18 +79,13 @@ def test_download_artifact_from_page_2(gh, session, zip_bytes):
         {"name": "foo", "id": 789},
     ]
     session.register("GET", "/repos/foo/bar/actions/runs/123/artifacts")(
-        json={"artifacts": artifacts_page_1}
+        json={"artifacts": artifacts_page_1, "total_count": 3}
     )
     session.register(
         "GET",
         "/repos/foo/bar/actions/runs/123/artifacts",
         params={"page": "2"},
-    )(json={"artifacts": artifacts_page_2})
-    session.register(
-        "GET",
-        "/repos/foo/bar/actions/runs/123/artifacts",
-        params={"page": "3"},
-    )(json={})
+    )(json={"artifacts": artifacts_page_2, "total_count": 3})
 
     session.register("GET", "/repos/foo/bar/actions/artifacts/789/zip")(
         content=zip_bytes(filename="foo.txt", content="bar")
@@ -117,13 +107,8 @@ def test_download_artifact__no_artifact(gh, session):
         {"name": "bar", "id": 456},
     ]
     session.register("GET", "/repos/foo/bar/actions/runs/123/artifacts")(
-        json={"artifacts": artifacts}
+        json={"artifacts": artifacts, "total_count": 1}
     )
-    session.register(
-        "GET",
-        "/repos/foo/bar/actions/runs/123/artifacts",
-        params={"page": "2"},
-    )(json={})
 
     with pytest.raises(github.NoArtifact):
         github.download_artifact(
@@ -136,9 +121,7 @@ def test_download_artifact__no_artifact(gh, session):
 
 
 def test_download_artifact__no_file(gh, session, zip_bytes):
-    artifacts = [
-        {"name": "foo", "id": 789},
-    ]
+    artifacts = [{"name": "foo", "id": 789}]
     session.register("GET", "/repos/foo/bar/actions/runs/123/artifacts")(
         json={"artifacts": artifacts}
     )
@@ -161,6 +144,59 @@ def test_download_artifact__no_file(gh, session, zip_bytes):
         )
 
 
+def test_fetch_artifacts_empty_response(gh, session):
+    session.register("GET", "/repos/foo/bar/actions/runs/123/artifacts")(
+        json={"artifacts": [], "total_count": 0}
+    )
+
+    repo_path = gh.repos("foo/bar")
+
+    result = github._fetch_artifacts(
+        repo_path=repo_path,
+        run_id=123,
+    )
+
+    assert not list(result)
+
+
+def test_fetch_artifacts_single_page(gh, session):
+    artifacts = [{"name": "bar", "id": 456}]
+
+    session.register("GET", "/repos/foo/bar/actions/runs/123/artifacts")(
+        json={"artifacts": artifacts, "total_count": 1}
+    )
+
+    repo_path = gh.repos("foo/bar")
+
+    result = github._fetch_artifacts(
+        repo_path=repo_path,
+        run_id=123,
+    )
+
+    assert list(result) == artifacts
+
+
+def test_fetch_artifacts_multiple_pages(gh, session):
+    artifacts_page_1 = [{"name": "bar", "id": 456}]
+    artifacts_page_2 = [{"name": "bar", "id": 789}]
+
+    session.register("GET", "/repos/foo/bar/actions/runs/123/artifacts")(
+        json={"artifacts": artifacts_page_1, "total_count": 2}
+    )
+    session.register(
+        "GET", "/repos/foo/bar/actions/runs/123/artifacts", params={"page": "2"}
+    )(json={"artifacts": artifacts_page_2, "total_count": 2})
+
+    repo_path = gh.repos("foo/bar")
+
+    result = github._fetch_artifacts(
+        repo_path=repo_path,
+        run_id=123,
+    )
+
+    assert list(result) == artifacts_page_1 + artifacts_page_2
+
+
 def test_get_branch_from_workflow_run(gh, session):
     json = {
         "head_branch": "other",

diff --git a/tests/integration/test_main.py b/tests/integration/test_main.py
@@ -811,12 +811,7 @@ def test_action__workflow_run__no_artifact(
     session.register(
         "GET",
         "/repos/py-cov-action/foobar/actions/runs/123/artifacts",
-    )(json={"artifacts": [{"name": "wrong_name"}]})
-    session.register(
-        "GET",
-        "/repos/py-cov-action/foobar/actions/runs/123/artifacts",
-        params={"page": "2"},
-    )(json={})
+    )(json={"artifacts": [{"name": "wrong_name"}], "total_count": 1})
 
     result = main.action(
         config=workflow_run_config(),
@@ -857,12 +852,12 @@ def test_action__workflow_run__post_comment(
     session.register(
         "GET",
         "/repos/py-cov-action/foobar/actions/runs/123/artifacts",
-    )(json={"artifacts": [{"name": "python-coverage-comment-action", "id": 789}]})
-    session.register(
-        "GET",
-        "/repos/py-cov-action/foobar/actions/runs/123/artifacts",
-        params={"page": "2"},
-    )(json={})
+    )(
+        json={
+            "artifacts": [{"name": "python-coverage-comment-action", "id": 789}],
+            "total_count": 1,
+        }
+    )
 
     session.register(
         "GET",