fix(ci): run validators/ unit tests in make test, exclude from covergae gate#1918
fix(ci): run validators/ unit tests in make test, exclude from covergae gate#1918mohityadav8 wants to merge 1 commit into
Conversation
…ge gate Makefile excluded /validators from 'go list ./...' in make test, so regression suites added in NVIDIA#1745/NVIDIA#1748 (and the older nvidia-smi, inference-perf, nccl, conformance tests) never ran in CI. - Drop -e /validators from the make test package filter. - Exclude validators/ from the test-coverage threshold calc (its package coverage is 41-92%, pulling aggregate from 80.0% to 75.7% and breaking the 80% gate) while still running its tests. Fixes NVIDIA#1752 Signed-off-by: Mohit Yadav <ymohit799057@gmail.com>
📝 WalkthroughWalkthroughThe Makefile now runs Go tests for validator packages by excluding only Estimated code review effort: 1 (Trivial) | ~5 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with 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.
Inline comments:
In `@Makefile`:
- Around line 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.
- 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.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: e3e0e1da-8358-4521-85c5-1944dfa606f0
📒 Files selected for processing (1)
Makefile
| echo "Test coverage:"; \ | ||
| go tool cover -func=coverage.out | tail -1 |
There was a problem hiding this comment.
🎯 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.
| .PHONY: test-coverage | ||
| test-coverage: test ## Runs tests and enforces coverage threshold (from .settings.yaml quality.coverage_threshold) | ||
| @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/%//'); \ |
There was a problem hiding this comment.
🎯 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.
1 participant
Makefile excluded /validators from 'go list ./...' in make test, so regression suites added in #1745/#1748 (and the older nvidia-smi, inference-perf, nccl, conformance tests) never ran in CI.
Fixes #1752