feat: add btt tests for finalize methods #159
Conversation
|
since we're using a "valid scenario" approach doesn't seem bad to me |
test/unit/BCoWPool/BCoWPool.tree
Outdated
| // as we previously did in fuzzed tests, here we only test the new behaviour | ||
| // introduced by the extension, however we don't go as far as just testing the | ||
| // _afterFinalize hook to avoid an internal change to BPool.finalize from | ||
| // inadvertedly breaking the derived method |
There was a problem hiding this comment.
noted, i'd remove this comment
There was a problem hiding this comment.
hmm ok, but i don't think the comment belongs here, whatever decision we take, is taken, shouldn't be on the tree
test/unit/BPool/BPool.tree
Outdated
| │ └── it should revert | ||
| └── when preconditions are met | ||
| ├── it finalizes the pool | ||
| ├── it mints initial pool supply to controller |
There was a problem hiding this comment.
it mints initial pool supply + it pushes initial pool supply to controller
test/unit/BPool/BPoolBase.sol
Outdated
There was a problem hiding this comment.
watch out this contract doesn't have the .t.sol termination
| vm.mockCall(tokens[1], abi.encodeCall(IERC20.approve, (vaultRelayer, type(uint256).max)), abi.encode(true)); | ||
| } | ||
|
|
||
| function test_WhenCalled() external { |
There was a problem hiding this comment.
is the result of implementing feedback from this: #159 (comment) 😭 what did you have in mind?
There was a problem hiding this comment.
is this because Finalize is the only method in the tree for now?
There was a problem hiding this comment.
yes, precisely. That's why PRs depending on this all have renames of these methods 😭
we previously decided to test the behaviour of or are you referring to something else? |
|
what's the difference in rationale between |
in If you want to sacrifice the extra testing in |
test/unit/BCoWPool/BCoWPool.tree
Outdated
| @@ -0,0 +1,8 @@ | |||
| BCoWPool::Finalize | |||
There was a problem hiding this comment.
| BCoWPool::Finalize | |
| BCoWPool::finalize |
There was a problem hiding this comment.
If I do it this way then the contract has to be named BCoWPoolfinalize which isn't as clean
There was a problem hiding this comment.
tbh, i don't really like the "implicit tree" in which you need to read BPool::finalize to know what happens inside BCoWPool::finalize, in the case that BCoWPool definitely overrides BPool one, i'd be ok with having both in different tests, in this case, i think i very much prefer the _afterFinalize hook as we had in other examples
i don't have strong opinions on it either, if you think is good like this i'm ok with it
| @@ -1,4 +1,4 @@ | |||
| BCoWPool::Finalize | |||
| BCoWPool::_afterFinalize | |||
wei3erHase
changed the title
proposal: what do you thing of refactoring
BPoolbtt unit tests so the pool deployer is the test runner, so the normal case is for the caller to be the pool's controller, and we don't need a branch+modifier for it.that's the main difference I found between tests for bpool and bcowpool, do you have any opinion on any of them being safer/cleaner?