Impl unit tests #16
Impl unit tests #16
Conversation
LCOV of commit
|
| import {IIBCModuleInitializer} from "@hyperledger-labs/yui-ibc-solidity/contracts/core/26-router/IIBCModule.sol"; | ||
| import {ILightClient} from "@hyperledger-labs/yui-ibc-solidity/contracts/core/02-client/ILightClient.sol"; | ||
|
|
||
| contract DeployAll is Script, Config { |
There was a problem hiding this comment.
I split the function because solhint complained about function-max-lines (I wonder why it didn't complain before? I don't know why).
| 'test/*' \ | ||
| 'script/*' \ | ||
| 'src/proto/**' \ | ||
| 'src/core/PacketHandler.sol' \ | ||
| 'src/core/ContractRegistry.sol' \ |
There was a problem hiding this comment.
Exclude because coverage measurement is not required
test/CrossModule.t.sol
Outdated
| mod.onChanCloseConfirm(m); | ||
| } | ||
|
|
||
| function test_onChanCloseConfirm_Succeeds_WhenCallerHasRole() public { |
There was a problem hiding this comment.
Nit: Could you explain the naming convention for test cases? I don't know how snake-case and camel-case are being used. Typically, the following conventions are used.
https://getfoundry.sh/guides/best-practices/writing-tests#organizing-and-naming-tests
There was a problem hiding this comment.
Sticking to Foundry’s vibe: test_<Function>_<Description>.
Underscores only split segments; inside a segment we use UpperCamel, e.g. ReturnsSelfAndVersionWhenCallerHasRole.
Revert cases are tagged with RevertIf/When/On_….
fix: 51a84ae
test/TxAtomicSimple.t.sol
Outdated
| return _module; | ||
| } | ||
|
|
||
| function extHandlePacket(Packet calldata p) external returns (bytes memory ack) { |
There was a problem hiding this comment.
Nit: When using Test Harness, the following naming convention is recommended.
e.g. exposed_myInternalMethod
https://getfoundry.sh/guides/best-practices/writing-tests#internal-functions
test/TxAtomicSimple.t.sol
Outdated
| Packet memory packet = _mkPacketWithCall(txId, callInfo); | ||
|
|
||
| // assert event emitted | ||
| vm.expectEmit(true, true, true, true); |
There was a problem hiding this comment.
Since all arguments are always set to true, it’s simpler and better to just write vm.expectEmit(address(harness)). In addition, testing the emitter address is important.
test/TxAtomicSimple.t.sol
Outdated
| bytes memory callInfo = hex"c0ffee"; | ||
| Packet memory packet = _mkPacketWithCall(txId, callInfo); | ||
|
|
||
| // assert event emitted |
There was a problem hiding this comment.
Nit: Obvious comment is not needed.
test/TxAtomicSimple.t.sol
Outdated
| ); | ||
| } | ||
|
|
||
| function test_handlePacket_RevertInModule_ReturnsFAILED_AndEmitsEvent() public { |
There was a problem hiding this comment.
It’s a bit tricky, but this test case doesn’t revert, so the test name should be changed.
script/DeployAll.s.sol