feat(fuzz): ability to declare fuzz test fixtures #7428

grandizzy · 2024-03-18T15:46:49Z

Motivation

Closes #3521
Proposed implementation for #735 (review) with fixtures declared in Solidity tests

As mentioned in #3521 the number of fuzzed edge cases is too high (only 29% unique), this is due to:

int and uint strategies fall back to generating edge cases in case there's no fixture to use, see

foundry/crates/evm/fuzz/src/strategies/uint.rs

Lines 114 to 118 in 0026488

    
           fn generate_fixtures_tree(&self, runner: &mut TestRunner) -> NewTree<Self> { 
        
               // generate edge cases if there's no fixtures 
        
               if self.fixtures.is_empty() { 
        
                   return self.generate_edge_tree(runner) 
        
               }

foundry/crates/evm/fuzz/src/strategies/int.rs

Lines 130 to 134 in 0026488

    
           fn generate_fixtures_tree(&self, runner: &mut TestRunner) -> NewTree<Self> { 
        
               // generate edge cases if there's no fixtures 
        
               if self.fixtures.is_empty() { 
        
                   return self.generate_edge_tree(runner) 
        
               }

defining fixtures is not supported (initialized as vec![]) so this gives a much higher weight for generating edge cases, see

foundry/crates/evm/fuzz/src/strategies/param.rs

Lines 36 to 38 in 0026488

    
           DynSolType::Int(n @ 8..=256) => { 
        
               super::IntStrategy::new(n, vec![]).prop_map(move |x| DynSolValue::Int(x, n)).boxed() 
        
           }

foundry/crates/evm/fuzz/src/strategies/param.rs

Lines 39 to 41 in 0026488

    
           DynSolType::Uint(n @ 8..=256) => { 
        
               super::UintStrategy::new(n, vec![]).prop_map(move |x| DynSolValue::Uint(x, n)).boxed() 
        
           }

Solution

Proposed solution:

fall back to random values in case there's no fixtures defined
introduce concept and ability to programmatically define fixtures for named parameters in Solidity tests by declaring them as functions or storage arrays (replaces solution provided for Invariant violation not found on simple 2 step owner transfer test #5868 in PR feat invariant (#5868) - configure calldata fuzzed addresses dictionary #7240 too)

Fixtures can be defined as array of values to be used in fuzzed tests (support for address, address, uint, int and bytes types) by

declaring a function prefixed with fixture and followed by param name to be fuzzed. Function should return an (fixed size or dynamic) array of values to be used for fuzzing. For example, fixtures to be used when fuzzing parameter named owner of type address can be defined in Solidity test as

function fixtureOwner() public returns (address[] memory)

declaring a storage array prefixed with fixture and followed by param name to be fuzzed. For example, fixtures to be used when fuzzing parameter named amount of type uint32 can be defined as

uint32[] public fixtureAmount = [1122334455, 1, 555];

test fixtures can be defined as both storage arrays and functions at the same time
if the return type is not an array or is not same type as the named parameter type then an error is raised and fuzzed is performed with random values

Fixtures with `setUp` sample:

To define fixtures for owner fuzzed parameter of type address

address[] ownerFixture;

function setUp() public {
    ownerFixture.push(address(1));
    ownerFixture.push(address(2));
    ownerFixture.push(address(3));
}

function fixtureOwner() public returns (address[] memory) {
    return ownerFixture;
}

write fuzzed tests by using owner param name so all defined fixtures are used when fuzzing

function testFuzz_Address(address owner) public {
}

if a function returning different fixture type is declared, e.g.

function fixtureOwner() public returns (uint[] memory) {
}

then fuzzer will panic

Fixtures without `setUp` sample:

int256[] nosetupFixture;

function fixture_nosetup() public returns (int256[] memory) {
    int256[] memory nosetupFixture = new int256[](2);
    nosetupFixture[0] = -78787;
    nosetupFixture[1] = 77777;
    return nosetupFixture;
}

function testFuzz_NoSetup(int256 nosetup) public {
}

Fixtures from storage arrays:

contract FuzzTest is Test {
    bytes32[] public fixtureKey = [bytes32("abcd1234")];

    function testFuzzKey(bytes32 _key) public {
    }
}

Fixtures as fixed sized arrays:

 function fixtureMagic() external returns (int32[3] memory) {
    int32[3] memory magic;
    magic[0] = 1;
    magic[1] = -777;
    magic[2] = -2;
    return magic;
}

Some numbers collected from running without and with fixtures for a uint fuzzed param:

Run (with fixtures)	edge	random	fixtures
1	11.4%	45.2%	43.24%
2	12.5%	51.19%	36.3%
3	9.86%	52.63%	37.5%
4	12.92%	43.53%	43.53%

Run (no fixtures)	edge	random
1	13.7%	86.3%
2	9.5%	90.5%
3	8.2%	91.8%
4	8.7%	91.3%

mattsse

smol nits,
ptal @DaniPopes @klkvr

mattsse · 2024-03-20T11:28:20Z

crates/evm/fuzz/src/lib.rs

+        Self { inner: Arc::new(fixtures) }
+    }
+
+    pub fn param_fixtures(&self, param_name: &String) -> Option<&[DynSolValue]> {


needs oneline doc

mattsse · 2024-03-20T11:28:39Z

crates/evm/fuzz/src/strategies/address.rs

+/// `function fixture_owner() public returns (address[] memory)`.
+/// Use `owner` named parameter in fuzzed test in order to create a custom strategy
+/// `function testFuzz_ownerAddress(address owner, uint amount)`.
+#[derive(Debug)]


Suggested change

#[derive(Debug)]

#[derive(Debug, Default)]

#[non_exhaustive]

DaniPopes · 2024-03-21T05:38:15Z

crates/forge/bin/cmd/test/mod.rs

@@ -221,7 +221,7 @@ impl TestArgs {
            *test_pattern = Some(debug_test_pattern.clone());
        }

-        let outcome = self.run_tests(runner, config, verbosity, &filter).await?;
+        let outcome = self.run_tests(runner, config.clone(), verbosity, &filter).await?;


These clones are not necessary

DaniPopes · 2024-03-21T05:38:37Z

crates/forge/bin/cmd/test/mod.rs

@@ -181,7 +181,7 @@ impl TestArgs {

        let test_options: TestOptions = TestOptionsBuilder::default()
            .fuzz(config.clone().fuzz)
-            .invariant(config.invariant)
+            .invariant(config.clone().invariant)


move clone after .invariant to clone only that object instead of the entire config

same above for fuzz

DaniPopes · 2024-03-21T05:43:18Z

crates/forge/src/runner.rs

+    fn fuzz_fixtures(&mut self, address: Address) -> FuzzFixtures {
+        // collect test fixtures param:array of values
+        let mut fixtures = HashMap::new();
+        self.contract.functions().for_each(|func| {


use a for loop with .filter(|f| f.name.is_fixture())

DaniPopes · 2024-03-21T05:43:41Z

crates/forge/src/runner.rs

+                    self.executor.call(CALLER, address, func, &[], U256::ZERO, None)
+                {
+                    fixtures.insert(
+                        func.name.strip_prefix("fixture_").unwrap().to_string(),


we should check if there's a function with the stripped name and issue a warning

DaniPopes · 2024-03-21T05:45:22Z

crates/forge/src/runner.rs

+                {
+                    fixtures.insert(
+                        func.name.strip_prefix("fixture_").unwrap().to_string(),
+                        decoded_result,


we should check for validity of the decoded result here instead of later when running the tests since all necessary info to do this is available here

that would be nice to have but needs special handling of handler based testing (where fuzzed functions are not in same contract that defines setUp/invariant_/fixture_ functions). It can be done but it adds code complexity to look up such info in all contracts created when setUp, wdyt?

…ures

grandizzy · 2024-03-21T06:59:35Z

I addressed comments (but #7428 (comment)) and additionally moved fuzz_calldata_from_state from state mod in same calldata mod as fuzz_calldata, think it makes more sense to have grouped them like this (more code refactoring could be done as these functions are similar). Please recheck

…lidity test

grandizzy · 2024-03-25T14:00:40Z

hey @mattsse @DaniPopes I added fixtures for string and bytes too and a Solidity test to cover all combinations, pls have a look when you get time. thank you

DaniPopes · 2024-03-22T20:21:00Z

crates/forge/bin/cmd/test/mod.rs

@@ -202,7 +202,7 @@ impl TestArgs {
        let runner = MultiContractRunnerBuilder::default()
            .set_debug(should_debug)
            .initial_balance(evm_opts.initial_balance)
-            .evm_spec(config.evm_spec_id())
+            .evm_spec(config.clone().evm_spec_id())


Please remove this clone

sorry, missed this comment, removed with a5e8da0

grandizzy · 2024-04-11T05:24:37Z

sorry, just got a chance to read through, looking great! a couple questions
unfortunately Solidity does not offer great UX for dealing with arrays, so I think we should also consider adding support for reading fixtures from storage arrays:
contract Test {
    uint256[] public fixureName = [1, 2, 3];
}
vs
contract Test {
    function fixureName() public view returns(uint256[] memory vals) {
        vals = new uint256[](3);
        vals[0] = 1;
        vals[1] = 2;
        vals[2] = 3;
    }
}
yep, good call, I think this way we could even drop the inline config and do the best effort to match / map fixtures from functions within test contract, will look into adding such

@klkvr I added support for reading fixtures from storage and dropped the inline fixtures config with fb86084 Didn't find a nicer way to populate fixtures from storage array but to call function with incremented indexes, pls lmk if you see a nicer way for doing this. see
fb86084#diff-98c09a5d1cb595f37109d7b1c0109ee32dd74dc1d253cdda9a63e4ce32bf49d5R227-R251

…ot of param type

…rent type. Keep level of randomness in fixture strategy, at par with uint / int strategies.

klkvr

@grandizzy we shouldn't just panic on user errors, I meant that we should also have validation somewhere which will throw a error, thus making panic more safe

we can probably either do another pass through ABIs before running tests to check if there are any mismatches between fixture types and parameter types or validate this when generating strategies.

klkvr · 2024-04-17T16:20:16Z

crates/forge/src/runner.rs

+    /// `function fixture_owner() public returns (address[] memory){}`
+    /// returns an array of addresses to be used for fuzzing `owner` named parameter in scope of the
+    /// current test.
+    fn fuzz_fixtures(&mut self, address: Address) -> FuzzFixtures {


I think we can just have this check here

grandizzy · 2024-04-17T17:06:21Z

@grandizzy we shouldn't just panic on user errors, I meant that we should also have validation somewhere which will throw a error, thus making panic more safe

we can probably either do another pass through ABIs before running tests to check if there are any mismatches between fixture types and parameter types or validate this when generating strategies.

@klkvr if I am not missing something a fully validation when collecting fixtures from invariant contract isn't possible as there could be targets created during runs that we don't have at the moment test is set up. Even if we manage to do the validation then the option would be to panic if params not of same types (otherwise it will still panic later in the run). If my rationale is correct what do you think if we just use some default values in case fixture of different type (like 0 for uint, adrress(0), empty string, etc.) and raise errors instead panic, they are quite noisy in the console and hard to be missed, so dev will know fixtures needs to be amended. thanks!

klkvr · 2024-04-17T20:39:29Z

@klkvr if I am not missing something a fully validation when collecting fixtures from invariant contract isn't possible as there could be targets created during runs that we don't have at the moment test is set up

hmm, I see. what if we do prop_filter_map and reject such values thus forcing fallback to default strategy? also display a error/warning to user

… for fixture strategy and raise error

grandizzy · 2024-04-18T07:14:03Z

@klkvr if I am not missing something a fully validation when collecting fixtures from invariant contract isn't possible as there could be targets created during runs that we don't have at the moment test is set up

hmm, I see. what if we do prop_filter_map and reject such values thus forcing fallback to default strategy? also display a error/warning to user

oh, awesome stuff, this should handle all cases, added in 997c5d4

klkvr · 2024-04-18T10:56:51Z

crates/evm/fuzz/src/strategies/mod.rs

+            proptest::prop_oneof![
+                50 => {
+                    let custom_fixtures: Vec<DynSolValue> =
+                        fixtures.iter().enumerate().map(|(_, value)| value.to_owned()).collect();
+                    let custom_fixtures_len = custom_fixtures.len();
+                    any::<prop::sample::Index>()
+                        .prop_filter_map("invalid fixture", move |index| {
+                            let index = index.index(custom_fixtures_len);
+                            $strategy_value(custom_fixtures.get(index))
+                    })
+                },
+                50 => $default_strategy
+            ].boxed()


shouldn't we apply prop_filter_map to the entire prop_oneof! output? I believe right now if we only have invalid fixtures it will fail with max rejects while it'd be better if it used default strategy instead

was thinking that could be better to fail (faster) with max rejects rather than run tests with a config that is not really desired, but can change to apply to both strategies, wdyt?

right, let's keep it like this for now

klkvr

LGTM! nice work

@mattsse @DaniPopes mind taking a look?

mattsse

nice!

fix(fuzz): deduplicate fuzz inputs

ef15bab

mattsse assigned DaniPopes Mar 18, 2024

grandizzy added 2 commits March 19, 2024 12:08

Fix tests, collect fixtures in test setup, arc fixtures

3c48666

Merge remote-tracking branch 'origin' into issue-3521-fixtures

e1f8a95

grandizzy force-pushed the issue-3521-fixtures branch from 2398972 to e1f8a95 Compare March 19, 2024 11:05

grandizzy changed the title ~~[WIP] fix(fuzz): deduplicate fuzz inputs~~ [WIP] feat(fuzz): ability to declare fuzz test fixtures Mar 19, 2024

grandizzy force-pushed the issue-3521-fixtures branch 2 times, most recently from 12c4e0c to 5b92ee1 Compare March 19, 2024 12:07

Cleanup

254017c

grandizzy force-pushed the issue-3521-fixtures branch from 5b92ee1 to 254017c Compare March 19, 2024 14:52

grandizzy added 3 commits March 20, 2024 08:14

Merge remote-tracking branch 'origin' into issue-3521-fixtures

8586bf4

Use fixture_ prefix

14c869a

Update tests to reflect that random values are used if no fixtures

d0442de

grandizzy changed the title ~~[WIP] feat(fuzz): ability to declare fuzz test fixtures~~ feat(fuzz): ability to declare fuzz test fixtures Mar 20, 2024

grandizzy marked this pull request as ready for review March 20, 2024 10:33

grandizzy requested review from DaniPopes and mattsse as code owners March 20, 2024 10:33

mattsse requested changes Mar 20, 2024

View reviewed changes

Review changes

7e4b14c

DaniPopes requested changes Mar 21, 2024

View reviewed changes

grandizzy added 2 commits March 21, 2024 07:57

Group fuzz_calldata and fuzz_calldata_from_state in calldata mod

972d993

Review changes: remove unnecessary clones, nicer code to collect fixt…

708e4db

…ures

grandizzy requested review from DaniPopes and mattsse March 21, 2024 06:59

grandizzy added 2 commits March 22, 2024 17:30

Merge remote-tracking branch 'origin' into issue-3521-fixtures

f93254a

Add support for bytes and string fixtures, fixture strategy macro. So…

9276893

…lidity test

grandizzy force-pushed the issue-3521-fixtures branch from 69e8c51 to 9276893 Compare March 25, 2024 13:56

DaniPopes reviewed Mar 25, 2024

View reviewed changes

Support fixed size arrays fixtures

ca4d44b

grandizzy changed the title ~~feat(fuzz): ability to declare fuzz test fixtures~~ WIP feat(fuzz): ability to declare fuzz test fixtures Apr 11, 2024

grandizzy marked this pull request as draft April 11, 2024 05:15

grandizzy added 5 commits April 11, 2024 09:10

Update comment

acdf92d

Use DynSolValue::type_strategy for address and fixed bytes fuzzed params

12115fa

Add prefix to mark a storage array or a function as fixture

e76fe8e

Fix test

3ac9e33

Simplify code / fixture strategy macro, panic if configured fixture n…

7e95208

…ot of param type

grandizzy marked this pull request as ready for review April 11, 2024 17:04

grandizzy changed the title ~~WIP feat(fuzz): ability to declare fuzz test fixtures~~ feat(fuzz): ability to declare fuzz test fixtures Apr 11, 2024

grandizzy added 2 commits April 13, 2024 15:27

Consistent panic with fixture strategy if uint / int fixture of diffe…

4cf19cb

…rent type. Keep level of randomness in fixture strategy, at par with uint / int strategies.

Merge remote-tracking branch 'origin' into issue-3521-fixtures

53c64d3

klkvr requested changes Apr 17, 2024

View reviewed changes

klkvr reviewed Apr 17, 2024

View reviewed changes

grandizzy added 2 commits April 18, 2024 09:25

Review changes: don't panic when invalid fixture, use prop_filter_map…

997c5d4

… for fixture strategy and raise error

Merge remote-tracking branch 'origin' into issue-3521-fixtures

a127780

grandizzy requested a review from klkvr April 18, 2024 07:14

klkvr reviewed Apr 18, 2024

View reviewed changes

klkvr approved these changes Apr 18, 2024

View reviewed changes

mattsse approved these changes Apr 22, 2024

View reviewed changes

DaniPopes approved these changes Apr 22, 2024

View reviewed changes

mattsse merged commit 008922d into foundry-rs:master Apr 22, 2024
19 checks passed

This was referenced Apr 26, 2024

Reset to upstream phylaxsystems/phoundry#12

Merged

feat/forge bin as lib phylaxsystems/phoundry#13

Merged

zerosnacks mentioned this pull request Jun 26, 2024

Fuzz Test Snapshotting Concept #1085

Closed

mds1 mentioned this pull request Jul 24, 2024

feat: deduplicate fuzz inputs #3521

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(fuzz): ability to declare fuzz test fixtures #7428

feat(fuzz): ability to declare fuzz test fixtures #7428

grandizzy commented Mar 18, 2024 •

edited

Loading

mattsse left a comment

mattsse Mar 20, 2024

mattsse Mar 20, 2024

DaniPopes Mar 21, 2024

DaniPopes Mar 21, 2024

DaniPopes Mar 21, 2024

DaniPopes Mar 21, 2024

DaniPopes Mar 21, 2024

grandizzy Mar 21, 2024

grandizzy commented Mar 21, 2024 •

edited

Loading

grandizzy commented Mar 25, 2024

DaniPopes Mar 22, 2024

grandizzy Mar 26, 2024

grandizzy commented Apr 11, 2024

klkvr left a comment •

edited

Loading

klkvr Apr 17, 2024

grandizzy commented Apr 17, 2024

klkvr commented Apr 17, 2024 •

edited

Loading

grandizzy commented Apr 18, 2024

klkvr Apr 18, 2024 •

edited

Loading

grandizzy Apr 18, 2024

klkvr Apr 18, 2024

klkvr left a comment •

edited

Loading

mattsse left a comment

	fn generate_fixtures_tree(&self, runner: &mut TestRunner) -> NewTree<Self> {
	// generate edge cases if there's no fixtures
	if self.fixtures.is_empty() {
	return self.generate_edge_tree(runner)
	}

	DynSolType::Int(n @ 8..=256) => {
	super::IntStrategy::new(n, vec![]).prop_map(move \|x\| DynSolValue::Int(x, n)).boxed()
	}

	DynSolType::Uint(n @ 8..=256) => {
	super::UintStrategy::new(n, vec![]).prop_map(move \|x\| DynSolValue::Uint(x, n)).boxed()
	}

feat(fuzz): ability to declare fuzz test fixtures #7428

feat(fuzz): ability to declare fuzz test fixtures #7428

Conversation

grandizzy commented Mar 18, 2024 • edited Loading

Motivation

Solution

Fixtures with setUp sample:

Fixtures without setUp sample:

Fixtures from storage arrays:

Fixtures as fixed sized arrays:

mattsse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grandizzy commented Mar 21, 2024 • edited Loading

grandizzy commented Mar 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grandizzy commented Apr 11, 2024

klkvr left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grandizzy commented Apr 17, 2024

klkvr commented Apr 17, 2024 • edited Loading

grandizzy commented Apr 18, 2024

klkvr Apr 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klkvr left a comment • edited Loading

Choose a reason for hiding this comment

mattsse left a comment

Choose a reason for hiding this comment

grandizzy commented Mar 18, 2024 •

edited

Loading

Fixtures with `setUp` sample:

Fixtures without `setUp` sample:

grandizzy commented Mar 21, 2024 •

edited

Loading

klkvr left a comment •

edited

Loading

klkvr commented Apr 17, 2024 •

edited

Loading

klkvr Apr 18, 2024 •

edited

Loading

klkvr left a comment •

edited

Loading