From 2317e49906d5bbe118003b96dcea6436e2f23659 Mon Sep 17 00:00:00 2001 From: thedavidmeister Date: Fri, 14 Aug 2026 17:03:10 +0000 Subject: [PATCH 1/2] test(env): hold both `DEPLOYMENT_SUITE` values in one test, not two MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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) --- test/src/abstract/RainDeployBroadcast.t.sol | 63 ++++++++++++++++----- 1 file changed, 49 insertions(+), 14 deletions(-) diff --git a/test/src/abstract/RainDeployBroadcast.t.sol b/test/src/abstract/RainDeployBroadcast.t.sol index 67656dc..a1c9424 100644 --- a/test/src/abstract/RainDeployBroadcast.t.sol +++ b/test/src/abstract/RainDeployBroadcast.t.sol @@ -26,33 +26,68 @@ contract RainDeployBroadcastTest is Test { sDeploy = new ExampleDeploy(); } - /// A mistyped suite MUST fail naming every valid suite, and MUST do so - /// before `DEPLOYMENT_KEY` is read. `DEPLOYMENT_KEY` is deliberately unset - /// here: if the key were read first, this would fail on the missing key and - /// send the reader after the wrong thing entirely. - function testRunUnknownSuiteRevertsBeforeReadingTheKey() external { - vm.setEnv("DEPLOYMENT_SUITE", "address-registry"); + /// The suite comes from `DEPLOYMENT_SUITE`, an unset one is no suite rather + /// than a default, and both answers are reached before `DEPLOYMENT_KEY` is + /// read. + /// + /// ONE test for both values, because `vm.setEnv` writes the forge PROCESS' + /// environment. It is scoped to neither a test nor a contract, nothing + /// 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, and this contract was exactly that until it was seen losing + /// the race: the unset case resolved the `address-registry` the other test + /// had written. Sequenced inside one test the two values cannot interleave, + /// and every write to that variable in this repo is now the one below, + /// after the only read that needs it absent. + /// + /// ## Unset first, and genuinely unset + /// + /// `vm.setEnv("DEPLOYMENT_SUITE", "")` would set a variable that is PRESENT + /// and empty, which never reaches `vm.envOr`'s default — so it would pass + /// just as happily if that default became a real suite key, and a deploy + /// that picks something when told nothing is how the wrong contract reaches + /// a chain. Only an absent variable exercises the default, and absent is a + /// state no cheatcode restores, so this half runs before anything sets it. + /// + /// That the reported key is empty rather than a suite is what says there is + /// no default; `RainDeploySuitesBaseTest.testEmptySuiteIsUnknown` is what + /// says an empty key is unknown. + /// + /// ## Then a suite nobody declared + /// + /// A mistyped suite MUST fail naming every valid suite. The reported key is + /// the value of THAT variable under THAT name, which is what a set value + /// distinct from the default proves and an unset one cannot. + /// + /// ## Both before the key + /// + /// `DEPLOYMENT_KEY` is deliberately unset. `vm.envUint` reverts on an unset + /// variable, so were the key read first, either half would fail on the + /// missing key — and a mistyped suite would send the deployer after the + /// wrong thing entirely. + function testRunSelectsTheSuiteFromTheEnvBeforeTheKeyAndNeverDefaults() external { + // Both absences are inputs, so they are asserted rather than assumed: a + // variable set outside this test reports itself by name here instead of + // as a surprising revert payload, or as an ordering this no longer + // discriminates. + assertFalse(vm.envExists("DEPLOYMENT_SUITE")); + assertFalse(vm.envExists("DEPLOYMENT_KEY")); vm.expectRevert( abi.encodeWithSelector( UnknownDeploymentSuite.selector, - "address-registry", + "", "address-registry-0-0-1, second-address, address-registry-candidate" ) ); sDeploy.run(); - } - /// An unset `DEPLOYMENT_SUITE` MUST be an unknown suite rather than a - /// default. A deploy that picks something when told nothing is how the - /// wrong contract reaches a chain. - function testRunUnsetSuiteReverts() external { - vm.setEnv("DEPLOYMENT_SUITE", ""); + vm.setEnv("DEPLOYMENT_SUITE", "address-registry"); vm.expectRevert( abi.encodeWithSelector( UnknownDeploymentSuite.selector, - "", + "address-registry", "address-registry-0-0-1, second-address, address-registry-candidate" ) ); From 229847d22d47c7f1249977ac7145dbf1c1e0eeba Mon Sep 17 00:00:00 2001 From: thedavidmeister Date: Fri, 14 Aug 2026 17:17:10 +0000 Subject: [PATCH 2/2] test(env): set `DEPLOYMENT_KEY` unreadable rather than assume it unset MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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) --- test/src/abstract/RainDeployBroadcast.t.sol | 33 ++++++++++++++------- 1 file changed, 22 insertions(+), 11 deletions(-) diff --git a/test/src/abstract/RainDeployBroadcast.t.sol b/test/src/abstract/RainDeployBroadcast.t.sol index a1c9424..e2f2594 100644 --- a/test/src/abstract/RainDeployBroadcast.t.sol +++ b/test/src/abstract/RainDeployBroadcast.t.sol @@ -37,8 +37,9 @@ contract RainDeployBroadcastTest is Test { /// variable, and this contract was exactly that until it was seen losing /// the race: the unset case resolved the `address-registry` the other test /// had written. Sequenced inside one test the two values cannot interleave, - /// and every write to that variable in this repo is now the one below, - /// after the only read that needs it absent. + /// and every `vm.setEnv` in this repo is now one of the two below — with + /// the write to `DEPLOYMENT_SUITE` after the only read that needs it + /// absent, so there is no write left for another test to observe. /// /// ## Unset first, and genuinely unset /// @@ -61,17 +62,27 @@ contract RainDeployBroadcastTest is Test { /// /// ## Both before the key /// - /// `DEPLOYMENT_KEY` is deliberately unset. `vm.envUint` reverts on an unset - /// variable, so were the key read first, either half would fail on the - /// missing key — and a mistyped suite would send the deployer after the - /// wrong thing entirely. + /// `DEPLOYMENT_KEY` is set to something `vm.envUint` cannot parse, and that + /// unreadability is what makes the ordering observable: a suite failure + /// while the key is garbage is the outcome that says the key had not been + /// read yet, and a mistyped suite that failed on the key instead would send + /// the deployer after the wrong thing entirely. + /// + /// Set rather than absent, because absence is not this test's to give. + /// `rainix-sol-test` exports `DEPLOYMENT_KEY` from the org's deploy secret + /// onto the job that runs this suite, so a test that assumed it unset + /// asserted the ordering only in a bare shell — and a key that PARSES makes + /// both orderings produce the same revert, so it would assert nothing about + /// ordering in the one place the suite actually runs. function testRunSelectsTheSuiteFromTheEnvBeforeTheKeyAndNeverDefaults() external { - // Both absences are inputs, so they are asserted rather than assumed: a - // variable set outside this test reports itself by name here instead of - // as a surprising revert payload, or as an ordering this no longer - // discriminates. + // `DEPLOYMENT_SUITE` has to be ABSENT and no cheatcode makes it so, so + // the precondition is asserted: a value set outside this test reports + // itself by name here rather than as a surprising revert payload. assertFalse(vm.envExists("DEPLOYMENT_SUITE")); - assertFalse(vm.envExists("DEPLOYMENT_KEY")); + // `DEPLOYMENT_KEY` is asserted about by nothing, because this test SETS + // it. It only has to be unparseable, and a value it writes is a value + // no CI job's secret can change. + vm.setEnv("DEPLOYMENT_KEY", "not a private key"); vm.expectRevert( abi.encodeWithSelector(