Summary
tests/e2e/run.sh contains one duplicated assertion and four tests that can never fail. Both patterns inflate the reported test count without adding coverage, and the always-passing tests are the more serious of the two: they read as guarantees but assert nothing.
Found while reviewing the file for duplication during #2357. All of these predate that PR — filing separately rather than expanding its scope.
1. Duplicate assertion in test_snapshot
tests/e2e/run.sh runs the identical command twice under two different test names, with nothing in between:
if kubectl get cm "$SNAPSHOT_CM" -n "$SNAPSHOT_NAMESPACE" > /dev/null 2>&1; then
pass "snapshot/agent"
else
...
fail "snapshot/agent" "Snapshot ConfigMap not created"
return 1
fi
# Verify ConfigMap was created
msg "--- Test: Snapshot ConfigMap ---"
if kubectl get cm "$SNAPSHOT_CM" -n "$SNAPSHOT_NAMESPACE" > /dev/null 2>&1; then
pass "snapshot/configmap-created"
The first branch return 1s on failure, so the second check is unreachable in the only case where it could fail. Two tests reported, one condition verified.
Suggested fix: delete snapshot/configmap-created.
2. Four tests that pass on every branch
Each of these calls pass in both the success and failure branch, so it cannot fail:
| Test |
Behavior |
snapshot/gpu-data |
pass, or warn + pass |
validate/job-cleanup |
pass, or warn + pass |
validate/job-custom-namespace |
pass, or warn + pass |
validate/expected-resources-fail |
two pass sites |
validate/job-cleanup is the one worth attention. It reads as coverage for validator Job cleanup, but its condition is jobs_after -le jobs_before — satisfied whenever cleanup does nothing at all — and the else branch passes anyway with a warning. There is currently no failing input for it.
Suggested fix: make each assert a real condition, or call skip when the precondition genuinely isn't met. skip does not increment the pass count, so a legitimately unavailable environment stops being reported as a passing test.
Why this matters
The suite currently reports these as passes in CI. Someone reading a green run reasonably concludes validator Job cleanup is verified end-to-end; it isn't. That gap was only visible because #2357 added a real cleanup assertion next to it (snapshot/isolation/self-cleanup, which asserts an exact object count) and the contrast made the older test's vacuity obvious.
Out of scope / already handled
The obsolete setup/rbac fixture — a pre-created ServiceAccount named aicr bound to an aicr-e2e-reader ClusterRole — was removed in #2357, since per ADR-020 the agent now creates and deletes its own run-scoped RBAC and that fixture had become both unused and a trigger for the agent's adoption-drift warning.
Summary
tests/e2e/run.shcontains one duplicated assertion and four tests that can never fail. Both patterns inflate the reported test count without adding coverage, and the always-passing tests are the more serious of the two: they read as guarantees but assert nothing.Found while reviewing the file for duplication during #2357. All of these predate that PR — filing separately rather than expanding its scope.
1. Duplicate assertion in
test_snapshottests/e2e/run.shruns the identical command twice under two different test names, with nothing in between:The first branch
return 1s on failure, so the second check is unreachable in the only case where it could fail. Two tests reported, one condition verified.Suggested fix: delete
snapshot/configmap-created.2. Four tests that pass on every branch
Each of these calls
passin both the success and failure branch, so it cannot fail:snapshot/gpu-datawarn+ passvalidate/job-cleanupwarn+ passvalidate/job-custom-namespacewarn+ passvalidate/expected-resources-failpasssitesvalidate/job-cleanupis the one worth attention. It reads as coverage for validator Job cleanup, but its condition isjobs_after -le jobs_before— satisfied whenever cleanup does nothing at all — and the else branch passes anyway with a warning. There is currently no failing input for it.Suggested fix: make each assert a real condition, or call
skipwhen the precondition genuinely isn't met.skipdoes not increment the pass count, so a legitimately unavailable environment stops being reported as a passing test.Why this matters
The suite currently reports these as passes in CI. Someone reading a green run reasonably concludes validator Job cleanup is verified end-to-end; it isn't. That gap was only visible because #2357 added a real cleanup assertion next to it (
snapshot/isolation/self-cleanup, which asserts an exact object count) and the contrast made the older test's vacuity obvious.Out of scope / already handled
The obsolete
setup/rbacfixture — a pre-created ServiceAccount namedaicrbound to anaicr-e2e-readerClusterRole — was removed in #2357, since per ADR-020 the agent now creates and deletes its own run-scoped RBAC and that fixture had become both unused and a trigger for the agent's adoption-drift warning.