Code coverage based on concrete tests only

[smol-ninja]

Currently, code coverage includes both fuzz and concrete tests. The fuzz tests require that functions are not reverting. That's why we use assume and bound in them. Thus, they only test for non reverting situations.

On the other hand, concrete tests is to test for each and every line and all possible branches, which is what coverage should be telling us. Currently, even if some of the branches are missed in concrete but called in fuzz, the coverage will not be able to detect that.

An example is missing notNull modifier in isTransferrable (cantina finding).

Thus, I propose to run coverage only using concrete tests.

RFC @sablier-labs/solidity.

Here is a comparison. As you can see the coverage for SablierFlow.sol went down when calculating it based on concrete tests, demonstrating that there are missing concrete tests for some lines / branches.

--mp "tests/{integration}/**/*.sol"	--mp "tests/integration/concrete/**/*.sol"

PS: similar suggestion for lockup.

[PaulRBerg]

IIRC, there were cases in Lockup circa 2023 when only the fuzz tests covered some branches because it was very, very difficult to get a concrete test to cover them.

But anyway, I will let you two decide and lead the way.

[andreivladbrg]

Thus, they only test for non reverting situations

that's correct, but since we have all reverting scenarios in concrete tests, how is the coverage affected?

concrete tests is to test for each and every line and all possible branches

this is not quiet possible as we have lines like this one:

flow/src/SablierFlow.sol

Lines 668 to 673 in 04f3ed6

	// Although the refundable amount should never exceed the balance, this condition is checked
	// to avoid exploits in case of a bug.
	if (refundableAmount > _streams[streamId].balance) {
	revert Errors.SablierFlow_InvalidCalculation(streamId, _streams[streamId].balance, amount);
	}

---

btw, one of the reasons for having a lower coverage on concrete compared to concrete + fuzz is due to missing getter tests, which would be addressed in #299 (in fuzz we use the getters often so the lines are covered) let's see how will these tests change this

[smol-ninja]

@andreivladbrg you were right. #299 increased coverage for SablierFlowBase, with concrete tests, to 100% on all accounts from (96%, 97%, 100%, 92%) before. However, it only increased coverage for SablierFlow by 0.2%-0.4% (just a note and no call to action at this time).

[andreivladbrg]

increased coverage for SablierFlowBase, with concrete tests, to 100% on all

great news

it only increased coverage for SablierFlow by 0.2%-0.4% (just a note and no call to action at this time).

oh, that's less that i would've estimated, interesting, thanks for the update