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

Table tests #858

Open
onbjerg opened this issue Mar 7, 2022 · 27 comments
Open

Table tests #858

onbjerg opened this issue Mar 7, 2022 · 27 comments
Labels
C-forge Command: forge Cmd-forge-test Command: forge test D-average Difficulty: average P-low Priority: low T-feature Type: feature

Comments

@onbjerg
Copy link
Member

onbjerg commented Mar 7, 2022

Component

Forge

Describe the feature you would like

Table tests are a way to generate test cases based on a dataset of parameters (the table), enabling code reuse in tests that use the same assertions but with predefined data. An example of a table test can be found here.

There are a few ways to go about this (after brainstorming with @gakonst) these seem like the most viable options:

Setup/run functions

This approach uses a setUpTable* function that returns the table with testdata and a testTable* that runs assertions on each entry in the table. Due to constraints with Solidity (again) the UX is going to suck a bit:

function setUpTableSums() public returns (bytes[] memory) {
  bytes[] memory entries = new bytes[](2);
  entries[0] = abi.encode(1, 2, 3);
  entries[1] = abi.encode(4, 5, 9);
}

function testTableSums(uint256 a, uint256 b, uint256 expected) public {
  assertEq(a + b, expected);
}

The setUpTable functions are called once during setup and the table is stored in-memory. The testTable function is then called in parallel with each entry in the table. If an entry fails we should mark the table test as a failure and provide some info on what entries failed.

The setUpTable functions returns an array of bytes: each entry in this array is calldata we pass directly to the table test function by prepending the signature of the table test function.

Pros:

  • Solidity-only

Cons:

  • Uses ABI encode

External files

Same as above, but the table is in a file named after some convention. Each entry is ABI encoded and then passed to the table test function

Pros:

  • No ABI encode

Cons:

  • Uses external files

Additional context

No response

@onbjerg onbjerg added T-feature Type: feature Cmd-forge-test Command: forge test C-forge Command: forge P-low Priority: low D-average Difficulty: average labels Mar 7, 2022
@gakonst
Copy link
Member

gakonst commented Mar 7, 2022

Any reason why we cannot avoid the abi encode like this?

struct Case {
   uint256 a;
   uint256 b;
   uint256 expected;
}

function setUpTableSums() public returns (Case[] memory) {
  Case[] memory entries = new Case[](2);
  entries[0] = Case(1, 2, 3);
  entries[1] = Case(4, 5, 9);
}

function testTableSums(Case memory case) public {
  assertEq(case.a + case.b, case.expected);
}

@onbjerg
Copy link
Member Author

onbjerg commented Mar 7, 2022

Hmm, thinking about it, you're right. Since we have the ABI we would know how to serialize/deserialize the return data

@d-xo
Copy link

d-xo commented Mar 24, 2022

Maybe you could use some magic modifiers that you pull out of the solidity AST at test time? The following could be interpreted as "fuzz testTableSums, always using the provided concrete inputs as a part of the fuzzing campaign".

modifier inputSums(uint a, uint b, uint expected) { _; }

function testTableSums(uint a, uint b, uint expected) 
  public
  inputSums(1, 2, 3)
  inputSums(4, 5, 9)
{
  assertEq(a + b, expected);
}

@d-xo
Copy link

d-xo commented Mar 24, 2022

I guess it should also be possible to parse some custom natspec comments with a list of inputs?

@emilianobonassi
Copy link

emilianobonassi commented Mar 24, 2022

+1 to this feature

as a workaround i am actually using a modifier iterating over cases

struct Case {
  uint256 a;
  uint256 b;
  uint256 expected;
}

Case[] entries;

Case c;

modifier parametrizedTest() {
  for (uint256 i = 0; i < entries.length; i++) {
    c = entries[i];
    _;
  }
}

function setUp() public override {
  entries.push(Case(1, 2, 3));
  entries.push(Case(4, 5, 9));
}

function testTableSums() public parametrizedTest {
  assertEq(c.a + c.b, c.expected);
}

@sambacha
Copy link
Contributor

I guess it should also be possible to parse some custom natspec comments with a list of inputs?

0.8.13 supports the natspec directive of custom now

// from https://github.com/contractshark/vscode-solidity-extenstion/pull/7/files#diff-9b75242a14d24953968395cc5a946df5d3bdda1043d761a91e8d3f10157aa016R117-R120

// https://docs.soliditylang.org/en/latest/natspec-format.html#tags
  "natspec-tag-custom": {
            "match": "(@custom:)\\b",
            "name": "storage.type.custom.natspec"
        },

something like @custom:requires $TEST_ACCOUNT etc etc see #979

@PaulRBerg
Copy link
Contributor

+1 for this feature. I'm porting my math library to Foundry and there are lots and lots of situations where table tests would be useful.

Possibly dumb question, but couldn't we abstract away the ABI encoding in the case of integers? I imagine that the most needed variable type for table tests will be uint256.

function setUpTableSums() public returns (uint256[] memory) {
    uint256[] memory entries = new uint256[](2);
    entries[0] = 4;
    entries[1] = 5;
}

Couldn't Foundry ABI encode and decode these, under the hood?

@PaulRBerg
Copy link
Contributor

In the meantime, if you're using @emilianobonassi's solution (which is great!) and compiling with Solidity v0.8, consider wrapping the increment in an unchecked block:

modifier parametrizedTest() {
    for (uint256 i = 0; i < entries.length) {
        c = entries[i];
        _;
        unchecked {
            i += 1;
        }
    }
}

This should make the execution slightly faster, especially if you have lots of test cases.

@ChainsightLabs
Copy link

Any reason why we cannot avoid the abi encode like this?

struct Case {
   uint256 a;
   uint256 b;
   uint256 expected;
}

function setUpTableSums() public returns (Case[] memory) {
  Case[] memory entries = new Case[](2);
  entries[0] = Case(1, 2, 3);
  entries[1] = Case(4, 5, 9);
}

function testTableSums(Case memory case) public {
  assertEq(case.a + case.b, case.expected);
}

This pattern looks good. It lets the user define their own table entry structure as fancy or dumbed down as they like.

We would definitely use table tests to set up test inputs that reside inside .csv files or json tables. You can do it now, but it is hard to parallelize and can take quite some time if you are forking on many blocks with the parallelization.

@ChainsightLabs
Copy link

In the meantime, if you're using @emilianobonassi's solution (which is great!) and compiling with Solidity v0.8, consider wrapping the increment in an unchecked block:

modifier parametrizedTest() {
    for (uint256 i = 0; i < entries.length) {
        c = entries[i];
        _;
        unchecked {
            i += 1;
        }
    }
}

This should make the execution slightly faster, especially if you have lots of test cases.

Unfortunately, I think this solution does not parallelize each entry in the evm test runner, right?

@PaulRBerg
Copy link
Contributor

@ChainsightLabs it doesn't, but it's fast enough for my needs.

@PaulRBerg
Copy link
Contributor

PaulRBerg commented Nov 17, 2022

One downside to @emilianobonassi's solution is that it fudges the gas reports. Extra gas has to be spent on pushing the entries in the storage array.

Unfortunately I can't think of any workaround to this issue .. except for Foundry supporting native table tests and handling the gas metering for us.

@emilianobonassi
Copy link

One downside to @emilianobonassi's solution is that it fudges the gas reports. Extra gas has to be spent on pushing the entries in the storage array.

Unfortunately I can't think of any workaround to this issue .. except for Foundry supporting native table tests and handling the gas metering for us.

yep @PaulRBerg

not sure how much will save but you can delete the array entries after the execution in the modifier

another proposal would be do a smoketest which print how much gas a noop test will need for your cases so you can offset from the real tests (this is what i actually use)

another one, but we need to await for eip inclusion, is to support transient storage tload/tstore

@PaulRBerg
Copy link
Contributor

PaulRBerg commented Nov 17, 2022

Deleting the array doesn't reduce the gas cost significantly (due to EIP-3529).

Transient storage will be great when it is finally available.

Interesting idea with the smoketest but it wouldn't work in my case because I have many different numbers of array entries per test. There's no baseline "noop test".

@emilianobonassi
Copy link

not sure yet possible in foundry but a cheatcode to change gasleft offsetting in the modifier will make it

the cheatcode will be useful too to test gas-dependant logics

@PaulRBerg
Copy link
Contributor

Big brain idea.

@emilianobonassi
Copy link

should not be hard

underlying evm supports gas reset

https://github.com/bluealloy/revm/blob/main/crates/revm/src/gas.rs#L45

should be properly exposed as cheatcode

https://github.com/foundry-rs/foundry/blob/master/evm/src/executor/inspector/cheatcodes/mod.rs#L177

@emilianobonassi
Copy link

related to #2429

@gakonst
Copy link
Member

gakonst commented Nov 17, 2022

Any reason why we cannot avoid the abi encode like this?

struct Case {
   uint256 a;
   uint256 b;
   uint256 expected;
}

function setUpTableSums() public returns (Case[] memory) {
  Case[] memory entries = new Case[](2);
  entries[0] = Case(1, 2, 3);
  entries[1] = Case(4, 5, 9);
}

function testTableSums(Case memory case) public {
  assertEq(case.a + case.b, case.expected);
}

Feeling strongly about this being the "right" solution here but open minded to alternatives.

@ChainsightLabs
Copy link

I keep coming upon use cases for this feature, does anyone else agree that the priority could be bumped up from Low to something higher?

@gakonst
Copy link
Member

gakonst commented Nov 19, 2022

Sounds good - happy to provide resources for reviewing the PR if somebody would be up for taking a first stab at it. Need to think about if we can actually prioritize this - please give me some time on that.

@PaulRBerg
Copy link
Contributor

PaulRBerg commented Nov 21, 2022

Just bumped into another limitation of the parametrizedTest modifier solution proposed above - it doesn't work for tests that expect reverts!

function testTableSums() public parametrizedTest {
    vm.expectRevert();
    callSomeFunction(c.a, c.b);
}

The test will show as passed, but it will stop looking for additional reverts once it reaches the first revert.

This is probably related to #3723.

Update: yes, can confirm that this problem is related to #3723, albeit in a very specific circumstance. When you are testing free functions, you cannot have multiple vm.expectRevert statements for multiple calls to free functions.

@ChainsightLabs
Copy link

bump

@PaulRBerg
Copy link
Contributor

@ChainsightLabs in the meantime, take a look at how I implemented parameterized tests in my math library PRBMath:

I have extensively used the parameterizedTest approach suggested above.

@grandizzy
Copy link
Collaborator

this looks a lot like the new fixtures feature that was introduced recently https://book.getfoundry.sh/forge/fuzz-testing#fuzz-test-fixtures can people check it and share thoughts?

@zerosnacks
Copy link
Member

bump, would be great to get feedback on https://book.getfoundry.sh/forge/fuzz-testing#fuzz-test-fixtures

@sakulstra
Copy link
Contributor

@zerosnacks tried using this and while i think it's useful it's quite different to how i'd use table tests in other systems.

If I have a function function test_input(uint256 amount) and i want to test it with the values 0..128, fuzz with fixtures just seems suboptimal.
I cannot really archive this with the current means i think.
What i'd need to do is:

  • initialize some state var and push values 0..128
  • perhaps limit the fuzz /// forge-config: default.fuzz.runs = 128, but then there is no guarantee that all values will eventually be iterated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-forge Command: forge Cmd-forge-test Command: forge test D-average Difficulty: average P-low Priority: low T-feature Type: feature
Projects
Status: Todo
Development

No branches or pull requests

10 participants