From 207eb7e07b8e41c218417a817a174dd716163dd7 Mon Sep 17 00:00:00 2001 From: Abhimanyu Saharan Date: Sun, 4 Oct 2026 08:09:12 +0530 Subject: [PATCH 1/6] Expand backend unit coverage and enforce PR reports --- .github/workflows/ci.yml | 116 ++++++- CONTRIBUTING.md | 27 +- packages/core/src/catalog_cache.py | 3 +- pyproject.toml | 25 ++ scripts/ci/coverage_report.py | 161 +++++++++ scripts/ci/python-tests.sh | 16 +- scripts/pre_commit_checks.py | 12 +- tests/test_catalog_snapshot_unit.py | 116 +++++++ tests/test_ci_gates.py | 11 +- tests/test_coverage_report.py | 303 +++++++++++++++++ tests/test_job_retry_unit.py | 220 ++++++++++++ tests/test_mcp_oauth_unit.py | 404 +++++++++++++++++++++++ tests/test_pre_commit_checks.py | 12 +- tests/test_publication_policy_unit.py | 291 ++++++++++++++++ tests/test_reading_streaks_unit.py | 125 +++++++ tests/test_relationship_coverage_unit.py | 210 ++++++++++++ tests/test_scheduler_unit.py | 263 +++++++++++++++ tests/test_search_indexer.py | 75 +++++ tests/test_search_records_unit.py | 219 ++++++++++++ tests/test_topic_decision_worker_unit.py | 289 ++++++++++++++++ uv.lock | 115 +++++++ 21 files changed, 3003 insertions(+), 10 deletions(-) create mode 100644 scripts/ci/coverage_report.py create mode 100644 tests/test_catalog_snapshot_unit.py create mode 100644 tests/test_coverage_report.py create mode 100644 tests/test_job_retry_unit.py create mode 100644 tests/test_mcp_oauth_unit.py create mode 100644 tests/test_publication_policy_unit.py create mode 100644 tests/test_reading_streaks_unit.py create mode 100644 tests/test_relationship_coverage_unit.py create mode 100644 tests/test_scheduler_unit.py create mode 100644 tests/test_search_records_unit.py create mode 100644 tests/test_topic_decision_worker_unit.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 78f6d5011..b2e80f055 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -29,9 +29,115 @@ jobs: sync-command: uv sync --all-packages --locked lint-command: bash .github/scripts/python-checks.sh test-command: bash scripts/ci/python-tests.sh unit - test-results-path: reports/python-unit.xml + test-results-path: | + reports/python-unit.xml + reports/coverage/ test-results-name: python-unit-arm64-${{ github.run_attempt }} name: Unit / Python lint and tests (ARM64) + coverage-report: + name: Report / Backend unit coverage PR comment + needs: python-unit + if: >- + always() && !cancelled() && github.event_name == 'pull_request' && + github.event.pull_request.head.repo.full_name == github.repository && + github.actor != 'dependabot[bot]' + runs-on: ubuntu-24.04 + timeout-minutes: 5 + permissions: + contents: read + pull-requests: write + steps: + # No checkout: downloaded reports are data, never executable code. + - uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 + continue-on-error: true + with: + name: python-unit-arm64-${{ github.run_attempt }} + path: report + - uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 + env: + UNIT_RESULT: ${{ needs.python-unit.result }} + with: + script: | + const fs = require('fs'); + const marker = ''; + const pr = context.payload.pull_request; + const current = await github.rest.pulls.get({...context.repo, pull_number: pr.number}); + if (current.data.head.sha !== pr.head.sha) return; + const url = `${context.serverUrl}/${context.repo.owner}/${context.repo.repo}/actions/runs/${context.runId}`; + const workspaces = [ + 'apps/admin-api', 'apps/aggregator', 'apps/api', 'apps/article-enrichment-worker', + 'apps/cli', 'apps/images-worker', 'apps/mcp', 'apps/notifications', + 'apps/search-indexer', 'apps/source-discovery-worker', 'apps/user-api', + 'packages/core', 'packages/http' + ]; + const counters = ['covered_lines', 'num_statements', 'covered_branches', 'num_branches']; + const integer = value => Number.isSafeInteger(value) && value >= 0; + const metrics = value => { + if (!value || !counters.every(key => integer(value[key])) || + value.covered_lines > value.num_statements || value.covered_branches > value.num_branches) { + throw new Error('Invalid coverage counts'); + } + const percent = (covered, total) => total ? 100 * covered / total : 100; + return [percent(value.covered_lines, value.num_statements), + percent(value.covered_branches, value.num_branches), + percent(value.covered_lines + value.covered_branches, value.num_statements + value.num_branches)]; + }; + let summary, measured; + try { + const raw = fs.readFileSync('report/coverage/summary.json', 'utf8'); + if (raw.length > 30000) throw new Error('Oversized coverage report'); + summary = JSON.parse(raw); + if (summary.schema !== 1 || summary.valid !== true || + !['passed', 'failed'].includes(summary.status) || + !Number.isFinite(summary.minimum) || summary.minimum <= 0 || summary.minimum > 100 || + !integer(summary.unit_exit_code) || !integer(summary.source_files) || !summary.source_files || + Object.keys(summary.packages).sort().join('|') !== workspaces.join('|')) { + throw new Error('Incomplete coverage report'); + } + measured = metrics(summary.totals); + if (!summary.totals.num_statements) throw new Error('Empty coverage report'); + const sum = Object.fromEntries(counters.map(key => [key, 0])); + for (const name of workspaces) { + metrics(summary.packages[name]); + for (const key of counters) sum[key] += summary.packages[name][key]; + } + if (!counters.every(key => sum[key] === summary.totals[key])) throw new Error('Inconsistent totals'); + const tests = summary.tests; + if (!tests || !['total', 'passed', 'failed', 'skipped'].every(key => integer(tests[key])) || + !tests.total || tests.total !== tests.passed + tests.failed + tests.skipped) { + throw new Error('Invalid unit test counts'); + } + } catch { + summary = null; + } + const lines = [marker, '### Backend unit coverage', '']; + if (summary) { + const tests = summary.tests; + const passed = process.env.UNIT_RESULT === 'success' && summary.status === 'passed' && + summary.unit_exit_code === 0 && tests.failed === 0 && tests.skipped === 0 && + Number(measured[2].toFixed(2)) >= summary.minimum; + lines.push(`**${passed ? 'PASS' : 'FAIL'}** · minimum combined coverage **${summary.minimum.toFixed(2)}%**.`, '', + `Tests: ${tests.passed} passed, ${tests.failed} failed, ${tests.skipped} skipped. Source files: ${summary.source_files}.`, '', + '| Workspace | Lines | Branches | Combined |', '| --- | ---: | ---: | ---: |'); + const row = (name, values) => `| ${name} | ${values.map(value => `${value.toFixed(2)}%`).join(' | ')} |`; + lines.push(row('All backend code', measured)); + for (const name of workspaces) lines.push(row(name, metrics(summary.packages[name]))); + lines.push('', 'Unit coverage includes every backend workspace. PostgreSQL/Redis integration tests run separately.'); + } else { + lines.push('**Coverage unavailable:** the Python unit job did not produce a valid report. Inspect its logs; this does not count as a passing coverage check.'); + } + lines.push('', `Commit: ${pr.head.sha.slice(0, 12)} · [HTML, JSON, XML reports and job logs](${url}) · attempt ${process.env.GITHUB_RUN_ATTEMPT}`); + const body = lines.join('\n'); + const comments = await github.paginate(github.rest.issues.listComments, {...context.repo, issue_number: pr.number}); + const previous = comments.find(comment => comment.user.type === 'Bot' && comment.body?.startsWith(marker)); + const latest = await github.rest.pulls.get({...context.repo, pull_number: pr.number}); + if (latest.data.head.sha !== pr.head.sha) return; + if (previous) { + await github.rest.issues.updateComment({...context.repo, comment_id: previous.id, body}); + } else { + await github.rest.issues.createComment({...context.repo, issue_number: pr.number, body}); + } + if (!summary) core.setFailed('Backend coverage artifact is missing or invalid'); admin-web: uses: abhi1693/actions/.github/workflows/node-ci.yml@afba12afb85bf40be9328c92dc53b779f18d5c6d # v1.0.1 with: @@ -211,6 +317,7 @@ jobs: if: ${{ always() && !cancelled() }} needs: - python-unit + - coverage-report - admin-web - user-web - extensions @@ -229,9 +336,10 @@ jobs: env: EVENT_NAME: ${{ github.event_name }} REF_TYPE: ${{ github.ref_type }} + COVERAGE_COMMENT_REQUIRED: ${{ github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name == github.repository && github.actor != 'dependabot[bot]' }} RESULTS: ${{ toJSON(needs) }} run: | - echo "$RESULTS" | jq -e --arg event "$EVENT_NAME" --arg ref_type "$REF_TYPE" ' + echo "$RESULTS" | jq -e --arg event "$EVENT_NAME" --arg ref_type "$REF_TYPE" --arg comment "$COVERAGE_COMMENT_REQUIRED" ' ([.["python-unit"], .["admin-web"], .["user-web"], .extensions, .["python-integration"], .["migration-upgrade"], .["reader-parity"], .["admin-browser"], .["live-browser"], .security] @@ -240,4 +348,6 @@ jobs: then .["release-images"].result == "success" and .containers.result == "skipped" else .containers.result == "success" and .["release-images"].result == "skipped" end) and (if $event == "pull_request" then .performance.result == "success" - else .performance.result == "skipped" end)' + else .performance.result == "skipped" end) and + (if $comment == "true" then .["coverage-report"].result == "success" + else .["coverage-report"].result == "skipped" end)' diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 9aa0e380c..3781d05e6 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -71,8 +71,8 @@ Run the checks relevant to your change: # Backend lint, formatting, types and version consistency bash .github/scripts/python-checks.sh -# Backend unit tests; no dependency services are started -bash scripts/test.sh -q -m 'not integration' +# Backend unit tests with line and branch coverage; no services are started +bash scripts/ci/python-tests.sh unit # Reader app npm run web:lint @@ -92,6 +92,29 @@ Pre-commit runs formatting and the affected projects' lint, type and unit checks it does not replace CI or integration testing. If a formatter changes files, review and stage those changes before retrying the commit. +Backend unit coverage includes every Python source file in all 13 backend +workspaces, including modules the tests never import. The minimum combined line +and branch coverage is **67.50%**, configured by `tool.coverage.report.fail_under` in +`pyproject.toml`; both CI and backend pre-commit checks enforce it. Keep this +minimum when adding code, and raise it as coverage improves. A missing workspace, +missing report, empty suite, failure, or skipped unit test fails the CI report gate. + +CI updates one coverage comment on each same-repository PR, showing test counts +and line, branch, and combined coverage for each workspace. Superseded PR commits +cannot overwrite the current comment. Fork PRs and Dependabot runs retain the job +summary and report artifacts because their tokens cannot write PR comments. +Download the Python unit artifact for HTML, JSON, and Cobertura XML reports; local +reports are in `reports/coverage/` (open `html/index.html`). Integration results do +not inflate the unit coverage score and remain independent CI requirements. + +For a focused test run, omit coverage or temporarily disable its whole-suite +minimum for that command: + +```sh +uv run --locked pytest -q tests/test_mcp_oauth_unit.py +uv run --locked pytest -q tests/test_mcp_oauth_unit.py --cov --cov-fail-under=0 +``` + Integration tests require explicit `DEVFEED_TEST_DATABASE_URL` and `DEVFEED_TEST_REDIS_URL`. The PostgreSQL database name must end in `_test`, and Redis must use database **15**. These tests truncate tables and flush Redis: never diff --git a/packages/core/src/catalog_cache.py b/packages/core/src/catalog_cache.py index e5e5d5d97..c419fb664 100644 --- a/packages/core/src/catalog_cache.py +++ b/packages/core/src/catalog_cache.py @@ -49,8 +49,9 @@ def revisions(): if body is not None: entry = json.loads(body) if entry["revision"] == [list(item) for item in before]: + value = entry["value"] record_cache_read("catalog", "hit") - return entry["value"] + return value except CacheUnavailable: outcome, reason = "bypass", "cache_unavailable" except (ValueError, UnicodeError, KeyError, TypeError): diff --git a/pyproject.toml b/pyproject.toml index 9273a1548..60a895eca 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -78,6 +78,7 @@ dev = [ "pre-commit>=4.3,<5", "jsonschema>=4.23,<5", "testcontainers>=4.10,<5", + "pytest-cov>=7,<8", ] loadtest = [ "locust==2.46.6", @@ -103,6 +104,30 @@ select = ["E", "F", "I", "UP", "B", "SIM", "ASYNC2"] testpaths = ["tests"] markers = ["integration: requires a dedicated PostgreSQL database and Redis"] +[tool.coverage.run] +branch = true +data_file = "reports/coverage/.coverage-unit" +source = [ + "packages/core/src", + "packages/http/src", + "apps/api/src", + "apps/admin-api/src", + "apps/user-api/src", + "apps/mcp/src", + "apps/aggregator/src", + "apps/notifications/src", + "apps/cli/src", + "apps/search-indexer/src", + "apps/article-enrichment-worker/src", + "apps/images-worker/src", + "apps/source-discovery-worker/src", +] + +[tool.coverage.report] +fail_under = 67.50 +precision = 2 +show_missing = true + [tool.mypy] python_version = "3.12" plugins = ["pydantic.mypy"] diff --git a/scripts/ci/coverage_report.py b/scripts/ci/coverage_report.py new file mode 100644 index 000000000..590c8288c --- /dev/null +++ b/scripts/ci/coverage_report.py @@ -0,0 +1,161 @@ +"""Validate complete backend unit coverage and produce a bounded PR reporting artifact.""" + +import argparse +import json +import math +import tomllib +import xml.etree.ElementTree as ET +from pathlib import Path + +COUNTERS = ("covered_lines", "num_statements", "covered_branches", "num_branches") + + +def counts(value: dict) -> dict: + result = {name: value[name] for name in COUNTERS} + if any(type(number) is not int or number < 0 for number in result.values()): + raise ValueError("Invalid coverage counts") + if ( + result["covered_lines"] > result["num_statements"] + or result["covered_branches"] > result["num_branches"] + ): + raise ValueError("Covered counts exceed measured code") + return result + + +def percentages(value: dict) -> dict: + lines, branches = value["num_statements"], value["num_branches"] + return { + **value, + "line_percent": 100 * value["covered_lines"] / lines if lines else 100.0, + "branch_percent": 100 * value["covered_branches"] / branches if branches else 100.0, + "combined_percent": 100 + * (value["covered_lines"] + value["covered_branches"]) + / (lines + branches) + if lines + branches + else 100.0, + } + + +def summarize(root: Path, test_exit_code: int) -> dict: + config = tomllib.loads((root / "pyproject.toml").read_text()) + coverage = config["tool"]["coverage"] + members = config["tool"]["uv"]["workspace"]["members"] + sources = coverage["run"]["source"] + if coverage["run"].get("branch") is not True or set(sources) != { + f"{member}/src" for member in members + }: + raise ValueError("Coverage must include every backend workspace with branches") + if ( + len(sources) != len(set(sources)) + or coverage["run"].get("omit") + or coverage["report"].get("omit") + ): + raise ValueError("Coverage sources must not be duplicated or omitted") + minimum = coverage["report"]["fail_under"] + if type(minimum) not in (int, float) or not math.isfinite(minimum) or not 0 < minimum <= 100: + raise ValueError("Coverage minimum must be a finite percentage") + document = json.loads((root / "reports/coverage/unit.json").read_text()) + if document["meta"].get("branch_coverage") is not True: + raise ValueError("Branch coverage was not measured") + expected = { + path.relative_to(root).as_posix() + for source in sources + for path in (root / source).rglob("*.py") + } + if not expected or set(document["files"]) != expected: + raise ValueError("Coverage inventory differs from backend source files") + packages = {member: dict.fromkeys(COUNTERS, 0) for member in sorted(members)} + for filename, entry in document["files"].items(): + member = next(member for member in members if filename.startswith(f"{member}/src/")) + for name, count in counts(entry["summary"]).items(): + packages[member][name] += count + total = {name: sum(package[name] for package in packages.values()) for name in COUNTERS} + if counts(document["totals"]) != total or total["num_statements"] == 0: + raise ValueError("Coverage totals do not match measured source") + cases = list(ET.parse(root / "reports/python-unit.xml").getroot().iter("testcase")) + if not cases: + raise ValueError("No unit tests were reported") + failed = sum( + case.find("failure") is not None or case.find("error") is not None for case in cases + ) + skipped = sum(case.find("skipped") is not None for case in cases) + tests = { + "total": len(cases), + "failed": failed, + "skipped": skipped, + "passed": len(cases) - failed - skipped, + } + total = percentages(total) + passed = ( + test_exit_code == 0 + and failed == 0 + and skipped == 0 + and round(total["combined_percent"], 2) >= minimum + ) + return { + "schema": 1, + "valid": True, + "status": "passed" if passed else "failed", + "minimum": minimum, + "unit_exit_code": test_exit_code, + "source_files": len(expected), + "totals": total, + "packages": {member: percentages(value) for member, value in packages.items()}, + "tests": tests, + } + + +def markdown(summary: dict) -> str: + if not summary["valid"]: + return ( + "### Backend unit coverage\n\nCoverage report is missing or invalid. " + "Inspect the Python unit job logs.\n" + ) + total, tests = summary["totals"], summary["tests"] + rows = [ + "### Backend unit coverage", + "", + f"**{summary['status'].upper()}** — minimum combined coverage " + f"**{summary['minimum']:.2f}%**.", + "", + f"Tests: {tests['passed']} passed, {tests['failed']} failed, {tests['skipped']} skipped. " + f"Source files: {summary['source_files']}.", + "", + "| Workspace | Lines | Branches | Combined |", + "| --- | ---: | ---: | ---: |", + ] + for name, value in [("All backend code", total), *summary["packages"].items()]: + rows.append( + f"| {name} | {value['line_percent']:.2f}% | {value['branch_percent']:.2f}% | " + f"{value['combined_percent']:.2f}% |" + ) + rows.extend( + [ + "", + "Unit coverage includes every backend workspace. " + "PostgreSQL/Redis integration tests run separately.", + "", + ] + ) + return "\n".join(rows) + + +def main() -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--root", type=Path, default=Path(__file__).resolve().parents[2]) + parser.add_argument("--test-exit-code", type=int, required=True) + args = parser.parse_args() + try: + summary = summarize(args.root, args.test_exit_code) + except (OSError, ValueError, KeyError, TypeError, ET.ParseError, StopIteration): + summary = {"schema": 1, "valid": False, "status": "incomplete"} + destination = args.root / "reports/coverage" + destination.mkdir(parents=True, exist_ok=True) + (destination / "summary.json").write_text(json.dumps(summary, indent=2) + "\n") + (destination / "summary.md").write_text(markdown(summary)) + print(markdown(summary)) + return 0 if summary["status"] == "passed" else 1 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/scripts/ci/python-tests.sh b/scripts/ci/python-tests.sh index 7a6f387a6..330879cb7 100644 --- a/scripts/ci/python-tests.sh +++ b/scripts/ci/python-tests.sh @@ -18,7 +18,21 @@ mkdir -p reports export DEVFEED_DATABASE_URL=postgresql+psycopg://ci@database.invalid/ci export DEVFEED_REDIS_URL=redis://redis.invalid/15 if [ "$suite" != integration ]; then - bash scripts/test.sh -q -m 'not integration' --junitxml=reports/python-unit.xml + rm -rf reports/coverage + rm -f reports/python-unit.xml + unit_status=0 + bash scripts/test.sh -q -m 'not integration' --junitxml=reports/python-unit.xml \ + --cov --cov-report=term:skip-covered --cov-report=json:reports/coverage/unit.json \ + --cov-report=xml:reports/coverage/unit.xml --cov-report=html:reports/coverage/html \ + || unit_status=$? + report_status=0 + uv run --locked python scripts/ci/coverage_report.py --test-exit-code "$unit_status" \ + || report_status=$? + if [ -n "${GITHUB_STEP_SUMMARY:-}" ]; then + cat reports/coverage/summary.md >> "$GITHUB_STEP_SUMMARY" + fi + if [ "$unit_status" -ne 0 ]; then exit "$unit_status"; fi + if [ "$report_status" -ne 0 ]; then exit "$report_status"; fi uv run --locked python scripts/ci/check_reports.py junit reports/python-unit.xml fi if [ "$suite" = unit ]; then exit 0; fi diff --git a/scripts/pre_commit_checks.py b/scripts/pre_commit_checks.py index a6b475e02..dbabca890 100644 --- a/scripts/pre_commit_checks.py +++ b/scripts/pre_commit_checks.py @@ -44,7 +44,17 @@ def checks_for(paths: set[str]) -> list[tuple[str, ...]]: ("uv", "run", "--locked", "ruff", "check", "."), ("uv", "run", "--locked", "ruff", "format", "--check", "."), ("uv", "run", "--locked", "mypy"), - ("uv", "run", "--locked", "pytest", "-q", "-m", "not integration"), + ( + "uv", + "run", + "--locked", + "pytest", + "-q", + "-m", + "not integration", + "--cov", + "--cov-report=term:skip-covered", + ), ] ) admin = shared_web or any( diff --git a/tests/test_catalog_snapshot_unit.py b/tests/test_catalog_snapshot_unit.py new file mode 100644 index 000000000..ef47df79f --- /dev/null +++ b/tests/test_catalog_snapshot_unit.py @@ -0,0 +1,116 @@ +"""Revision-cache safety and fallback policy independent of PostgreSQL/Redis I/O.""" + +import json +from types import SimpleNamespace +from unittest.mock import Mock + +import pytest +from devfeed_core import catalog_cache +from devfeed_core.cache import CacheUnavailable + +REVISIONS = [("tags", "tag-revision"), ("topics", "topic-revision")] +NAMES = ("topics", "tags") + + +@pytest.fixture +def snapshot(monkeypatch): + session = Mock() + session.execute.side_effect = [REVISIONS, REVISIONS] + cache = SimpleNamespace(namespace="test", redis=Mock(), _run=lambda call: call()) + cache.redis.get.return_value = None + loader = Mock(return_value=[{"name": "Python"}]) + metrics = Mock() + monkeypatch.setattr(catalog_cache, "get_settings", lambda: SimpleNamespace(cache_enabled=True)) + monkeypatch.setattr(catalog_cache, "get_cache", lambda: cache) + monkeypatch.setattr(catalog_cache, "record_cache_read", metrics) + return SimpleNamespace(session=session, cache=cache, loader=loader, metrics=metrics) + + +def test_matching_revisions_reuse_cached_catalog_without_loading_database_rows(snapshot): + snapshot.cache.redis.get.return_value = json.dumps({"revision": REVISIONS, "value": []}) + assert catalog_cache.snapshot(snapshot.session, NAMES, snapshot.loader) == [] + snapshot.loader.assert_not_called() + snapshot.cache.redis.set.assert_not_called() + assert snapshot.session.execute.call_count == 1 + snapshot.metrics.assert_called_once_with("catalog", "hit") + + +@pytest.mark.parametrize( + "body,reason", + [ + (None, None), + (json.dumps({"revision": [["tags", "old"], ["topics", "old"]], "value": []}), None), + (b"\xff", "invalid_entry"), + ("not json", "invalid_entry"), + ("{}", "invalid_entry"), + ("[]", "invalid_entry"), + (json.dumps({"revision": REVISIONS}), "invalid_entry"), + ], +) +def test_misses_and_invalid_entries_reload_and_publish_only_the_current_revision( + snapshot, body, reason +): + snapshot.cache.redis.get.return_value = body + assert ( + catalog_cache.snapshot(snapshot.session, NAMES, snapshot.loader) + == snapshot.loader.return_value + ) + snapshot.loader.assert_called_once_with() + snapshot.session.flush.assert_called_once_with() + snapshot.metrics.assert_called_once_with("catalog", "miss", reason) + args, kwargs = snapshot.cache.redis.set.call_args + assert args[0] == "test:catalog:v1:tags,topics" + assert json.loads(args[1]) == { + "revision": [list(row) for row in REVISIONS], + "value": snapshot.loader.return_value, + } + assert kwargs == {"ex": 600} + + +def test_concurrent_catalog_edit_never_publishes_a_mixed_snapshot(snapshot): + snapshot.session.execute.side_effect = [REVISIONS, [("tags", "new"), REVISIONS[1]]] + assert ( + catalog_cache.snapshot(snapshot.session, NAMES, snapshot.loader) + == snapshot.loader.return_value + ) + snapshot.cache.redis.set.assert_not_called() + + +@pytest.mark.parametrize("failed_operation", ["get", "set"]) +def test_cache_outage_keeps_database_catalog_available(snapshot, failed_operation): + getattr(snapshot.cache.redis, failed_operation).side_effect = CacheUnavailable + assert ( + catalog_cache.snapshot(snapshot.session, NAMES, snapshot.loader) + == snapshot.loader.return_value + ) + snapshot.loader.assert_called_once_with() + if failed_operation == "get": + snapshot.metrics.assert_called_once_with("catalog", "bypass", "cache_unavailable") + + +def test_missing_revisions_bypass_storage_without_guessing_cache_freshness(snapshot): + snapshot.session.execute.side_effect = [REVISIONS[:1]] + assert ( + catalog_cache.snapshot(snapshot.session, NAMES, snapshot.loader) + == snapshot.loader.return_value + ) + snapshot.cache.redis.get.assert_not_called() + snapshot.cache.redis.set.assert_not_called() + + +def test_disabled_cache_uses_loader_without_flushing_or_querying_revisions(snapshot, monkeypatch): + monkeypatch.setattr(catalog_cache, "get_settings", lambda: SimpleNamespace(cache_enabled=False)) + assert ( + catalog_cache.snapshot(snapshot.session, NAMES, snapshot.loader) + == snapshot.loader.return_value + ) + snapshot.session.flush.assert_not_called() + snapshot.session.execute.assert_not_called() + snapshot.metrics.assert_called_once_with("catalog", "bypass", "disabled") + + +def test_database_failure_is_not_converted_to_a_successful_cache_response(snapshot): + snapshot.loader.side_effect = RuntimeError("database unavailable") + with pytest.raises(RuntimeError, match="database unavailable"): + catalog_cache.snapshot(snapshot.session, NAMES, snapshot.loader) + snapshot.cache.redis.set.assert_not_called() diff --git a/tests/test_ci_gates.py b/tests/test_ci_gates.py index 7a4997ca8..c84aa7bfa 100644 --- a/tests/test_ci_gates.py +++ b/tests/test_ci_gates.py @@ -106,13 +106,15 @@ def test_ci_shards_reject_invalid_partition(tmp_path, index, count): ("schedule", "branch"), ], ) -def test_ci_required_accepts_only_the_expected_successes(event, ref_type): +@pytest.mark.parametrize("comment_required", [False, True]) +def test_ci_required_accepts_only_the_expected_successes(event, ref_type, comment_required): # Execute the actual gate, including the mutually exclusive release/check jobs. workflow = (ROOT / ".github/workflows/ci.yml").read_text() gate = textwrap.dedent(workflow.split(" required:\n", 1)[1].split(" run: |\n", 1)[1]) release = event == "push" and ref_type == "tag" results = { "python-unit": {"result": "success"}, + "coverage-report": {"result": "success" if comment_required else "skipped"}, "admin-web": {"result": "success"}, "user-web": {"result": "success"}, "extensions": {"result": "success"}, @@ -126,6 +128,12 @@ def test_ci_required_accepts_only_the_expected_successes(event, ref_type): "release-images": {"result": "success" if release else "skipped"}, "performance": {"result": "success" if event == "pull_request" else "skipped"}, } + declared_needs = ( + workflow.split(" required:\n", 1)[1] + .split(" needs:\n", 1)[1] + .split(" runs-on:", 1)[0] + ) + assert {line.strip().removeprefix("- ") for line in declared_needs.splitlines()} == set(results) def evaluate(): return subprocess.run( @@ -134,6 +142,7 @@ def evaluate(): **os.environ, "EVENT_NAME": event, "REF_TYPE": ref_type, + "COVERAGE_COMMENT_REQUIRED": str(comment_required).lower(), "RESULTS": json.dumps(results), }, capture_output=True, diff --git a/tests/test_coverage_report.py b/tests/test_coverage_report.py new file mode 100644 index 000000000..2140e0810 --- /dev/null +++ b/tests/test_coverage_report.py @@ -0,0 +1,303 @@ +"""Fail-closed coverage inventories, measured counters, and PR comment rendering.""" + +import importlib.util +import json +import subprocess +import textwrap +from pathlib import Path + +import pytest + +ROOT = Path(__file__).resolve().parents[1] +SPEC = importlib.util.spec_from_file_location( + "coverage_report", ROOT / "scripts/ci/coverage_report.py" +) +report = importlib.util.module_from_spec(SPEC) +SPEC.loader.exec_module(report) + + +@pytest.fixture +def measurement(tmp_path): + (tmp_path / "pyproject.toml").write_text( + '[tool.uv.workspace]\nmembers = ["apps/api", "packages/core"]\n' + '[tool.coverage.run]\nbranch = true\nsource = ["apps/api/src", "packages/core/src"]\n' + "[tool.coverage.report]\nfail_under = 65.0\n" + ) + paths = ["apps/api/src/main.py", "packages/core/src/unimported.py"] + for name in paths: + path = tmp_path / name + path.parent.mkdir(parents=True) + path.write_text("def value():\n return 1\n") + covered = {"covered_lines": 3, "num_statements": 4, "covered_branches": 1, "num_branches": 2} + document = { + "meta": {"branch_coverage": True}, + "files": {name: {"summary": dict(covered)} for name in paths}, + "totals": {name: value * 2 for name, value in covered.items()}, + } + (tmp_path / "reports/coverage").mkdir(parents=True) + (tmp_path / "reports/python-unit.xml").write_text( + '' + ) + + def save(): + (tmp_path / "reports/coverage/unit.json").write_text(json.dumps(document)) + + save() + return tmp_path, document, save + + +def test_report_counts_all_backend_files_and_separates_lines_from_branches(measurement): + root, _, _ = measurement + value = report.summarize(root, 0) + assert value["status"] == "passed" and value["source_files"] == 2 + assert value["totals"]["line_percent"] == 75 + assert value["totals"]["branch_percent"] == 50 + assert value["totals"]["combined_percent"] == pytest.approx(66.6666667) + assert value["tests"] == {"total": 2, "passed": 2, "failed": 0, "skipped": 0} + assert set(value["packages"]) == {"apps/api", "packages/core"} + assert "66.67%" in report.markdown(value) + + +@pytest.mark.parametrize("exit_code", [1, 2, 5]) +def test_successful_testcases_cannot_hide_pytest_or_coverage_failure(measurement, exit_code): + root, _, _ = measurement + assert report.summarize(root, exit_code)["status"] == "failed" + + +@pytest.mark.parametrize("tag", ["failure", "error", "skipped"]) +def test_nonpassing_testcases_fail_the_report(measurement, tag): + root, _, _ = measurement + (root / "reports/python-unit.xml").write_text( + f"<{tag}/>" + ) + assert report.summarize(root, 0)["status"] == "failed" + + +def test_coverage_below_the_configured_minimum_fails(measurement): + root, _, _ = measurement + path = root / "pyproject.toml" + path.write_text(path.read_text().replace("65.0", "67.0")) + assert report.summarize(root, 0)["status"] == "failed" + + +@pytest.mark.parametrize( + "change", + [ + "missing-file", + "extra-file", + "totals", + "negative", + "boolean", + "overcount", + "branches", + "empty", + ], +) +def test_incomplete_or_falsified_measurements_are_rejected(measurement, change): + root, document, save = measurement + if change == "missing-file": + document["files"].pop("packages/core/src/unimported.py") + elif change == "extra-file": + document["files"]["tests/test_example.py"] = next(iter(document["files"].values())) + elif change == "branches": + document["meta"]["branch_coverage"] = False + elif change == "empty": + for entry in document["files"].values(): + entry["summary"] = dict.fromkeys(report.COUNTERS, 0) + document["totals"] = dict.fromkeys(report.COUNTERS, 0) + else: + document["totals"]["covered_lines"] = { + "totals": 1, + "negative": -1, + "boolean": True, + "overcount": 99, + }[change] + save() + with pytest.raises(ValueError): + report.summarize(root, 0) + + +@pytest.mark.parametrize( + "change", ["source", "duplicate", "branch", "omit", "minimum", "empty-tests"] +) +def test_configuration_cannot_hide_workspaces_or_accept_empty_suites(measurement, change): + root, _, _ = measurement + path = root / "pyproject.toml" + value = path.read_text() + if change == "source": + value = value.replace( + 'source = ["apps/api/src", "packages/core/src"]', 'source = ["apps/api/src"]' + ) + elif change == "duplicate": + value = value.replace( + 'source = ["apps/api/src", "packages/core/src"]', + 'source = ["apps/api/src", "packages/core/src", "apps/api/src"]', + ) + elif change == "branch": + value = value.replace("branch = true", "branch = false") + elif change == "omit": + value += 'omit = ["*/low_coverage.py"]\n' + elif change == "minimum": + value = value.replace("65.0", "nan") + else: + (root / "reports/python-unit.xml").write_text('') + path.write_text(value) + with pytest.raises(ValueError): + report.summarize(root, 0) + + +@pytest.mark.parametrize( + "broken", ["missing-coverage", "invalid-json", "missing-junit", "invalid-xml"] +) +def test_cli_always_writes_an_incomplete_report_on_invalid_input(measurement, broken): + root, _, _ = measurement + path = root / ( + "reports/coverage/unit.json" + if broken in {"missing-coverage", "invalid-json"} + else "reports/python-unit.xml" + ) + if broken.startswith("missing"): + path.unlink() + else: + path.write_text("invalid input") + result = subprocess.run( + [ + "uv", + "run", + "--locked", + "python", + str(ROOT / "scripts/ci/coverage_report.py"), + "--root", + str(root), + "--test-exit-code", + "0", + ], + cwd=ROOT, + capture_output=True, + text=True, + ) + assert result.returncode == 1 + assert json.loads((root / "reports/coverage/summary.json").read_text()) == { + "schema": 1, + "valid": False, + "status": "incomplete", + } + assert "missing or invalid" in (root / "reports/coverage/summary.md").read_text() + + +def run_comment(tmp_path, *, scenario="success"): + workflow = (ROOT / ".github/workflows/ci.yml").read_text() + script = textwrap.dedent( + workflow.split(" script: |\n", 1)[1].split(" admin-web:\n", 1)[0] + ) + wrapper = """ + const fs = require('fs'); + const vm = require('vm'); + const input = JSON.parse(fs.readFileSync(process.argv[2], 'utf8')); + const calls = []; + const head = input.stale ? 'new-head' : 'abcdef1234567890'; + let reads = 0; + const github = {rest: { + pulls: {get: async () => ({data: {head: { + sha: input.lateStale && reads++ ? 'new-head' : head}}})}, + issues: {listComments: {}, createComment: async value => calls.push(['create', value]), + updateComment: async value => calls.push(['update', value])} + }, paginate: async () => input.previous ? [{id: 7, + user: {type: input.human ? 'User' : 'Bot'}, + body: '\\nold'}] : []}; + const context = {repo: {owner: 'owner', repo: 'repo'}, + serverUrl: 'https://github.com', runId: 123, + payload: {pull_request: {number: 4, head: {sha: 'abcdef1234567890'}}}}; + const sandbox = {require: () => ({readFileSync: () => JSON.stringify(input.summary)}), + github, context, + core: {setFailed: message => calls.push(['failed', message])}, + process: {env: {UNIT_RESULT: input.result, GITHUB_RUN_ATTEMPT: '2'}}}; + vm.runInNewContext('(async () => {' + input.script + '})()', sandbox) + .then(() => console.log(JSON.stringify(calls))) + .catch(error => {console.error(error); process.exit(1);}); + """ + workspaces = [ + "apps/admin-api", + "apps/aggregator", + "apps/api", + "apps/article-enrichment-worker", + "apps/cli", + "apps/images-worker", + "apps/mcp", + "apps/notifications", + "apps/search-indexer", + "apps/source-discovery-worker", + "apps/user-api", + "packages/core", + "packages/http", + ] + counters = {"covered_lines": 3, "num_statements": 4, "covered_branches": 1, "num_branches": 2} + summary = { + "schema": 1, + "valid": True, + "status": "passed", + "minimum": 65, + "unit_exit_code": 0, + "source_files": 270, + "totals": {key: value * 13 for key, value in counters.items()}, + "packages": dict.fromkeys(workspaces, counters), + "tests": {"total": 2, "passed": 2, "failed": 0, "skipped": 0}, + } + if scenario == "malformed": + summary["totals"]["covered_lines"] = "@everyone