Add the SONiC golden-test harness - #2564
Merged
Merged
Conversation
9 tasks
Add the Python side of the SONiC config-generation E2E golden test: tests/e2e/generate.py and tests/e2e/compare.py. generate.py drives sync_sonic() against a live NetBox and asserts success itself, because sync_sonic() returns only a device -> config dict and swallows per-device failures internally (it logs and moves on so one bad device does not abort the whole sync). To surface those failures here, generate.py installs a loguru sink that fails the run on any ERROR record, then exports the resulting config_db.json files for comparison. compare.py checks the exported files against tests/e2e/golden/: exact file-set equality (nothing missing, nothing extra) plus a structural diff of each file's JSON content, so a mismatch reports the offending keys/paths rather than an opaque "files differ". tests/unit/e2e/test_generate.py and test_compare.py cover both modules without needing a live NetBox. Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Roger Luethi <luethi@osism.tech>
berendt
force-pushed
the
sonic-e2e-v2-scripts
branch
from
August 5, 2026 15:10
572bb79 to
9128f47
Compare
ideaship
marked this pull request as ready for review
August 5, 2026 15:49
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
allow_coverage_lossparameter onregenerate()is currently unused; either wire it into the behavior or remove it from the signature to avoid confusion about its effect.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `allow_coverage_loss` parameter on `regenerate()` is currently unused; either wire it into the behavior or remove it from the signature to avoid confusion about its effect.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Regeneration is the only path by which coverage can silently drop: in the normal comparison path an emptied table (or a disappearing device) changes the golden file and fails the comparison, but inside a several-hundred-line regenerated JSON diff either kind of loss is invisible. regenerate() now returns a list of entries describing coverage that was lost. There are two shapes: a "<file>: <TABLE>" entry for a table that was populated in the previous golden and became empty in the export, and a "<file>: file removed, had N populated tables" entry for a golden file whose device stopped being exported altogether and is being removed by the existing stale-file cleanup. The latter is the largest-granularity loss there is, and the most likely one in this project: a SONiC device is only generated when it is active, carries the managed-by-metalbox tag, and its role is in DEFAULT_SONIC_ROLES, so a single fixture typo silently drops a device's entire golden with no error. A stale golden that had no populated tables to begin with is not a loss and is still removed silently. main() prints this report and exits non-zero when the list is non-empty, unless the new --allow-coverage-loss flag is passed, making an intentional removal explicit in the command someone ran. Goldens are still written (or removed) either way; the guard only changes whether the run is reported as a failure. A count of lost tables/files was considered and rejected as the wrong shape: losing one table while gaining another would leave a count unchanged. The flag is read only in main(): regenerate() writes the goldens and reports what was lost, and whether that counts as failure is a CLI policy decision rather than part of its contract. The report header no longer claims every loss was "populated and are now empty" -- a removed golden file is also reported here and that wording did not fit it. Unit tests cover main()'s --regenerate branch itself (report text and exit code) both with and without --allow-coverage-loss; the existing tests all exercised regenerate() directly and never went through the CLI. Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Roger Luethi <luethi@osism.tech>
ideaship
force-pushed
the
sonic-e2e-v2-scripts
branch
from
August 5, 2026 19:51
9128f47 to
6e2542a
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.
The comparison and generation logic, as plain Python with no infrastructure
attached —
compare.py(diff an export against the goldens, or rewrite them)and
generate.py(drivesync_sonic()and capture ERROR-level records).Reviewable in isolation: 33 unit tests cover both modules and pass with no
compose file, no fixtures and no goldens in the tree. Nothing here can run the
end-to-end test yet — that arrives two PRs later.
The second commit adds the regeneration coverage guard, which is the
non-obvious part. Regeneration is the only path by which coverage can silently
drop: in a normal comparison an emptied table changes the golden and fails the
diff, but inside a several-hundred-line regenerated JSON diff the same loss is
invisible.
--regeneratetherefore reports lost coverage and exits non-zerounless
--allow-coverage-lossis passed.