Skip to content

ci(coverage): add Codecov integration with 60% project gate#1245

Open
kovtcharov-amd wants to merge 2 commits into
mainfrom
feat/codecov-879
Open

ci(coverage): add Codecov integration with 60% project gate#1245
kovtcharov-amd wants to merge 2 commits into
mainfrom
feat/codecov-879

Conversation

@kovtcharov-amd
Copy link
Copy Markdown
Collaborator

Unit tests already ran pytest-cov but the data was never uploaded — no visibility into coverage trends, no PR gates. Now every unit-test run generates coverage.xml and uploads it to Codecov (py3.12 leg only to avoid duplicate reports), with a 60% project target and 70% patch target so regressions surface before merge.

Note: CODECOV_TOKEN must be added to the repo's Actions secrets (Settings → Secrets → Actions) for the upload step to authenticate. Without it the upload silently no-ops (fail_ci_if_error: false).

Test plan

  • Verify CODECOV_TOKEN secret is configured in repo settings
  • Merge and confirm the Codecov upload step runs on the next push to main
  • Open a test PR touching src/gaia/ and verify Codecov posts a coverage status check

Closes #879

@github-actions github-actions Bot added dependencies Dependency updates devops DevOps/infrastructure changes labels May 29, 2026
@github-actions
Copy link
Copy Markdown
Contributor

Minimal and correct CI addition — the implementation is clean, well-scoped, and the .codecov.yml exclusion list is well-considered. Two minor points before merge.

Summary

The PR wires the existing pytest-cov output to Codecov with sensible project (60%) and patch (70%) gates. The .codecov.yml exclusions correctly limit reporting to production code, fail_ci_if_error: false ensures Codecov outages don't break the unit-test gate, and the setup.py addition closes the gap for local developers. No blocking issues found.

Issues Found

🟢 Minor — codecov-action is one major version behind (.github/workflows/test_unit.yml:91)

codecov/codecov-action@v4 is current but not latest. v5 added OIDC-based tokenless authentication (no secret required on public repos) and prefers with: token: over the env-var approach used by v4. If upgrading, drop the env: block and move the token into with::

        uses: codecov/codecov-action@v5
        with:
          files: coverage.xml
          flags: unit
          fail_ci_if_error: false
          token: ${{ secrets.CODECOV_TOKEN }}

(v4 is stable and this is not a blocker — just noted for future-proofing.)

🟢 Minor — upload step silently skips on test failures (.github/workflows/test_unit.yml:90)

No if: guard means coverage is not uploaded when "Run unit tests" fails. Coverage from partial runs (a single flaky test, a new assertion failure) is lost, which can obscure regressions. Adding if: always() makes the intent explicit:

      - name: Upload coverage to Codecov
        if: always()
        uses: codecov/codecov-action@v4

This is a design choice — some teams intentionally skip failed-run uploads. Either way, explicit beats implicit.

Strengths

  • fail_ci_if_error: false is the right default — a Codecov outage should never break a PR gate, especially early in the integration's life.
  • .codecov.yml exclusion list (tests/, docs/, src/gaia/eval/, src/gaia/apps/webui/) scopes coverage correctly to production Python — eval harness and frontend bundle are correctly excluded.
  • The setup.py [dev] addition is the right place for pytest-cov — it was already installed explicitly in CI (line 65), and this closes the gap for contributors running pip install -e ".[dev]" locally.

Verdict

Approve with suggestions. Neither issue blocks merge. The core integration is correct, the PR description accurately documents the manual step required (CODECOV_TOKEN secret in repo settings), and the targets (60% project / 70% patch with 2% tolerance) are reasonable for a first-pass gate.

@github-actions
Copy link
Copy Markdown
Contributor

Solid, well-scoped CI addition — coverage was already being computed but silently discarded; this PR wires the upload and adds a 60%/70% gate with no unnecessary scope creep.

Summary

Three files, one logical change: generate coverage.xml, upload to Codecov, and declare the target thresholds. The .codecov.yml config is reasonable, the setup.py addition ensures local dev environments match CI, and fail_ci_if_error: false is a sensible bootstrap choice while the secret gets wired up. One minor nit worth fixing; no blockers.

Issues Found

🟢 No threshold on the patch target — may block PRs with slightly-under-70% new code (.codecov.yml:19)

The project gate has a 2% forgiveness window (threshold: 2%), but the patch gate does not. Any PR where new/changed lines land at 69.9% covered will fail outright. For a project where some utility modules are hard to unit-test, this could create unnecessary friction. Consider adding a small threshold for parity:

    patch:
      default:
        target: 70%
        threshold: 2%

🟢 Codecov action not pinned to a commit SHA (.github/workflows/test_unit.yml:92)

codecov/codecov-action@v5 uses a floating tag. Other Actions in this repo (e.g. actions/checkout@v6, actions/setup-python@v6) follow the same convention, so this is consistent with the codebase style — worth noting but not worth blocking over. If the project ever adds a supply-chain hardening pass, this would be a candidate to pin.

Strengths

  • if: always() on the upload step is exactly right — partial coverage data on a failing run is more useful than no data, and fail_ci_if_error: false prevents a missing secret from breaking CI during the rollout window.
  • Adding pytest-cov to setup.py's dev extras is a nice touch; the CI was already installing it explicitly (line 65), so this just closes the gap for local dev environments.
  • The ignore list in .codecov.yml is well-considered: tests/, src/gaia/eval/, and src/gaia/apps/webui/ are all either test infra or generated frontend code where line coverage is a poor signal.

Verdict

Approve — one optional nit on the patch threshold, no blockers. The secret configuration note in the PR description is the only remaining action before the gate is live.

@kovtcharov-amd kovtcharov-amd added the p2 low priority label May 29, 2026
@kovtcharov-amd kovtcharov-amd self-assigned this May 29, 2026
Copy link
Copy Markdown
Collaborator

@itomek itomek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Coverage was being computed and discarded; this wires the upload with codecov-action@v5 + if: always(), both correct (the earlier v4/if-always review notes are stale). One patch-gate behavior point noted inline. Separately, per the description the upload silently no-ops without CODECOV_TOKEN — the right bootstrap choice, but it means the 60% gate isn't actually live until the secret is configured, so confirm that's set so this doesn't sit as a no-op. Approving.


Generated by Claude Code

Comment thread .codecov.yml
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Project gets threshold: 2% but patch has no threshold, so a PR whose changed lines land at 69.9% fails the patch status outright. If this status is meant to block merges, add threshold: 2% here for parity with project; if advisory-only at first, fine as-is — just be explicit about which, since require_ci_to_pass: true plus a hard patch target reads as blocking.


Generated by Claude Code

Ovtcharov added 2 commits May 29, 2026 09:33
Unit tests already ran pytest-cov but never uploaded the data. Now coverage.xml
is generated and uploaded to Codecov on the py3.12 matrix leg, with a 60% project
target and 70% patch target. pytest-cov is declared as a dev dependency so local
runs pick it up too.

Closes #879
Add `if: always()` so partial-run coverage is still uploaded when tests
fail. Upgrade to codecov-action@v5 and move token from env to the `with`
block per the v5 migration guide.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dependencies Dependency updates devops DevOps/infrastructure changes p2 low priority

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Coverage: aggregate with Codecov and gate at 60%

2 participants