test(env): hold both DEPLOYMENT_SUITE values in one test, not two - #37
Conversation
`vm.setEnv` writes the forge process' environment: scoped to neither a
test nor a contract, restored by nothing, and read by tests forge runs
concurrently. Two tests each writing that variable is two tests racing
over it, and the unset case was seen resolving the `address-registry`
the other test had written.
Sequencing both values inside a single test is what removes the sharing
— it is the only ordering forge guarantees, and there is now exactly one
write to that variable in the repo.
Unset is now genuinely unset rather than `setEnv("", ...)`, which set a
variable that was present and empty and so never reached `vm.envOr`'s
default. Both absences are asserted, because both are inputs: an unset
`DEPLOYMENT_SUITE` is what makes the empty key report a missing default
rather than an empty value, and an unset `DEPLOYMENT_KEY` is what makes
the revert say the suite was resolved first.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 93 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
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 (1)
WalkthroughThe change replaces two deployment suite validation tests with one sequenced test. The test checks unset and invalid ChangesDeployment suite validation
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This PR sequences shared deployment-suite environment values within one test and strengthens coverage of unset, variable-name, and evaluation-order behavior; no actionable merge-blocking risk remains beyond normal checks and review. 🚥 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 |
`rainix-sol-test` exports `DEPLOYMENT_KEY: ${{ secrets.PRIVATE_KEY }}`
onto the job that runs this suite, so the variable is PRESENT everywhere
this test actually runs and `assertFalse(vm.envExists("DEPLOYMENT_KEY"))`
was red in CI and green in a bare shell.
Absence was the wrong input to reach for anyway. The ordering claim is
"the suite was resolved before the key was read", and a key that PARSES
makes both orderings produce the same revert — so under CI's own key the
assertion would have discriminated nothing even had it passed. The test
now SETS the key to something `vm.envUint` cannot parse, which is an
input it controls and which makes the ordering observable in every
environment.
`DEPLOYMENT_SUITE` still has to be genuinely absent — that is what keeps
`vm.envOr`'s default under test rather than restating an empty string —
and nothing in CI sets it, so that assertion stays.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`test/src/abstract/RainDeployBroadcast.t.sol` is the only conflict, and both sides changed the same two assertions for unrelated reasons. Main (#37) folded `testRunUnknownSuiteRevertsBeforeReadingTheKey` and `testRunUnsetSuiteReverts` into one test, because `vm.setEnv` writes the forge PROCESS' environment and two concurrent tests holding two values for `DEPLOYMENT_SUITE` were racing over one variable. The unset half has to run before anything sets the variable, and asserts `vm.envExists` is false as its precondition. This branch added a second candidate, so the suite keys a mistyped `DEPLOYMENT_SUITE` reports grew a `second-address-candidate`. Both survive: main's single sequenced test, with the four-key list in BOTH expected reverts. The branch's `testRunUnsetSuiteReverts` is dropped rather than carried — it is the very `vm.setEnv("DEPLOYMENT_SUITE", "")` main removed, and keeping it would both restore the race and falsify the `envExists` precondition of the test that replaced it. Auto-merge had already taken main's structure while updating only the SECOND key list, leaving the first assertion naming three suites; that is corrected here, not inherited. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Closes #36.
vm.setEnvwrites the forge PROCESS' environment. It is scoped to neither atest nor a contract, no cheatcode unsets or restores it, and forge runs tests
concurrently — so two tests holding two values for one variable is two tests
racing over one variable.
RainDeployBroadcastTestwas exactly that, and wasseen losing the race: the unset case resolved the
address-registrythe othertest had written.
The fix
The two values are sequenced inside ONE test, which is the only ordering forge
guarantees, and every
vm.setEnvin the repo is now in that one test — theDEPLOYMENT_SUITEwrite after the only read that needs it absent, so there is nowrite left for another test to observe. Nothing else in the repo reads
DEPLOYMENT_SUITEorDEPLOYMENT_KEY: the only reader isrun(), and the onlycaller of
run()is this test.There is no smaller fix. Foundry offers no scoped or restorable env:
setEnvisstd::env::set_varon the process, andunsetEnvdoes not exist. Making thevalue injectable instead would remove the only assertion that the value comes
from a variable of THAT name (killed as M2 below), and serialising the suite with
--threads 1would trade the whole suite's parallelism to leave the sharing inplace.
The two variables are not the same problem
DEPLOYMENT_SUITEhas to be genuinely ABSENT, and that is the assertion doingthe work.
vm.setEnv("DEPLOYMENT_SUITE", "")set a variable that was PRESENT andempty, so
vm.envOr's default was never reached and a default that silentlybecame a real suite key passed — the exact hazard that test's own comment names,
"a deploy that picks something when told nothing". Nothing in CI sets that
variable (checked: neither
rainix-sol-testnor this repo's caller workflow), soassertFalse(vm.envExists("DEPLOYMENT_SUITE"))holds and states the preconditionby name.
DEPLOYMENT_KEYis the opposite: absence is not this test's to give.rainix-sol-testsetsDEPLOYMENT_KEY: ${{ secrets.PRIVATE_KEY }}on the jobthat runs this suite, so it is present everywhere the test really runs. So the
test SETS it, to something
vm.envUintcannot parse. That is a stronger oraclethan absence, not a weaker one: the ordering claim is "the suite was resolved
before the key was read", and a key that PARSES makes both orderings produce the
same revert — so under CI's own key, asserting absence would have discriminated
nothing about ordering even if it had passed. Unreadable-by-construction makes
M3 die in every environment.
Mutation matrix
forge test --match-path test/src/abstract/RainDeployBroadcast.t.sol --match-test testRunSelectsTheSuiteFromTheEnvBeforeTheKeyAndNeverDefaults,run under each ambient
DEPLOYMENT_KEYstate the suite meets — unset (bareshell), set to a valid key (CI with the secret populated), set to empty (CI with
the secret absent, which still exports the variable):
src/abstract/RainDeployBroadcast.solenvOrdefault""→"address-registry-0-0-1""DEPLOYMENT_SUITE"→"DEPLOYMENT_SUIT"envUint("DEPLOYMENT_KEY")moved abovesuiteByName(...)M1 and M3 fail with
Error != expected error: vm.envUint: failed parsing $DEPLOYMENT_KEY as type uint256: parser error != UnknownDeploymentSuite("", ...);M2 with
UnknownDeploymentSuite("", ...) != UnknownDeploymentSuite("address-registry", ...).Two more results from the same matrix:
testRunUnknownSuiteRevertsBeforeReadingTheKeyand
testRunUnsetSuiteRevertsboth PASS. A deploy script that picks a realsuite when told nothing survived the old suite; it is killed here.
DEPLOYMENT_KEY, reproducesthe CI red exactly —
[FAIL: assertion failed] ... (gas: 3980), the same gasfigure as the failing job
— and passes in a bare shell. That is the whole of the CI failure, confirmed
rather than assumed: the
-vvvtrace shows bothVM::envExistscalls followedby one
VM::assertFalse(true), i.e. theDEPLOYMENT_KEYassertion.forge fmt --checkpasses.forge test --no-match-contract Chainon this branch,run with
DEPLOYMENT_KEYset as CI sets it, is 92 passed / 38 failed where all 38are
vm.createSelectFork: environment variable *_RPC_URL not foundinLibRainDeployTest— what #32 is about and #34 fixes. This branch changes onefile and it is not that one.
QA
testRunSelectsTheSuiteFromTheEnvBeforeTheKeyAndNeverDefaults— passes unmutated (M0) and fails under each of M1, M2 and M3, in all three
ambient
DEPLOYMENT_KEYstates (unset, valid, empty): 12 runs, one per cell ofthe matrix above. Each is its own
forge test --match-path test/src/abstract/RainDeployBroadcast.t.sol --match-test <name>with the[PASS]/[FAIL: ...]line captured, so everycell is a run that actually selected and executed the test rather than a filter
matching nothing. The two tests this PR does not touch —
testDeployNetworksDefaultsToSupportedNetworks,testSelectedSuiteCarriesTheRecordedPins— are unchanged.
src/abstract/RainDeployBroadcast.sol, the codeunder test; killer is the test above in every case, in every environment):
vm.envOr("DEPLOYMENT_SUITE", string(""))→string("address-registry-0-0-1")→ KILLED:
vm.envUint: failed parsing $DEPLOYMENT_KEY as type uint256 != UnknownDeploymentSuite("", ...).This mutant SURVIVES the pre-PR tests —
testRunUnknownSuiteRevertsBeforeReadingTheKeyand
testRunUnsetSuiteRevertsboth PASS under it. That gap is what the newtest closes.
"DEPLOYMENT_SUITE"→"DEPLOYMENT_SUIT"→ KILLED:UnknownDeploymentSuite("", ...) != UnknownDeploymentSuite("address-registry", ...).uint256 deployerPrivateKey = vm.envUint("DEPLOYMENT_KEY");moved abovethe
suiteByName(...)line → KILLED:vm.envUint: failed parsing $DEPLOYMENT_KEY as type uint256 != UnknownDeploymentSuite("", ...).Killed under a VALID ambient key too, which is the case that matters: this
mutant is invisible to any test that lets a parseable key reach
envUint,because then both orderings revert identically.
Manual sol artifactshands the script a suite key asDEPLOYMENT_SUITEand keycustody as
DEPLOYMENT_KEY, so the expected behaviour is fixed independently ofthe code: the key the caller passed is the key reported back, passing no suite
selects no suite, and neither answer requires a usable private key. The
valid-suite list in the expected payload is the fixture's own declaration
(
ExampleDeploySuites), asserted separately byRainDeploySuitesBaseTest.testSuiteNamesIsTheRegistry.DEPLOYMENT_SUITE, so one reads the other's value #36 asks for the sharedvm.setEnvstate removed withouteither assertion getting weaker, and names the three behaviours the result must
still discriminate — the
envOrdefault becoming a suite key, the variable namechanging, and the key being read before the suite. Covered by M1, M2 and M3
respectively, plus the sharing itself: every
vm.setEnvin the repo is in thisone test, with the
DEPLOYMENT_SUITEwrite after the only read that needs itabsent.