diff --git a/README.md b/README.md index cb45955..277a78b 100644 --- a/README.md +++ b/README.md @@ -16,12 +16,13 @@ The action handles everything else automatically: gzip/base64 encoding, resolvin ## Inputs -| Input | Required | Description | -|-------|----------|-------------| -| `file` | Yes | Path to the Cobertura XML coverage report | -| `language` | Yes | Linguist language name (e.g. `Java`, `Go`, `Python`) | -| `label` | Yes | Label for the report (e.g. `code-coverage/jacoco`) | -| `token` | No | GitHub token (defaults to `github.token`) | +| Input | Required | Default | Description | +|-------|----------|---------|-------------| +| `file` | Yes | | Path to the Cobertura XML coverage report | +| `language` | Yes | | Linguist language name (e.g. `Java`, `Go`, `Python`) | +| `label` | Yes | | Label for the report (e.g. `code-coverage/jacoco`) | +| `fail-on-error` | No | `true` | Whether to fail the workflow step if the upload fails | +| `token` | No | `github.token` | GitHub token with `code-quality:write` permission | ## Permissions @@ -35,6 +36,23 @@ permissions: For push-only workflows where the action looks up PR numbers via `gh pr list`, also add `pull-requests: read`. +## Error handling + +By default, the action fails the workflow step (exits with code 1) when the upload is unsuccessful. This ensures you notice when coverage data is not being stored. + +If coverage upload is best-effort in your workflow and you don't want a transient API failure to block CI, set `fail-on-error: false`: + +```yaml +- uses: actions/upload-code-coverage@v1 + with: + file: cobertura.xml + language: Java + label: code-coverage/jacoco + fail-on-error: false +``` + +When `fail-on-error` is `false`, upload errors are still surfaced as `::error::` annotations in the workflow log, but the step exits 0. + ## Event handling The action auto-detects the event type and resolves the correct values: diff --git a/action.yml b/action.yml index 8164026..4cf4821 100644 --- a/action.yml +++ b/action.yml @@ -11,6 +11,10 @@ inputs: label: description: 'Label for the coverage report (e.g. "code-coverage/jacoco")' required: true + fail-on-error: + description: 'Whether to fail the workflow step if the upload fails (set to false to treat upload errors as warnings)' + required: false + default: 'true' token: description: 'GitHub token with code-quality:write permission' required: false @@ -26,6 +30,7 @@ runs: INPUT_FILE: ${{ inputs.file }} INPUT_LANGUAGE: ${{ inputs.language }} INPUT_LABEL: ${{ inputs.label }} + FAIL_ON_ERROR: ${{ inputs.fail-on-error }} GITHUB_EVENT_NAME: ${{ github.event_name }} GITHUB_REPOSITORY: ${{ github.repository }} GITHUB_API_URL: ${{ github.api_url }} diff --git a/test_upload_coverage.py b/test_upload_coverage.py index 2a233cd..d2e5f84 100644 --- a/test_upload_coverage.py +++ b/test_upload_coverage.py @@ -47,6 +47,7 @@ def setUp(self): "GITHUB_REPOSITORY": "octo-org/octo-repo", "GITHUB_API_URL": "https://api.github.com", "GH_TOKEN": "test-token", + "FAIL_ON_ERROR": "true", } def tearDown(self): @@ -63,6 +64,8 @@ def request_payload(self, opener): request = opener.call_args.args[0] return json.loads(request.data.decode("utf-8")) + # --- File validation --- + def test_file_not_found_exits_with_error_annotation(self): env = dict(self.base_env, INPUT_FILE=str(self.test_dir / "missing.xml")) @@ -72,11 +75,13 @@ def test_file_not_found_exits_with_error_annotation(self): self.assertIn("::error::Coverage file not found", output) opener.assert_not_called() - def test_successful_upload_exits_zero(self): + # --- Successful uploads --- + + def test_201_exits_zero_with_success_message(self): exit_code, output, opener = self.run_main() self.assertEqual(0, exit_code) - self.assertEqual("", output) + self.assertIn("Coverage report uploaded successfully.", output) opener.assert_called_once() def test_successful_with_pr_number_uses_pull_request_number(self): @@ -95,70 +100,225 @@ def test_successful_with_ref_uses_ref_and_omits_pull_request_number(self): self.assertEqual("refs/heads/main", payload["ref"]) self.assertNotIn("pull_request_number", payload) + # --- HTTP 200 (accepted but not stored) --- + + def test_200_with_message_emits_warning(self): + opener = mock.Mock(return_value=FakeResponse( + status=200, + body=b'{"message":"commit is not the latest on branch"}' + )) + + exit_code, output, _ = self.run_main(opener=opener) + + self.assertEqual(0, exit_code) + self.assertIn("::warning::", output) + self.assertIn("commit is not the latest on branch", output) + self.assertIn("HTTP 200", output) + + def test_200_without_message_emits_generic_warning(self): + opener = mock.Mock(return_value=FakeResponse(status=200, body=b'{}')) + + exit_code, output, _ = self.run_main(opener=opener) + + self.assertEqual(0, exit_code) + self.assertIn("::warning::", output) + self.assertIn("HTTP 200 but expected 201", output) + + def test_200_with_invalid_json_emits_generic_warning(self): + opener = mock.Mock(return_value=FakeResponse(status=200, body=b'not json')) + + exit_code, output, _ = self.run_main(opener=opener) + + self.assertEqual(0, exit_code) + self.assertIn("::warning::", output) + self.assertIn("HTTP 200 but expected 201", output) + + # --- HTTP 403 (permissions) --- + def test_403_not_authorized_exits_with_permissions_error(self): - error = HTTPError( - url="https://api.github.com/repos/octo-org/octo-repo/code-coverage/report", - code=403, - msg="Forbidden", - hdrs=None, - fp=io.BytesIO(b'{"message":"not authorized"}'), - ) + opener = mock.Mock(return_value=FakeResponse( + status=403, + body=b'{"message":"not authorized"}' + )) - exit_code, output, _ = self.run_main(opener=mock.Mock(side_effect=error)) + exit_code, output, _ = self.run_main(opener=opener) self.assertEqual(1, exit_code) self.assertIn("code-quality: write", output) + self.assertIn("HTTP 403", output) def test_403_other_message_exits_with_generic_error(self): - error = HTTPError( - url="https://api.github.com/repos/octo-org/octo-repo/code-coverage/report", - code=403, - msg="Forbidden", - hdrs=None, - fp=io.BytesIO(b'{"message":"forbidden"}'), - ) + opener = mock.Mock(return_value=FakeResponse( + status=403, + body=b'{"message":"forbidden"}' + )) - exit_code, output, _ = self.run_main(opener=mock.Mock(side_effect=error)) + exit_code, output, _ = self.run_main(opener=opener) self.assertEqual(1, exit_code) - self.assertIn("Coverage upload failed with status 403", output) - self.assertIn('forbidden', output) + self.assertIn("Coverage upload failed (HTTP 403)", output) + self.assertIn("forbidden", output) + + # --- Other error codes --- def test_400_bad_request_exits_with_status_and_body(self): - error = HTTPError( - url="https://api.github.com/repos/octo-org/octo-repo/code-coverage/report", - code=400, - msg="Bad Request", - hdrs=None, - fp=io.BytesIO(b'{"message":"bad request"}'), - ) + opener = mock.Mock(return_value=FakeResponse( + status=400, + body=b'{"message":"bad request"}' + )) - exit_code, output, _ = self.run_main(opener=mock.Mock(side_effect=error)) + exit_code, output, _ = self.run_main(opener=opener) self.assertEqual(1, exit_code) - self.assertIn("status 400", output) + self.assertIn("HTTP 400", output) self.assertIn("bad request", output) def test_500_server_error_exits_with_status_and_body(self): - error = HTTPError( - url="https://api.github.com/repos/octo-org/octo-repo/code-coverage/report", - code=500, - msg="Server Error", - hdrs=None, - fp=io.BytesIO(b'{"message":"boom"}'), - ) + opener = mock.Mock(return_value=FakeResponse( + status=500, + body=b'{"message":"boom"}' + )) - exit_code, output, _ = self.run_main(opener=mock.Mock(side_effect=error)) + exit_code, output, _ = self.run_main(opener=opener) self.assertEqual(1, exit_code) - self.assertIn("status 500", output) + self.assertIn("HTTP 500", output) self.assertIn("boom", output) + # --- Network errors --- + def test_network_error_exits_with_error(self): - exit_code, output, _ = self.run_main(opener=mock.Mock(side_effect=URLError("connection refused"))) + opener = mock.Mock(side_effect=URLError("connection refused")) + + exit_code, output, _ = self.run_main(opener=opener) self.assertEqual(1, exit_code) - self.assertIn("connection refused", output) + self.assertIn("::error::", output) + self.assertIn("could not reach the API", output) + + # --- Unexpected status codes --- + + def test_unexpected_status_code_emits_notice_and_exits_zero(self): + opener = mock.Mock(return_value=FakeResponse(status=202, body=b'accepted')) + + exit_code, output, _ = self.run_main(opener=opener) + + self.assertEqual(0, exit_code) + self.assertIn("::notice::", output) + self.assertIn("unexpected HTTP 202", output) + + # --- fail-on-error: true (default) --- + + def test_fail_on_error_true_exits_1_on_4xx(self): + env = dict(self.base_env, FAIL_ON_ERROR="true") + opener = mock.Mock(return_value=FakeResponse(status=400, body=b'{"message":"nope"}')) + + exit_code, output, _ = self.run_main(env=env, opener=opener) + + self.assertEqual(1, exit_code) + self.assertIn("fail-on-error", output) + + def test_fail_on_error_true_exits_1_on_5xx(self): + env = dict(self.base_env, FAIL_ON_ERROR="true") + opener = mock.Mock(return_value=FakeResponse(status=502, body=b'bad gateway')) + + exit_code, output, _ = self.run_main(env=env, opener=opener) + + self.assertEqual(1, exit_code) + + def test_fail_on_error_true_exits_1_on_network_error(self): + env = dict(self.base_env, FAIL_ON_ERROR="true") + opener = mock.Mock(side_effect=URLError("timeout")) + + exit_code, output, _ = self.run_main(env=env, opener=opener) + + self.assertEqual(1, exit_code) + + # --- fail-on-error: false --- + + def test_fail_on_error_false_exits_0_on_4xx(self): + env = dict(self.base_env, FAIL_ON_ERROR="false") + opener = mock.Mock(return_value=FakeResponse(status=400, body=b'{"message":"nope"}')) + + exit_code, output, _ = self.run_main(env=env, opener=opener) + + self.assertEqual(0, exit_code) + self.assertIn("::error::", output) + self.assertIn("fail-on-error", output) + + def test_fail_on_error_false_exits_0_on_5xx(self): + env = dict(self.base_env, FAIL_ON_ERROR="false") + opener = mock.Mock(return_value=FakeResponse(status=500, body=b'error')) + + exit_code, output, _ = self.run_main(env=env, opener=opener) + + self.assertEqual(0, exit_code) + self.assertIn("::error::", output) + + def test_fail_on_error_false_exits_0_on_network_error(self): + env = dict(self.base_env, FAIL_ON_ERROR="false") + opener = mock.Mock(side_effect=URLError("connection refused")) + + exit_code, output, _ = self.run_main(env=env, opener=opener) + + self.assertEqual(0, exit_code) + self.assertIn("::error::", output) + + def test_fail_on_error_false_exits_0_on_403(self): + env = dict(self.base_env, FAIL_ON_ERROR="false") + opener = mock.Mock(return_value=FakeResponse( + status=403, + body=b'{"message":"not authorized"}' + )) + + exit_code, output, _ = self.run_main(env=env, opener=opener) + + self.assertEqual(0, exit_code) + self.assertIn("::error::", output) + self.assertIn("code-quality: write", output) + + def test_fail_on_error_false_still_exits_1_on_missing_file(self): + """File-not-found is a local config error, not an upload failure.""" + env = dict(self.base_env, FAIL_ON_ERROR="false", INPUT_FILE="/nonexistent") + opener = mock.Mock() + + exit_code, output, _ = self.run_main(env=env, opener=opener) + + self.assertEqual(1, exit_code) + opener.assert_not_called() + + def test_fail_on_error_false_still_exits_1_on_missing_ref_and_pr(self): + """Missing ref/PR is a config error, not an upload failure.""" + env = dict(self.base_env, FAIL_ON_ERROR="false", REF="", PR_NUMBER="") + opener = mock.Mock() + + exit_code, output, _ = self.run_main(env=env, opener=opener) + + self.assertEqual(1, exit_code) + opener.assert_not_called() + + # --- Diagnostic logging --- + + def test_upload_parameters_logged_in_group(self): + exit_code, output, _ = self.run_main() + + self.assertIn("::group::Upload parameters", output) + self.assertIn("commit_oid: deadbeef", output) + self.assertIn("ref: refs/heads/main", output) + self.assertIn("pr_number: ", output) + self.assertIn("language: Python", output) + self.assertIn("label: code-coverage/test", output) + self.assertIn("::endgroup::", output) + + def test_upload_parameters_shows_pr_number_when_set(self): + env = dict(self.base_env, REF="", PR_NUMBER="99") + + exit_code, output, _ = self.run_main(env=env) + + self.assertIn("pr_number: 99", output) + self.assertIn("ref: ", output) + + # --- Payload structure --- def test_payload_structure_and_encoding(self): exit_code, _, opener = self.run_main() @@ -185,6 +345,53 @@ def test_neither_ref_nor_pr_number_exits_with_error(self): self.assertIn("Either PR_NUMBER or REF must be provided", output) opener.assert_not_called() + # --- Error annotations include fail-on-error hint --- + + def test_error_annotations_include_fail_on_error_hint(self): + opener = mock.Mock(return_value=FakeResponse(status=500, body=b'oops')) + + exit_code, output, _ = self.run_main(opener=opener) + + self.assertIn("fail-on-error: false", output) + + def test_network_error_annotation_includes_fail_on_error_hint(self): + opener = mock.Mock(side_effect=URLError("dns failure")) + + exit_code, output, _ = self.run_main(opener=opener) + + self.assertIn("fail-on-error: false", output) + + def test_403_permissions_error_includes_fail_on_error_hint(self): + opener = mock.Mock(return_value=FakeResponse( + status=403, + body=b'{"message":"not authorized"}' + )) + + exit_code, output, _ = self.run_main(opener=opener) + + self.assertIn("fail-on-error: false", output) + + + def test_error_response_strips_documentation_url(self): + """Only the message field is shown, not the full JSON with documentation_url. + + TODO(GA): When docs are published at docs.github.com/rest/code-quality/code-coverage, + include the documentation_url in the output and update this test accordingly. + """ + body = json.dumps({ + "message": "Code quality is not enabled for this repository.", + "documentation_url": "https://docs.github.com/rest/code-quality/code-coverage", + "status": "403", + }) + opener = mock.Mock(return_value=FakeResponse(status=403, body=body.encode())) + + exit_code, output, _ = self.run_main(opener=opener) + + self.assertEqual(1, exit_code) + self.assertIn("Code quality is not enabled", output) + self.assertNotIn("documentation_url", output) + self.assertNotIn("docs.github.com", output) + if __name__ == "__main__": unittest.main() diff --git a/upload_coverage.py b/upload_coverage.py index 2858b61..322db06 100755 --- a/upload_coverage.py +++ b/upload_coverage.py @@ -7,19 +7,61 @@ import urllib.error import urllib.request from pathlib import Path -from typing import Mapping, Optional +from typing import Mapping, Optional, Tuple PERMISSIONS_ERROR = ( - "Coverage upload returned 403 Forbidden. Ensure the calling job has " + "Coverage upload returned HTTP {status}. Ensure the calling job has " "'code-quality: write' permission. See https://github.com/actions/upload-code-coverage#permissions" ) +FAIL_ON_ERROR_HINT = ( + "To treat upload errors as warnings, add 'fail-on-error: false' to the action inputs." +) + def emit_annotation(level: str, message: str) -> None: print(f"::{level}::{message}") +def log_upload_parameters( + *, + commit_oid: str, + ref: str, + pr_number: str, + language: str, + label: str, + file_path: str, +) -> None: + file_size = Path(file_path).stat().st_size + print("::group::Upload parameters") + print(f" commit_oid: {commit_oid}") + print(f" ref: {ref or ''}") + print(f" pr_number: {pr_number or ''}") + print(f" language: {language}") + print(f" label: {label}") + print(f" file: {file_path} ({file_size} bytes)") + print("::endgroup::") + + +def _extract_message(body: str) -> str: + """Extract the human-readable message from an API JSON response. + + Falls back to the raw body if parsing fails or no message field exists. + We intentionally strip documentation_url and other fields because the + docs URL currently 404s (pre-GA). + TODO(GA): Once docs are live, consider including documentation_url in output. + """ + try: + data = json.loads(body) + message = data.get("message", "") + if message: + return message + except (json.JSONDecodeError, AttributeError, TypeError): + pass + return body + + def encode_coverage_report(file_path: str) -> str: data = Path(file_path).read_bytes() return base64.b64encode(gzip.compress(data)).decode("ascii") @@ -58,7 +100,8 @@ def upload_report( api_url: str, token: str, opener=urllib.request.urlopen, -) -> int: +) -> Tuple[int, str]: + """Upload the coverage report. Returns (status_code, response_body).""" request = urllib.request.Request( url=f"{api_url.rstrip('/')}/repos/{repository}/code-coverage/report", data=json.dumps(payload).encode("utf-8"), @@ -72,18 +115,49 @@ def upload_report( try: with opener(request) as response: - response.read() - return response.getcode() + body = response.read().decode("utf-8", errors="replace") + return response.getcode(), body except urllib.error.HTTPError as error: body = error.read().decode("utf-8", errors="replace") - if error.code == 403 and "not authorized" in body.lower(): - emit_annotation("error", PERMISSIONS_ERROR) - else: - emit_annotation("error", f"Coverage upload failed with status {error.code}: {body}") - return error.code + return error.code, body except urllib.error.URLError as error: - emit_annotation("error", f"Coverage upload failed: {error.reason}") - return 1 + return 0, str(error.reason) + + +def handle_response(status: int, body: str, fail_on_error: bool) -> int: + """Process the upload response. Returns the process exit code.""" + if status == 0: + # Network error (could not reach the API) + emit_annotation("error", f"Coverage upload failed: could not reach the API. {FAIL_ON_ERROR_HINT}") + return 1 if fail_on_error else 0 + + if status == 201: + print("Coverage report uploaded successfully.") + return 0 + + if status == 200: + # API accepted but did not store (e.g. commit not latest on branch) + try: + message = json.loads(body).get("message", "") + except (json.JSONDecodeError, AttributeError): + message = "" + if message: + emit_annotation("warning", f"Coverage upload returned HTTP 200 (report not stored): {message}") + else: + emit_annotation("warning", "Coverage upload returned HTTP 200 but expected 201. The report may not have been stored.") + return 0 + + if status >= 400: + if status == 403 and "not authorized" in body.lower(): + emit_annotation("error", f"{PERMISSIONS_ERROR.format(status=status)}. {FAIL_ON_ERROR_HINT}") + else: + display_body = _extract_message(body) + emit_annotation("error", f"Coverage upload failed (HTTP {status}): {display_body}. {FAIL_ON_ERROR_HINT}") + return 1 if fail_on_error else 0 + + # Unexpected status code + emit_annotation("notice", f"Coverage upload returned unexpected HTTP {status}: {body}") + return 0 def main(environ: Optional[Mapping[str, str]] = None, opener=urllib.request.urlopen) -> int: @@ -94,20 +168,37 @@ def main(environ: Optional[Mapping[str, str]] = None, opener=urllib.request.urlo emit_annotation("error", f"Coverage file not found: {file_path}") return 1 + fail_on_error = env.get("FAIL_ON_ERROR", "true").lower() != "false" + + commit_oid = env.get("COMMIT_OID", "") + ref = env.get("REF", "") + pr_number = env.get("PR_NUMBER", "") + language = env.get("INPUT_LANGUAGE", "") + label = env.get("INPUT_LABEL", "") + + log_upload_parameters( + commit_oid=commit_oid, + ref=ref, + pr_number=pr_number, + language=language, + label=label, + file_path=file_path, + ) + try: payload = build_payload( file_path=file_path, - language=env.get("INPUT_LANGUAGE", ""), - label=env.get("INPUT_LABEL", ""), - commit_oid=env.get("COMMIT_OID", ""), - ref=env.get("REF", ""), - pr_number=env.get("PR_NUMBER", ""), + language=language, + label=label, + commit_oid=commit_oid, + ref=ref, + pr_number=pr_number, ) except ValueError as error: emit_annotation("error", str(error)) return 1 - status = upload_report( + status, body = upload_report( payload=payload, repository=env.get("GITHUB_REPOSITORY", ""), api_url=env.get("GITHUB_API_URL", "https://api.github.com"), @@ -115,7 +206,7 @@ def main(environ: Optional[Mapping[str, str]] = None, opener=urllib.request.urlo opener=opener, ) - return 0 if 200 <= status < 300 else 1 + return handle_response(status, body, fail_on_error) if __name__ == "__main__":