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): include data from fuzz/invariant runs in gas reports #7324

Merged
merged 1 commit into from
Mar 11, 2024

Conversation

klkvr
Copy link
Member

@klkvr klkvr commented Mar 6, 2024

Motivation

Closes #7313

Now the output of the test --gas-report from the repro is the following:

| test/Inv.t.sol:ContractA contract |                 |       |        |        |         |
|-----------------------------------|-----------------|-------|--------|--------|---------|
| Deployment Cost                   | Deployment Size |       |        |        |         |
| 154332                            | 534             |       |        |        |         |
| Function Name                     | min             | avg   | median | max    | # calls |
| increment                         | 48554           | 48554 | 48554  | 48554  | 1       |
| setNumber                         | 48795           | 68117 | 49077  | 103999 | 256     |


| test/Inv.t.sol:Counter contract |                 |     |        |     |         |
|---------------------------------|-----------------|-----|--------|-----|---------|
| Deployment Cost                 | Deployment Size |     |        |     |         |
| 137113                          | 421             |     |        |     |         |
| Function Name                   | min             | avg | median | max | # calls |
| number                          | 305             | 305 | 305    | 305 | 257     |

Solution

We now collect up to FuzzConfig::gas_report_samples/InvariantConfig::gas_report_samples additional traces of successfull runs of fuzz/invariant tests, then identify and include them into gas report.

Additional samples are not collected when gas report is disabled so there should be no performance overhead for defaut runs.

Not sure if I've chosen the best approach for testing this. Existing --gas-report tests operated on stdout and I needed access to the resulted GasReport object to test count of calls.

@mds1
Copy link
Collaborator

mds1 commented Mar 6, 2024

With forge snapshot we use a seed to ensure consistent results—sanity checking that --gas-report has the same behavior?

Also it sounds like we always collect gas report samples even if the gas report is not requested, is the idea that the default gas_report_samples is small enough that performance impact is negligible, even for large test suites?

@klkvr
Copy link
Member Author

klkvr commented Mar 6, 2024

Also it sounds like we always collect gas report samples even if the gas report is not requested, is the idea that the default gas_report_samples is small enough that performance impact is negligible, even for large test suites?

We are overriding any configured gas_report_samles with 0 if --gas-report flag is not provided which results in underlying testing code not collecting any additional traces

With forge snapshot we use a seed to ensure consistent results—sanity checking that --gas-report has the same behavior?

No, we can consider that, but current output of --gas-report is not consistent as well and depends on the latest fuzz run inputs.

@mds1
Copy link
Collaborator

mds1 commented Mar 6, 2024

Also it sounds like we always collect gas report samples even if the gas report is not requested, is the idea that the default gas_report_samples is small enough that performance impact is negligible, even for large test suites?

We are overriding any configured gas_report_samles with 0 if --gas-report flag is not provided which results in underlying testing code not collecting any additional traces

Ah thanks, missed this
https://github.com/foundry-rs/foundry/pull/7324/files#diff-289e6e32b190bdcdd1d4512466e34391afa813e748f99ed7316054a0352e7db8R149-R151

With forge snapshot we use a seed to ensure consistent results—sanity checking that --gas-report has the same behavior?

No, we can consider that, but current output of --gas-report is not consistent as well and depends on the latest fuzz run inputs.

Got it, happy to do that in a separate PR to avoid scope creep here

@mattsse
Copy link
Member

mattsse commented Mar 11, 2024

good to merge @klkvr ?

@klkvr
Copy link
Member Author

klkvr commented Mar 11, 2024

yep, sending

@klkvr klkvr merged commit 15c8d11 into foundry-rs:master Mar 11, 2024
19 checks passed
@klkvr klkvr deleted the klkvr/gas-report-fuzz-full branch March 11, 2024 12:23
@klkvr klkvr mentioned this pull request Mar 11, 2024
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.

Gas report for fuzz test and internal transactions: confusing output
3 participants