Skip to content

fix(foundry-test): stop DeployPlatform/UpgradePlatform tests racing on DEPLOYMENTS_NETWORK #311

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
robertocarlous merged 1 commit into from
Nov 2, 2026
Merged

fix(foundry-test): stop DeployPlatform/UpgradePlatform tests racing on DEPLOYMENTS_NETWORK #311

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion foundry/script/DeployPlatform.s.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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/<network>.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);
Expand Down Expand Up @@ -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);
}
Expand Down
29 changes: 13 additions & 16 deletions foundry/test/DeployPlatformScript.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -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;
Expand Down Expand Up @@ -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");
Expand Down
31 changes: 11 additions & 20 deletions foundry/test/UpgradePlatformScript.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}

Expand All @@ -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;
Expand All @@ -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
Expand All @@ -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");
}
}
Loading