-
Notifications
You must be signed in to change notification settings - Fork 47
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add rpcserver test for RenewAccount
endpoint
#326
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, LGTM 🎉
rpcserver_test.go
Outdated
checkResponse func(*poolrpc.RenewAccountResponse) error | ||
mockSetter func(*poolrpc.RenewAccountRequest, | ||
*account.MockManager, *MockMarshaler, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: closing parenthesis of method definition should be on same line as last argument (to better distinguish from method call). Same in the actual implementation below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🍔
order/interfaces.go
Outdated
// Note: this matches the sha256.Size definition. However, mockgen | ||
// complains about not being able to find the sha256 package. When | ||
// that bug is fixed, we can change back to [sha256.Size]byte. | ||
const size = 32 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: checksumSize
/hashSize
?
@@ -123,7 +123,7 @@ type ManagerConfig struct { | |||
} | |||
|
|||
// Manager is responsible for the management of accounts on-chain. | |||
type Manager struct { | |||
type manager struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's helpful to add a compile time enforcer for the interface:
var _ Manager = (*manager)(nil)
|
||
resp, err := srv.RenewAccount(ctx, req) | ||
if tc.expectedError != "" { | ||
assert.EqualError(t, err, tc.expectedError) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: require
and the expectation comes first iirc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like for EqualError
this is the right order?
https://github.com/stretchr/testify/blob/master/assert/assertions.go#L1367
0a6e92e
to
9195066
Compare
The `mockgen` tool seems to have a bug and it is not able to parse a valid go file. I had to change the `[sha256.Size]byte` to `[size]byte` and define `hashSize` in the order package in the same way it is defined in the sha256 one.
The Marshaler interface is used to transform internal types to decorated RPC ones. It decouples that functionality form the rpcServer so it is easier to test exhaustively.
9195066
to
ce0ba9a
Compare
Code extracted from #323 (removing the sponsoring part)