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.

Sign up for GitHub

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

Jump to bottom

Invariants feat: add config option to turn off shrinking #4868

Merged
merged 6 commits into from
May 12, 2023
Merged

Invariants feat: add config option to turn off shrinking #4868

merged 6 commits into from
May 12, 2023

Conversation

grandizzy
Copy link
Collaborator

@grandizzy grandizzy commented May 2, 2023 •
edited
Loading

Motivation

See #4867, when running invariants test with many runs and depth (1000) the test hangs in shrinking scenario (performed for each invariant). In this case sometimes is preferable to fail fast and address the issue.

Solution

  • add invariant_shrink_sequence option, default true, tries to reduce number of calls in scenario to min
  • when set to false test will fail fast and output the original number of calls in scenario

grandizzy added 2 commits May 2, 2023 22:05
@grandizzy
- add try_shrinking config in [invariant], default true, tries to red…
d340bfb 
…uce number of calls in scenario to min

- when set to false test will just error with the original number of calls in scenario
@grandizzy
Add comment
6d8bf9e
@grandizzy grandizzy mentioned this pull request May 3, 2023
Closed
@mds1
Copy link
Collaborator

mds1 commented May 3, 2023 •
edited
Loading

@0xMelkor note that once this is merged we'll need to update #4744 to support this option in there too

@0xMelkor
Copy link
Contributor

0xMelkor commented May 3, 2023

@0xMelkor note that once this is merged we'll need to update #4744 to support this option in there too

Hi @mds1, sure.

mattsse
mattsse requested changes May 4, 2023
View reviewed changes
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

smol nit, otherwise lgtm

evm/src/fuzz/invariant/error.rs Outdated
Comment on lines 97 to 106
let _calls = match self.test_error {
// Don't use at the moment.
TestError::Abort(_) => return Ok(None),
TestError::Fail(_, ref calls) => calls,
TestError::Fail(_, ref _calls) => _calls,
};

let calls = self.try_shrinking(calls, &executor);
if self.shrink {
let _calls = self.try_shrinking(_calls, &executor);
} else {
trace!(target: "forge::test", "Shrinking disabled.");
Copy link
Member

Choose a reason for hiding this comment

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

please remove _ prefix

@grandizzy
Copy link
Collaborator Author

smol nit, otherwise lgtm

yep, added such to avoid warning: unused variable: if this is intentional, prefix it with an underscore is there a different preferred way to disable this?

@mattsse
Copy link
Member

mattsse commented May 4, 2023

but

let _calls =

let _calls = self.try_shrinking(_calls, &executor);

so I think it should be let calls = ...; let _ self.try_shrinking(calls,&executor)

@grandizzy
Copy link
Collaborator Author

but

let _calls =

let _calls = self.try_shrinking(_calls, &executor);

so I think it should be let calls = ...; let _ self.try_shrinking(calls,&executor)

oops, right, changing

@grandizzy
Copy link
Collaborator Author

but

let _calls =

let _calls = self.try_shrinking(_calls, &executor);
so I think it should be let calls = ...; let _ self.try_shrinking(calls,&executor)

oops, right, changing

fixed with 0226461

@grandizzy grandizzy requested a review from mattsse May 4, 2023 18:31
@grandizzy
Copy link
Collaborator Author

but

let _calls =

let _calls = self.try_shrinking(_calls, &executor);
so I think it should be let calls = ...; let _ self.try_shrinking(calls,&executor)

oops, right, changing

fixed with 0226461

hey @mattsse just bumping this, when you have time please check the change, would really love to get this in. thank you!

mattsse
mattsse approved these changes May 11, 2023
View reviewed changes
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

cool, tysm!

lgtm!

Evalir
Evalir approved these changes May 12, 2023
View reviewed changes
Copy link
Member

@Evalir Evalir left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM! Just pushed something upstream to make it compatible with #4744. Nice work!

deferring to @mattsse for merge

config/src/invariant.rs
@@ -61,6 +64,7 @@ impl InlineConfigParser for InvariantConfig {
"depth" => conf_clone.depth = parse_config_u32(key, value)?,
"fail-on-revert" => conf_clone.fail_on_revert = parse_config_bool(key, value)?,
"call-override" => conf_clone.call_override = parse_config_bool(key, value)?,
"shrink-sequence" => conf_clone.shrink_sequence = parse_config_bool(key, value)?,
Copy link
Member

Choose a reason for hiding this comment

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

added the new shrink-sequence key to the invariant inlineconfigparser so it works with #4744

@mattsse mattsse merged commit 9eb8974 into foundry-rs:master May 12, 2023
@grandizzy
Copy link
Collaborator Author

thank you all!

@grandizzy grandizzy mentioned this pull request Jul 7, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Evalir Evalir Evalir approved these changes

@mattsse mattsse mattsse approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@grandizzy @mds1 @0xMelkor @mattsse @Evalir