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(evm): add shrinking to invariant testing #2745

Merged
merged 3 commits into from
Aug 15, 2022

Conversation

joshieDo
Copy link
Collaborator

Motivation

Traces from invariant failures can really be long. Especially with a high invariant_depth value. This aims to reduce the sequence to its minimum representation.

Solution

Add some basic shrinking:

/// Tries to shrink the failure case to its smallest sequence of calls.
///
/// Sets an anchor at the beginning (index=0) and tries to remove all other calls one by one,
/// until it reaches the last one. The elements which were removed and lead to a failure are
/// kept in the removal list. The removed ones that didn't lead to a failure are inserted
/// back into the sequence.
///
/// Once it reaches the end, it increments the anchor, resets the removal list and starts the
/// same process again.
///
/// Returns the smallest sequence found.

Is there a need for it to be optional?

@joshieDo joshieDo added A-testing Area: testing T-feature Type: feature Cmd-forge-test Command: forge test labels Aug 12, 2022
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.

sounds reasonable,
wdyt @onbjerg?

Copy link
Member

@gakonst gakonst left a comment

Choose a reason for hiding this comment

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

should we add a unit test which ensures that the number of calls actually is shrinked?

Comment on lines 147 to 148
// Checks the invariant.
if let Some(func) = &self.func {
Copy link
Member

Choose a reason for hiding this comment

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

Nit - can pull this outside of the loop and return false early, before iterating?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Slightly modified it, but the goal here is to check that the invariant has, in fact, been broken after a call.

So pulling this outside of the loop is not what we want

}
Some(element)
})
.collect::<Vec<_>>();
Copy link
Member

Choose a reason for hiding this comment

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

nit: you don't need to collect here given you'll iterate over this immutably again in fails_successfully

Copy link
Member

@onbjerg onbjerg left a comment

Choose a reason for hiding this comment

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

the calls for invariant tests should be a proptest ValueTree/Strategy. the shrinking logic is in ValueTree::complicate (more calls?) and ValueTree::simplify. i think looking at how the value trees for a Vec<T> works would suffice. generally this looks ok but I'd prefer we use the "standard" approaches if possible so it's easier to migrate later/maintain as opposed to it being a totally separate thing

proptest will already rerun the failing case and call complicate/simplify to try and shrink the example, so a lot of this logic essentially duplicates that behavior

@joshieDo
Copy link
Collaborator Author

We are currently using proptest only for strategy/value generation on invariant tests.

Might be my inexperience with the library, but at the time of the invariant implementation I hit some limitations, which lead me to go around its standard use.

Example, for a run of depth 15, I want to be able to generate 15 values lazily (as we fill the dictionary with more in-context values), instead of getting all 15 values straightaway. Which would disregard any newly generated contracts, for example. Context:

// The strategy only comes with the first `input`. We fill the rest of the `inputs`
// until the desired `depth` so we can use the evolving fuzz dictionary
// during the run. We need another proptest runner to query for random
// values.

from inside test_runner.run(| | {

// Generates the next call from the run using the recently updated
// dictionary.
inputs.extend(
strat
.new_tree(&mut branch_runner.borrow_mut())
.map_err(|_| TestCaseError::Fail("Could not generate case".into()))?
.current(),
);
}

This however, made it impossible to use the shrink functionality, since proptest will try to shrink with only the first generated input. That's why there's a line with .no_shrink() somewhere.

@onbjerg
Copy link
Member

onbjerg commented Aug 15, 2022

@joshieDo That's not entirely correct - proptest entirely discards the ValueTree on every test case, so the "first generated value" will be unique every time the closure we give proptest is run. In other words, the "first generated value" is in the context of a single test case (1 run), not the entire test (256 runs). All of the logic around ValueTree's are only really run whenever a test case fails to try and shrink it

@onbjerg
Copy link
Member

onbjerg commented Aug 15, 2022

Talked async w @joshieDo, I don't have enough of an overview of how the primary strategy works for us to meaningfully progress on possibly using proptest for the shrinking, so you can ignore my comments on the PR - we can look at it later

@gakonst gakonst merged commit 44cd200 into foundry-rs:master Aug 15, 2022
iFrostizz pushed a commit to iFrostizz/foundry that referenced this pull request Nov 9, 2022
* add shrinking

* collect modified sequence once

* add test for shrinking
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testing Area: testing Cmd-forge-test Command: forge test T-feature Type: feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants