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.

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(test): add fuzz tests failure persistence #7336

Merged
merged 7 commits into from
Mar 12, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
22 changes: 17 additions & 5 deletions crates/config/src/fuzz.rs
Original file line number Diff line number Diff line change
@@ -1,13 +1,17 @@
//! Configuration for fuzz testing.

use crate::inline::{
parse_config_u32, InlineConfigParser, InlineConfigParserError, INLINE_CONFIG_FUZZ_KEY,
use crate::{
inline::{
parse_config_u32, InlineConfigParser, InlineConfigParserError, INLINE_CONFIG_FUZZ_KEY,
},
Config,
};
use alloy_primitives::U256;
use serde::{Deserialize, Serialize};
use std::path::PathBuf;

/// Contains for fuzz testing
#[derive(Clone, Copy, Debug, PartialEq, Eq, Serialize, Deserialize)]
#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)]
pub struct FuzzConfig {
/// The number of test cases that must execute for each property test
pub runs: u32,
Expand All @@ -22,6 +26,10 @@ pub struct FuzzConfig {
/// The fuzz dictionary configuration
#[serde(flatten)]
pub dictionary: FuzzDictionaryConfig,
/// Path where fuzz failures are recorded and replayed, defaults to `~/.foundry/cache/fuzz`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we persist the data in the project's local ./cache folder instead? I haven't looked at exactly what's in the file, but if the fuzz data is unique to the project we probably should use the local cache. cc @klkvr

Copy link
Member

@klkvr klkvr Mar 11, 2024

Choose a reason for hiding this comment

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

Yeah, this should definitely be somewhere in project's root. Let's write to ./{cache_path}/fuzz by default.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh, right, I pushed a commit to write in project root dir

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should we persist the data in the project's local ./cache folder instead? I haven't looked at exactly what's in the file, but if the fuzz data is unique to the project we probably should use the local cache. cc @klkvr

yeah, the fuzz data is unique per fuzzed test

pub failure_persist_dir: PathBuf,
/// Name of the file to record fuzz failures, defaults to `failures`
pub failure_persist_file: String,
}

impl Default for FuzzConfig {
Expand All @@ -31,6 +39,8 @@ impl Default for FuzzConfig {
max_test_rejects: 65536,
seed: None,
dictionary: FuzzDictionaryConfig::default(),
failure_persist_dir: Config::foundry_fuzz_cache_dir().unwrap(),
failure_persist_file: "failures".to_string(),
}
}
}
Expand All @@ -47,8 +57,7 @@ impl InlineConfigParser for FuzzConfig {
return Ok(None)
}

// self is Copy. We clone it with dereference.
let mut conf_clone = *self;
let mut conf_clone = self.clone();

for pair in overrides {
let key = pair.0;
Expand All @@ -59,6 +68,7 @@ impl InlineConfigParser for FuzzConfig {
"dictionary-weight" => {
conf_clone.dictionary.dictionary_weight = parse_config_u32(key, value)?
}
"failure-persist-file" => conf_clone.failure_persist_file = value,
_ => Err(InlineConfigParserError::InvalidConfigProperty(key))?,
}
}
Expand Down Expand Up @@ -127,11 +137,13 @@ mod tests {
let configs = &[
"forge-config: default.fuzz.runs = 42424242".to_string(),
"forge-config: default.fuzz.dictionary-weight = 42".to_string(),
"forge-config: default.fuzz.failure-persist-file = fuzz-failure".to_string(),
Copy link
Collaborator

@mds1 mds1 Mar 11, 2024

Choose a reason for hiding this comment

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

Am I correct that this means that by default, each run overwrites the previous? If so I think that's good, to prevent the cache from silently getting too large. We should also make sure an env var args are supported so it's easy to set a one-off name or one-off input file without modifying the foundry.toml

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Re file persistence:
when Proptest finds a failing test case it writes the case in file and then subsequent runs will first replay those test cases before generating new inputs. So the file is not overwritten and that ensures there's no regression.
Re file size - I don't think this can grow too much considering that

  • once a test fails you have to fix the case in order to get new inputs and possible failures / new lines written in persisted file
  • one can use the inline config to redirect failure cases in different files

Re env var: there's FOUNDRY_FUZZ_FAILURE_PERSIST_FILE that can be used to specify the input file

];
let base_config = FuzzConfig::default();
let merged: FuzzConfig = base_config.try_merge(configs).expect("No errors").unwrap();
assert_eq!(merged.runs, 42424242);
assert_eq!(merged.dictionary.dictionary_weight, 42);
assert_eq!(merged.failure_persist_file, "fuzz-failure".to_string());
}

#[test]
Expand Down
5 changes: 5 additions & 0 deletions crates/config/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1349,6 +1349,11 @@ impl Config {
Some(Self::foundry_cache_dir()?.join("etherscan"))
}

/// Returns the path to foundry fuzz cache dir `~/.foundry/cache/fuzz`
pub fn foundry_fuzz_cache_dir() -> Option<PathBuf> {
Some(Self::foundry_cache_dir()?.join("fuzz"))
}

/// Returns the path to foundry's keystores dir `~/.foundry/keystores`
pub fn foundry_keystores_dir() -> Option<PathBuf> {
Some(Self::foundry_dir()?.join("keystores"))
Expand Down
9 changes: 8 additions & 1 deletion crates/forge/bin/cmd/test/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,10 @@ pub struct TestArgs {
#[arg(long, env = "FOUNDRY_FUZZ_RUNS", value_name = "RUNS")]
pub fuzz_runs: Option<u64>,

/// File to rerun fuzz failures from.
#[arg(long)]
pub fuzz_input_file: Option<String>,

#[command(flatten)]
filter: FilterArgs,

Expand Down Expand Up @@ -174,7 +178,7 @@ impl TestArgs {
let profiles = get_available_profiles(toml)?;

let test_options: TestOptions = TestOptionsBuilder::default()
.fuzz(config.fuzz)
.fuzz(config.clone().fuzz)
.invariant(config.invariant)
.profiles(profiles)
.build(&output, project_root)?;
Expand Down Expand Up @@ -492,6 +496,9 @@ impl Provider for TestArgs {
if let Some(fuzz_runs) = self.fuzz_runs {
fuzz_dict.insert("runs".to_string(), fuzz_runs.into());
}
if let Some(fuzz_input_file) = self.fuzz_input_file.clone() {
fuzz_dict.insert("failure_persist_file".to_string(), fuzz_input_file.into());
}
dict.insert("fuzz".to_string(), fuzz_dict.into());

if let Some(etherscan_api_key) =
Expand Down
28 changes: 21 additions & 7 deletions crates/forge/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,9 @@ use foundry_config::{
validate_profiles, Config, FuzzConfig, InlineConfig, InlineConfigError, InlineConfigParser,
InvariantConfig, NatSpec,
};
use proptest::test_runner::{RngAlgorithm, TestRng, TestRunner};
use proptest::test_runner::{
FailurePersistence, FileFailurePersistence, RngAlgorithm, TestRng, TestRunner,
};
use std::path::Path;

pub mod coverage;
Expand Down Expand Up @@ -93,8 +95,17 @@ impl TestOptions {
where
S: Into<String>,
{
let fuzz = self.fuzz_config(contract_id, test_fn);
self.fuzzer_with_cases(fuzz.runs)
let fuzz_config = self.fuzz_config(contract_id, test_fn).clone();
let failure_persist_path = fuzz_config
.failure_persist_dir
.join(fuzz_config.failure_persist_file)
.into_os_string()
.into_string()
.unwrap();
self.fuzzer_with_cases(
fuzz_config.runs,
Some(Box::new(FileFailurePersistence::Direct(failure_persist_path.leak()))),
)
}

/// Returns an "invariant" test runner instance. Parameters are used to select tight scoped fuzz
Expand All @@ -109,7 +120,7 @@ impl TestOptions {
S: Into<String>,
{
let invariant = self.invariant_config(contract_id, test_fn);
self.fuzzer_with_cases(invariant.runs)
self.fuzzer_with_cases(invariant.runs, None)
}

/// Returns a "fuzz" configuration setup. Parameters are used to select tight scoped fuzz
Expand Down Expand Up @@ -140,10 +151,13 @@ impl TestOptions {
self.inline_invariant.get(contract_id, test_fn).unwrap_or(&self.invariant)
}

pub fn fuzzer_with_cases(&self, cases: u32) -> TestRunner {
// TODO: Add Options to modify the persistence
pub fn fuzzer_with_cases(
&self,
cases: u32,
file_failure_persistence: Option<Box<dyn FailurePersistence>>,
) -> TestRunner {
let config = proptest::test_runner::Config {
failure_persistence: None,
failure_persistence: file_failure_persistence,
cases,
max_global_rejects: self.fuzz.max_test_rejects,
..Default::default()
Expand Down
10 changes: 7 additions & 3 deletions crates/forge/src/runner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,7 @@ impl<'a> ContractRunner<'a> {
debug_assert!(func.is_test());
let runner = test_options.fuzz_runner(self.name, &func.name);
let fuzz_config = test_options.fuzz_config(self.name, &func.name);
self.run_fuzz_test(func, should_fail, runner, setup, *fuzz_config)
self.run_fuzz_test(func, should_fail, runner, setup, fuzz_config.clone())
} else {
debug_assert!(func.is_test());
self.run_test(func, should_fail, setup)
Expand Down Expand Up @@ -601,8 +601,12 @@ impl<'a> ContractRunner<'a> {

// Run fuzz test
let start = Instant::now();
let fuzzed_executor =
FuzzedExecutor::new(self.executor.clone(), runner.clone(), self.sender, fuzz_config);
let fuzzed_executor = FuzzedExecutor::new(
self.executor.clone(),
runner.clone(),
self.sender,
fuzz_config.clone(),
);
let state = fuzzed_executor.build_fuzz_state();
let result = fuzzed_executor.fuzz(func, address, should_fail, self.revert_decoder);

Expand Down
57 changes: 54 additions & 3 deletions crates/forge/tests/it/fuzz.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
//! Fuzz tests.

use crate::config::*;
use alloy_primitives::U256;
use std::collections::BTreeMap;

use alloy_primitives::{Bytes, U256};
use forge::fuzz::CounterExample;

use forge::result::{SuiteResult, TestStatus};
use foundry_test_utils::Filter;
use std::collections::BTreeMap;

use crate::config::*;

#[tokio::test(flavor = "multi_thread")]
async fn test_fuzz() {
Expand Down Expand Up @@ -103,3 +107,50 @@ async fn test_fuzz_collection() {
)]),
);
}

#[tokio::test(flavor = "multi_thread")]
async fn test_persist_fuzz_failure() {
let filter = Filter::new(".*", ".*", ".*fuzz/FuzzFailurePersist.t.sol");
let mut runner = runner();
runner.test_options.fuzz.runs = 1000;

macro_rules! get_failure_result {
() => {
runner
.test_collect(&filter)
.get("fuzz/FuzzFailurePersist.t.sol:FuzzFailurePersistTest")
.unwrap()
.test_results
.get("test_persist_fuzzed_failure(uint256,int256,address,bool,string,(address,uint256),address[])")
.unwrap()
.counterexample
.clone()
};
}

// record initial counterexample calldata
let intial_counterexample = get_failure_result!();
let initial_calldata = match intial_counterexample {
Some(CounterExample::Single(counterexample)) => counterexample.calldata,
_ => Bytes::new(),
};

// run several times and compare counterexamples calldata
for _ in 0..10 {
let new_calldata = match get_failure_result!() {
Some(CounterExample::Single(counterexample)) => counterexample.calldata,
_ => Bytes::new(),
};
// calldata should be the same with the initial one
assert_eq!(initial_calldata, new_calldata);
}

// write new failure in different file
runner.test_options.fuzz.failure_persist_file = "failure1".to_string();
let new_calldata = match get_failure_result!() {
Some(CounterExample::Single(counterexample)) => counterexample.calldata,
_ => Bytes::new(),
};
// empty file is used to load failure so new calldata is generated
assert_ne!(initial_calldata, new_calldata);
}
4 changes: 3 additions & 1 deletion crates/forge/tests/it/test_helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,8 @@ pub static TEST_OPTS: Lazy<TestOptions> = Lazy::new(|| {
max_fuzz_dictionary_values: 10_000,
max_calldata_fuzz_dictionary_addresses: 0,
},
failure_persist_dir: tempfile::tempdir().unwrap().into_path(),
failure_persist_file: "testfailure".to_string(),
})
.invariant(InvariantConfig {
runs: 256,
Expand Down Expand Up @@ -124,6 +126,6 @@ pub fn fuzz_executor<DB: DatabaseRef>(executor: Executor) -> FuzzedExecutor {
executor,
proptest::test_runner::TestRunner::new(cfg),
CALLER,
TEST_OPTS.fuzz,
TEST_OPTS.fuzz.clone(),
)
}
29 changes: 29 additions & 0 deletions testdata/fuzz/FuzzFailurePersist.t.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
// SPDX-License-Identifier: MIT OR Apache-2.0
pragma solidity 0.8.18;

import "ds-test/test.sol";
import "../cheats/Vm.sol";

struct TestTuple {
address user;
uint256 amount;
}

contract FuzzFailurePersistTest is DSTest {
Vm vm = Vm(HEVM_ADDRESS);

function test_persist_fuzzed_failure(
uint256 x,
int256 y,
address addr,
bool cond,
string calldata test,
TestTuple calldata tuple,
address[] calldata addresses
) public {
// dummy assume to trigger runs
vm.assume(x > 1 && x < 1111111111111111111111111111);
vm.assume(y > 1 && y < 1111111111111111111111111111);
require(false);
}
}
Loading