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: add timeouts to fuzz testing #9394

Merged
merged 11 commits into from
Nov 29, 2024

Conversation

smartcontracts
Copy link
Contributor

@smartcontracts smartcontracts commented Nov 24, 2024

Adds --fuzz-timeout-secs to fuzz tests which will cause a property test to timeout after a certain number of seconds. Also adds --fuzz-allow-timeouts so that timeouts are optionally not considered to be failures.

First time writing Rust so I have no idea if this is the idiomatic way of doing things, happy to change anything based on suggestions! Would also be great to understand where the best place is to add tests for this sort of PR.

Keeping this as a draft for now. I want to add support for the same feature within invariant tests but I couldn't easily understand how FOUNDRY_INVARIANT_RUNS actually gets parsed from the environment. Apparently FOUNDRY_INVARIANT_RUNS can't be set in the CLI (there's no --invariant-runs flag). Any pointers here would be much appreciated.

This is sketch for #9393

Adds --fuzz-timeout-secs to fuzz tests which will cause a property
test to timeout after a certain number of seconds. Also adds
--fuzz-allow-timeouts so that timeouts are optionally not
considered to be failures.
@grandizzy
Copy link
Collaborator

grandizzy commented Nov 25, 2024

Thank you! Supportive for such, let's have this

  • as configs in foundry.toml
[fuzz]
timeout=60

which would be applied to all fuzz tests (I don't think the allow time is needed, just consider that if timeout is set then it implies we allow timeouts on test)

  • inline, defined per test as:
/// forge-config: default.fuzz.timeout=60
function test_function_with_timeout(uint256 x) public {...}
  • not sure if really needed to have them as global args but don't see any harm keeping them there too...

Same configs could be applied for invariants too, e.g.

[invariant]
timeout=60

and

/// forge-config: default.invariant.timeout=60

Lmk if you have issues implementing these, can provide more details / help with

@smartcontracts
Copy link
Contributor Author

@grandizzy cool, will start making these changes! Regarding allowing timeouts, it might be worth considering that some people may want some way to detect that a fuzz run took too long and timed out. For now I would just allow timeouts by default and if someone asks for the feature we can add --reject-timeouts or whatever.

@smartcontracts smartcontracts force-pushed the sc/fuzz-invariant-timeouts branch from 23c9395 to b2a5f8d Compare November 25, 2024 16:50
@smartcontracts
Copy link
Contributor Author

Ok @grandizzy went ahead and made various changes. Please note the change to use Ok instead of returning an error. I think this is more accurate if we want to consider timeouts to not be failures by default. We could return an error if we want to add a --reject-timeouts flag in the future.

@grandizzy
Copy link
Collaborator

grandizzy commented Nov 26, 2024

Ok @grandizzy went ahead and made various changes. Please note the change to use Ok instead of returning an error. I think this is more accurate if we want to consider timeouts to not be failures by default. We could return an error if we want to add a --reject-timeouts flag in the future.

the problem with Ok instead failing test is that for a config as

    /// forge-config: default.fuzz.runs = 4294967295
    /// forge-config: default.fuzz.timeout = 60
    function testFuzz_SetNumber(uint256 x) public {

the test still loops / consume cpu cycles for way more than 1 minute and don't think that's desired

@smartcontracts
Copy link
Contributor Author

the test still loops / consume cpu cycles for way more than 1 minute and don't think that's desired

Ah yeah you're right, ok, will fix!

@smartcontracts smartcontracts force-pushed the sc/fuzz-invariant-timeouts branch from 75b81e0 to b704851 Compare November 26, 2024 15:59
@smartcontracts
Copy link
Contributor Author

@grandizzy I'm back to failing for timeouts, thanks for the tip there. I believe the PR works as-is, not exactly sure where I should be adding tests but happy to do so if you could point me to the test file I should modify!

@grandizzy
Copy link
Collaborator

@grandizzy I'm back to failing for timeouts, thanks for the tip there. I believe the PR works as-is, not exactly sure where I should be adding tests but happy to do so if you could point me to the test file I should modify!

Awesome, could you please make sure

cargo +nightly fmt -- --check
cargo +nightly clippy --all --all-targets --all-features -- -D warnings

are passing, CI is failing now. Re tests, let's add one similar with

// tests that inline max-test-rejects config is properly applied
forgetest_init!(test_inline_max_test_rejects, |prj, cmd| {
prj.wipe_contracts();

that set inline config for a big number of runs and a timeout of 5 seconds and we assert test is passing. you can test it as

cargo test test_name_here

Copy link
Collaborator

@grandizzy grandizzy left a comment

Choose a reason for hiding this comment

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

Last nit re invariant impl, let's fix clippy and format too

crates/evm/evm/src/executors/invariant/mod.rs Outdated Show resolved Hide resolved
@smartcontracts smartcontracts marked this pull request as ready for review November 27, 2024 03:07
- move logic to interrupt invariant test in depth loop
- add and reuse start_timer fn and TEST_TIMEOUT constant
- add fuzz and invariant tests
- fix failing test
@grandizzy grandizzy self-requested a review November 27, 2024 09:09
Copy link
Collaborator

@grandizzy grandizzy left a comment

Choose a reason for hiding this comment

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

lgtm, thank you! I made couple of changes as

  • move logic to interrupt invariant test in depth loop
  • add and reuse start_timer fn and TEST_TIMEOUT constant
  • add fuzz and invariant tests
  • fix failing test
    Pending other reviews before merging

@smartcontracts
Copy link
Contributor Author

@grandizzy sweet! Let me know if I can help in any way. Thanks for all your review here, very much appreciated 🙏

Comment on lines 110 to 111
if let Some((start_time, timeout)) = start_time {
if start_time.elapsed() > timeout {
Copy link
Member

Choose a reason for hiding this comment

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

I guess since this is opt-in, checking via elapsed for now is fine, and we can track improvements separately, but I'd suggest to wrap this if let Some into a helper type like Timeout(Option<Instant>) and an is_timed_out then this becomes easier to change

Copy link
Collaborator

Choose a reason for hiding this comment

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

@mattsse I added FuzzTestTimer with da87095 please check

@grandizzy grandizzy force-pushed the sc/fuzz-invariant-timeouts branch from ac33250 to da87095 Compare November 28, 2024 10:44
@grandizzy grandizzy requested a review from mattsse November 28, 2024 11:00
Copy link
Member

@DaniPopes DaniPopes left a comment

Choose a reason for hiding this comment

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

lgtm

@grandizzy grandizzy dismissed mattsse’s stale review November 28, 2024 20:19

please re review

@mattsse mattsse merged commit 2e9f536 into foundry-rs:master Nov 29, 2024
21 checks passed
@smartcontracts smartcontracts deleted the sc/fuzz-invariant-timeouts branch November 29, 2024 17:22
rplusq pushed a commit to rplusq/foundry that referenced this pull request Nov 29, 2024
* feat: add timeouts to fuzz testing

Adds --fuzz-timeout-secs to fuzz tests which will cause a property
test to timeout after a certain number of seconds. Also adds
--fuzz-allow-timeouts so that timeouts are optionally not
considered to be failures.

* simplify timeout implementation

* use u32 for timeout

* switch back to failing for timeouts

* clippy

* Nits:
- move logic to interrupt invariant test in depth loop
- add and reuse start_timer fn and TEST_TIMEOUT constant
- add fuzz and invariant tests
- fix failing test

* Fix fmt

* Changes after review: introduce FuzzTestTimer

---------

Co-authored-by: grandizzy <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants