-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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(test): only compile files needed for tests #7334
feat(test): only compile files needed for tests #7334
Conversation
Awesome, will test this!
Good callout. Is a potential fix here for forge to error if it sees a contract name in |
Oh, that's a nice idea. Yeah, we can manage the list of contracts which were compiled before current test run and only allow I don't think that this will cause issues with Vyper: afaik people are using ffi to compile and get bytecode of Vyper contract (like here, for example: https://github.com/pcaversaccio/snekmate/blob/main/lib/utils/VyperDeployer.sol#L53-L80) |
I still think there are cases where it might cause issues, like seaport, where they use one profile to compile, then another to test: https://github.com/ProjectOpenSea/seaport?tab=readme-ov-file#foundry-tests (cc @emo-eth to confirm since you used to work on seaport) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 on @mds1 suggestion
this is now always invoked?
the overhead should be minimal?
perhaps we can skip this if there's no filter?
crates/forge/bin/cmd/test/mod.rs
Outdated
project.solc_config.settings.output_selection.0 = BTreeMap::from([( | ||
"*".to_string(), | ||
BTreeMap::from([("*".to_string(), vec!["abi".to_string()])]), | ||
)]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's an easier way to do this, if not we should make this easier in foundry-compilers
After #87 we'll spend additional time only obtaining abi for dirty sources. If there's no filters we will filеer out contracts which don't have any invariant/test functions which reduces the full compilation time and is reasonable imo This additional step results in ~4-5% build time increase for normal builds and in <1% increase for via-ir builds, so it should worth it |
8da2d53
to
dadce48
Compare
I've added logic for runtime validation of This should make those cheatcodes impl more stable as we won't make any assumptions on artifact location. I've also added ability to pass artifacts in the format of This check can be disabled with |
Ref foundry-rs/foundry#7334 (comment) with that helper it will be `OutputSelection::common_output_selection(["abi".to_string()])`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me, remember to close #7045 (and other issues if you find some) with this pr
ec2063d
to
5c4e204
Compare
Some checks are failing after compilers update, working on fixing those |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
amazing
Testing on optimism codebase using a test suite that is pretty self contained and does not require most of the codebase: Before this PRUsing 9f6bb3b
After this PRUsing e758bd2, looks like it could not find the test
|
@mds1 can't really reproduce this failure locally, getting:
can you check if issue still persists with purged cache and share logs with |
Confirmed it is working now 👍
|
* feat(forge test): only compile files needed for tests * remove comment * clippy * update fixtures * getCode + getDeployedCode updates * fixes * fix path matching * clippy * add config flag * fix * docs * fmt * patch compilers * fix Cargo.toml * update patch * update patch * doc * rm space * cargo cheats * new output selection fn * log compiler errors on failure * fixes
* chore: make cast use an alloy provider * move initial methods to alloy * feat(`foundry-common`): NameOrAddress ENS util (#7122) * feat(foundry-common): NameOrAddress ENS util * chore: rename err * chore: remove from impl for str * chore: unrelated fix from alloy upgrade * nit * feat(`cast`): Move non `tx` methods to alloy (#7129) * chore: add alloy contract * feat(cast): migrate most methods to alloy * chore: leave todo for converting a tx envelope into an rpc tx * fix: use proper type for storage * readd decodetx for now * chore: extend txbuilder to build an alloy tx request * feat: migrate most methods bar send/decode raw tx * fix: include tx data * simplify txbuilder * chore: simplify back access_list * chore: remove unnecesary conversion * fmt * doctests * fmt * do not use trait * Update crates/cast/bin/main.rs Co-authored-by: Matthias Seitz <[email protected]> * cleanup builder * clippy * fix doc comments --------- Co-authored-by: Matthias Seitz <[email protected]> * DocumentMut * wip * wip * wip: bump alloy * wip * wip * wip * [wip] migrate to alloy providers and signers (#7425) wip * fix wallets after alloy bump * clean up deps * use serde on consensus types * update TypedTransaction for anvil * make anvil compile * wip: make script compile * fix script * make forge compile * fix: anvil tests * bump alloy * fix tests * fix tx builder * fix cargo.toml * fix cargo.toml * fix script gas price logic * remove ethers from anvil * clippy * rm all_derives * deps * fmt * fix tests * configure clippy * clippy * add feature * fix cargo deny * fix persist * fix doctests * fmt * fix clap * review fixes * fmt * bump alloy * Update cargo.toml * fmt * fixes * ethers clean-up * fix(fmt): fix indent closing parenthesis enclosed in { } (#7557) * fix(fmt): fix indent closing parenthesis enclosed in { } * Fix testdata bad formatting * feat(test): only compile files needed for tests (#7334) * feat(forge test): only compile files needed for tests * remove comment * clippy * update fixtures * getCode + getDeployedCode updates * fixes * fix path matching * clippy * add config flag * fix * docs * fmt * patch compilers * fix Cargo.toml * update patch * update patch * doc * rm space * cargo cheats * new output selection fn * log compiler errors on failure * fixes * fix: do not flood dictionary with data dependent on fuzz inputs (#7552) * fix dictionary * clippy + fmt * fix * Feat: Index cheatcode for Strings (#7539) * feat: index cheatcode * some nits to make it work * nit: use as_str() * final changes * chore: reviewed changes * chore: reduce logs in tests (#7566) * fix(script): decode custom error in script fail message (#7563) * clippy * bump alloy * AnyNetwork * bump alloy * add comment * clippy * bump alloy * fixes * refactor cast logs to use alloy (#7594) * refactor cast logs to use alloy * fmt * make clippy happy * cleanup * doc nits --------- Co-authored-by: evalir <[email protected]> --------- Co-authored-by: Matthias Seitz <[email protected]> Co-authored-by: Arsenii Kulikov <[email protected]> Co-authored-by: grandizzy <[email protected]> Co-authored-by: Krishang <[email protected]> Co-authored-by: DaniPopes <[email protected]> Co-authored-by: bernard-wagner <[email protected]>
Motivation
Compile contracts requesting only abi output and filter out abis which match given filters or just start with
test
/invariant
This allowed to move some checks to be performed earlier ("No tests match the provided pattern")
This might be breaking for some codebases using
vm.getDeployedCode
as we won't recompile contracts which are not appearing as dependecy of test contract, however if contract being fetched throughvm.getDeployedCode
is imported from test file, then it will still be recompiled and I believe that it's a common pattern used, for example, in optimism codebase (https://github.com/ethereum-optimism/optimism/blob/53573e0ea6a807a125784cc5c7df07cbb4dbe3bc/packages/contracts-bedrock/scripts/Deployer.sol#L25)Closes #7045
cc @mds1