From cba6e97fcdaedc2aad5c3b25f32be20ec2068e87 Mon Sep 17 00:00:00 2001 From: DaniPopes <57450786+DaniPopes@users.noreply.github.com> Date: Mon, 23 Sep 2024 03:20:29 +0200 Subject: [PATCH] chore: deprecate --debug regex argument (#8930) * chore: deprecate --debug regex argument * fix: enable full internal decoding if exactly one test matched --- crates/evm/traces/src/folded_stack_trace.rs | 13 +-- crates/forge/bin/cmd/test/mod.rs | 98 ++++++++------------- crates/forge/tests/cli/test_cmd.rs | 13 +-- 3 files changed, 52 insertions(+), 72 deletions(-) diff --git a/crates/evm/traces/src/folded_stack_trace.rs b/crates/evm/traces/src/folded_stack_trace.rs index de76f8e80408..ee3a05afad29 100644 --- a/crates/evm/traces/src/folded_stack_trace.rs +++ b/crates/evm/traces/src/folded_stack_trace.rs @@ -30,14 +30,13 @@ impl EvmFoldedStackTraceBuilder { let node = &nodes[idx]; let func_name = if node.trace.kind.is_any_create() { - let default_contract_name = "Contract".to_string(); - let contract_name = node.trace.decoded.label.as_ref().unwrap_or(&default_contract_name); + let contract_name = node.trace.decoded.label.as_deref().unwrap_or("Contract"); format!("new {contract_name}") } else { let selector = node .selector() .map(|selector| selector.encode_hex_with_prefix()) - .unwrap_or("fallback".to_string()); + .unwrap_or_else(|| "fallback".to_string()); let signature = node.trace.decoded.call_data.as_ref().map(|dc| &dc.signature).unwrap_or(&selector); @@ -114,9 +113,11 @@ impl EvmFoldedStackTraceBuilder { /// Helps to translate a function enter-exit flow into a folded stack trace. /// /// Example: -/// fn top() { child_a(); child_b() } // consumes 500 gas -/// fn child_a() {} // consumes 100 gas -/// fn child_b() {} // consumes 200 gas +/// ```solidity +/// function top() { child_a(); child_b() } // consumes 500 gas +/// function child_a() {} // consumes 100 gas +/// function child_b() {} // consumes 200 gas +/// ``` /// /// For execution of the `top` function looks like: /// 1. enter `top` diff --git a/crates/forge/bin/cmd/test/mod.rs b/crates/forge/bin/cmd/test/mod.rs index 47a4bd075c02..0eebbb9a0ee7 100644 --- a/crates/forge/bin/cmd/test/mod.rs +++ b/crates/forge/bin/cmd/test/mod.rs @@ -20,7 +20,7 @@ use foundry_cli::{ opts::CoreBuildArgs, utils::{self, LoadConfig}, }; -use foundry_common::{compile::ProjectCompiler, evm::EvmArgs, fs, shell}; +use foundry_common::{cli_warn, compile::ProjectCompiler, evm::EvmArgs, fs, shell}; use foundry_compilers::{ artifacts::output_selection::OutputSelection, compilers::{multi::MultiCompilerLanguage, CompilerSettings, Language}, @@ -67,29 +67,20 @@ pub struct TestArgs { #[arg(value_hint = ValueHint::FilePath)] pub path: Option, - /// Run a test in the debugger. - /// - /// The argument passed to this flag is the **regex** of the test function signature you want - /// to run, and it works the same as --match-test. - /// - /// If more than one test matches your specified criteria, you must add additional filters - /// until only one test is found (see --match-contract and --match-path). + /// Run a single test in the debugger. /// /// The matching test will be opened in the debugger regardless of the outcome of the test. /// /// If the matching test is a fuzz test, then it will open the debugger on the first failure - /// case. - /// If the fuzz test does not fail, it will open the debugger on the last fuzz case. - /// - /// For more fine-grained control of which fuzz case is run, see forge run. - #[arg(long, value_name = "TEST_FUNCTION")] - debug: Option, + /// case. If the fuzz test does not fail, it will open the debugger on the last fuzz case. + #[arg(long, value_name = "DEPRECATED_TEST_FUNCTION_REGEX")] + debug: Option>, /// Generate a flamegraph for a single test. Implies `--decode-internal`. /// /// A flame graph is used to visualize which functions or operations within the smart contract /// are consuming the most gas overall in a sorted manner. - #[arg(long, conflicts_with = "flamechart")] + #[arg(long)] flamegraph: bool, /// Generate a flamechart for a single test. Implies `--decode-internal`. @@ -99,18 +90,13 @@ pub struct TestArgs { #[arg(long, conflicts_with = "flamegraph")] flamechart: bool, - /// Whether to identify internal functions in traces. + /// Identify internal functions in traces. /// - /// If no argument is passed to this flag, it will trace internal functions scope and decode - /// stack parameters, but parameters stored in memory (such as bytes or arrays) will not be - /// decoded. + /// This will trace internal functions and decode stack parameters. /// - /// To decode memory parameters, you should pass an argument with a test function name, - /// similarly to --debug and --match-test. - /// - /// If more than one test matches your specified criteria, you must add additional filters - /// until only one test is found (see --match-contract and --match-path). - #[arg(long, value_name = "TEST_FUNCTION")] + /// Parameters stored in memory (such as bytes or arrays) are currently decoded only when a + /// single function is matched, similarly to `--debug`, for performance reasons. + #[arg(long, value_name = "DEPRECATED_TEST_FUNCTION_REGEX")] decode_internal: Option>, /// Print a gas report. @@ -342,19 +328,15 @@ impl TestArgs { let env = evm_opts.evm_env().await?; // Enable internal tracing for more informative flamegraph. - if should_draw { + if should_draw && self.decode_internal.is_none() { self.decode_internal = Some(None); } // Choose the internal function tracing mode, if --decode-internal is provided. - let decode_internal = if let Some(maybe_fn) = self.decode_internal.as_ref() { - if maybe_fn.is_some() { - // If function filter is provided, we enable full tracing. - InternalTraceMode::Full - } else { - // If no function filter is provided, we enable simple tracing. - InternalTraceMode::Simple - } + let decode_internal = if self.decode_internal.is_some() { + // If more than one function matched, we enable simple tracing. + // If only one function matched, we enable full tracing. This is done in `run_tests`. + InternalTraceMode::Simple } else { InternalTraceMode::None }; @@ -373,13 +355,18 @@ impl TestArgs { .alphanet(evm_opts.alphanet) .build(project_root, &output, env, evm_opts)?; - let mut maybe_override_mt = |flag, maybe_regex: Option<&Regex>| { - if let Some(regex) = maybe_regex { + let mut maybe_override_mt = |flag, maybe_regex: Option<&Option>| { + if let Some(Some(regex)) = maybe_regex { + cli_warn!( + "specifying argument for --{flag} is deprecated and will be removed in the future, \ + use --match-test instead" + ); + let test_pattern = &mut filter.args_mut().test_pattern; if test_pattern.is_some() { eyre::bail!( "Cannot specify both --{flag} and --match-test. \ - Use --match-contract and --match-path to further limit the search instead." + Use --match-contract and --match-path to further limit the search instead." ); } *test_pattern = Some(regex.clone()); @@ -387,12 +374,8 @@ impl TestArgs { Ok(()) }; - maybe_override_mt("debug", self.debug.as_ref())?; - maybe_override_mt( - "decode-internal", - self.decode_internal.as_ref().and_then(|v| v.as_ref()), - )?; + maybe_override_mt("decode-internal", self.decode_internal.as_ref())?; let libraries = runner.libraries.clone(); let mut outcome = self.run_tests(runner, config, verbosity, &filter, &output).await?; @@ -401,18 +384,10 @@ impl TestArgs { let (suite_name, test_name, mut test_result) = outcome.remove_first().ok_or_eyre("no tests were executed")?; - let arena = test_result + let (_, arena) = test_result .traces .iter_mut() - .find_map( - |(kind, arena)| { - if *kind == TraceKind::Execution { - Some(arena) - } else { - None - } - }, - ) + .find(|(kind, _)| *kind == TraceKind::Execution) .unwrap(); // Decode traces. @@ -425,6 +400,7 @@ impl TestArgs { let test_name = test_name.trim_end_matches("()"); let file_name = format!("cache/{label}_{contract}_{test_name}.svg"); let file = std::fs::File::create(&file_name).wrap_err("failed to create file")?; + let file = std::io::BufWriter::new(file); let mut options = inferno::flamegraph::Options::default(); options.title = format!("{label} {contract}::{test_name}"); @@ -435,13 +411,13 @@ impl TestArgs { } // Generate SVG. - inferno::flamegraph::from_lines(&mut options, fst.iter().map(|s| s.as_str()), file) + inferno::flamegraph::from_lines(&mut options, fst.iter().map(String::as_str), file) .wrap_err("failed to write svg")?; println!("\nSaved to {file_name}"); // Open SVG in default program. - if opener::open(&file_name).is_err() { - println!("\nFailed to open {file_name}. Please open it manually."); + if let Err(e) = opener::open(&file_name) { + eprintln!("\nFailed to open {file_name}; please open it manually: {e}"); } } @@ -488,12 +464,7 @@ impl TestArgs { trace!(target: "forge::test", "running all tests"); let num_filtered = runner.matching_test_functions(filter).count(); - if (self.debug.is_some() || - self.decode_internal.as_ref().map_or(false, |v| v.is_some()) || - self.flamegraph || - self.flamechart) && - num_filtered != 1 - { + if num_filtered != 1 && (self.debug.is_some() || self.flamegraph || self.flamechart) { let action = if self.flamegraph { "generate a flamegraph" } else if self.flamechart { @@ -512,6 +483,11 @@ impl TestArgs { ); } + // If exactly one test matched, we enable full tracing. + if num_filtered == 1 && self.decode_internal.is_some() { + runner.decode_internal = InternalTraceMode::Full; + } + if self.json { let results = runner.test_collect(filter); println!("{}", serde_json::to_string(&results)?); diff --git a/crates/forge/tests/cli/test_cmd.rs b/crates/forge/tests/cli/test_cmd.rs index c3a5a548ab37..63a4a9210699 100644 --- a/crates/forge/tests/cli/test_cmd.rs +++ b/crates/forge/tests/cli/test_cmd.rs @@ -2176,16 +2176,12 @@ Warning: the following cheatcode(s) are deprecated and will be removed in future ); forgetest_init!(requires_single_test, |prj, cmd| { - cmd.args(["test", "--debug", "test"]).assert_failure().stderr_eq(str![[r#" + cmd.args(["test", "--debug"]).assert_failure().stderr_eq(str![[r#" Error: 2 tests matched your criteria, but exactly 1 test must match in order to run the debugger. Use --match-contract and --match-path to further limit the search. -Filter used: - match-test: `test` - - "#]]); cmd.forge_fuse().args(["test", "--flamegraph"]).assert_failure().stderr_eq(str![[r#" Error: @@ -2202,3 +2198,10 @@ Use --match-contract and --match-path to further limit the search. "#]]); }); + +forgetest_init!(deprecated_regex_arg, |prj, cmd| { + cmd.args(["test", "--decode-internal", "test_Increment"]).assert_success().stderr_eq(str![[r#" +warning: specifying argument for --decode-internal is deprecated and will be removed in the future, use --match-test instead + +"#]]); +});