Feature request: add a tag for the unit test coverage approach

[simon-something]

It would be interesting to find a way to specify the coverage used while building a tree (and reflect it in the test contract name or top-level comment).

For instance, for the following function and coverage type, we'd scaffold 2 tests contracts which we could name Foo_Test_Branch and Foo_Test_FullPath

bool foo (bool x, bool y) {
    if (a && b) {
        return true;
    }
    return false;
}

based on the following coverages (amongst other possibilities):

• branch coverage: where we're assessing if we covered every result of every decision structure (tested both true and false in an if for instance)

flowchart TD
    A[foo] --> C{a && b}
    C -->|True| D[return true]
    C -->|False| E[return false]

Loading

with the tree

TestMe::foo(branch)
├── when a and b are true
│   └── it should return true
└── when a is false
    └── it should return false

• path coverage: where we're assessing if we are covering each flow within the function under test

flowchart TD
    A[foo] --> C{a}
    C -->|True| D{b}
    C -->|False| E{b}
    D -->|True| F(true)
    D -->|False| G(false)
    E -->|False| H(false)
    E -->|False| I(false)

Loading

with the following tree

TestMe::foo(full path)
├── given a is true
│   ├── when b is true
│   │   └── it should return true
│   └── when b is false
│       └── it should return false
└── given a is false
    ├── when b is true
    │   └── it should return false
    └── when b is false
        └── it should return false

[alexfertel]

I see, that sounds super interesting. It's a tiny bit big in scope, but I think it's reasonable.

Could you give a more practical example? If I understand correctly, full path contains branch, so it's not that useful.

Also, it would be nice to get @PaulRBerg 's opinion as well.

[simon-something]

I see, that sounds super interesting. It's a tiny bit big in scope, but I think it's reasonable.

Could you give a more practical example? If I understand correctly, full path contains branch, so it's not that useful.

Also, it would be nice to get @PaulRBerg 's opinion as well.

Modified the op with something hopefully more illustrative.

There is indeed an inclusion relation between branch and path, as a full path coverage should be the most exhaustive possible coverage (iirc, one can compute the number of path, as it's just some graph theory), but

1. not always doable (see https://www.inf.ed.ac.uk/teaching/courses/st/2017-18/Path-coverage.pdf for a pretty cool example)
2. sometime has some paths which are not carrying any new information (2 conditions which are fully independent for instance)

This feature request is quite small tho, as it's basically including a tag to the contract (without breaking check down the line)

[alexfertel]

not always doable (see inf.ed.ac.uk/teaching/courses/st/2017-18/Path-coverage.pdf for a pretty cool example)

Ah, this is great, now I see it clearly, thanks for sharing!

This is really, really useful, so we should actually prioritize it. I think we need to design a bit how it will look like though, not convinced that emitting different contracts is the way.

[alexfertel]

Just to be clear, having two different trees for the same function in the same .tree is not enough for this use case?

TestMe::foo // branch
├── when a and b are true
│   └── it should return true
└── when a is false
    └── it should return false
   
TestMe::foo // Full path
├── given a is true
│   ├── when b is true
│   │   └── it should return true
│   └── when b is false
│       └── it should return false
└── given a is false
    ├── when b is true
    │   └── it should return false
    └── when b is false
        └── it should return false

generates

contract TestMe {
    function test_FooWhenAAndBAreTrue() external {
        // it should return true
    }

    function test_FooWhenAIsFalse() external {
        // it should return false
    }

    modifier givenAIsTrue() {
        _;
    }

    function test_FooWhenBIsTrue() external givenAIsTrue {
        // it should return true
    }

    function test_FooWhenBIsFalse() external givenAIsTrue {
        // it should return false
    }

    modifier givenAIsFalse() {
        _;
    }

    function test_FooWhenBIsTrue2() external givenAIsFalse {
        // it should return false
    }

    function test_FooWhenBIsFalse2() external givenAIsFalse {
        // it should return false
    }
}

[simon-something]

You're right, it would be for this example, but in real use-case, one usually pick a single coverage (so this would rather pin the difference between different functions tested using different coverage - random example in the /trees/ files of this pr).

For instance, if you're testing a contract with 3 external functions, maybe 2 of them are super short/simple flow (and a path coverage is realistic) while the third one has a huge number of branches and conditional op (so branch coverage is probably better), imo, this difference should be documented somewhere.
I tend to write natspecs for my test function in forge, and add it there - maybe having the equivalent while scaffolding would make more sense (eg any comment at the root level or just above in the .tree is included in the test function natspecs as @dev for instance)

[simon-something]

NB this could be used for other test "tags" too actually (fuzz vs static for instance), so a tree comment which is translated to a /// @dev makes probably more sense

[alexfertel]

imo, this difference should be documented somewhere.

I see, I think this is the key point, right? It shouldn't be surprising to people reading which kind of coverage they are using.

I agree this should be clear.

An interesting thing to think about is how does the test evolve with the evolution of the function. Say that we used branch coverage initially for this function:

bool foo (bool a, bool b) {
    if (a && b) {
        return true;
    }
    return false;
}

TestMe::foo(branch)
├── when a and b are true
│   └── it should return true
└── when a is false
    └── it should return false

(which note that is not an entirely correct spec, because the second branch should be something like "Otherwise")

But then we change it to:

bool foo (bool a, bool b) {
    if (a) {
      if (b) {
	    return true;
      }
      return false;
    }
    else {
      if (b) {
        return false;
      }
      return true;
    }
}

(this function is just a == b, but it's a dumb example, bear with me)

Now path coverage is the easier way to maintain the spec.

NB this could be used for other test "tags" too actually (fuzz vs static for instance)

Could you expand a bit with an example of this? Sorry about all the back and forth, I want to make sure we get this right.

[PaulRBerg]

unfortunately not able to reviesw this proposal at this time, tagging @andreivladbrg and @smol-ninja in case they want to contribute.

[simon-something]

But then we change it to:

Path-coverage driven development :D ? But yes, you're now laying out the different path explicitly, not sure we should all start coding such tho 🥲

Could you expand a bit with an example of this? Sorry about all the back and forth, I want to make sure we get this right.

I don't do such separation personally (I rather tend to cover a given path then fuzz within it, a bit like how concolic fuzzing would work iic), but some repo's are maintaining different folder for static vs fuzzed tests, maybe interesting to have this kind of added doc for them too?

I had a bit of a shower thought and was thinking of closing this issue for another one with a slightly different scope (encompassing this one): _parsing the comments after a root or a when/given in the .tree file and emit them as @dev natspecs (for the contract or the test function). What do you think @alexfertel ? This could include any coverage or extra-info, replicated in both files

[alexfertel]

Sry for the delayed response!

_parsing the comments after a root or a when/given in the .tree file and emit them as @dev natspecs (for the contract or the test function). What do you think @alexfertel ? This could include any coverage or extra-info, replicated in both files

Hmmm, could you give a practical example of how this would look like?

[simon-something]

Sure! In other words, it's including the comments as additional dev natspecs (except for the it, as they're already translated to comments)

TestMe::foo // This is a branch coverage.
├── when a and b are true
│   └── it should return true
└── when a is false // b false would cover the same branch.
    └── it should return false

would generate

/// @dev This is a branch coverage.
contract TestMe {
    function test_FooWhenAAndBAreTrue() external {
        // it should return true
    }

    /// @dev b false would cover the same branch.
    function test_FooWhenAIsFalse() external {
        // it should return false
    }
}

[alexfertel]

Cool, that makes sense 👍

The example is not accurate though, because its are what is mapped 1 to 1 with test functions. Consider this dummy example:

TestMe::foo // This is a branch coverage.
└── when a is false // first comment.
	└── when b is false // second comment.
    	└── it should return false

which generates

contract TestMefoo {
    modifier whenAIsFalse() {
        _;
    }

    function test_WhenBIsFalse() external whenAIsFalse {
        // it should return false
    }
}

Which comment should become the natspec?

[simon-something]

I think this should become:

/// @dev This is a branch coverage.
contract TestMefoo {

    /// @dev first comment.
    modifier whenAIsFalse() {
        _;
    }

    /// @dev second comment.
    function test_WhenBIsFalse() external whenAIsFalse {
        // it should return false
    }
}

wdyt?

[alexfertel]

Sgtm, let's do it!

[alexfertel]

I think this doesn't need a CLI flag since it's additive and makes sense.

I'd rather we don't emit @dev because that way we can support users who don't want the @dev in the emitted comments. This is debatable though, if we want to be opinionated, that's fine too! Lmk what you think.

[alexfertel]

As always, if you need any help lmk. If you don't plan to work on it lmk as well.

[simon-something]

Hmm, true, natspecs only makes sense if docs are generated, which is perhaps not routinely done for tests.
I'll give it a go soon, few other deadlines before (and the 2 other issues I've opened;) Thx for the help!

[simon-something]

WIP branch: https://github.com/drgorillamd/bulloak/tree/feat/function-comments
I guess once tokenisation/parsing is done, it should be straightforward (famous last words)

[alexfertel]

It's looking good! Parsing is the roughest part of the code, so good luck! 🙈