From ac3108e1874547e9248c1fc9f2975dc2bdf4325d Mon Sep 17 00:00:00 2001 From: Fabio Gallotti Date: Mon, 5 Aug 2024 12:23:19 +0200 Subject: [PATCH] improvements on pagination after code review --- .gitignore | 2 - Makefile | 15 ++---- coverage_comment/github.py | 35 +++++++++----- tests/integration/test_github.py | 80 +++++++++++++++++++++++--------- tests/integration/test_main.py | 19 +++----- 5 files changed, 91 insertions(+), 60 deletions(-) diff --git a/.gitignore b/.gitignore index c84e7412..95d0cfed 100644 --- a/.gitignore +++ b/.gitignore @@ -3,6 +3,4 @@ .coverage dev-env-vars .coverage* -.python-version __pycache__ -.vscode diff --git a/Makefile b/Makefile index 9161aeab..e7ee4bde 100644 --- 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 index cefcd0d5..9a06931e 100644 --- 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 index 123b0c40..a5b99ec1 100644 --- 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 index ca6d1cca..f925e436 100644 --- 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",