Discussion around tests #122
Replies: 2 comments 9 replies
-
Maybe we should have kept the
I agree we should change it.
I don't have a strong opinion on this so I'm ok.
I agree with this comment
Agree
I'm not sure about what you're suggesting here? Removing those tests? |
Beta Was this translation helpful? Give feedback.
-
@QGarchery you may have an interesting opinion on the last point |
Beta Was this translation helpful? Give feedback.
-
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:
.t.sol
, which we don't use anymore in recent repositories.import "./TestCommonHeapOrdering.t.sol";
instead ofimport {TestCommonHeapOrdering} from "./TestCommonHeapOrdering.t.sol";
).EDIT: issue: Conform to Morpho's style #125 - pending fix: Code style change #126
./test/
.EDIT: pending fix: Test folder partitioning #130
ConcreteHeapOrdering.sol
) that is essentially the "contract version" of the library. However, not all tests use this (seeTestHeap.t.sol
), and some of them add functions that are not in the library in the first place (seeaccountsValue
,accountsId
andindexOf
fromConcreteHeapOrdering.sol
andConcreteThreeHeapOrdering.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
EDIT: issue: Testing suite improvement #127 - pending partial fix: Heap tests improvement #131
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 values0
,1
, and2
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
Beta Was this translation helpful? Give feedback.
All reactions