feat: Group invariant campaigns by test contract#14844
Conversation
0xKarl98
requested review from
0xrusowsky,
DaniPopes,
grandizzy,
mattsse and
zerosnacks
as code owners
0xKarl98
requested review from
figtracer,
mablr and
stevencartavia
as code owners
| Suite assert_all: 2/2 invariants broken | ||
| ... | ||
| invariant_with_[..]() ([..]) | ||
| Suite predicates: |
There was a problem hiding this comment.
I think we should just say "Assertion Tests" and "Invariant/Property Tests"
There was a problem hiding this comment.
Maybe add the invariant contract name as well?
grandizzy
left a comment
grandizzy
left a comment
There was a problem hiding this comment.
thanks, that goes in the right direction! I think the target should be to get rid of assert_all altogether as now we assume campaign by test contract, hence redundant. Would also be nice to preserve the behavior of --mt to give the possibility of running single test & clean output for dev cycle (nice to have, not 100% sure if still possible?)
# Conflicts: # crates/evm/evm/src/executors/invariant/mod.rs
|
@ptal again , have addressed your concerns |
grandizzy
left a comment
grandizzy
left a comment
There was a problem hiding this comment.
looks good, only some nits, pls check
| @@ -797,6 +864,7 @@ impl<'a, FEN: FoundryEvmNetwork> FunctionRunner<'a, FEN> { | |||
| let runner = self.invariant_runner(); | |||
| let invariant_config = self.config.invariant.clone(); | |||
There was a problem hiding this comment.
The skip preflight just above this only checks the campaign anchor (func). With contract-level invariant campaigns, secondary predicates can also call vm.skip; those are now swallowed because we return before building per-predicate results. This shows up in test_cmd::skip_output: invariant_skipInvariantReason() disappears and the suite drops from 6 skipped to 5.
Suggested fix: preflight every predicate in invariants, collect skipped predicates as InvariantPredicateResult { status: TestStatus::Skipped, reason }, render skip reasons in write_invariant_predicate_results ([SKIP: reason] name), and exclude skipped predicates from the fuzz campaign. If all predicates skip, return a skipped campaign result carrying those predicate results.
| Invariant/Property Tests: 2/2 invariants broken | ||
| [FAIL: first broken] invariant_first | ||
| [FAIL: second broken] invariant_second | ||
| 2 invariant failure(s) persisted to /private/var/folders/p4/rl3tc43j51s311jt6pq7zq180000gn/T/[..]/cache/invariant/failures/StaleSecondaryTest — rerun to shrink |
There was a problem hiding this comment.
This hard-codes a macOS temp root, so the snapshot fails on Linux CI where the same logical path is under /tmp/<test-dir>/cache/invariant/failures/StaleSecondaryTest. Please redact the temp project root here, e.g. expect [..]/cache/invariant/failures/StaleSecondaryTest — rerun to shrink (and same below), so the assertion checks the invariant failure subpath without depending on host temp-dir layout.
|
Have addressed your nits , ptal again @decofe |
mattsse
left a comment
mattsse
left a comment
There was a problem hiding this comment.
Reviewed locally. The invariant campaign grouping looks acceptable to me, and cargo check -p forge --tests passed.
Amp-Thread-ID: https://ampcode.com/threads/T-019e5027-f55a-7699-a6d4-0c1fae6ff469 Co-authored-by: Amp <amp@ampcode.com>
| // anchor without searching by name. | ||
| let invariant_fns: Vec<(&Function, bool)> = live_invariants | ||
| .into_iter() | ||
| .filter(|(invariant_fn, _)| { |
There was a problem hiding this comment.
This drops selected secondary predicates with compatible persisted failures from the campaign without adding them to the result. I reproduced this: first broad --mt invariant_ persists a secondary failure, then the same broad run exits 0 and reports only the passing anchor. Persisted selected failures should still fail/report, not just warn.
| let Some((anchor, rest)) = funcs.split_first() else { | ||
| return true; | ||
| }; | ||
| let anchor_config = match self.inline_config(Some(anchor)) { |
There was a problem hiding this comment.
invariant_suite_configs_match only compares config.invariant, but merged campaigns run under the anchor’s executor config. I reproduced a secondary with forge-config: default.evm_version = "shanghai": it passes alone, but fails when merged under the anchor’s Cancun config. Please include executor-affecting inline config in compatibility, or don’t merge when function-level config differs.
### 🕓 Changelog This PR updates the version `pragma`s in all 🐍Vyper source files (`git ls-files '*.vy' '*.vyi' | wc -l`) to target 🐍Vyper's latest master version `0.5.0a3` (`git grep -l '=0.5.0a2' | xargs sed -i 's/=0.5.0a2/=0.5.0a3/g'`). In addition, it updates the MetaMask [`eth_signTypedData_v4`](https://docs.metamask.io/metamask-connect/evm/reference/json-rpc-api/eth_signTypedData_v4) reference URL to the current documentation path, removes the obsolete `assert_all = false` `foundry.toml` configuration, and updates the invariant comments following [foundry-rs/foundry#14844](foundry-rs/foundry#14844), which made all invariants in a contract share a single campaign by default. --------- Signed-off-by: pcaversaccio <pascal.caversaccio@hotmail.ch>
6 participants
On oss-272
Make the invariant test contract the campaign and corpus boundary for compatible boolean invariants. Per-predicate and per-handler failures are still reported individually, optimization invariants keep separate corpus state, and legacy failure caches remain readable with a warning. This also preserves counterexamples in JUnit output.