From 62fb2793efbb1f35e1b584f418ed28a8ac6c6aa4 Mon Sep 17 00:00:00 2001 From: umermjd11 Date: Mon, 2 Nov 2026 21:40:45 +0500 Subject: [PATCH] fix(foundry-test): stop DeployPlatform/UpgradePlatform tests racing on DEPLOYMENTS_NETWORK PR No. 254 added two tests in different contracts that vm.setEnv the process-wide DEPLOYMENTS_NETWORK to different values. forge runs test contracts in parallel, so the Upgrade test could switch it between the Deploy test's setEnv and its write, and the Deploy test then read a file that was never written. develop went red on cf80216 and PR No. 304 was dropped from the merge queue by the same failure. - DeployPlatform: _writeDeployments now delegates to a new _writeDeploymentsTo(d, deployer, outPath); the real script path still resolves the file through DeploymentsPath, so its output is unchanged. - Both harnesses expose _writeDeploymentsTo, and both tests pass their own file path explicitly. No test sets DEPLOYMENTS_NETWORK or GIT_COMMIT_HASH any more (the upgrade test already passed the commit as an argument), which also takes DeploymentsPathTest's env read out of the race. Co-Authored-By: Claude Opus 5.5 --- foundry/script/DeployPlatform.s.sol | 10 +++++++- foundry/test/DeployPlatformScript.t.sol | 29 ++++++++++------------ foundry/test/UpgradePlatformScript.t.sol | 31 +++++++++--------------- 3 files changed, 33 insertions(+), 37 deletions(-) diff --git a/foundry/script/DeployPlatform.s.sol b/foundry/script/DeployPlatform.s.sol index a2cbbef..20163b4 100644 --- a/foundry/script/DeployPlatform.s.sol +++ b/foundry/script/DeployPlatform.s.sol @@ -406,7 +406,16 @@ contract DeployPlatform is DeploymentsPath { /// extcodehashes (for `dincli system verify-deployment` check 1 and 3), /// the deployer (ProxyAdmin owner, check 4), and the optional git commit /// hash recorded via GIT_COMMIT_HASH env var (check meta). + /// Writes to this chain's foundry/deployments/.json (DeploymentsPath). function _writeDeployments(Deployment memory d, address deployer) internal { + _writeDeploymentsTo(d, deployer, _deploymentsFile("")); + } + + /// @dev _writeDeployments with an explicit output path. Tests call this with + /// their own file instead of setting DEPLOYMENTS_NETWORK: vm.setEnv + /// changes the environment shared by forge's parallel test contracts, + /// so two tests setting it to different values race (PR No. 254). + function _writeDeploymentsTo(Deployment memory d, address deployer, string memory outPath) internal { // Resolve implementations via EIP-1967 slot reads. address implTreasury = Upgrades.getImplementationAddress(d.dinTreasury); address implToken = Upgrades.getImplementationAddress(d.dinToken); @@ -461,7 +470,6 @@ contract DeployPlatform is DeploymentsPath { string memory outDir = string.concat(vm.projectRoot(), "/deployments"); vm.createDir(outDir, true); - string memory outPath = _deploymentsFile(""); vm.writeJson(finalJson, outPath); console.log("Deployments written to:", outPath); } diff --git a/foundry/test/DeployPlatformScript.t.sol b/foundry/test/DeployPlatformScript.t.sol index ce8aac9..56cc218 100644 --- a/foundry/test/DeployPlatformScript.t.sol +++ b/foundry/test/DeployPlatformScript.t.sol @@ -8,10 +8,10 @@ import {OwnableUpgradeable} from "@openzeppelin/contracts-upgradeable/access/Own import {DeployPlatform} from "../script/DeployPlatform.s.sol"; -/// @dev Exposes the internal _writeDeployments for white-box testing. +/// @dev Exposes the internal _writeDeploymentsTo for white-box testing. contract DeployPlatformHarness is DeployPlatform { - function writeDeployments(Deployment memory d, address deployer) external { - _writeDeployments(d, deployer); + function writeDeploymentsTo(Deployment memory d, address deployer, string memory outPath) external { + _writeDeploymentsTo(d, deployer, outPath); } } import {DinCoordinator} from "../src/DinCoordinator.sol"; @@ -28,10 +28,10 @@ import {DinEmission} from "../src/DinEmission.sol"; /// environment, so no deploy test depends on env. The env-read path /// (readTokenomics) is covered by exactly one test, which is the only /// place in foundry/test that sets these tokenomics keys; see its comment. -/// test_writeDeployments_writesImplAndCodehashMetadata calls _writeDeployments -/// via DeployPlatformHarness using DEPLOYMENTS_NETWORK=test-impl-metadata, -/// so it writes foundry/deployments/test-impl-metadata.json rather than -/// clobbering foundry/deployments/localhost.json. Run with: +/// test_writeDeployments_writesImplAndCodehashMetadata calls +/// _writeDeploymentsTo via DeployPlatformHarness with an explicit path, +/// foundry/deployments/test-impl-metadata.json, so it never clobbers +/// localhost.json and never sets DEPLOYMENTS_NETWORK. Run with: /// forge test --match-contract DeployPlatformScriptTest -vv contract DeployPlatformScriptTest is Test { DeployPlatform internal script; @@ -169,22 +169,19 @@ contract DeployPlatformScriptTest is Test { } /// @dev Verifies the JSON-writing cycle used by dincli system verify-deployment. - /// Calls _writeDeployments via DeployPlatformHarness (which exposes the - /// internal function) with a DEPLOYMENTS_NETWORK override to avoid - /// clobbering foundry/deployments/localhost.json. - /// - /// vm.setEnv cannot unset a key, so DEPLOYMENTS_NETWORK stays set for the - /// rest of the run. Any future test that reads DEPLOYMENTS_NETWORK MUST - /// go into this function, or it will see "test-impl-metadata". + /// Calls _writeDeploymentsTo via DeployPlatformHarness with this test's + /// own file. It deliberately doesn't set DEPLOYMENTS_NETWORK: the + /// process environment is shared by forge's parallel test contracts, and + /// UpgradePlatformScriptTest setting it to another file name made this + /// test read a file that was never written (PR No. 254). function test_writeDeployments_writesImplAndCodehashMetadata() public { - vm.setEnv("DEPLOYMENTS_NETWORK", "test-impl-metadata"); DeployPlatform.Tokenomics memory t = script.defaultTokenomics(); DeployPlatform.Deployment memory d = script.deploy(t, deployer); - harness.writeDeployments(d, deployer); string memory path = string.concat( vm.projectRoot(), "/deployments/test-impl-metadata.json" ); + harness.writeDeploymentsTo(d, deployer, path); string memory content = vm.readFile(path); address implTreasury = vm.parseJsonAddress(content, ".dinTreasuryImpl"); diff --git a/foundry/test/UpgradePlatformScript.t.sol b/foundry/test/UpgradePlatformScript.t.sol index db40e13..9112db2 100644 --- a/foundry/test/UpgradePlatformScript.t.sol +++ b/foundry/test/UpgradePlatformScript.t.sol @@ -19,11 +19,11 @@ contract UpgradePlatformHarness is UpgradePlatform { } } -/// @dev Exposes _writeDeployments to create the initial deployments JSON needed +/// @dev Exposes _writeDeploymentsTo to create the initial deployments JSON needed /// by the upgrade metadata test, without coupling to DeployPlatformScript.t.sol. contract DeployHarness is DeployPlatform { - function writeDeployments(Deployment memory d, address deployer_) external { - _writeDeployments(d, deployer_); + function writeDeploymentsTo(Deployment memory d, address deployer_, string memory outPath) external { + _writeDeploymentsTo(d, deployer_, outPath); } } @@ -32,10 +32,9 @@ contract DeployHarness is DeployPlatform { /// deployments JSON records the correct implementation address, /// extcodehash, and (when GIT_COMMIT_HASH is supplied) commit hash. /// -/// test_writeUpgradeMetadata_writesImplCodehashAndCommit sets -/// DEPLOYMENTS_NETWORK=test-upgrade-metadata, which stays set for the -/// entire forge process. Any future test in this contract that reads -/// DEPLOYMENTS_NETWORK MUST go into that function. +/// The test passes its own file, foundry/deployments/test-upgrade-metadata.json, +/// explicitly and sets no environment variables: vm.setEnv changes the +/// environment shared by forge's parallel test contracts (PR No. 254). /// Run with: forge test --match-contract UpgradePlatformScriptTest -vv contract UpgradePlatformScriptTest is Test { UpgradePlatformHarness internal harness; @@ -53,23 +52,16 @@ contract UpgradePlatformScriptTest is Test { /// (a) recorded implementation address matches the upgraded impl, /// (b) recorded codehash matches extcodehash(newImpl), /// (c) .deployedAtCommit is written when a non-empty commit is passed. - /// - /// Both assertions live in one function because vm.setEnv cannot unset - /// a key, so DEPLOYMENTS_NETWORK stays as "test-upgrade-metadata" and - /// GIT_COMMIT_HASH stays set for the rest of the run. Any future test - /// that reads either key MUST go into this function. function test_writeUpgradeMetadata_writesImplCodehashAndCommit() public { - vm.setEnv("DEPLOYMENTS_NETWORK", "test-upgrade-metadata"); + string memory path = string.concat( + vm.projectRoot(), "/deployments/test-upgrade-metadata.json" + ); // Write an initial deployments JSON so the three-arg vm.writeJson calls // inside _writeUpgradeMetadata have an existing file to update. DeployPlatform.Tokenomics memory t = deployScript.defaultTokenomics(); DeployPlatform.Deployment memory d = deployScript.deploy(t, deployer); - deployHarness.writeDeployments(d, deployer); - - string memory path = string.concat( - vm.projectRoot(), "/deployments/test-upgrade-metadata.json" - ); + deployHarness.writeDeploymentsTo(d, deployer, path); // Deploy DinTokenV2 to get a real implementation address and codehash. // We only need the bytecode to be present; the contract need not be @@ -91,11 +83,10 @@ contract UpgradePlatformScriptTest is Test { // --- (c): commit hash written when non-empty --- string memory commit = "abc1234def5678900000000000000000000000000"; - vm.setEnv("GIT_COMMIT_HASH", commit); harness.writeUpgradeMetadata(newImpl, "dinToken", path, commit); content = vm.readFile(path); string memory recordedCommit = vm.parseJsonString(content, ".deployedAtCommit"); - assertEq(recordedCommit, commit, ".deployedAtCommit must match GIT_COMMIT_HASH"); + assertEq(recordedCommit, commit, ".deployedAtCommit must match the commit passed in"); } }