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

Conversation

grandizzy
Copy link
Collaborator

@grandizzy grandizzy commented Mar 7, 2024

Motivation

closes #2551

Solution

  • use proptest FileFailurePersistence to create Fuzz TestRunner with (for invariant tests custom implementation is needed as we already replay and shrink the sequence, will be done with feat: fuzz corpus saving and replay in standard format #2552 )
  • default persistence file is ~/.foundry/cache/fuzz/failures
  • both dir and file are configurable to make switching between different working directories / files handy
  • output dir can be changed by setting failure_persist_dir in toml
  • files can be customized by passing --fuzz-input-file arg, by setting failure_persist_file in toml file or per test by using forge-config: default.fuzz.failure-persist-file inline config

TBD:
had to remove Copy from config as is required for file and dir options...
couple of tests failing in CI but passing locally

@grandizzy grandizzy marked this pull request as ready for review March 7, 2024 20:11
@grandizzy grandizzy changed the title feat(forge): add fuzz tests failure persistence feat(test): add fuzz tests failure persistence Mar 9, 2024
@@ -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

@@ -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

@klkvr
Copy link
Member

klkvr commented Mar 11, 2024

Shouldn't it be also possible to enable this for invariants by generating first call + seed for next calls by invariant strategy?

@mattsse mattsse requested a review from klkvr March 12, 2024 12:03
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

ty!

pending @klkvr

test failures should be fixed on master

@grandizzy
Copy link
Collaborator Author

Shouldn't it be also possible to enable this for invariants by generating first call + seed for next calls by invariant strategy?

Yeah, I got to the conclusion that the best way is to have #2552 implemented for invariants, reasons being

@mattsse mattsse merged commit 5fe9143 into foundry-rs:master Mar 12, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: fuzz failure persistence
4 participants