Skip to content

fix(ci): run validators/ unit tests in make test, exclude from covergae gate #1918

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mohityadav8 wants to merge 1 commit into
base: main
Choose a base branch
from
+2 −2
Open

fix(ci): run validators/ unit tests in make test, exclude from covergae gate #1918

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,13 +226,13 @@ test: test-shell ## Runs unit tests with race detector and coverage (use -short
echo "Running tests with race detector..."; \
KUBEBUILDER_ASSETS=$$(setup-envtest use -p path 2>/dev/null || echo "") \
AICR_CRITERIA_STRICT=1 \
GOFLAGS="-mod=vendor" go test -short -count=1 -race -timeout=$(TEST_TIMEOUT) -covermode=atomic -coverprofile=coverage.out $$(go list ./... | grep -v -e /tests/chainsaw/ -e /validators) || exit 1; \
GOFLAGS="-mod=vendor" go test -short -count=1 -race -timeout=$(TEST_TIMEOUT) -covermode=atomic -coverprofile=coverage.out $$(go list ./... | grep -v -e /tests/chainsaw/) || exit 1; \

@njhensley njhensley Jul 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🟡 Minor — No guard that the revived tests stay revived (unmet acceptance criterion of #1752)

#1752 lists verbatim: "Add a guard proving the tests from #1748 and #1745 are discovered and executed." The fix here is a negative-space change — deleting a grep term — and the exact regression class it repairs went unnoticed for four months. Nothing asserts that validators/deployment/nvidia_smi_test.go (#1748) and validators/performance/inference_perf_test.go (#1745) are in the executed package set.

Blast radius: The same silent-exclusion regression can recur; the new coverage filter is a second instance of the same footgun.

Fix: test-shell (Makefile:224) already runs tools/*_test.sh and is a natural home for a ~15-line hermetic assertion that the executed package set contains github.com/NVIDIA/aicr/validators/deployment and .../validators/performance. Low-probability regression, so reasonable to defer — but since the PR says Fixes #1752, either add it or re-scope the issue rather than letting it auto-close with 2 of 5 criteria unmet.

echo "Test coverage:"; \
go tool cover -func=coverage.out | tail -1
Comment on lines 230 to 231

@coderabbitai coderabbitai Bot Jul 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Keep displayed coverage consistent with the enforced metric.

test-coverage invokes test, so it first prints coverage including validators and then reports the filtered percentage below. These totals can differ substantially; apply the same filter here or label this as all-package coverage.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Makefile` around lines 230 - 231, Update the coverage output in the
test-coverage target so it uses the same validator-excluding filter as the
enforced coverage metric, keeping the displayed total consistent with the
reported percentage.

@njhensley njhensley Jul 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🟡 Minor — Two divergent coverage numbers for the same commit; decide deliberately which one is published

This line still prints the unfiltered total, so a single make test-coverage run emits Test coverage: 75.8% followed two lines later by Coverage: 80.2% (threshold: 80%) — a 4.4-point contradiction with no explanation, which reads like a computation bug. The same raw coverage.out also feeds the shields.io badge gist (75.8% flips the color bucket from brightgreen (>=80) to green (>=70)), the coverage-pr/coverage-baseline artifacts, and the go-coverage-report delta comment.

Blast radius: Contributor confusion locally, plus a ~4.4pt drop in the publicly reported coverage badge on main that reflects no real regression.

Fix: Filter once at profile-production time (see the blocker) so this line, the gate, the badge, and the delta report all agree. If the filter must stay in test-coverage, at minimum label the two numbers explicitly, e.g. Total (incl. validators, ungated): … vs Gated (excl. validators): …. The point is to make it a deliberate choice rather than an accident.


.PHONY: test-coverage
test-coverage: test ## Runs tests and enforces coverage threshold (from .settings.yaml quality.coverage_threshold)

@njhensley njhensley Jul 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🔵 Nitpick — Carve-out is undocumented, and docs/contributor/tests.md newly says something false

No Makefile comment explains the exclusion — which is exactly how the original -e /validators survived unexamined for four months. Separately, the word "project-wide" in docs/contributor/tests.md:94 (and the mirrored AGENTS.md/.claude/CLAUDE.md:585) becomes factually wrong once validators are carved out.

Blast radius: Maintainability — a future contributor either "fixes" the filter (re-breaking the gate) or widens it without realizing it is a deliberate carve-out.

Fix: Add a comment naming the reason, the tracking issue, and the removal condition. Fix "project-wide" in docs/contributor/tests.md:94 and AGENTS.md/CLAUDE.md:585 (those two must change together — AGENTS.md is a CI-enforced mirror). Note the 75%/70%-vs-actual-80 drift in those same lines plus DEVELOPMENT.md:458 is entirely pre-existing — flagging it, not asking you to fix it here.

@coverage=$$(go tool cover -func=coverage.out | grep total | awk '{print $$3}' | sed 's/%//'); \
@coverage=$$(grep -v '/validators/' coverage.out | go tool cover -func=/dev/stdin | grep total | awk '{print $$3}' | sed 's/%//'); \

@coderabbitai coderabbitai Bot Jul 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

Document the reduced scope of the coverage gate.

This now excludes validator coverage while docs/contributor/tests.md:94-96 still describes the threshold as project-wide. Either include validators in the aggregate or document and enforce a separate validator coverage policy, as required by the PR objective.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@Makefile` at line 235, Update the coverage gate around the coverage variable
to align with the documented project-wide threshold: either remove the
/validators/ exclusion from the aggregate calculation, or add and enforce a
separate validator coverage policy and update the contributor testing
documentation accordingly.

@njhensley njhensley Jul 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🔴 Blocker — Coverage exclusion is in a target CI never invokes; merge gate will hard-fail at 75.8% vs the 80% threshold

The filter only affects make test-coverage. CI never runs that target — grep -rn 'test-coverage' .github/ returns zero hits. The actual CI path is qualification.yaml:61 → .github/actions/go-test/action.yml:72 (run: make test) → .github/actions/go-coverage with coverage_file: ./coverage.out. That action recomputes go tool cover -func="$COVERAGE_FILE" on the unfiltered profile (go-coverage/action.yml:65), and its final Enforce Threshold step does exit 1 (:180-185) — ungated, no continue-on-error.

The threshold is genuinely 80, traced end to end: .settings.yaml:156 coverage_threshold: '80' → load-versions/action.yml:209 → qualification.yaml:63 → go-test/action.yml:79. The action's THRESHOLD=70 fallback is not taken. Both merge-gate.yaml:174 and on-push.yaml:54 pass coverage_report: true, and merge-gate's dorny/paths-filter (predicate-quantifier: every, ['**','!**.md','!docs/**','!LICENSE']) matches Makefile, so the tests job does run for this PR.

Measured independently three times at this head:

unfiltered (what CI gates on):        total: (statements) 75.8%   → FAIL vs 80
filtered   (what make test-coverage): total: (statements) 80.2%   → pass

On main today, validators are never compiled into the profile at all, so main's coverage.out is the filtered set — CI passes at 80.2% with ~0.2pt of headroom. This PR pushes it 4.4pt under.

Blast radius: The tests job fails on this PR and on every subsequent PR/main push until reverted. It also breaks the repo's stated invariant ("if make qualify passes locally, CI will pass") in the most confusing direction: local make qualify goes green at 80.2% while the required check goes red at 75.8%, with no local way to reproduce.

Fix: Put the exclusion where CI actually reads it. Cleanest is to filter at profile-production time so every consumer (gate, badge, artifacts, delta comment) sees one number — in the test target:

GOFLAGS="-mod=vendor" go test ... -coverprofile=coverage.full.out $$(go list ./... | grep -v -e /tests/chainsaw/) || exit 1; \
grep -v '^github.com/NVIDIA/aicr/validators/' coverage.full.out > coverage.out; \

…and revert this line to go tool cover -func=coverage.out. Alternatively add a coverage_exclude input to .github/actions/go-coverage and source the pattern from .settings.yaml so the Makefile and the action cannot drift. Either way, confirm the merge-gate Test job is green before merging — it has not run on this PR yet (only the labelers and CodeRabbit have).

@njhensley njhensley Jul 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🟡 Minor — Exemption is unbounded: no floor, no expiry, no tracking issue

Per-package coverage today: validators 41.2%, chainsaw 73.3%, conformance 52.6%, deployment 58.3%, helper 10.8%, internal/allocmode 92.2%, performance 56.5%. Nothing now prevents that from decaying — the gated number literally cannot move for validator code, which is the part of AICR that runs unattended on GPU clusters.

Blast radius: Long-term erosion of validator test quality. Undocumented carve-outs tend to become permanent.

Fix: To be clear about proportionality: "exempt now, ratchet later" is the right call for a two-line CI fix. Don't lower the global threshold (that degrades the gate for all of pkg/) and don't write tests for 7 packages here. Just bound it — a follow-up issue for a separate lower validators floor (~4 lines of make: grep '/validators/' coverage.out | go tool cover -func=/dev/stdin, compared against e.g. 50) converts an open-ended carve-out into a ratchet.

@njhensley njhensley Jul 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🟡 Minor — Unanchored grep pattern will silently swallow future */validators/* packages

grep -v '/validators/' matches the segment anywhere in the import path. Zero false positives and zero false negatives today (verified in both directions), and the mode: atomic header is correctly preserved. But a future internal/validators/ or pkg/x/validators/ package would drop out of the gate with no error and no signal.

Blast radius: Future maintainers — a real coverage regression in a newly added */validators/* package would not be caught.

Fix: Use grep -v '^github.com/NVIDIA/aicr/validators/' — verified to select the identical line set today. Credit where due: the current pattern correctly does not catch the singular pkg/validator/** tree, which was the likelier accidental-exclusion trap.

echo "Coverage: $$coverage% (threshold: $(COVERAGE_THRESHOLD)%)"; \

@njhensley njhensley Jul 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🟡 Minor — Pre-existing fail-open in the coverage gate (informational — your change improves one case)

test-coverage has no set -o pipefail. On a corrupt profile $coverage ends up empty, bc emits a parse error, [ -eq 1 ] errors with 'unary operator expected', the if body is skipped, and the recipe prints "Coverage check passed" and exits 0. Reproduced in a scratch Makefile on both the pre- and post-change recipes, so this is pre-existing and not introduced here. Your change incidentally improves one case: a missing coverage.out now fails closed (empty stdin → total: 0.0% → gate errors) where it previously failed open.

Blast radius: Local make qualify / make test-coverage only — CI is unaffected because .github/actions/go-coverage uses set -euo pipefail and explicitly checks for an empty $COVERAGE.

Fix: Not yours to fix in this PR. Worth a drive-by follow-up: if [ -z "$$coverage" ]; then echo "ERROR: failed to compute coverage"; exit 1; fi after the assignment, mirroring what the CI action already does.

if [ $$(echo "$$coverage < $(COVERAGE_THRESHOLD)" | bc) -eq 1 ]; then \
echo "ERROR: Coverage $$coverage% is below threshold $(COVERAGE_THRESHOLD)%"; \
Expand Down