Testing branching rationale

[PaulRBerg]

Context

In #90, I have completely refactored our test files to more accurately reflect the testing branches written in the *.tree file. This discussion delves into the rationale that went into the refactor and how I arrived at the final version used in the PR.

Problem

The testing branches are not mirrored exactly in the testing files.

Solutions

Given this testing tree:

function.t.sol
├── when foo
│  └── it should revert
└── when not foo
   ├── when bar
   │  └── it should revert
   └── when not bar
      └── it should work

V1

You can see a full example in this GitHub gist: create-v1.t.sol.

SablierV2Linear__Function__Foo {}

SablierV2Linear__Function__NotFoo__Bar {}

SablierV2Linear__Function__NotFoo__NotBar {}

V2

You can see a full example in this GitHub gist: create-v2.t.sol.

SablierV2Linear__Function {
    function testCannot__Foo() external {}
    
    function testCannot__NotFoo__Bar() external {}
    
    function testCannot__NotFoo__NotBar() external {}
}

V3

You can see a full example in this GitHub gist: create-v3.t.sol.

SablierV2Linear__Function__Foo {}

contract NotFoo {}

SablierV2Linear__Function__Bar is NotFoo {}

contract NotBar {}

SablierV2Linear__Function is NotFoo, NotBar {}

My Decision

I have ultimately chosen V3, because it was the only that:

1. Scales
2. Remains readable at scale

V1 did not scale, because Forge enforces a limit on how large a contract name can be (os error 63), and it also wasn't very readable at scale, due to the long contract names. Whereas V2 scaled, in that Foundry could run the tests even with very long function names, but again the readability wasn't great.

The only slight problem with V3 is that in case where the tree branches in multiple directions (a good example is getWithdrawableAmount.tree), the contract names sort of have to be written using the V1 methodology, lest we get duplicates. But this shouldn't be a problem because usually the branching in one direction stops earlier than the bottom-most branch.

Screenshots

OS Error 63

[andreivladbrg]

I do not agree at all with this system of writing tests. It adds complexity and creates chaos.

A testing branch should be mirrored by a function test, not by a contract.

Why would you create this complex inheritance system in the first place when you can simply write explanatory comments?

Starting from the tree you've already written above:

/// when foo
function testFailFunction__foo() external{}

/// when not foo, when bar
function testFailFunction__bar() external{}

/// when not foo, when not bar
function testFunction() external{} 

Issues

1. Compilation time

Because you've added so much complexity to the code, the compilation time has increased by x4-5 times, this is absurd to me. Don't we use foundry because it is fast? You make it run in circles and it loses its beauty.

V3 compiling time

From this commit 01dd010:
[⠑] Solc 0.8.15 finished in 146.60s

Actual compiling time

From this commit 833bf92:
[⠒] Solc 0.8.15 finished in 36.03s

I've actually tested this multiple times and I've never gotten a time shorter than 2 minutes and 24 seconds from the v3 commit. You can say foundry saves the artifacts, but this fact is true when there are no changes in the code (only if you compile the same code). Even the slightest change will make you wait x4-5 more time for the compilation.

2. Chaotic display in terminal

Because each function is written in a different contract the tests will be displayed in a very messy way.

Instead of having the tests grouped much cleaner in the same contract.

Imagine how long would take to solve a failing test with the traces command.

3. Coding time

It increases the time of writing the tests.

Conclusion

From my point of view there is nothing objective to this way of structuring the tests, it even makes the work much harder.

[PaulRBerg]

I think you haven't read the rationale I presented in this comment?

Chaotic display in terminal

I agree that it's not as pretty as before, and likewise the tests are not grouped like they used to be, but at least in my experience it is rare that you need to run all the tests locally. Usually, you use the --match-contract and --match-test flags to isolate one specific test suite. Then, you can just push to remote, knowing that the CI system will take care of running all the tests.

Why would you create this complex inheritance system in the first place when you can simply write explanatory comments?

Because:

1. Comments create opportunities for debate regarding formatting, wording, etc., whereas by using contracts, you are restricted to the CamelCase naming, and the other idiosyncratic features of defining Solidity contracts.
2. Comments are not automatically formatted by Prettier.

Also, I don't think that complexity should ever be put forth as an argument against a potential improvement related to our testing system. It's fine for tests to be complex as long as they are clear and make it hard to fool ourselves - it's really just the production contracts that have to be gas efficient and easy to use.

Even the slightest change will make you wait x4-5 more time for the compilation.

Sure, but if that means you want 50ms instead of 10ms, surely you don't notice that time at all, do you?

When the artifacts are cached by Foundry (i.e. in the vast majority of cases), compilation time doesn't take a lot of time.

Imagine how long would take to solve a failing test with the traces command.

I've been using this system for over a week now, it's really easy to identify a failing test, because Foundry lists the failures at the bottom of the console output. There's also the --match-contract and --match-test flags.

It increases the time of writing the tests.

Actually, not, it's just a question of getting used to the new format. And that wouldn't take long. And as I explained, by virtue of being an objective naming system, we would actually save time by not mulling over comment formatting and function naming.

But even if this were true, it would actually be a GOOD thing for a new proposal to make us spend more time testing! Again, I emphasize how important it is now to fool ourselves while doing this.

From my point of view there is nothing objective to this way of structuring the tests, it even makes the work much harder.

This is an oxymoron. You cannot respond to a claim "X is objective" by saying that "In my opinion, X is not objective".

[PaulRBerg]

What do you think about this, @andreivladbrg?

V4

function testCannotFunction__Foo() external{}

modifier NotFoo { _; }

function testCannotFunction__Bar() NotFoo external{}

modifier NotBar { _; }

function testFunction() NotFoo NotBar external {} 

This should address your two main concerns.

1. It would maintain the grouping of tests in the console output (even if I personally don't find this important).
2. It shouldn't increase the compilation time because no additional contracts are created.

[andreivladbrg]

Yes, V4 would be much better than V3.

Even tho, I still think that we shouldn't use any of the solidity's functionalities and keep it simple with only comments.

Regarding comments, I don't think there is something to debate about, you just copy and paste the branch name from the .tree file.

V4 will work for me.

[PaulRBerg]

Ok, glad you like V4.

Besides the formatting issues discussed below, the major problem with comments is that they would be harder to maintain over time. Misspelled comments still compile just fine, whereas with modifiers, if the branch name is not updated correctly everywhere, the contract wouldn't compile because it would reference non-existent modifiers. This is why I'm so adamant about using Solidity syntax instead of comments.

Say for example that we realize that there is a logical mistake in a branch. If we use comments, refactoring that branch would take quite a few "find and replace" operations, and at the end you wouldn't be sure that you did in fact update all the places where the older branch was written.

Regarding comments, I don't think there is something to debate about, you just copy and paste the branch name from the .tree file.

There is. There is no automatic prettier formatting applied to comments. Therefore, we have to agree on several aspects:

1. Do we re-apply the tree symbols (│ and └──)?
2. Do we list the branches sequentially, or do we list them as an unordered list (with the - symbol)?
3. Do we put the branches before or after the actual test descriptor ("it should ...")

[andreivladbrg]

Given that we settled on v4, there is nothing left to say on my side.

[PaulRBerg]

Interestingly, someone else wrote an article about this approach already, though they have inspired from a tweet that I made a while ago:

Take your Solidity testing to the next level with state building!

[PaulRBerg]

Closing, we ended up using the "V4" technique.

[PaulRBerg]

For future reference: I will give a talk about this testing branching technique at EthCC.