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

[mds1]

Component

Forge

Describe the feature you would like

Supersedes #744

For the [fuzz] and [invariant] sections, you must define configuration per-profile and it applies to all tests. Some tests need more/less fuzzing than others, so it would be great to configure this on a per-test basis. Below is a proposal for how to structure the foundry.toml to allow this.

Similarly, it would be great to allow different solc settings per contract (e.g. 1 optimizer run for MyBigContract but 10M optimizer runs for all others). This proposal would enable that as well.

# OVERVIEW
# Each fuzz/invariant section can have subprofiles. In these subprofiles you
# can define all the standard fuzz/invariant options, but additionally the same
# matching flags that `forge test` has are supported. This means good naming
# conventions are important to make targeting your tests easy.

# EXAMPLE
# Here is an example using fuzz configs to do the following:
#
# Default profile:
#   - 250 fuzz runs by default
#   - `low` tests execute 50 fuzz runs
#   - `high` tests execute 500 fuzz runs
# CI profile:
#   - 1000 fuzz runs by default
#   - `low` tests execute 500 fuzz runs
#   - `high` tests execute 5000 fuzz runs with 100_000 max rejections
#
# In this example, we only use the match-test flag, but any supported match
# flags can be used.

# First we define our default profile
[profile.default.fuzz]
runs = 250 # 250 runs by default for the default profile

[profile.default.fuzz.low]
# Any test that matches the provided regex uses this subprofile
match-test = "testFuzzLow.*"
runs = 50

[profile.default.fuzz.high]
match-test = "testFuzzHigh.*"
runs = 500

# Now we define our CI profile
[profile.ci.fuzz]
runs = 1000

[profile.ci.fuzz.low]
match-test = "testFuzzLow.*"
runs = 500

[profile.ci.fuzz.high]
match-test = "testFuzzHigh.*"
runs = 5000
max-global-rejects = 100_000

# EXTENSIONS
# You can imagine this extending to e.g. solidity configurations too when a
# [solidity] table is added, such as shown below:

# We define a lite mode to keep the optimizer off to speed up dev/testing.
# Since there's no `lite` fuzz config, it uses the default profile's settings.
[profile.lite.solidity]
optimizer = false

# In our default solidity profile we apply 10M optimizer runs.
[profile.default.solidity]
optimizer = true
optimizer-runs = 10_000_000

# But we have one really big contract, so we use 1 optimizer run to minimize
# its size.
[profile.default.solidity.size]
optimizer = true
optimizer-runs = 1
match-contract = "MyBigContract"

Additional context

No response

[gakonst]

To select the profile ci one would do FOUNDRY_CONFIG=ci forge t, how would one choose the fuzzer profile?

[mds1]

FOUNDRY_CONFIG=ci forge t would use the settings under profile.ci.fuzz for fuzz tests. Within that profile are sub profiles: profile.ci.fuzz is the default config for that profile, and in the above example there's also profile.ci.fuzz.low and profile.ci.fuzz.high. So forge would:

• Look at all match-* keys under profile.ci.fuzz.low, and apply the CI low config to those tests.
• Look at all match-* keys under profile.ci.fuzz.high, and apply the CI high config to those tests. (If there's a conflict, we should just go with whatever was processed last and print a warning so the user can fix it)
• All other tests use the default under profile.ci.fuzz.
• If nothing is specified under profile.ci.fuzz, it falls back to profile.default.fuzz which is an alias for fuzz

[rkrasiuk]

i've got mixed feelings about the config complexity this introduces. @mds1 wdtyt about keeping the sections packed & simply adding the overrides to [fuzz] & [invariant] sections

[fuzz]
runs = 250
overrides = { "testFuzzLow.*" = { runs = 50 }, "testFuzzHigh.*" = { runs = 500 }}

[profile.ci.fuzz]
runs = 1000
overrides = { "testFuzzLow.*" = { runs = 500 }, "testFuzzHigh.*" = { runs = 5000, max-global-rejects = 100_000 } }

[mds1]

Yea I feel the same way about the complexity, but couldn't think of a good alternative 😕

In general I like your overrides suggestion, but:

1. I think we should support matching by either test name, contract, or file, whereas that looks like just test name. This can be resolved by something like overrides-match-path, overrides-no-match-test, etc.
2. Inline tables for overrides can get long, and I'd want to split them onto multiple lines for readability. However the toml spec says:

Inline tables are intended to appear on a single line ... No newlines are allowed between the curly braces unless they are valid within a value. Even so, it is strongly discouraged to break an inline table onto multiples lines. If you find yourself gripped with this desire, it means you should be using standard tables.

Which was part of my motivation for the original format

[rkrasiuk]

yeah, makes sense. not a fan of long lines either hence mixed feelings. one more idea is to have a cheatcode for a contract or test to set the config values, however that'd be both cumbersome to implement and for the end user to track across the codebase

[mds1]

Yea I do think cheatcodes could be nice UX, see approach 4 in #744. However even that can get cumbersome since there's multiple parameters to set—we'd probably want a struct that defines all fuzz values and use some magic values to mean "I'm not setting this one, fallback to the default". But like you said it's probably significantly more work to implement and harder for users to track.

I think the original proposal feels complex and hard to read because everything is visually "flat", whereas if this were formatted as JSON it would be more readable due to the indentation changes. Therefore I think using some comments to visually separate profiles can make it more readable (inline tables would also help, though technically they're supposed to be limited to one line I believe):

# =================================
# ======== Default Profile ========
# =================================

# -------- General --------

[profile.default]
verbosity = 3

# -------- Solidity --------

[profile.lite.solidity]
optimizer = false

[profile.default.solidity]
optimizer = true
optimizer-runs = 10_000_000

[profile.default.solidity.size]
optimizer = true
optimizer-runs = 1
match-contract = "MyBigContract"

# -------- Fuzz --------

[profile.default.fuzz]
runs = 250 # 250 runs by default for the default profile

[profile.default.fuzz.low]
match-test = "testFuzzLow.*"
runs = 50

[profile.default.fuzz.high]
match-test = "testFuzzHigh.*"
runs = 500

# ====================
# ======== CI ========
# ====================

# -------- Fuzz --------

[profile.ci.fuzz]
runs = 1000

[profile.ci.fuzz.low]
match-test = "testFuzzLow.*"
runs = 500

[profile.ci.fuzz.high]
match-test = "testFuzzHigh.*"
runs = 5000
max-global-rejects = 100_000

[PaulRBerg]

everything is visually "flat" .. Therefore I think using some comments to visually separate profiles can make it more readable

An alternative is to format the TOML with Taplo, which can be enabled with the Even Better TOML extension in VSCode:

"[toml]": {
  "editor.defaultFormatter": "tamasfe.even-better-toml"
}

[mds1]

Agreed, using taplo to indent the keys would also help. I personally do that for all toml files and find it much more readable

[mds1]

I'm starting to think comment directives might the way to go here. Originally we didn't go this route because we thought it required compilation + /// @custom: natspec support which is only in recent solidity versions. However, we can use a regular comment with a custom syntax such as // forge-config:profileName:section:setting = value. For example:

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

In this example:

• The default profile runs test_SimpleFuzzTest 100 times and the ci profile runs it 500 times
• The default profile runs test_ImportantFuzzTest 500 times and the ci profile runs it 500 times

Benefits over the config file approach:

• The config is a lot simpler, cleaner, and more readable since it's directly above the fuzz test, so you don't need to keep flipping back and forth to the TOML file
• Can be parsed with solang so it should be fast, and not reliant on solidity features or compilation

Similar to the original proposal here, those comment directives could be supported above tests (to apply to a single test), above contracts (to apply to all tests in that contract), or at the root of a file (to apply to all tests in that file), with the most specific ones taking precendence.

The main downside is the added noise to tests, but I think it's worth it.

cc @brockelmore @transmissions11 @PaulRBerg @PatrickAlphaC for UX thoughts. I think this would be a huge unlock for the fuzzer

[PaulRBerg]

@mds1 100% in favor of your feature proposal; it would be super useful.

Regarding /// @custom NatSpec tags (which were added in Solidity v0.8.2) - later down the line, Foundry could add retroactive support for @custom:fuzz-runs NUMBER on top of // forge-config: (that is, once the vast majority of Solidity projects switch to >=0.8.2).

[PatrickAlphaC]

Why not use modifiers instead of comments? I really dislike the idea of using comments as a sort of function decorator.

What are the thoughts on:

foundry.toml

[profile.ci.fuzz]
runs = 1000

[profile.ci.fuzz.low]
match-test = "testFuzzLow.*"
runs = 500

[profile.ci.fuzz.high]
match-test = "testFuzzHigh.*"
runs = 5000
max-global-rejects = 100_000

MyTest.t.sol:

contract MyTest is Test {
  // it takes a list of strings as profile, and a list of uint256 as the number of fuzz runs repective with the profile
  function test_SimpleFuzzTest(uint256 x) public fuzzRuns(["ci.fuzz.low"], [500]){
    // --- snip ---
  }

  function test_ImportantFuzzTest(uint256 x) public fuzzRuns(["ci.fuzz.low"], [500]) {
    // --- snip ---
  }
}

Additionally, we could overload modifiers to accept one parameter to override everything in the config.

 function test_ImportantFuzzTest(uint256 x) public fuzzRuns(500) {
    // --- snip ---
  }

That way, we could rely on the compiler that our modifiers are setup more correctly.

This could be part of the foundry standard test library.

[PaulRBerg]

Compared to comments, using modifiers for test granularity has two downsides:

1. Much more difficult to make breaking changes (imagine your code not compiling after foundryup because the granularity modifier changed)
2. Would interfere with my state tree technique for writing tests

[sambacha]

Compared to comments, using modifiers for test granularity has two downsides:

1. Much more difficult to make breaking changes (imagine your code not compiling after foundryup because the granularity modifier changes)

2. Would interfere with my state tree technique for writing tests

3. They are comments at the end of the day, so there is no realistic chance of the compiler trying to use it.

It should be noted that dapple (dapptools progenitor) uses this comment style as well (or use to at least).

[PatrickAlphaC]

Hmm... I see your points. They make sense. But maybe there is some advantage to having the compiler take it on. For example, if you format your custom runs incorrectly, the compiler will catch it vs you'd test your code incorrectly. I sort of like the idea that having a modifier would make sure that I'm formatting my fuzz runs right.

I don't love the idea of comments as the driving force. I think it isn't very clear. for newer devs especially.

"Comments don't affect how your code runs, oh, except in this case in foundry." <- This feels weird to teach.

Comments becoming an important factor in how your test suite runs are... meh. I like python's decorators, but I suppose we can't do something like that in solidity/foundry.

This isn't a hill I'll die on as I see your points. But TL;DR, my thoughts are that using comments you'd:

1. Make code harder to understand
2. Not gain compiler benefits

But you get:

1. Easier maintainability
2. Arguably nicer to read syntax
3. Not have to worry about compiler issues

[PaulRBerg]

@PatrickAlphaC nice write-up; my take is that the benefits of using comments outweigh the cons.

the compiler will catch it vs you'd test your code incorrectly.

Surely Foundry can catch misconfigured inputs. And if you don't use the correct comment syntax, you can quite quickly notice this when you run your tests and see that the fuzzing config didn't apply (e.g. you want to fuzz 50 times but in fact you fuzz 1,000 times, this is easy to see).

I don't love the idea of comments as the driving force

IMO, that's a bit of a stretch; foundry.toml is the driving force. Comments would be used on a case-by-case basis.

"Comments don't affect how your code runs, oh, except in this case in foundry." <- This feels weird to teach.

1. This is only about running tests, not production code.
2. In any case, comments already affect the bytecode of smart contracts (because of metadata), so the logic of comments in Solidity has to be taught anyway.

[PatrickAlphaC]

Thanks for the response. Re: "In any case, comments already affect the bytecode of smart contracts (because of metadata), so the logic of comments in Solidity has to be taught anyway."

They affect the code size, but not the running code. I just know that someone is going to post something on stack exchange asking us to debug their tests, and the answer is that their comments are breaking it.

In any case, I can't think of a better solution, so I think comments might be the best bet.

Excited for this feature.

[0xMySt1c]

# In our default solidity profile we apply 10M optimizer runs.
[profile.default.solidity]
optimizer = true
optimizer-runs = 10_000_000

# But we have one really big contract, so we use 1 optimizer run to minimize
# its size.
[profile.default.solidity.size]
optimizer = true
optimizer-runs = 1
match-contract = "MyBigContract"

Just curious how would the build know not to run optimizer 10,000,000x on MyBigContract?

[PaulRBerg]

Given that #4744 was merged, should we close this issue?

[mds1]

Closing, thanks!