Skip to content

Commit

Permalink
chore: deprecate --debug regex argument (#8930)
Browse files Browse the repository at this point in the history
* chore: deprecate --debug regex argument

* fix: enable full internal decoding if exactly one test matched
  • Loading branch information
DaniPopes authored Sep 23, 2024
1 parent dab9036 commit cba6e97
Show file tree
Hide file tree
Showing 3 changed files with 52 additions and 72 deletions.
13 changes: 7 additions & 6 deletions crates/evm/traces/src/folded_stack_trace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down Expand Up @@ -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`
Expand Down
98 changes: 37 additions & 61 deletions crates/forge/bin/cmd/test/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand Down Expand Up @@ -67,29 +67,20 @@ pub struct TestArgs {
#[arg(value_hint = ValueHint::FilePath)]
pub path: Option<GlobMatcher>,

/// 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<Regex>,
/// 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<Option<Regex>>,

/// 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`.
Expand All @@ -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<Option<Regex>>,

/// Print a gas report.
Expand Down Expand Up @@ -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
};
Expand All @@ -373,26 +355,27 @@ 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<Regex>>| {
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());
}

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

Expand Down Expand Up @@ -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 {
Expand All @@ -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)?);
Expand Down
13 changes: 8 additions & 5 deletions crates/forge/tests/cli/test_cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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
"#]]);
});

0 comments on commit cba6e97

Please sign in to comment.