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(test): allow custom txes before unit and fuzz test #8497

Merged
merged 12 commits into from
Jul 26, 2024

Conversation

grandizzy
Copy link
Collaborator

@grandizzy grandizzy commented Jul 23, 2024

Motivation

Closes #8485
Closes #1543

Atm unit / fuzz tests are performed only against the state resulting from setup call, but would be nice to support configuration of other txes / state modification for individual tests. This PR adds the possibility of per unit / fuzz test configuration of before test selectors / txes.

Configuration can be specified by implementing beforeTestSetup function

function beforeTestSetup(bytes4 testSelector) public pure returns (bytes[] memory beforeTestCalldata)

where

  • bytes4 testSelector is the selector of the test that will run on before txes modified state
  • bytes[] memory beforeTestCalldata is an array of arbitrary calldatas that will be applied before test run
    E.g. for configuring testC to be executed on state modified by testA and setB(uint256) functions the beforeTestCalldata can be implemented as
    function beforeTestSetup(bytes4 testSelector) public pure returns (bytes[] memory beforeTestCalldata) {
        if (testSelector == this.testC.selector) {
            beforeTestCalldata = new bytes[](2);
            beforeTestCalldata[0] = abi.encodePacked(this.testA.selector);
            beforeTestCalldata[1] = abi.encodeWithSignature("setB(uint256)", 111);
        }
    }
  • To continue test execution the before test call should be success.

Solution

@grandizzy grandizzy marked this pull request as ready for review July 23, 2024 11:39
crates/evm/evm/src/executors/mod.rs Outdated Show resolved Hide resolved
crates/forge/src/runner.rs Outdated Show resolved Hide resolved
- do not unwrap func
- check if `beforeTestSelectors` exists
- move logic in prepare_unit_test fn
- apply same logic to fuzz tests
@grandizzy grandizzy changed the title feat(test): allow custom txes before unit test feat(test): allow custom txes before unit and fuzz test Jul 23, 2024
crates/common/src/traits.rs Outdated Show resolved Hide resolved
@grandizzy grandizzy requested a review from DaniPopes July 23, 2024 16:24
@grandizzy grandizzy marked this pull request as draft July 25, 2024 14:41
@grandizzy grandizzy marked this pull request as ready for review July 25, 2024 15:06
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.

the new comments are very helpful, ty

crates/forge/src/runner.rs Outdated Show resolved Hide resolved
Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>
crates/forge/src/runner.rs Outdated Show resolved Hide resolved
// Apply before test configured functions (if any).
let before_test_fns: Vec<_> =
self.contract.abi.functions().filter(|func| func.name.is_before_test_setup()).collect();
if before_test_fns.len() == 1 {
Copy link
Member

Choose a reason for hiding this comment

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

we should generalize this "check fn exists in abi -> call" to all the callSolDefault, can be followup

crates/forge/src/runner.rs Outdated Show resolved Hide resolved
@grandizzy grandizzy requested a review from DaniPopes July 26, 2024 09:56
crates/forge/src/runner.rs Outdated Show resolved Hide resolved
@grandizzy grandizzy requested a review from DaniPopes July 26, 2024 10:13
@grandizzy grandizzy enabled auto-merge (squash) July 26, 2024 10:17
@grandizzy grandizzy merged commit 4a41367 into foundry-rs:master Jul 26, 2024
20 checks passed
@grandizzy grandizzy deleted the before-test branch July 26, 2024 13:06
@mds1
Copy link
Collaborator

mds1 commented Jul 26, 2024

What does this give us over just having users write a regular solidity function that they call at the beginning of a test? You can always use --isolate mode to separate out transactions. (I only skimmed the linked issues as I catch up on some github notifications so apologies if I missed the rationale).

Edit: rereading it, I'm unclear if this executes before setUp, or between setUp and the test itself? In either case there are workarounds though through standard solidity patterns

@grandizzy
Copy link
Collaborator Author

What does this give us over just having users write a regular solidity function that they call at the beginning of a test? You can always use --isolate mode to separate out transactions. (I only skimmed the linked issues as I catch up on some github notifications so apologies if I missed the rationale).

Edit: rereading it, I'm unclear if this executes before setUp, or between setUp and the test itself? In either case there are workarounds though through standard solidity patterns

@mds1 I added docs here, pls lmk if this makes sense
https://book.getfoundry.sh/forge/writing-tests#before-test-setups

@mds1
Copy link
Collaborator

mds1 commented Jul 26, 2024

In those docs, wouldn't an alternate implementation of the first code block simply be the below? The below feels more clear to read, since the beforeTestSetup can be obfuscated when it lives in a contract you inherit from, so test behavior is less obvious

// run with `forge test --isolate` if you need tx side effects like selfdestruct
contract ContractTest is Test {
    uint256 a;
    uint256 b;

    function testA() public {
        require(a == 0);
        a += 1;
    }

    function setB(uint256 value) public {
        b = value;
    }

    function testC() public {
        testA();
        setB(1);
        assertEq(a, 1);
        assertEq(b, 1);
    }
}

@grandizzy
Copy link
Collaborator Author

grandizzy commented Jul 26, 2024

So could #1543 scenario be accomplished with isolate?

@mds1
Copy link
Collaborator

mds1 commented Jul 26, 2024

I believe it should, cc @klkvr who implemented isolate mode to confirm

@gsalzer
Copy link

gsalzer commented Jul 29, 2024

In those docs, wouldn't an alternate implementation of the first code block simply be the below? The below feels more clear to read, since the beforeTestSetup can be obfuscated when it lives in a contract you inherit from, so test behavior is less obvious

@mds1 Is it possible to specify isolation mode within the test file, maybe even for a single test alone? Like by asserting at the beginning of a test that we are in isolation mode, or by setting it programmatically? I didn't find it in the docs.

One nice thing about beforeTestSetup is that it explicitly fixes the intended transaction patterns for the tests. If the isolation flag has to be set in the foundry environment, then the execution of the test with wrong flags might show passed tests, which actually would fail with the correct options. On the other hand, if we consider foundry.toml as part of the test specification, then we can set the isolation flag there.

@gsalzer
Copy link

gsalzer commented Jul 29, 2024

@mds1 It seems that your version of the code with isolation behaves differently from the example in the foundry book using beforeTestSetup. The debug output shows separate tx for the latter, but only one for the version with isolation.

$ forge test --match-path mds1.t.sol --isolate -vvvvv
[⠒] Compiling...
No files changed, compilation skipped

Ran 2 tests for test/mds1.t.sol:ContractTest
[PASS] testA() (gas: 22553)
Traces:
  [22553] ContractTest::testA()
    └─ ← [Stop] 

[PASS] testC() (gas: 44986)
Traces:
  [44986] ContractTest::testC()
    └─ ← [Stop] 
$ forge test --match-path fbook.t.sol -vvvvv
[⠒] Compiling...
No files changed, compilation skipped

Ran 2 tests for test/fbook.t.sol:ContractTest
[PASS] testA() (gas: 22487)
Traces:
  [22487] ContractTest::testA()
    └─ ← [Stop] 

[PASS] testC() (gas: 4524)
Traces:
  [22487] ContractTest::testA()
    └─ ← [Stop] 

  [22336] ContractTest::setB(1)
    └─ ← [Stop] 

  [4524] ContractTest::testC()
    └─ ← [Stop] 
$ forge --version
forge 0.2.0 (6822860 2024-07-29T00:20:30.057117769Z)

benwjhack pushed a commit to CompassLabs/foundry-test that referenced this pull request Sep 11, 2024
)

* feat(test): allow performing txes before unit test

* Changes after review:
- do not unwrap func
- check if `beforeTestSelectors` exists
- move logic in prepare_unit_test fn
- apply same logic to fuzz tests

* Review: Before test is not a test kind

* Changes after review: beforeTestSetup new fn signature

* Remove obsolete struct from test

* Update crates/forge/src/runner.rs

Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>

* Changes after review: avoid executor clone

* Fix Cow::Borrowed usage

---------

Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants