feat: adding bcowpool verify btt tests #155
Conversation
test/unit/BCoWPool/BCoWPoolBase.sol
Outdated
There was a problem hiding this comment.
watch out this contract and BPoolBase don't have the .t.sol ending, is this intended?
There was a problem hiding this comment.
yes, the idea being files where testcases are defined end in .t.sol, while all others don't, I tried to be consistent with test/utils/Utils.sol not ending in .t.sol
There was a problem hiding this comment.
hmmm i understand that the coverage for example skips all .t.sol tests alltogether, so perhaps we should name them all as .t.sol
| function test_WhenPreconditionsAreMet() external { | ||
| // it should return | ||
| bCoWPool.verify(correctOrder); | ||
| } |
There was a problem hiding this comment.
since we checked for order sellAmount + 1, how would you feel here fuzzing from 0 -> calculatedOut to not lose the branch where sellAmount is lesser than calculated (that is the same as when it's ==, but we're not showing that behaviour)
There was a problem hiding this comment.
seems reasonable, I don't think it's a different branch since it covers the exact same code. As far as I understood, we can use a single testcase for different values if they don't cover different lines/branches of code.
CCing @drgorillamd in case he considers we should use a separate testcase
There was a problem hiding this comment.
This makes sense to me, you have a doubt about it? I'm missing something?
| └── when preconditions are met | ||
| └── it should return |
There was a problem hiding this comment.
i feel like we're forgetting about querying the balances, are we not?
wei3erHase
changed the title
| ├── it should ask the balance of the buy token | ||
| └── it should ask the balance of the sell token |
to be merged after #149
closes BAL-152