feat: ux fuzz invariant #4744

0xMelkor · 2023-04-14T19:23:54Z

Motivation

Provide a solution for #3062, #4085 and updates #3411.

Some tests need more/less fuzzing than others, so it would be great to configure this on a per-test basis.

As described in the issue we are going to support Solidity comments as a new configuration source.

contract MyTest is Test {
  /// forge-config: default.fuzz.runs = 100
  /// forge-config: ci.fuzz.runs = 500
  function test_SimpleFuzzTest(uint256 x) public {
    // --- snip ---
  }

  /// forge-config: default.fuzz.runs = 500
  /// forge-config: ci.fuzz.runs = 10000
  function test_ImportantFuzzTest(uint256 x) public {
    // --- snip ---
  }
}

Solution

Tasks

Create a parser able to convert structured text into configuration structs (FuzzConfig, InvariantConfig)
Parser should take care of the current execution profile (ci, default, ecc.)
Use ProjectCompileOutput to extract inline per-test configs.
Provide new configs to the test runner extending the behavior of TestOptions.
Override project config at runtime if required by the specific test

NOTE:
For the sake of KISS I used a standalone parser (ConfParser) instead of a figment::Provider. If you prefer the second approach just tell me.

Cheers

Unit tests

…d text Unit tests

Unit tests

Meaningful e2e test

mattsse

really appreciate this.

before we proceed with this, we should discuss how exactly this would look like in the toml file.

could you please add same examples so we can discuss,

cc @mds1

0xMelkor · 2023-04-16T15:51:07Z

could you please add same examples so we can discuss

Hi @mattsse,

This works inlining config comments in solidity test functions, to override wider scoped toml params.

E2E tests can help to understand:

Annotated sol tests FuzzInlineConf.t.sol, InvariantInlineConf.t.sol;
E2E tests show how to extract FuzzConfig and InvariantConfig from compiled inline comments;

Let's discuss this.

mds1 · 2023-04-16T21:54:48Z

before we proceed with this, we should discuss how exactly this would look like in the toml file.

@mattsse I don't think we should need any config file changes here. Like @0xMelkor said, the idea is that the toml file's profiles would give the default config, and you can override the config at the file, contract, or test level using comments like this

Hey @0xMelkor, should this work yet? I setup a new project and modified the default tests to look like this:

pragma solidity ^0.8.13;

import "forge-std/Test.sol";
import "../src/Counter.sol";

contract CounterTest is Test {
    Counter public counter;

    function setUp() public {
        counter = new Counter();
        counter.setNumber(0);
    }

    function testSetNumberNoChange(uint256 x) public {
        counter.setNumber(x);
        assertEq(counter.number(), x);
    }

    // forge-config: default.fuzz.runs = 111
    // forge-config: ci.fuzz.runs = 222
    function testSetNumberCustom(uint256 x) public {
        counter.setNumber(x);
        assertEq(counter.number(), x);
    }
}

And the config file looks like this:

[profile.ci]
  fuzz = { runs = 1000 }

But when I run this branch, testSetNumberCustom and testSetNumberNoChange always have the same number of fuzz runs. I tried using both 2 and 3 leading slashes for the comment, with the same result

0xMelkor · 2023-04-16T22:18:37Z

Hey @0xMelkor, should this work yet?

Hi @mds1,
For now we can load the configs but the execution part is not yet integrated.

If you agree with the proposed design I would continue with the execution part.
Just let me know.

Thank you guys

mds1 · 2023-04-16T22:48:01Z

Ah got it, sorry didn't realize! This design lgtm, cc @mattsse @PaulRBerg @PatrickAlphaC for any final thoughts on UX here.

Per the discussion in #3062 I think one important part is throwing an error if we see a comment that starts with forge-config that we're not able to parse properly, to ensure you don't accidentally test with the wrong config due a typo

PaulRBerg · 2023-04-17T07:35:32Z

The proposed design looks great, thanks for picking up this task @0xMelkor!

I agree with @mds1 that it would be helpful to throw an error when the comment contains an erroneous config. If we don't do this, there will be lots and lots of red herrings in Foundry code bases, i.e. comments that mislead readers by making them believe there is a bespoke fuzzing configuration when in fact there isn't any.

Regarding execution - the comment config will be disregarded if it refers to a profile other than the one used by Forge during runtime, correct?

- TestOptions struct extended to track test specific configs - Tests

PatrickAlphaC · 2023-04-18T12:57:38Z

Nice!!
Does this work for fail_on_revert for invariants?

pragma solidity ^0.8.13;

import "forge-std/Test.sol";
import "../src/Counter.sol";

contract CounterTest is Test {
    Counter public counter;

    function setUp() public {
        counter = new Counter();
        counter.setNumber(0);
    }

    // forge-config: default.invariant.fail_on_revert = false
    function invariant_SetNumberCustom() public {
        assert(counter.number() < 100);
    }
}

0xMelkor · 2023-04-18T15:01:30Z

Does this work for fail_on_revert for invariants?

Yes this field is mapped

foundry/config/src/invariant.rs

Line 62 in 7d540c0

"fail-on-revert" => conf.fail_on_revert = value.parse()?,

The difference is that it must be provided in kebab case:

    // forge-config: default.invariant.fail-on-revert = false
    function invariant_SetNumberCustom() public {
        assert(counter.number() < 100);
    }

This PR is still in draft. Very soon these configs will be applied at execution time.

…tion

0xMelkor · 2023-04-19T10:32:44Z

Hi guys,

Fuzz inline configs are interpreted and applied at runtime. I added some tests that confirm my assertion.
Invariant integration is coming soon.

… tests + Docs

0xMelkor · 2023-05-06T11:16:23Z

Suggestions applied @Evalir
We will bring light where is missing mate.

…the foundry.toml path cannot be resolved

0xMelkor · 2023-05-08T07:19:10Z

Some tests were failing because the new logic cannot find a foundry.toml to extract profiles:

foundry/cli/src/cmd/forge/test/mod.rs

Line 155 in fa97f9a

let profiles = get_available_profiles(toml)?;

For such reason I added a fallback value in the case the file doesn't exist:

foundry/config/src/utils.rs

Lines 182 to 187 in fa97f9a

    
           pub fn get_available_profiles(toml_path: impl AsRef<Path>) -> eyre::Result<Vec<String>> { 
        
               let mut result = vec![Config::DEFAULT_PROFILE.to_string()]; 
        
               if !toml_path.as_ref().exists() { 
        
                   return Ok(result) 
        
               }

Is this acceptable?

Failing tests:

can test cmd::can_check_snapshot
test test_cmd::can_fuzz_array_params
test test_cmd::suggest_when_no_tests_match
test test_cmd::warn_no_tests
test test_cmd::warn_no_tests_match
test test_cmd::can_test_with_match_path
test test_cmd::can_test_pre_bytecode_hash

mattsse

Changes look good, but failed tests are ominous,

passing on master

some of them look config-related I think.

what's the reason they're failing?

0xMelkor · 2023-05-08T12:34:31Z

No more problems with tests.
If a foundry.toml is not available, as it happens for some e2e tests, we return vec![Config::DEFAULT_PROFILE] as a fallback for all available profiles.
Discussed with @mattsse

mattsse · 2023-05-08T15:17:50Z

okay, @0xMelkor answered a few questions, the change regarding profiles fixed the failing tests.

it looks pretty good.

mind giving this another look as well @Evalir ?

Evalir

this lgtm—just the nit on the InlineConfigParser impls

Evalir · 2023-05-08T16:30:05Z

config/src/fuzz.rs

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


ok so if i understand correctly this is a copy that we're later mutating and returning? can we do self.clone()? This doesn't seem too idiomatic

config/src/invariant.rs

config/src/inline/mod.rs

mds1 · 2023-05-08T18:00:31Z

config/src/inline/mod.rs

+    /// The inner error
+    pub source: InlineConfigParserError,


Right now the error looks like this:

Inline config Error detected at test/Counter.t.sol:CounterTest:testSetNumber1:"288:82:20"

Not sure if useful, but I think @mattsse added an offset_to_line to the forge fmt error messages which may useful to get a line number here. Or if we can get rid of the quotes around the 288:82:20 that might look a bit nicer

Related, it looks like the error is actually duplicated, once in red, and again in the "context" section. Is that intentional?

mds1 · 2023-05-08T18:04:09Z

Two small comments otherwise this is awesome and looks great!

…was not accurate)

0xMelkor · 2023-05-09T05:40:28Z

Two small comments otherwise this is awesome and looks great!

Nice error now

0xMelkor · 2023-05-10T17:04:50Z

Hi guys,
Any chances to have this merged?
CI is green and last @mds1 comments are fixed.

mattsse

awesome!

everything lgtm!

@Evalir let's merge?

mattsse

awesome!

everything lgtm!

@Evalir let's merge?

Evalir · 2023-05-11T22:13:42Z

Sick! Happy with the changes too—merging!

Amazing work @0xMelkor ❤️

0xMelkor · 2023-05-11T22:43:10Z

Awesome!
We have also this docs PR
foundry-rs/book#880

0xMelkor added 6 commits April 14, 2023 09:00

Parse FuzzConfig from string (brief impl)

8fd577a

Unit tests

ConfParser trait is able to extract configurations out of a structure…

d8f91b8

…d text Unit tests

cargo +nightly fmt

94a1846

FuzzConfig implements ConfParser trait

efbaacb

Unit tests

InvariantConfig implements ConfParser trait

6e770e0

Unit tests

Parsing logic optimized

b1db51f

Meaningful e2e test

mattsse reviewed Apr 16, 2023

View reviewed changes

mattsse added A-testing Area: testing A-config Area: config labels Apr 16, 2023

0xMelkor added 2 commits April 16, 2023 11:56

Configurations can be parsed from project compilation output

b0a0bfc

E2E tests for inline configuration load

1d38733

- ConfParser: parse fn is now try_merge

7d540c0

- TestOptions struct extended to track test specific configs - Tests

0xMelkor added 4 commits April 18, 2023 21:59

Since TestOptions is no more Copy => TEST_OPTS constant is now a func…

f3b25fc

…tion

Inline config matcher uses stripped file prefixes to identify contracts

0868d67

TestOptionsBuilder docs

9f8672e

Inline fuzz configs are applied during fuzz test execution + E2E tests

09ff7db

0xMelkor added 2 commits April 19, 2023 21:18

Inline invariant configs are applied during fuzz test execution + E2E…

9fc175c

… tests + Docs

typos

211be7a

0xMelkor marked this pull request as ready for review April 19, 2023 19:37

0xMelkor added 2 commits April 28, 2023 09:07

Docs typo

b424e8d

cargo +nightly fmt

abcedb7

0xMelkor mentioned this pull request Apr 28, 2023

docs: ux fuzz invariant foundry-rs/book#880

Merged

0xMelkor added 2 commits May 6, 2023 13:03

review: clarify intent

c61f98e

review: document functions

2be61d6

0xMelkor added 3 commits May 7, 2023 12:28

review: applied case typos

fa06e62

FIX CI: Available profiles fallback to vec![current_profile] in case …

616d107

…the foundry.toml path cannot be resolved

Merge branch 'foundry-rs:master' into feat/ux-fuzz-invariant

fa97f9a

mattsse reviewed May 8, 2023

View reviewed changes

cargo +nightly fmt

b016d3d

Evalir requested changes May 8, 2023

View reviewed changes

Evalir approved these changes May 8, 2023

View reviewed changes

mds1 reviewed May 8, 2023

View reviewed changes

config/src/inline/mod.rs Outdated Show resolved Hide resolved

mds1 reviewed May 8, 2023

View reviewed changes

0xMelkor added 3 commits May 8, 2023 21:48

review: case typo

2360a1c

review: remove double quotes from src line

ed94236

review: removed duplicated error msg; removed row:col:len detail (it …

fcee8ec

…was not accurate)

fix CI

eef9149

mattsse approved these changes May 11, 2023

View reviewed changes

Evalir merged commit f3c20d5 into foundry-rs:master May 11, 2023

This was referenced May 13, 2023

feat (proposal): UX for granularity over fuzz, invariant, and solidity sections #3062

Closed

Selective fuzz runs with NatSpec comments "@custom:fuzz-runs NUMBER" #4085

Closed

ci: tone down fuzzing sablier-labs/v2-core#474

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: ux fuzz invariant #4744

feat: ux fuzz invariant #4744

0xMelkor commented Apr 14, 2023 •

edited

Loading

mattsse left a comment

0xMelkor commented Apr 16, 2023

mds1 commented Apr 16, 2023

0xMelkor commented Apr 16, 2023

mds1 commented Apr 16, 2023

PaulRBerg commented Apr 17, 2023

PatrickAlphaC commented Apr 18, 2023 •

edited

Loading

0xMelkor commented Apr 18, 2023 •

edited

Loading

0xMelkor commented Apr 19, 2023

0xMelkor commented May 6, 2023

0xMelkor commented May 8, 2023

mattsse left a comment

0xMelkor commented May 8, 2023 •

edited

Loading

mattsse commented May 8, 2023

Evalir left a comment

Evalir May 8, 2023

mds1 May 8, 2023 •

edited

Loading

mds1 May 8, 2023

mds1 commented May 8, 2023

0xMelkor commented May 9, 2023

0xMelkor commented May 10, 2023

mattsse left a comment

mattsse left a comment

Evalir commented May 11, 2023

0xMelkor commented May 11, 2023

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

feat: ux fuzz invariant #4744

feat: ux fuzz invariant #4744

Conversation

0xMelkor commented Apr 14, 2023 • edited Loading

Motivation

Solution

mattsse left a comment

Choose a reason for hiding this comment

0xMelkor commented Apr 16, 2023

mds1 commented Apr 16, 2023

0xMelkor commented Apr 16, 2023

mds1 commented Apr 16, 2023

PaulRBerg commented Apr 17, 2023

PatrickAlphaC commented Apr 18, 2023 • edited Loading

0xMelkor commented Apr 18, 2023 • edited Loading

0xMelkor commented Apr 19, 2023

0xMelkor commented May 6, 2023

0xMelkor commented May 8, 2023

mattsse left a comment

Choose a reason for hiding this comment

0xMelkor commented May 8, 2023 • edited Loading

mattsse commented May 8, 2023

Evalir left a comment

Choose a reason for hiding this comment

Evalir May 8, 2023

Choose a reason for hiding this comment

mds1 May 8, 2023 • edited Loading

Choose a reason for hiding this comment

mds1 May 8, 2023

Choose a reason for hiding this comment

mds1 commented May 8, 2023

0xMelkor commented May 9, 2023

0xMelkor commented May 10, 2023

mattsse left a comment

Choose a reason for hiding this comment

mattsse left a comment

Choose a reason for hiding this comment

Evalir commented May 11, 2023

0xMelkor commented May 11, 2023

0xMelkor commented Apr 14, 2023 •

edited

Loading

PatrickAlphaC commented Apr 18, 2023 •

edited

Loading

0xMelkor commented Apr 18, 2023 •

edited

Loading

0xMelkor commented May 8, 2023 •

edited

Loading

mds1 May 8, 2023 •

edited

Loading