Skip to content
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

feat: forge test snapshot flags #9710

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+57 −13
Open

feat: forge test snapshot flags #9710

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
70 changes: 57 additions & 13 deletions crates/forge/bin/cmd/test/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use super::{install, test::filter::ProjectPathsAwareFilter, watch::WatchArgs};
use alloy_primitives::U256;
use chrono::Utc;
use clap::{Parser, ValueHint};
use clap::{builder::FalseyValueParser, ArgAction, Parser, ValueHint};
use eyre::{Context, OptionExt, Result};
use forge::{
decode::decode_console_logs,
Expand Down Expand Up @@ -122,6 +122,14 @@ pub struct TestArgs {
#[arg(long, env = "FORGE_ALLOW_FAILURE")]
allow_failure: bool,

/// Enable/disable writing snapshot results
Copy link
Collaborator

@grandizzy grandizzy Jan 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Enable/disable writing snapshot results
/// Disable recording of gas 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)]
Copy link
Collaborator

@grandizzy grandizzy Jan 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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=]

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

snapshot_emit: bool,
Comment on lines +126 to +127
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/// Check snapshot results
Copy link
Collaborator

@grandizzy grandizzy Jan 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

#[arg(long, env = "FORGE_SNAPSHOT_CHECK", value_parser=FalseyValueParser::new())]
Copy link
Collaborator

@grandizzy grandizzy Jan 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

snapshot_check: bool,

/// Output test results as JUnit XML report.
#[arg(long, conflicts_with_all = ["quiet", "json", "gas_report", "summary", "list", "show_progress"], help_heading = "Display options")]
pub junit: bool,
Expand Down Expand Up @@ -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 {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

let differences_found = gas_snapshots.clone().into_iter().fold(
false,
|mut found, (group, snapshots)| {
Expand Down Expand Up @@ -717,17 +725,19 @@ impl TestArgs {
}
}

// Create `snapshots` directory if it doesn't exist.
fs::create_dir_all(&config.snapshots)?;

// Write gas snapshots to disk per group.
gas_snapshots.clone().into_iter().for_each(|(group, snapshots)| {
fs::write_pretty_json_file(
&config.snapshots.join(format!("{group}.json")),
&snapshots,
)
.expect("Failed to write gas snapshots to disk");
});
if self.snapshot_emit {
// Create `snapshots` directory if it doesn't exist.
fs::create_dir_all(&config.snapshots)?;

// Write gas snapshots to disk per group.
gas_snapshots.clone().into_iter().for_each(|(group, snapshots)| {
fs::write_pretty_json_file(
&config.snapshots.join(format!("{group}.json")),
&snapshots,
)
.expect("Failed to write gas snapshots to disk");
});
}
}

// Print suite summary.
Expand Down Expand Up @@ -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>)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(())
});
}

for env_val in [None, Some(false), Some(true)] {
match env_val {
None => std::env::remove_var(env_name),
Some(value) => std::env::set_var(env_name, value.to_string()),
}
test_fn(env_val);
}
}

#[test]
Copy link
Collaborator

@grandizzy grandizzy Jan 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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());
});

fn snapshot_emit_env() {
env_bool("FORGE_SNAPSHOT_EMIT", |env_val| {
let args: TestArgs = TestArgs::parse_from(["foundry-cli"]);
assert!(args.snapshot_emit == env_val.unwrap_or(true));
let args: TestArgs = TestArgs::parse_from(["foundry-cli", "--snapshot-emit"]);
assert!(args.snapshot_emit);
let args: TestArgs = TestArgs::parse_from(["foundry-cli", "--snapshot-emit=true"]);
assert!(args.snapshot_emit);
let args: TestArgs = TestArgs::parse_from(["foundry-cli", "--snapshot-emit=false"]);
assert!(!args.snapshot_emit);
});
}

#[test]
fn snapshot_check_env() {
env_bool("FORGE_SNAPSHOT_CHECK", |env_val| {
let args: TestArgs = TestArgs::parse_from(["foundry-cli"]);
assert!(args.snapshot_check == env_val.unwrap_or(false));
let args: TestArgs = TestArgs::parse_from(["foundry-cli", "--snapshot-check"]);
assert!(args.snapshot_check);
});
}

forgetest_async!(gas_report_fuzz_invariant, |prj, _cmd| {
// speed up test by running with depth of 15
let config = Config {
Expand Down
Loading