Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion .claude/CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -582,7 +582,7 @@ Follow the heading conventions in the `## Documentation Style` section above. Do
Before pushing a PR that changes Go source, check coverage on affected packages. Set `pkg` to the narrowest changed root — `$pkg/...` includes descendants (e.g. use `pkg=pkg/collector/topology`, not `pkg=pkg/collector`, unless you want a combined delta).
1. Current: `GOFLAGS="-mod=vendor" go test -coverprofile=cover.out ./$pkg/...` on each changed package.
2. Baseline (skip for new packages; commit changes first): `(git worktree add $TMPDIR/baseline origin/main && (cd $TMPDIR/baseline && GOFLAGS="-mod=vendor" go test -coverprofile=$TMPDIR/base.out ./$pkg/...); rc=$?; git worktree remove --force $TMPDIR/baseline; return $rc 2>/dev/null || (exit $rc))` — `$TMPDIR/base.out` survives cleanup and `rc` preserves test status. Compare with `go tool cover -func`.
3. **Block** if `make test-coverage` fails (enforces the project-wide 80% floor from `.settings.yaml`; do not use per-package profiles for this check).
3. **Block** if `make test-coverage` fails (enforces the 80% floor from `.settings.yaml`, excluding `validators/` — see #1752; do not use per-package profiles for this check).
4. **Flag** any package with per-package decrease > 0.5% (step 1 vs 2).
5. **Block** if any new exported func/method (`git diff origin/main -- $pkg/`, added uppercase `func` lines) has 0% coverage — add tests first.
6. Report the delta in the PR's Testing section (e.g. `pkg/recipe: 90.4% → 90.3% (-0.1%)`).
Expand Down
2 changes: 1 addition & 1 deletion AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -582,7 +582,7 @@ Follow the heading conventions in the `## Documentation Style` section above. Do
Before pushing a PR that changes Go source, check coverage on affected packages. Set `pkg` to the narrowest changed root — `$pkg/...` includes descendants (e.g. use `pkg=pkg/collector/topology`, not `pkg=pkg/collector`, unless you want a combined delta).
1. Current: `GOFLAGS="-mod=vendor" go test -coverprofile=cover.out ./$pkg/...` on each changed package.
2. Baseline (skip for new packages; commit changes first): `(git worktree add $TMPDIR/baseline origin/main && (cd $TMPDIR/baseline && GOFLAGS="-mod=vendor" go test -coverprofile=$TMPDIR/base.out ./$pkg/...); rc=$?; git worktree remove --force $TMPDIR/baseline; return $rc 2>/dev/null || (exit $rc))` — `$TMPDIR/base.out` survives cleanup and `rc` preserves test status. Compare with `go tool cover -func`.
3. **Block** if `make test-coverage` fails (enforces the project-wide 80% floor from `.settings.yaml`; do not use per-package profiles for this check).
3. **Block** if `make test-coverage` fails (enforces the 80% floor from `.settings.yaml`, excluding `validators/` — see #1752; do not use per-package profiles for this check).
4. **Flag** any package with per-package decrease > 0.5% (step 1 vs 2).
5. **Block** if any new exported func/method (`git diff origin/main -- $pkg/`, added uppercase `func` lines) has 0% coverage — add tests first.
6. Report the delta in the PR's Testing section (e.g. `pkg/recipe: 90.4% → 90.3% (-0.1%)`).
Expand Down
8 changes: 6 additions & 2 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -220,13 +220,17 @@ license-check: ## Check license is approved
test-shell: ## Runs shell unit tests (tools/*_test.sh; hermetic, no cluster)
@set -e; for t in tools/*_test.sh; do [ -e "$$t" ] || continue; echo "Running $$t..."; bash "$$t"; done

# validators/ tests run as part of `make test` but are excluded from the
# coverage.out this target emits: per-package coverage there runs 41-92%
# (see #1752), which would pull the project-wide gate from ~80% to ~75.8%.
.PHONY: test
test: test-shell ## Runs unit tests with race detector and coverage (use -short to skip integration tests)
@set -e; \
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.full.out $$(go list ./... | grep -v -e /tests/chainsaw/) || exit 1; \

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 — New coverage.full.out isn't cleaned or ignored

make clean (line 650) removes ./coverage.out but not the newly-introduced coverage.full.out, and .gitignore covers neither.

Blast radius: A run leaves an untracked artifact in the working tree.

Fix: Add coverage.full.out to the clean target's rm list (and optionally to .gitignore).

grep -v '^github.com/NVIDIA/aicr/validators/' coverage.full.out > coverage.out; \
echo "Test coverage:"; \
go tool cover -func=coverage.out | tail -1
Comment thread
coderabbitai[bot] marked this conversation as resolved.

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.


Expand Down Expand Up @@ -656,7 +660,7 @@ changelog-file: ## Updates CHANGELOG.md with changes since the last release

.PHONY: clean
clean: ## Cleans build artifacts (dist, coverage files, third-party notices)
@rm -rf ./dist ./bin ./coverage.out ./THIRD_PARTY_NOTICES.md ./.licenses-cache
@rm -rf ./dist ./bin ./coverage.out ./coverage.full.out ./THIRD_PARTY_NOTICES.md ./.licenses-cache
@go clean ./...
@echo "Cleaned build artifacts"

Expand Down
4 changes: 2 additions & 2 deletions docs/contributor/tests.md
Original file line number Diff line number Diff line change
Expand Up @@ -91,8 +91,8 @@ command name — there is no Cobra-style `SetOut`/`SetArgs`/`Execute`.)
Direct `fmt.Println` / `fmt.Printf` to stdout in `pkg/cli` breaks
this pattern and is a review-blocker.

**Coverage floor: 80%** (project-wide, from `.settings.yaml`
`quality.coverage_threshold`). `make test-coverage` enforces it.
**Coverage floor: 80%** (from `.settings.yaml`
`quality.coverage_threshold`; excludes `validators/`, see #1752). `make test-coverage` enforces it.
Per-package decreases > 0.5% are flagged for justification.

## Test Isolation (`--no-cluster`)
Expand Down