feat: forge test snapshot flags #9710

turbocrime · 2025-01-18T03:53:24Z

Motivation

Solution

i've added command-line flags and an environment variable FORGE_SNAPSHOT_EMIT which allow a user to control snapshot generation.

i also slightly refactored the use of environment variable FORGE_SNAPSHOT_CHECK.

existing default behaviors are maintained.

turbocrime · 2025-01-18T03:57:00Z

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

+    #[arg(long, env = "FORGE_SNAPSHOT_EMIT", action = ArgAction::Set, default_value="true", default_missing_value = "true", require_equals=true, num_args = 0..=1)]
+    snapshot_emit: bool,


this is a little strange but it's the only way to get decent behavior out of clap

turbocrime · 2025-01-18T03:59:04Z

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

@@ -662,7 +670,7 @@ impl TestArgs {
            if !gas_snapshots.is_empty() {
                // Check for differences in gas snapshots if `FORGE_SNAPSHOT_CHECK` is set.
                // Exiting early with code 1 if differences are found.
-                if std::env::var("FORGE_SNAPSHOT_CHECK").is_ok() {
+                if self.snapshot_check {


direct env var access is now moved to another clap flag.

the behavior now supports 'false' values as well, instead of just conditioning on presence.

turbocrime · 2025-01-19T20:58:22Z

cc @zerosnacks assigned to #9697

zerosnacks · 2025-01-20T09:46:12Z

Generally supportive on the approach, defaulting to true also makes sense here

grandizzy

thank you! left couple of comments

grandizzy · 2025-01-20T09:36:12Z

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

@@ -973,6 +983,40 @@ mod tests {
        test("--chain-id=42", Chain::from_id(42));
    }

+    fn env_bool(env_name: &str, test_fn: impl Fn(Option<bool>)) {


I don't think this is needed, you could use figment::Jail for such as in

foundry/crates/config/src/lib.rs

Lines 2996 to 3004 in 90a5fdf

fn test_toml_file_non_existing_config_var_failure() {

figment::Jail::expect_with(|jail| {

jail.set_env("FOUNDRY_CONFIG", "this config does not exist");

let _config = Config::load();

Ok(())

});

}

grandizzy · 2025-01-20T09:43:50Z

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

+    #[arg(long, env = "FORGE_SNAPSHOT_EMIT", action = ArgAction::Set, default_value="true", default_missing_value = "true", require_equals=true, num_args = 0..=1)]
+    snapshot_emit: bool,
+
+    /// Check snapshot results


Suggested change

/// Check snapshot results

/// Disable checking for differences in recorded gas snapshots.

grandizzy · 2025-01-20T09:44:47Z

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

@@ -122,6 +122,14 @@ pub struct TestArgs {
    #[arg(long, env = "FORGE_ALLOW_FAILURE")]
    allow_failure: bool,

+    /// Enable/disable writing snapshot results


Suggested change

/// Enable/disable writing snapshot results

/// Disable recording of gas snapshot results.

grandizzy · 2025-01-20T09:45:08Z

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

@@ -122,6 +122,14 @@ pub struct TestArgs {
    #[arg(long, env = "FORGE_ALLOW_FAILURE")]
    allow_failure: bool,

+    /// Enable/disable writing snapshot results
+    #[arg(long, env = "FORGE_SNAPSHOT_EMIT", action = ArgAction::Set, default_value="true", default_missing_value = "true", require_equals=true, num_args = 0..=1)]


maybe have FORGE_NO_SNAPSHOT_RECORD / no_snapshot_record like

/// Disable recording of snapshot results. #[arg(long, env = "FORGE_SNAPSHOT_NO_RECORD", help_heading = "Gas snapshot options")] no_snapshot_record: bool,

so one should only pass --no-snapshot-record arg. Also suggest adding a help heading to have them grouped, e.g.

Gas snapshot options: --snapshot-no-record Enable/disable writing snapshot results [env: FORGE_SNAPSHOT_NO_RECORD=] --snapshot-no-check Omit checking snapshot results [env: FORGE_SNAPSHOT_CHECK=]

in my use case, i'd like to mainly control via env vars. using a 'negative' env variable seems ergonomically poor.

additionally, clap seems to have trouble with 'negation' flags that interact with each other, especially if env variables are used, and especially when default values are present.

i can make another attempt at this, but clap seems to very strongly encourage the =[true/false] format.

I see, maybe put them in config then? You could then make different profiles to run gas snapshot without need of args

grandizzy · 2025-01-20T10:10:41Z

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

+        }
+    }
+
+    #[test]


IMO would be better to have test to pass these flags and check if the gas snapshot dir is (or is not) written to disk, something like

forgetest_init!(config_load_no_panic, |prj, cmd| { prj.add_test( "GasSnapshotRecordTest.t.sol", r#" import "forge-std/Test.sol"; contract GasSnapshotRecordTest is Test { function test_start_stop_gas_recording() public { vm.startSnapshotGas("testRecord"); uint256 a = 256; a++; vm.stopSnapshotGas(); } } "#, ) .unwrap(); cmd.args(["clean"]).assert_success(); cmd.forge_fuse().args(["test", "--mc", "GasSnapshotRecordTest", "--snapshot-no-record"]).assert_success(); assert!(!prj.root().join("snapshots/").exists()); cmd.forge_fuse().args(["test", "--mc", "GasSnapshotRecordTest"]).assert_success(); assert!(prj.root().join("snapshots/").exists()); });

grandizzy · 2025-01-20T10:16:14Z

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

+    snapshot_emit: bool,
+
+    /// Check snapshot results
+    #[arg(long, env = "FORGE_SNAPSHOT_CHECK", value_parser=FalseyValueParser::new())]


I think #[arg(long, env = "FORGE_SNAPSHOT_NO_CHECK", help_heading = "Gas snapshot options")] is enough, that is one could pass --snapshot-no-check arg to omit differences check.

FORGE_SNAPSHOT_CHECK is already established as an env var, and i was avoiding the introduction of another FORGE_SNAPSHOT_NO_CHECK var.

if both variables exist, it raises a lot of questions! i would be worried about creating a breaking change.

grandizzy · 2025-01-20T10:36:23Z

Generally supportive on the approach, defaulting to true also makes sense here

I'd suggest having args like --snapshot-no-record and --snapshot-no-check that would disable default behavior if passed, or something along these lines, would that work?

turbocrime · 2025-01-21T16:47:08Z

the environment variable FORGE_SNAPSHOT_CHECK is already established. adding an additional FORGE_SNAPSHOT_NO_CHECK seems like it could be confusing. interactions between the env and the flags become unclear.

i made the FORGE_SNAPSHOT_CHECK change because

using a clap flag seemed simple and more consistent than checking the env directly in the conditional
it collected all the available options and env var access in the clap options block
the current behavior simply conditions on presence, so something like FORGE_SNAPSHOT_CHECK="false" is interpreted as a truthy value!!

but now i see that also raises many questions, and it is not entirely consistent with the behavior of my added flag.

i will revert changes to FORGE_SNAPSHOT_CHECK and create an issue describing its present behavior. it may be good to resolve that, and all the questions there, before adding a new flag.

zerosnacks · 2025-01-30T15:38:12Z

i will revert changes to FORGE_SNAPSHOT_CHECK and create an issue describing its present behavior. it may be good to resolve that, and all the questions there, before adding a new flag.

added the gas_snapshot_check config entry + flag + suggested change to FORGE_SNAPSHOT_CHECK here: #9791

turbocrime · 2025-01-30T16:24:09Z

thanks for working on this, wound up with other tasks on my plate and wasn't able to make time

turbocrime · 2025-01-30T16:27:23Z

the emit behavior still remains to be implemented? will leave this open for now, might get to it with free time this week

zerosnacks · 2025-01-30T16:37:30Z

@turbocrime No worries! Feel free to ping me if you run into (time) issues / have questions

turbocrime requested review from DaniPopes, klkvr, mattsse, grandizzy, yash-atreya and zerosnacks as code owners January 18, 2025 03:53

feat: forge snapshot flags

ab193b2

turbocrime force-pushed the snapshot-flags branch from 0cd7bd4 to ab193b2 Compare January 18, 2025 03:54

turbocrime commented Jan 18, 2025

View reviewed changes

grandizzy requested changes Jan 20, 2025

View reviewed changes

zerosnacks self-assigned this Jan 21, 2025

grandizzy added T-feature Type: feature C-forge Command: forge labels Jan 30, 2025

zerosnacks mentioned this pull request Jan 30, 2025

feat: add gas_snapshot_check flag to config, fix FORGE_SNAPSHOT_CHECK behavior #9791

Merged

turbocrime closed this Jan 30, 2025

turbocrime reopened this Jan 30, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: forge test snapshot flags #9710

feat: forge test snapshot flags #9710

turbocrime commented Jan 18, 2025 •

edited

Loading

turbocrime Jan 18, 2025

turbocrime Jan 18, 2025

turbocrime commented Jan 19, 2025

zerosnacks commented Jan 20, 2025

grandizzy left a comment

grandizzy Jan 20, 2025

grandizzy Jan 20, 2025 •

edited

Loading

grandizzy Jan 20, 2025 •

edited

Loading

grandizzy Jan 20, 2025 •

edited

Loading

turbocrime Jan 21, 2025

grandizzy Jan 21, 2025

grandizzy Jan 20, 2025 •

edited

Loading

grandizzy Jan 20, 2025 •

edited

Loading

turbocrime Jan 21, 2025

grandizzy commented Jan 20, 2025

turbocrime commented Jan 21, 2025

zerosnacks commented Jan 30, 2025

turbocrime commented Jan 30, 2025

turbocrime commented Jan 30, 2025

zerosnacks commented Jan 30, 2025 •

edited

Loading

		#[arg(long, env = "FORGE_SNAPSHOT_EMIT", action = ArgAction::Set, default_value="true", default_missing_value = "true", require_equals=true, num_args = 0..=1)]
		snapshot_emit: bool,

	fn test_toml_file_non_existing_config_var_failure() {
	figment::Jail::expect_with(\|jail\| {
	jail.set_env("FOUNDRY_CONFIG", "this config does not exist");

	let _config = Config::load();

	Ok(())
	});
	}

	/// Check snapshot results
	/// Disable checking for differences in recorded gas snapshots.

	/// Enable/disable writing snapshot results
	/// Disable recording of gas snapshot results.

+                      }
+                  }
+                  #[test]

feat: forge test snapshot flags #9710

Are you sure you want to change the base?

feat: forge test snapshot flags #9710

Conversation

turbocrime commented Jan 18, 2025 • edited Loading

Motivation

Solution

turbocrime Jan 18, 2025

Choose a reason for hiding this comment

turbocrime Jan 18, 2025

Choose a reason for hiding this comment

turbocrime commented Jan 19, 2025

zerosnacks commented Jan 20, 2025

grandizzy left a comment

Choose a reason for hiding this comment

grandizzy Jan 20, 2025

Choose a reason for hiding this comment

grandizzy Jan 20, 2025 • edited Loading

Choose a reason for hiding this comment

grandizzy Jan 20, 2025 • edited Loading

Choose a reason for hiding this comment

grandizzy Jan 20, 2025 • edited Loading

Choose a reason for hiding this comment

turbocrime Jan 21, 2025

Choose a reason for hiding this comment

grandizzy Jan 21, 2025

Choose a reason for hiding this comment

grandizzy Jan 20, 2025 • edited Loading

Choose a reason for hiding this comment

grandizzy Jan 20, 2025 • edited Loading

Choose a reason for hiding this comment

turbocrime Jan 21, 2025

Choose a reason for hiding this comment

grandizzy commented Jan 20, 2025

turbocrime commented Jan 21, 2025

zerosnacks commented Jan 30, 2025

turbocrime commented Jan 30, 2025

turbocrime commented Jan 30, 2025

zerosnacks commented Jan 30, 2025 • edited Loading

turbocrime commented Jan 18, 2025 •

edited

Loading

grandizzy Jan 20, 2025 •

edited

Loading

grandizzy Jan 20, 2025 •

edited

Loading

grandizzy Jan 20, 2025 •

edited

Loading

grandizzy Jan 20, 2025 •

edited

Loading

grandizzy Jan 20, 2025 •

edited

Loading

zerosnacks commented Jan 30, 2025 •

edited

Loading