Repository navigation
Gate on full config_db table coverage - #2571
Conversation
369711b to
34a693a
Compare
34a693a to
bdf2899
Compare
bdf2899 to
8793a02
Compare
8793a02 to
f0dc3b3
Compare
f0dc3b3 to
63b1f23
Compare
63b1f23 to
c154634
Compare
c154634 to
f06dba9
Compare
f06dba9 to
eca8762
Compare
eca8762 to
65b9ebc
Compare
65b9ebc to
b0b921e
Compare
b0b921e to
a849095
Compare
a849095 to
0676f37
Compare
0676f37 to
ba7c315
Compare
ba7c315 to
d94bba1
Compare
d94bba1 to
f5dc9be
Compare
f5dc9be to
f2a9d25
Compare
f2a9d25 to
ab57053
Compare
ab57053 to
5cef59a
Compare
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="tests/unit/e2e/test_golden_coverage.py" line_range="27-29" />
<code_context>
+ "the generator can emit these config_db tables, but they are empty in "
+ "every file under tests/e2e/golden/: " + ", ".join(missing) + ". Seed a "
+ "device that populates them under tests/e2e/scenario/resources/ and "
+ "rewrite the goldens with `make sonic-e2e-regen`, or -- if the table "
+ "cannot be reached by any fixture -- exclude it here with a comment "
+ "saying why."
+ )
</code_context>
<issue_to_address>
**issue (bug_risk):** The failure message instructs maintainers to exclude an unreachable table, but the test has no exclusion set or mechanism; adding a comment alone does not change `missing`, so the test remains permanently failing for any genuinely unreachable emitted table.
**Triggers:** When a table is detected by the static generator scan but cannot be populated by any available fixture.
**Suggested fix:** Add an explicit, reviewed exclusion set containing the table and enforce that every exclusion has a nearby reason, or remove the unsupported exclusion advice from the message.
```suggestion
"rewrite the goldens with `make sonic-e2e-regen`."
```
</issue_to_address>Sourcery assessment
Approval pending. 1 finding to address first.
Blocking findings: tests/unit/e2e/test_golden_coverage.py:29
| "rewrite the goldens with `make sonic-e2e-regen`, or -- if the table " | ||
| "cannot be reached by any fixture -- exclude it here with a comment " | ||
| "saying why." |
There was a problem hiding this comment.
issue (bug_risk): The failure message instructs maintainers to exclude an unreachable table, but the test has no exclusion set or mechanism; adding a comment alone does not change missing, so the test remains permanently failing for any genuinely unreachable emitted table.
Triggers: When a table is detected by the static generator scan but cannot be populated by any available fixture.
Suggested fix: Add an explicit, reviewed exclusion set containing the table and enforce that every exclusion has a nearby reason, or remove the unsupported exclusion advice from the message.
| "rewrite the goldens with `make sonic-e2e-regen`, or -- if the table " | |
| "cannot be reached by any fixture -- exclude it here with a comment " | |
| "saying why." | |
| "rewrite the goldens with `make sonic-e2e-regen`." |
tests/e2e/coverage.py reports which config_db tables the golden set reaches, but nothing consumes it: it is wired into neither sonic_golden_test.sh nor the Zuul job, so the number is only seen by someone who runs `make sonic-e2e-coverage` by hand. That is how the "38 of 38" claim came to exist as an unverifiable one-off in the first place, and leaving it human-run lets it decay the same way again. Add a unit test asserting the set of emitted-but-uncovered tables is empty. It closes the one coverage failure nothing else catches: a newly emitted table arriving with no golden. Coverage lost in the other direction -- a table that had a golden becoming empty -- already fails the golden comparison, because the golden file itself changes, and the regeneration path is covered separately by the coverage guard in compare.py. It belongs in the unit suite rather than the E2E job because it needs neither NetBox nor a generated config, only the generator source and the committed goldens. It therefore costs milliseconds and runs on every change, where the E2E job runs on a file matcher and a 2400s budget. This needs no .zuul.yaml change: coverage.py already exposes emitted_tables() and covered_tables(). The assertion message names the missing tables and points at `make sonic-e2e-regen`, because the cost of this gate is that adding a generator table now obliges the same change to add golden coverage, and that is a full regeneration cycle rather than a two-line edit. If a table genuinely cannot be reached by any fixture, the message says to subtract it from the missing set here with a stated reason -- one visible exception, not a silently growing allowlist. This lands after the last scenario because it can only pass once the golden set is complete; the report itself lands with the first goldens, where it is still useful at 30 of 38. Assisted-by: Claude:claude-opus-5 Signed-off-by: Roger Luethi <luethi@osism.tech>
5cef59a to
23edd8f
Compare
Part of the series tracked in #2562, which explains the ordering and what each PR covers. Based on the preceding PR in the stack, so review only the top commits here.
Thirty lines, and the reason the coverage number cannot quietly rot: a unit test
asserting that no table the generator can emit is left without a golden.
It closes the one coverage failure nothing else catches — a newly emitted
table arriving with no golden. Coverage lost the other way (a table that had a
golden becoming empty) already fails the golden comparison, because the golden
file itself changes.
It lives in the unit suite rather than the E2E job because it needs neither
NetBox nor a generated config, only the generator source and the committed
goldens. So it costs milliseconds and runs on every change, where the E2E job
runs on a file matcher and a 2400s budget. No
.zuul.yamlchange is needed.It lands last because it can only pass once the golden set is complete — which
is what the PR below achieves.