docs: normalize governance test snapshot layout and add drift check - #647
Open
Rafiat30 wants to merge 2 commits into
Open
docs: normalize governance test snapshot layout and add drift check#647Rafiat30 wants to merge 2 commits into
Rafiat30 wants to merge 2 commits into
Conversation
anchor_removal_tests.rs wrapped its tests in a redundant `mod tests { }`
block even though it's already declared as its own top-level module
(`mod anchor_removal_tests;` in lib.rs), unlike its sibling test files
(anchor_auth_tests.rs, anchor_event_tests.rs,
anchor_no_event_error_tests.rs). That produced a convoluted nested
snapshot path (test_snapshots/anchor_removal_tests/tests/*.json) instead
of the flat one every sibling module uses
(test_snapshots/anchor_removal_tests/*.json), and CONTRIBUTING.md only
documented the single flat-`tests/` case, not the per-file-module or
nested-submodule shapes that actually exist in the workspace.
- Remove the redundant `mod tests` wrapper from anchor_removal_tests.rs
and align its imports with its sibling test files.
- Regenerate the governance snapshot files at the corrected, flat path.
- Rewrite CONTRIBUTING.md's "Test snapshots" section to document all
three snapshot-path shapes that occur in this workspace and when each
applies.
- Add scripts/check_test_snapshots.sh, which re-runs the workspace test
suite and fails if that leaves test_snapshots/ dirty, and wire it into
`make all` as a new `check_snapshots` target so drift is caught in CI.
- Fix a pre-existing duplicate-target bug in the Makefile where a second
`all:` recipe silently overrode the first, dropping `test_scripts` and
`wasm_size` from `make all` despite CONTRIBUTING.md documenting both
as part of it.
Closes Betta-Pay#533
|
@Rafiat30 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
…napshots # Conflicts: # Makefile
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
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.
Closes #533
Summary
Governance's
test_snapshotsdirectory had a convoluted, inconsistent layout:anchor_removal_tests.rswrapped its two tests in a redundantmod tests { }block even though it's declared as its own top-level module (mod anchor_removal_tests;inlib.rs) — unlike its sibling test files (anchor_auth_tests.rs,anchor_event_tests.rs,anchor_no_event_error_tests.rs), none of which have that extra wrapper. Soroban'sEnvtest harness names each snapshot file after the test's full module path, so that stray wrapper produced a nested path (test_snapshots/anchor_removal_tests/tests/*.json) instead of the flat one every sibling module uses (test_snapshots/anchor_removal_tests/*.json).CONTRIBUTING.mdalso only documented the single flattest_snapshots/tests/case, which doesn't match the two other legitimate shapes that already exist in this workspace (per-file top-level modules ingovernance_contract, and the nestedtests::<submodule>shape fromsettlement_contract'ssrc/tests/directory) — so the docs actively pointed contributors at the wrong path.Changes
Modified files
governance_contract/src/anchor_removal_tests.rs— removed the redundant innermod tests { }wrapper (functions are now direct members of theanchor_removal_testsmodule, matching every sibling test file); switcheduse crate::*;touse super::*;to match sibling import style.CONTRIBUTING.md— rewrote the "Test snapshots" section to document all three snapshot-path shapes that occur in this workspace (flat roottestsmodule, per-file top-level test modules, and nestedtests::<submodule>groups) and when each applies; updated the "CI parity" section to mention the new snapshot drift check.Makefile— added acheck_snapshotstarget wired intomake all. Also fixed a pre-existing duplicate-target bug: a secondfmt/allrecipe block later in the file was silently overriding the first (Make uses the last definition), which meantmake allwas actually only runningfmt check clippy test—test_scriptsandwasm_sizehad been silently dropped despite CONTRIBUTING.md documenting both as part of it.New files
scripts/check_test_snapshots.sh— re-runscargo test --workspace(which is how Soroban's test harness (re)writes snapshot files) and then fails with a diff summary if that leaves anything under*/test_snapshots/modified, added, or removed relative to the committed state. Follows the existing pattern inscripts/check_wasm_size.sh(sourcesscripts/lib/common.shfor logging helpers).Test files
governance_contract/test_snapshots/anchor_removal_tests/anchor_removal_clears_entry.1.jsonand.../rejects_removing_unregistered_anchor.1.json— regenerated at the corrected, flat path (git recognizes these as renames from the old nested.../tests/location).No production contract logic changed — this is a test-infrastructure and documentation normalization only.
Implementation details
#[test]function; there's no snapshot-naming config to change. Fixing the layout meant fixing the module structure (removing the straymod testswrapper) and letting the test harness regenerate the file at the now-correct path.check_test_snapshots.shintentionally re-runs the full suite rather than trying to infer staleness some other way — that's the same mechanism that produces the files in the first place, so it's the only reliable way to detect drift.Tests added
No new contract test cases were needed (this issue is about test plumbing, not contract behavior) — the two existing tests in
anchor_removal_tests.rsare unchanged in behavior, only in module structure. Coverage added:scripts/check_test_snapshots.shitself is the "test" for this issue: it fails the build iftest_snapshots/ever drifts from committed state again (e.g. if someone reintroduces a straymod testswrapper, or forgets to commit a regenerated snapshot after an intentional behavior change).How to test