Fix the four tests whose premise the first real release expires - #143
Conversation
Split RainDeployVerifySnapshot so the fixture harness inherits the checks without the real-record binding, re-point the two freeze guards at fixture roots, and re-home the empty-matrix property against a declared-empty fixture. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughThe change adds a reusable snapshot verification base, narrows the repository consumer contract, updates chain and filesystem fixtures, and preserves the existing Slither filter value. ChangesSnapshot verification and validation
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to This test-only change separates fixture validation from real release-record validation and isolates freeze-test roots; the supplied clean-tree and post-release runs pass all 332 tests, so no actionable merge-blocking risk remains after normal checks. Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Unblocks the
sol-v0.1.6release re-run. The tag's Package Release runhttps://github.com/rainlanguage/rain.deploy/actions/runs/32175800185 failed at
forge testAFTERcutRelease()had already succeeded: nothing was publishedor committed back, so
mainstill saysversion = "0.1.5"andsrc/generated/still holds only
candidate/.All four failures are one class — tests that only pass while this repo has
never cut a release. The first real freeze expired their premise. None of
them is a defect in the release machinery: the real-record check
RegistryDeploySnapshotTest.testEveryFrozenSnapshotIsReleasedPASSEDpost-freeze in that same run (log
19:19:19.4325, gas 137925), so the frozenrecord and the generated declaration agreed and
cutRelease()did exactly whatit is supposed to.
Test-only in effect: no tag is touched, nothing is published, and neither the
foundry.tomlversion bump nor a frozensrc/generated/0_1_6/is committedhere. The one
src/change is the split in #1 below, which the fix requires.1.
RainDeployVerifySnapshotTest.testEveryFrozenSnapshotIsReleased—FrozenSnapshotNotReleased("src/generated/0_1_6/MigrationRegistry.sol")Mechanism.
RainDeployVerifySnapshotTestwasis ExampleDeploySuites, RainDeployVerifySnapshot. The inheritedtestEveryFrozenSnapshotIsReleasedwalksLibRainDeploySnapshot.LIB_FS_ROOT—this repo's REAL record — and checks it against
releasedSuites(), which theharness overrides with fixtures. Pre-release the walk was empty, so the test
passed vacuously. Post-freeze it found the real
0_1_6/files, and the exemplardeclares neither:
AddressRegistrymatched only BY ACCIDENT, because thefixture imports the real candidate
AddressRegistryconstants whose creationcode derives the frozen file's address.
MigrationRegistryis not in theexemplar set at all, so it reverted.
The defect is a fixture harness inheriting a real-record test, not the test.
Fix — the abstract is split, and the harness inherits the narrower half.
src/abstract/RainDeployVerifySnapshotBase.sol(new) holds the five errors,all three check functions and the two tests whose subject is the inheriting
contract's own declaration:
testSnapshotInternallyConsistentandtestSnapshotMatchesSource.src/abstract/RainDeployVerifySnapshot.solis now that base plustestEveryFrozenSnapshotIsReleased, and is unchanged in name, in what itoffers and in how a consumer inherits it. The README's consumer story —
contract MyDeploySnapshotTest is MyDeploySuites, RainDeployVerifySnapshot {}— is untouched, and so is
RegistryDeploySnapshotTest: a non-virtualwalkof the single
LIB_FS_ROOTspelling, against this repo's realreleasedSuites(). That binding is exactly as strong as it was, which the QAbelow proves in the state that broke CI rather than asserting.
test/src/abstract/RainDeployVerifySnapshot.t.sol→RainDeployVerifySnapshotBase.t.sol, contractRainDeployVerifySnapshotTest→
RainDeployVerifySnapshotBaseTest, inheriting the base. Every one of its 24cases is unchanged; the record CHECK is still exercised there at every
position and against every shape of declaration, because
checkFrozenSnapshotsReleasedtakes the record as an argument. Only theBINDING to the real record went. The rename follows the tree convention: the
file now tests the base, and
RainDeployVerifySnapshot.solhas no fixturemirror for the same reason
RainDeployVerifyBase.soldoes not.slither.config.jsongains the new file by exact filename, perCLAUDE.md.Why this shape.
The two alternatives were considered and both are worse:
virtualroot or avirtualtest to override. The NatSpec ontestEveryFrozenSnapshotIsReleasedexists to defend the single spelling ofthe root: pointed anywhere else, the walk returns empty forever, the test
passes with no subject, and the one check standing between a dropped release
and a green suite is inert. A hook is that failure mode with a supported
spelling. Inheriting a narrower contract is a choice a reader sees in the
inheritance list; an emptied override is one they do not.
done durably.
releasedSuites()isinternal pure— it cannot read therecord — so the only durable spelling is to concatenate the generated
LibReleasedSuites.releasedSuites()into the exemplar. That contradictsExampleDeploySuites's own stated design ("what the abstracts are testedagainst is written here in full, and adding or removing a contract in
src/concrete/does not change it"), and it is inherited byRainDeployVerifyChainTest, which would then fork seven real networksasserting this repo's real deployments from a fixture contract. A hand-listed
0_1_6entry rots at0.1.7.2 & 3.
LibRainDeploySnapshotTest.testFreezeRefusesAnEmptyReleaseandtestFreezeLeavesNothingBehindWhenThereIsNothingToFreeze— bothSnapshotAlreadyFrozen("0_1_6", "src/generated/0_1_6")Mechanism. Both drove
freezethroughexternalFreeze, which froze intothe REAL
LIB_FS_ROOTunder the realdeployTag(vm).SnapshotAlreadyFrozenis checked before either guard under test, so once
src/generated/0_1_6/exists it preempts them and the tests report a refusal that is not the one they
are about.
Fix. Both now go through
externalFreezeAtagainst a fixture root of theirown —
test/generated-freeze-emptyandtest/generated-freeze-nothing— whichis the precedent already set by
FREEZE_FIXTURE_ROOT,FREEZE_MULTI_FIXTURE_ROOT,RECUT_FIXTURE_ROOTand friends, and for the samedocumented reason: every freeze test cuts the SAME tag, so two sharing a root
would have whichever ran second refused as a re-cut. The real
deployTag(vm)read is kept — it is the tag a freeze is about, and it is the ROOT that expired,
not the tag.
The guards they pin are unchanged and still real:
EmptyRelease,NothingToFreeze, and the write ordering that keeps a failed release retryable.testFreezeLeavesNothingBehindWhenThereIsNothingToFreezeis slightly strongerthan it was: its fixture root holds a rolling snapshot for another contract, so
the freeze reaches its LAST guard with only the named contract's file missing,
rather than failing at a root with nothing under it at all.
externalFreeze— the real-root wrapper — had no callers left and is deleted.No test freezes into
src/generated/any more, which is the point: a test thatcan only be pointed at the real record can only be run against a record it must
not leave a release in.
4.
RegistryDeployChainTest.testChainWithNothingToCheckForksNothing—2 != 0Mechanism. It asserted
releasedSuites().length == 0as its premise. Itsown NatSpec said the first release makes it fail and that failing is the check
working.
Fix — deleted, per the ruling that there is no point testing that something
has not been released.
RegistryDeployChainTestis now{}, likeRegistryDeploySnapshotTest, and its NatSpec no longer rests on "it hasreleased none".
The property it pinned is re-homed, not dropped. "The matrix forks nothing
when there is nothing to check" was pinned NOWHERE against a fixture — checked
before deleting.
RainDeployVerifyChainTest.testChainWithASingleSubjectDoesForkpins only the positive side (one subject → the matrix forks), and its own
NatSpec named
RegistryDeployChainTest's empty case as the other half of thatargument. So it now lives in
test/src/abstract/RainDeployVerifyChainEmpty.t.sol:RainDeployVerifyChainEmptyTestdeclares an empty released set and onecandidate, and runs the whole inherited entry point — so the derivation is
inside what is asserted — checking that no fork was selected. Its own contract
for the reason
RainDeployVerifyChainCandidateTestis its own contract: thesuites a contract inherits are the whole of what the matrix runs over, so a
second scope is a second contract. The cross-reference in
RainDeployVerifyChain.t.solnow points at it.Declared empty rather than observed empty is the whole difference: a deploy
repo's declaration is empty exactly once, before its first release, so a test
resting on that stops asserting anything the day the repo does the thing it
exists to do — which is precisely what happened here.
Not changed
script/Build.sol,src/abstract/BuildScript.solandLibRainDeploySnapshot.freezeare untouched.cutRelease()worked as designed.QA
LibRainDeploySnapshotTest.testFreezeRefusesAnEmptyRelease,LibRainDeploySnapshotTest.testFreezeLeavesNothingBehindWhenThereIsNothingToFreeze,testEveryFrozenSnapshotIsReleasedas bound by the fixture harness, andRainDeployVerifyChainEmptyTest.testChainWithNothingToCheckForksNothing(re-homing the deletedRegistryDeployChainTestcase) - each fails on base in the 0.1.6 release state, verified by the four logged failures of tag run 32175800185; Run 2 below is that same state on this branch, green.RainDeployVerifyChain.checkDeployedOnSupportedNetworksif (derived.length == 0) return;-> deleted -> killed byRainDeployVerifyChainEmptyTest.testChainWithNothingToCheckForksNothing; M2freezeEmptyReleaseguard -> deleted -> killed bytestFreezeRefusesAnEmptyRelease; M3freeze-> create<tag>/before the read loop -> killed bytestFreezeLeavesNothingBehindWhenThereIsNothingToFreeze; M4 (release state)LibMigrationRegistryReleased.releasedSuites()-> blanked tonew DeploySuite[](0)-> killed byRegistryDeploySnapshotTest.testEveryFrozenSnapshotIsReleasedwith the exact CI error. Details in the mutation pass below.FrozenSnapshotNotReleased("src/generated/0_1_6/MigrationRegistry.sol")) plus the guards' NatSpec-stated semantics (EmptyRelease,NothingToFreeze, nothing-left-behind ordering) - expected outcomes derived from the release lifecycle, independent of the code under test.Both acceptance runs are below, in both tree states, plus a mutation pass on
every test this PR moves or adds.
nix develop -cthroughout.Run 1 — clean tree, what
mainCI runsnix develop -c forge testRun 2 — release state, what the tag run hits
foundry.toml[external.package].version→0.1.6, thennix develop -c bash -c 'forge script ./script/Build.sol --sig "cutRelease()" && forge fmt',then
nix develop -c forge test. This is the exact state that produced the fourfailures.
First attempt:
327 passed, 5 failed— all five failures werevm.createSelectForktransport errors from the public HYPEREVM RPC that.env.examplepoints at (invalid block height/block not found), all inthe two live-fork chain suites and none in a test this PR touches; CI has a
dedicated archive-RPC secret for exactly this. A rerun of those five passed,
and a full re-run of the whole suite in the same release state is green:
RegistryDeploySnapshotTest.testEveryFrozenSnapshotIsReleasedin this state hasa real subject — the two files
cutRelease()froze — rather than the empty walkit gets on a clean tree.
Reverted afterwards: the version bump,
src/generated/0_1_6/and everyregenerated lib.
git statusis clean except the changes in this PR.Mutation pass
Each mutant is applied to the unmutated baseline committed on this branch, run,
and reverted. Every one is killed by the test it is aimed at.
if (derived.length == 0) return;inRainDeployVerifyChain.checkDeployedOnSupportedNetworks→RainDeployVerifyChainEmptyTest.testChainWithNothingToCheckForksNothingFAILs ("the matrix forked a network with nothing to check"). The re-homed
property is load-bearing.
EmptyReleaseguard infreeze→testFreezeRefusesAnEmptyReleaseFAILs ("next call did not revert asexpected").
<tag>/before the read loop infreeze→testFreezeLeavesNothingBehindWhenThereIsNothingToFreezeFAILs ("assertionfailed").
LibMigrationRegistryReleased.releasedSuites()back tonew DeploySuite[](0)and run--match-contract "RegistryDeploySnapshotTest|RainDeployVerifySnapshotBaseTest"→
RegistryDeploySnapshotTest.testEveryFrozenSnapshotIsReleasedFAILs withFrozenSnapshotNotReleased("src/generated/0_1_6/MigrationRegistry.sol")—the exact error from the CI run, now raised by the contract that should
raise it — while
RainDeployVerifySnapshotBaseTeststays green (24 passed).27 tests ran: 26 passed, 1 failed. The split left the real-record binding
exactly as strong as it was.
Static gate
nix develop -c pre-commit run --all-filesEvery hook passes on this branch's files. One finding, and it is not from this
PR:
denofmtrewraps one paragraph ofCLAUDE.mdthat is byte-identical tomain— pre-existing drift on a file this branch does not touch. The rewrapwas reverted, not committed; CI's actual static job (
rainix-sol-static:slither,
forge fmt --checkand the org convention checks) does not rundenofmt and is green on the same content on
main.Summary by CodeRabbit
New Features
Bug Fixes
Documentation