bug: forge test deletes any existing snapshots directory by design

[smartcontracts]

Component

Forge

Have you ensured that all of these are up to date?

• Foundry
• Foundryup

What version of Foundry are you on?

forge 0.2.0 (e5dbb7a 2024-12-03T00:25:01.006590000Z)

What command(s) is the bug in?

forge test

Operating System

macOS (Apple Silicon)

Describe the bug

Running forge test in our repository seems to delete any existing "snapshots" folder if it already exists. We use a folder called "snapshots" to contain all of our snapshots, so this folder isn't exclusive to forge things. I've gotten around this for now by changing our foundry.toml to set the configured snapshots folder to a fake directory.

From a quick glance it looks like this is the source of the issue:

foundry/crates/config/src/lib.rs

Lines 1042 to 1046 in 805d7ce

	// Remove snapshot directory.
	let snapshot_dir = project.root().join(&self.snapshots);
	if snapshot_dir.exists() {
	let _ = fs::remove_dir_all(&snapshot_dir);
	}

I haven't checked what this is actually being used for but if I have some time later I might take a stab at a fix.

[grandizzy]

@smartcontracts that dir is cleaned up to make sure gas snapshots are properly recorded for tests, @zerosnacks maybe we could pattern match and remove only the foundry gas snapshot files? Or should be reserved dir and should be reconfigured as above?

[zerosnacks]

Hi @smartcontracts

Thanks for flagging this, for context this is the relevant code section:

foundry/crates/forge/bin/cmd/test/mod.rs

Lines 309 to 318 in 3a1e76b

	// Remove the snapshots directory if it exists.
	// This is to ensure that we don't have any stale snapshots.
	// If `FORGE_SNAPSHOT_CHECK` is set, we don't remove the snapshots directory as it is
	// required for comparison.
	if std::env::var_os("FORGE_SNAPSHOT_CHECK").is_none() {
	let snapshot_dir = project_root.join(&config.snapshots);
	if snapshot_dir.exists() {
	let _ = fs::remove_dir_all(project_root.join(&config.snapshots));
	}
	}

This is an edge case that wasn't considered, I'll have a look at how best to handle this:

Some possible approaches:

• Detect whether the gas snapshot cheatcode is used and only then clear out (unreliable due to parallelism, hence why it was moved up)
• Use a *.gas.json or *.snapshot.json extension and only clear those out (breaking for current users, not very reliable either)
• To mark snapshots as a reserved directory, similar to .cache or out.

In the base case the snapshots directory should not be cleaned out if the developer does not use Foundry's gas snapshot cheatcodes.

The configured snapshots directory will also be cleaned upon forge clean.

For context: https://github.com/ethereum-optimism/optimism/tree/develop/packages/contracts-bedrock/snapshots

For now the workaround you are using should work and continue to work but it is clearly less than ideal.

[zerosnacks]

Additional issue highlighted in #9511 is that when the user runs a partial test forge test --mc SomeTestContract the snapshots directory is removed, removing all snapshots and only creating the one that ran. This is currently expected behavior to prevent staining (old snapshots staying around). I think it is worth considering whether the snapshot folder removal should be removed, letting the user handle the stain case, as it is interfering with user workflows.

[sakulstra]

@zerosnacks as i understand, currently:

1. all snapshots are removed
2. each test can write to whatever snapshot

If that's the case I think:

I think it is worth considering whether the snapshot folder removal should be removed, letting the user handle the stain case, as it is interfering with user workflows.

is the only option, given it's impossible to understand which snapshot is written by which test.

---

That said, i'm wondering if it would have made more sense to uniquify snapshots per test contract.
This way it would be far easier to reason if a snapshot is obsolete or not.

[zerosnacks]

Hi @sakulstra

1. Correct, we remove the directory incl. all files and recreate it
2. Correct, we currently do not enforce writing to specific files, any test can write to any file in the snapshots directory. This was designed this way so you can group snapshots thematically, regardless of what file it is in.

You raise an interesting point, if we were to enforce a function name pattern we could selectively remove files using the same match case. I'm a bit hesitant to introduce breaking changes here given its steady adoption.

Realistically the least breaking change would be to: remove the functionality of clearing out the directory upon forge test as well as avoiding removal on forge clean. This would address both of your issues at the cost of inattentive users checking in outdated snapshots.

I'll draft a PR and make sure this removal doesn't cause any regressions.

[sakulstra]

This was designed this way so you can group snapshots thematically, regardless of what file it is in.

I see the motivation, but idk if it's the best tradeoff. Thematic composition, one can still do in some post-processing, but fixing the dx is impossible in userland atm.

I'm a bit hesitant to introduce breaking changes here given its steady adoption.

Imho breaking changes - especially when with a clear reason, and migration path - are perfectly fine even now. Perhaps makes sense to reconsider changing after v1 though, when ppl can fix a proper version and delay the upgrade to when they have time to care about the migration 😅

[sakulstra]

I know is kinda ot in this issue, but fits on the overall topic 😅
It's a bit weird that while --gas-snapshot runs in --isolate it seems like snapshotGasLastCall does not force isolation.
Not sure if that's possible, but if it is probably would make sense. Getting very different results in the jsons from forge test and forge test --isolate.