Discussion around tests

[MerlinEgalite]

Context

Below is a part of the comment from @makcandrov about tests on this PR: #117. I'm creating this discussion to avoid polluting the PR.

Message

While refreshing this library, I came across many incoherent things and remnants of old designs and testing tricks. In order to refresh and harmonize all the files from this repository, I think that some other changes are needed, but I believe that they deserve separate PRs. They are:

• All the testing files use the extension .t.sol, which we don't use anymore in recent repositories.
• All the files use the old syntax of imports (import "./TestCommonHeapOrdering.t.sol"; instead of import {TestCommonHeapOrdering} from "./TestCommonHeapOrdering.t.sol";).
  EDIT: issue: Conform to Morpho's style #125 - pending fix: Code style change #126
• Maybe it would be better to gather all the invariant tests files in a separate folder under ./test/.
  EDIT: pending fix: Test folder partitioning #130
• Because we are testing libraries, we cannot make a relevant gas report if we directly call the internal functions on the heap. That's why some tests use a "concrete heap" contract (see ConcreteHeapOrdering.sol) that is essentially the "contract version" of the library. However, not all tests use this (see TestHeap.t.sol), and some of them add functions that are not in the library in the first place (see accountsValue, accountsId and indexOf from ConcreteHeapOrdering.sol and ConcreteThreeHeapOrdering.sol). In my opinion, all tests should use a "contract version" of the library but with strictly the same functions.
  EDIT: issue: Testing suite improvement #127 - pending partial fix: Heap tests improvement #131
• The testing suite generally lacks fuzzing.
  EDIT: issue: Testing suite improvement #127 - pending partial fix: Heap tests improvement #131
• Many tests check that the storage is exactly what we expect after a series of operations (see testRemoveShiftUp). I don't think these are good tests because they are highly dependent on the implementation of the heap. For the same series of operations, many different orders in the list could be correct. For example if we insert the values 0, 1, and 2 in the heap, the lists [2, 0, 1] and [2, 1, 0] would be correct. However, the testing suite would fail in one of these situations.
  EDIT: issue: Testing suite improvement #127 - pending partial fix: Heap tests improvement #131

[MerlinEgalite]

All the testing files use the extension .t.sol, which we don't use anymore in recent repositories.

Maybe we should have kept the .t.sol in other repositories to follow foundry best practices.

All th files use the old syntax of imports (import "./TestCommonHeapOrdering.t.sol"; instead of import {TestCommonHeapOrdering} from "./TestCommonHeapOrdering.t.sol";)

I agree we should change it.

Maybe it would be better to gather all the invariant tests files in a separate folder under ./test/.

I don't have a strong opinion on this so I'm ok.

Because we are testing libraries, ...

I agree with this comment

The testing suite generally lacks fuzzing.

Agree

Many tests check that the storage is exactly what we expect ...

I'm not sure about what you're suggesting here? Removing those tests?

[pakim249CAL]

Maybe it would be better to gather all the invariant tests files in a separate folder under ./test/.

I think this is a good practice to adopt especially for public users that are just viewing the repo to be able to understand more quickly how tests are structured.

Many tests check that the storage is exactly what we expect ...

I'm not sure about what you're suggesting here? Removing those tests?

I believe what is being suggested is to define tests based on how the data structure should behave rather than how it is implemented. There can be multiple orders of variables that would make a "correct" heap implementation, so tests should be structured to accept all good solutions.

I agree with all of Merlin's other points.

[QGarchery]

I agree with @pakim249CAL but I also don't like the .t.sol extension, because this convention does not work well with contract names. For example a file Heap.t.sol might have Heap as contract name and then it conflicts with the file Heap.sol which rightly contains the contract with the same name

[MerlinEgalite]

You mean it's an issue when you're doing a file or variable research @QGarchery right?

[QGarchery]

I was thinking about having a solidity file name that is not the same as the contract declaration it contains. So I like that a file is named TestHeap.sol and the contract declaration is contract TestHeap {.... With the .t.sol convention, the file name would be Heap.t.sol and the contract declaration would be contract Heap {... which is awkward because there might be another contract with that name

[MathisGD]

I was thinking about having a solidity file name that is not the same as the contract declaration it contains.

This is the "standard" in forge based repos (Heap.t.sol and contract TestHeap)

[makcandrov]

@QGarchery you may have an interesting opinion on the last point

[makcandrov]

btw I created the issue #127 to address the points 4 & 5. I am waiting for other opinions before addressing the last point

[QGarchery]

Not really, I completely agree that test should not be dependent on the implementation. The fact that they are is more a product of lack of time than anything else