diff --git a/.changelog/setup-skip-after-caught-revert.md b/.changelog/setup-skip-after-caught-revert.md new file mode 100644 index 0000000000000..1ce56edda0d68 --- /dev/null +++ b/.changelog/setup-skip-after-caught-revert.md @@ -0,0 +1,7 @@ +--- +forge: patch +foundry-evm: patch +foundry-cheatcodes: patch +--- + +Fixed `vm.skip` in `setUp` being reported as a failure when an earlier revert was caught before the skip. diff --git a/crates/cheatcodes/src/inspector.rs b/crates/cheatcodes/src/inspector.rs index e1f656a2d9fb0..1da707bc46d78 100644 --- a/crates/cheatcodes/src/inspector.rs +++ b/crates/cheatcodes/src/inspector.rs @@ -791,6 +791,12 @@ pub struct Cheatcodes { /// Test-scoped context holding data that needs to be reset every test run pub test_context: TestContext, + /// Revert payloads minted by the `skip` cheatcode during the current test call. + /// + /// A top-level revert is only classified as a skip when its data byte-equals one of these + /// payloads, so user-crafted `FOUNDRY::SKIP` revert data never skips a test on its own. + pub skip_payloads: Vec, + /// Whether to commit FS changes such as file creations, writes and deletes. /// Used to prevent duplicate changes file executing non-committing calls. pub fs_commit: bool, @@ -950,6 +956,7 @@ impl Cheatcodes { broadcastable_transactions: Default::default(), access_list: Default::default(), test_context: Default::default(), + skip_payloads: Default::default(), serialized_jsons: Default::default(), eth_deals: Default::default(), gas_metering: Default::default(), diff --git a/crates/cheatcodes/src/test.rs b/crates/cheatcodes/src/test.rs index 06e8cbdb66ab1..9d9f1330fecf3 100644 --- a/crates/cheatcodes/src/test.rs +++ b/crates/cheatcodes/src/test.rs @@ -2,7 +2,7 @@ use crate::{Cheatcode, Cheatcodes, CheatsCtxt, Result, Vm::*}; use alloy_chains::Chain as AlloyChain; -use alloy_primitives::{Address, U256}; +use alloy_primitives::{Address, Bytes, U256}; use alloy_sol_types::SolValue; use foundry_common::version::SEMVER_VERSION; use foundry_evm_core::{constants::MAGIC_SKIP, evm::FoundryEvmNetwork}; @@ -69,28 +69,14 @@ impl Cheatcode for sleepCall { impl Cheatcode for skip_0Call { fn apply_stateful(&self, ccx: &mut CheatsCtxt<'_, '_, FEN>) -> Result { let Self { skipTest } = *self; - if skipTest { - // Skip should not work if called deeper than at test level. - // Since we're not returning the magic skip bytes, this will cause a test failure. - ensure!(ccx.ecx.journal().depth() <= 1, "`skip` can only be used at test level"); - Err([MAGIC_SKIP, &[]].concat().into()) - } else { - Ok(Default::default()) - } + skip(ccx, skipTest, "") } } impl Cheatcode for skip_1Call { fn apply_stateful(&self, ccx: &mut CheatsCtxt<'_, '_, FEN>) -> Result { let Self { skipTest, reason } = self; - if *skipTest { - // Skip should not work if called deeper than at test level. - // Since we're not returning the magic skip bytes, this will cause a test failure. - ensure!(ccx.ecx.journal().depth() <= 1, "`skip` can only be used at test level"); - Err([MAGIC_SKIP, reason.as_bytes()].concat().into()) - } else { - Ok(Default::default()) - } + skip(ccx, *skipTest, reason) } } @@ -110,6 +96,24 @@ impl Cheatcode for getChain_1Call { } } +/// Reverts with the magic skip payload and records it in the state, so that the executor can +/// distinguish this genuine skip from user-crafted revert data carrying the same prefix. +fn skip( + ccx: &mut CheatsCtxt<'_, '_, FEN>, + skip_test: bool, + reason: &str, +) -> Result { + if !skip_test { + return Ok(Default::default()); + } + // Skip should not work if called deeper than at test level. + // Since we're not returning the magic skip bytes, this will cause a test failure. + ensure!(ccx.ecx.journal().depth() <= 1, "`skip` can only be used at test level"); + let payload = Bytes::from([MAGIC_SKIP, reason.as_bytes()].concat()); + ccx.state.skip_payloads.push(payload.clone()); + Err(payload.into()) +} + /// Adds or removes the given breakpoint to the state. fn breakpoint( state: &mut Cheatcodes, diff --git a/crates/evm/evm/src/executors/fuzz/mod.rs b/crates/evm/evm/src/executors/fuzz/mod.rs index 7d190e7dd0179..10e2e53407c59 100644 --- a/crates/evm/evm/src/executors/fuzz/mod.rs +++ b/crates/evm/evm/src/executors/fuzz/mod.rs @@ -18,7 +18,7 @@ use foundry_common::sh_println; use foundry_config::FuzzConfig; use foundry_evm_core::{ Breakpoints, - constants::{CHEATCODE_ADDRESS, MAGIC_ASSUME}, + constants::MAGIC_ASSUME, decode::{RevertDecoder, SkipReason}, evm::FoundryEvmNetwork, }; @@ -327,9 +327,7 @@ impl FuzzedExecutor { ..Default::default() }); } - if call.reverter == Some(CHEATCODE_ADDRESS) - && let Some(reason) = SkipReason::decode(&call.result) - { + if let Some(reason) = call.skip_reason() { return Ok(FuzzTestResult { skipped: true, reason: reason.0, ..Default::default() }); } @@ -359,13 +357,10 @@ impl FuzzedExecutor { result.logs = call.logs; result.gas_report_traces.extend(call.traces.into_iter().map(|trace| trace.arena)); } else { - let reason = if call.reverter == Some(CHEATCODE_ADDRESS) { - SkipReason::decode(&call.result) - .map(|reason| reason.to_string()) - .or_else(|| rd.maybe_decode(&call.result, call.exit_reason)) - } else { - rd.maybe_decode(&call.result, call.exit_reason) - }; + let reason = call + .skip_reason() + .map(|reason| reason.to_string()) + .or_else(|| rd.maybe_decode(&call.result, call.exit_reason)); result.reason = reason; let args = tx .call_details @@ -907,15 +902,12 @@ impl FuzzedExecutor { } worker.failure_run = fuzz_run; - // Only classify magic skip payloads when the revert originates from the - // cheatcode address. - let reason = if outcome.1.reverter == Some(CHEATCODE_ADDRESS) { - SkipReason::decode(&outcome.1.result) - .map(|reason| reason.to_string()) - .or_else(|| rd.maybe_decode(&outcome.1.result, status)) - } else { - rd.maybe_decode(&outcome.1.result, status) - }; + // Only classify magic skip payloads minted by the skip cheatcode. + let reason = outcome + .1 + .skip_reason() + .map(|reason| reason.to_string()) + .or_else(|| rd.maybe_decode(&outcome.1.result, status)); if self.config.show_logs { worker.logs.extend(outcome.1.logs.clone()); } else { diff --git a/crates/evm/evm/src/executors/mod.rs b/crates/evm/evm/src/executors/mod.rs index e30bfa0c76783..be74d379c5567 100644 --- a/crates/evm/evm/src/executors/mod.rs +++ b/crates/evm/evm/src/executors/mod.rs @@ -898,6 +898,8 @@ impl Executor { // Clear broadcastable transactions cheats.broadcastable_transactions.clear(); cheats.ignored_traces.ignored.clear(); + // Skip payloads are scoped to the call they were minted in. + cheats.skip_payloads.clear(); // if tracing was paused but never unpaused, we should begin next frame with tracing // still paused @@ -1296,6 +1298,11 @@ pub struct RawCallResult { /// The chisel state pub chisel_state: Option<(Vec, Vec)>, pub reverter: Option
, + /// Revert payloads minted by the `skip` cheatcode during this call. + /// + /// Copied out of the cheatcode state on conversion since `commit` moves that state back into + /// the executor before results are classified. + pub skip_payloads: Vec, } impl Default for RawCallResult { @@ -1328,6 +1335,7 @@ impl Default for RawCallResult { fork_block_number: None, chisel_state: None, reverter: None, + skip_payloads: Vec::new(), } } } @@ -1342,11 +1350,20 @@ impl RawCallResult { } } + /// Returns the skip reason if this call reverted with a genuine `vm.skip` payload. + /// + /// The revert data must byte-equal a payload recorded by the skip cheatcode during this call; + /// a matching `FOUNDRY::SKIP` prefix alone (user-crafted revert data) does not count. + pub fn skip_reason(&self) -> Option { + if !self.reverted || !self.skip_payloads.contains(&self.result) { + return None; + } + SkipReason::decode(&self.result) + } + /// Converts the result of the call into an `EvmError`. pub fn into_evm_error(self, rd: Option<&RevertDecoder>) -> EvmError { - if self.reverter == Some(CHEATCODE_ADDRESS) - && let Some(reason) = SkipReason::decode(&self.result) - { + if let Some(reason) = self.skip_reason() { return EvmError::Skip(reason); } let reason = rd.unwrap_or_default().decode(&self.result, self.exit_reason); @@ -1604,6 +1621,7 @@ fn convert_executed_result( .as_ref() .map(|c| c.broadcastable_transactions.clone()) .filter(|txs| !txs.is_empty()); + let skip_payloads = cheatcodes.as_ref().map(|c| c.skip_payloads.clone()).unwrap_or_default(); Ok(RawCallResult { exit_reason: Some(exit_reason), @@ -1633,6 +1651,7 @@ fn convert_executed_result( fork_block_number, chisel_state, reverter, + skip_payloads, }) } @@ -1871,8 +1890,9 @@ mod tests { #[test] fn cheatcode_skip_payload_is_classified_as_skip() { let raw = RawCallResult:: { + reverted: true, result: Bytes::from_static(b"FOUNDRY::SKIPwith reason"), - reverter: Some(CHEATCODE_ADDRESS), + skip_payloads: vec![Bytes::from_static(b"FOUNDRY::SKIPwith reason")], ..Default::default() }; @@ -1881,10 +1901,11 @@ mod tests { } #[test] - fn forged_skip_payload_from_non_cheatcode_is_execution_error() { + fn forged_skip_payload_is_execution_error() { let raw = RawCallResult:: { + reverted: true, result: Bytes::from_static(MAGIC_SKIP), - reverter: Some(CALLER), + reverter: Some(CHEATCODE_ADDRESS), ..Default::default() }; @@ -1893,10 +1914,11 @@ mod tests { } #[test] - fn skip_payload_without_reverter_is_execution_error() { + fn mismatched_skip_payload_is_execution_error() { let raw = RawCallResult:: { - result: Bytes::from_static(MAGIC_SKIP), - reverter: None, + reverted: true, + result: Bytes::from_static(b"FOUNDRY::SKIPforged"), + skip_payloads: vec![Bytes::from_static(b"FOUNDRY::SKIPgenuine")], ..Default::default() }; diff --git a/crates/evm/evm/src/executors/showmap.rs b/crates/evm/evm/src/executors/showmap.rs index 8b5af277a6b57..f10a9a12266c9 100644 --- a/crates/evm/evm/src/executors/showmap.rs +++ b/crates/evm/evm/src/executors/showmap.rs @@ -33,11 +33,7 @@ use alloy_json_abi::Function; use alloy_primitives::{Address, B256, Selector, hex, keccak256}; use eyre::Result; use foundry_config::FuzzCorpusConfig; -use foundry_evm_core::{ - constants::{CHEATCODE_ADDRESS, MAGIC_ASSUME}, - decode::SkipReason, - evm::FoundryEvmNetwork, -}; +use foundry_evm_core::{constants::MAGIC_ASSUME, evm::FoundryEvmNetwork}; use foundry_evm_coverage::HitMaps; use foundry_evm_fuzz::{BasicTxDetails, invariant::FuzzRunIdentifiedContracts}; use std::{ @@ -324,10 +320,7 @@ pub fn replay_corpus_to_showmap( let fingerprint = snapshot_edge_fingerprint(&call_result); // `vm.assume` rejects and cheatcode `vm.skip` are discarded by the campaign: the call // is not committed, checked, or counted toward coverage. - if call_result.result.as_ref() == MAGIC_ASSUME - || (call_result.reverter == Some(CHEATCODE_ADDRESS) - && SkipReason::decode(&call_result.result).is_some()) - { + if call_result.result.as_ref() == MAGIC_ASSUME || call_result.skip_reason().is_some() { continue; } // Coverage-collection asymmetry across calls within a stateful sequence: @@ -521,10 +514,7 @@ pub fn replay_sequence_for_minimization( tx.call_details.calldata.get(..4).map(Selector::from_slice).unwrap_or_default(); let fingerprint = snapshot_edge_fingerprint(&call_result); - if call_result.result.as_ref() == MAGIC_ASSUME - || (call_result.reverter == Some(CHEATCODE_ADDRESS) - && SkipReason::decode(&call_result.result).is_some()) - { + if call_result.result.as_ref() == MAGIC_ASSUME || call_result.skip_reason().is_some() { observation.skipped += 1; continue; } diff --git a/crates/forge/src/runner.rs b/crates/forge/src/runner.rs index 37e470b51f016..96919939ba9d9 100644 --- a/crates/forge/src/runner.rs +++ b/crates/forge/src/runner.rs @@ -37,7 +37,7 @@ use foundry_config::{ Config, FuzzConfig, FuzzCorpusConfig, FuzzDictionaryConfig, InlineConfig, InvariantConfig, }; use foundry_evm::{ - constants::{CALLER, CHEATCODE_ADDRESS, MAGIC_ASSUME}, + constants::{CALLER, MAGIC_ASSUME}, core::{backend::DatabaseExt, evm::FoundryEvmNetwork}, decode::{RevertDecoder, SkipReason}, executors::{ @@ -1745,9 +1745,7 @@ impl<'a, FEN: FoundryEvmNetwork> FunctionRunner<'a, FEN> { &self, raw_call_result: &RawCallResult, ) -> Result, String> { - if raw_call_result.reverter == Some(CHEATCODE_ADDRESS) - && let Some(reason) = SkipReason::decode(&raw_call_result.result) - { + if let Some(reason) = raw_call_result.skip_reason() { return Err(format!("vm.skip during concrete replay: {reason}")); } diff --git a/crates/forge/tests/cli/test_cmd/mod.rs b/crates/forge/tests/cli/test_cmd/mod.rs index a6c6c86fda5b2..a8bbf70551ff0 100644 --- a/crates/forge/tests/cli/test_cmd/mod.rs +++ b/crates/forge/tests/cli/test_cmd/mod.rs @@ -3352,6 +3352,178 @@ Ran 1 test suite [ELAPSED]: 0 tests passed, 0 failed, 1 skipped (1 total tests) "#]]); }); +// +forgetest_init!(skip_setup_after_caught_revert, |prj, cmd| { + prj.add_test( + "SkipAfterCaughtRevert.t.sol", + r#" +import "forge-std/Test.sol"; + +contract Reverter { + fallback() external { + revert("caught"); + } +} + +contract SkipAfterCaughtRevert is Test { + function setUp() public { + (bool success,) = address(new Reverter()).call(""); + require(!success); + vm.skip(true, "skip after caught revert"); + } + + function test_neverRuns() public pure {} +} + "#, + ); + + cmd.args(["test", "--isolate", "--mc", "SkipAfterCaughtRevert"]).assert_success().stdout_eq( + str![[r#" +[COMPILING_FILES] with [SOLC_VERSION] +[SOLC_VERSION] [ELAPSED] +Compiler run successful! + +Ran 1 test for test/SkipAfterCaughtRevert.t.sol:SkipAfterCaughtRevert +[SKIP: skipped: skip after caught revert] setUp() ([GAS]) +Suite result: ok. 0 passed; 0 failed; 1 skipped; [ELAPSED] + +Ran 1 test suite [ELAPSED]: 0 tests passed, 0 failed, 1 skipped (1 total tests) + +"#]], + ); +}); + +forgetest_init!(forged_skip_payload_fails_setup, |prj, cmd| { + prj.add_test( + "ForgedSkip.t.sol", + r#" +import "forge-std/Test.sol"; + +contract ForgedSkip is Test { + uint256 internal marker; + + function setUp() public { + marker = 1; + bytes memory reason = bytes("FOUNDRY::SKIPnot a real skip"); + assembly { + revert(add(reason, 32), mload(reason)) + } + } + + function test_neverRuns() public pure {} +} + "#, + ); + + cmd.args(["test", "--mc", "ForgedSkip"]).assert_failure().stdout_eq(str![[r#" +[COMPILING_FILES] with [SOLC_VERSION] +[SOLC_VERSION] [ELAPSED] +Compiler run successful! + +Ran 1 test for test/ForgedSkip.t.sol:ForgedSkip +[FAIL: FOUNDRY::SKIPnot a real skip] setUp() ([GAS]) +Suite result: FAILED. 0 passed; 1 failed; 0 skipped; [ELAPSED] + +Ran 1 test suite [ELAPSED]: 0 tests passed, 1 failed, 0 skipped (1 total tests) + +Failing tests: +Encountered 1 failing test in test/ForgedSkip.t.sol:ForgedSkip +[FAIL: FOUNDRY::SKIPnot a real skip] setUp() ([GAS]) + +Encountered a total of 1 failing tests, 0 tests succeeded + +Tip: Run `forge test --rerun` to retry only the 1 failed test +Tip: Run `forge test --debug --match-test ` to inspect one failing test in the debugger + +"#]]); +}); + +forgetest_init!(forged_skip_after_caught_skip_fails_setup, |prj, cmd| { + prj.add_test( + "ForgedSkipAfterCaughtSkip.t.sol", + r#" +import "forge-std/Test.sol"; + +contract ForgedSkipAfterCaughtSkip is Test { + function setUp() public { + // Catch a genuine skip so a payload is recorded, then revert with different skip bytes. + (bool success,) = address(vm).call( + abi.encodeWithSignature("skip(bool,string)", true, "genuine") + ); + require(!success); + + bytes memory reason = bytes("FOUNDRY::SKIPforged"); + assembly { + revert(add(reason, 32), mload(reason)) + } + } + + function test_neverRuns() public pure {} +} + "#, + ); + + cmd.args(["test", "--mc", "ForgedSkipAfterCaughtSkip"]).assert_failure().stdout_eq(str![[r#" +[COMPILING_FILES] with [SOLC_VERSION] +[SOLC_VERSION] [ELAPSED] +Compiler run successful! + +Ran 1 test for test/ForgedSkipAfterCaughtSkip.t.sol:ForgedSkipAfterCaughtSkip +[FAIL: FOUNDRY::SKIPforged] setUp() ([GAS]) +Suite result: FAILED. 0 passed; 1 failed; 0 skipped; [ELAPSED] + +Ran 1 test suite [ELAPSED]: 0 tests passed, 1 failed, 0 skipped (1 total tests) + +Failing tests: +Encountered 1 failing test in test/ForgedSkipAfterCaughtSkip.t.sol:ForgedSkipAfterCaughtSkip +[FAIL: FOUNDRY::SKIPforged] setUp() ([GAS]) + +Encountered a total of 1 failing tests, 0 tests succeeded + +Tip: Run `forge test --rerun` to retry only the 1 failed test +Tip: Run `forge test --debug --match-test ` to inspect one failing test in the debugger + +"#]]); +}); + +// A caught genuine skip that is re-raised byte-identically still counts as a skip: the payload +// provenance is byte equality with what the skip cheatcode minted, not the revert call chain. +forgetest_init!(caught_skip_reraised_identical_is_skipped, |prj, cmd| { + prj.add_test( + "ReraisedSkip.t.sol", + r#" +import "forge-std/Test.sol"; + +contract ReraisedSkip is Test { + function setUp() public { + (bool success, bytes memory data) = address(vm).call( + abi.encodeWithSignature("skip(bool,string)", true, "reraised") + ); + require(!success); + assembly { + revert(add(data, 32), mload(data)) + } + } + + function test_neverRuns() public pure {} +} + "#, + ); + + cmd.args(["test", "--mc", "ReraisedSkip"]).assert_success().stdout_eq(str![[r#" +[COMPILING_FILES] with [SOLC_VERSION] +[SOLC_VERSION] [ELAPSED] +Compiler run successful! + +Ran 1 test for test/ReraisedSkip.t.sol:ReraisedSkip +[SKIP: skipped: reraised] setUp() ([GAS]) +Suite result: ok. 0 passed; 0 failed; 1 skipped; [ELAPSED] + +Ran 1 test suite [ELAPSED]: 0 tests passed, 0 failed, 1 skipped (1 total tests) + +"#]]); +}); + forgetest_init!(should_generate_junit_xml_report, |prj, cmd| { prj.insert_ds_test(); prj.insert_vm(); diff --git a/crates/forge/tests/cli/test_cmd/repros.rs b/crates/forge/tests/cli/test_cmd/repros.rs index 30ff341ddf1ec..3c153101952a2 100644 --- a/crates/forge/tests/cli/test_cmd/repros.rs +++ b/crates/forge/tests/cli/test_cmd/repros.rs @@ -1102,3 +1102,53 @@ Ran 1 test suite [ELAPSED]: 2 tests passed, 0 failed, 0 skipped (2 total tests) "#]]); }); + +// https://github.com/foundry-rs/foundry/issues/16197 +forgetest_init!(issue_16197, |prj, cmd| { + prj.add_test( + "Issue16197.t.sol", + r#" +import "forge-std/Test.sol"; + +contract Deployment { + function ping() external pure { + revert("deployment probe"); + } +} + +// Mirrors the shape of the issue: an inherited base `setUp` performs substantial setup work +// whose internals catch a revert before the test's own `setUp` calls `vm.skip`. +contract CommonBase is Test { + Deployment internal deployment; + + function setUp() public virtual { + deployment = new Deployment(); + (bool success,) = address(deployment).call(abi.encodeWithSignature("ping()")); + require(!success, "probe call should revert"); + } +} + +contract Issue16197Test is CommonBase { + function setUp() public override { + super.setUp(); + vm.skip(true, "probe after super"); + } + + function test_probe_succeeds() public pure {} +} + "#, + ); + + cmd.args(["test", "--mc", "Issue16197Test"]).assert_success().stdout_eq(str![[r#" +[COMPILING_FILES] with [SOLC_VERSION] +[SOLC_VERSION] [ELAPSED] +Compiler run successful! + +Ran 1 test for test/Issue16197.t.sol:Issue16197Test +[SKIP: skipped: probe after super] setUp() ([GAS]) +Suite result: ok. 0 passed; 0 failed; 1 skipped; [ELAPSED] + +Ran 1 test suite [ELAPSED]: 0 tests passed, 0 failed, 1 skipped (1 total tests) + +"#]]); +});