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

invariant shrink #6683: check if test failed instead revert #7257

Merged
merged 3 commits into from
Feb 28, 2024

Conversation

grandizzy
Copy link
Collaborator

Motivation

see #6683 (comment)
Atm shrinking doesn't apply for invariants using asserts but only for invariants that reverts (require)

Solution

  • when deciding if a sequence can fail test check if test failed (instead if reverted only)
  • added test with assert and require to ensure they shrink at same len

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.

I believe this makes sense,

one q re clone

  • lint issue

if error_call_result.reverted {
let is_success = executor.is_raw_call_success(
self.addr,
call_result.state_changeset.clone().unwrap(),
Copy link
Member

Choose a reason for hiding this comment

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

do we need the clone here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the next param is &call_result which borrows the value so if not cloning fails with

225 |                     &call_result,
    |                     ^^^^^^^^^^^^ value borrowed here after partial move

Copy link
Member

Choose a reason for hiding this comment

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

hmm, the pub fn is_raw_call_success( fn is not very good, I see why we need the state as a separate arg

but this means for this call, we can use mut call_result.state_changeset.take().unwrap()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thank you, this makes sense, I made following change in 9d2b073 + made test more robust as they can change in less steps

@mattsse mattsse merged commit 6ca3734 into foundry-rs:master Feb 28, 2024
19 checks passed
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.

3 participants