feat: track ignored tests

[mattsse]

Motivation

Closes #1123

filters "ignored" tests and include them in the test output.

what should the test output look like?

just

[IGNORED] <skip|ingore>testFuzz(uint256) ?

Solution

[onbjerg]

I think [SKIP] is better (works w/ pass and fail)

Edit: Also to keep in line with current naming conventions testSkip would probably also be the most appropriate :smile:

[gakonst]

+1 Skip

[mds1]

Ah good point, it would be nice to support contract-level skips too, but contract SkipMyContract would be a new naming approach + seems more likely to clash with a real contract name than a test prefix. I'd be ok with it, but could also be an argument to go the cheatcode route instead.

The cheatcode approach does give more flexibility, since you can have vm.skip(bool) and put some condition in there, so I think I've convinced myself that it's the better approach

[mattsse]

this adds `skip[tT]est.*´ naming convention to identify skipped test

output is now:

Running 1 test for src/dummy.sol:DummyTest
[SKIP] skipTestAnother()
[SKIP] skiptestCall()
[PASS] testCall() (gas: 179)
Test result: ok. 1 passed; 0 failed; 2 skipped; finished in 3.27ms

needs followup to integrates this on a Contract name level

[mattsse]

that's a good point, it would however make something like testEnsureWeCanSkip impossible, but that's fine I think

will revise

[onbjerg]

No it's only the start of the test function: testSkip*. However you bring up a good point separately: it would make something like testFail not skippable :( Not sure

[mattsse]

it is now test(.*)[sS]kip

for contracts, this means we should stick to <name>TestSkip

[mattsse]

No it's only the start of the test function: testSkip*. However you bring up a good point separately: it would make something like testFail not skippable :( Not sure

it would need to be testSkipFail then.

but not sure, I'd expect it to be either at the start or end

[mds1]

Some issues the naming convention approach may cause:

• forge-std has a test called testSkip to test it's skip helper method, so that will need to be renamed
• Maker has a parameter named skip and a corresponding test named test_cage_skip
• Fei has a test called testQueueSkipCycle
• I didn't find any that were testSkip* but I may have missed some

Found with https://sourcegraph.com/search?q=context:global+function+test%28.*%29%5BsS%5Dkip+lang:Solidity+&patternType=regexp

[mattsse]

thanks for this.

I don't have strong feelings about this convention, changing this would only require changing the TestFunctionExt::is_test_skipped impl

but I guess we narrowed it down to

• testSkip.*
• test(.*)Skip

following the convention for Fuzz, testSkip(Fuzz|Fail).* would make sense

[mds1]

Based on existing tests it seems testSkip.* makes the most sense. Though I do feel a cheatcode would be simpler UX since it doesn't require remembering the nuances on the ordering/naming, and makes --match-test simpler to use also

[gakonst]

Feel like we want this to be with a cheatcode. Also we got a bunch of things going on at the same time so vote close and reprioritize separately cc @mattsse

[mattsse]

sg, is not a critical feature so closing and turn this into cheatcode