Skip to content

Conversation

@positiveblue
Copy link
Contributor

Code extracted from #323 (removing the sponsoring part)

Copy link
Contributor

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, LGTM 🎉

checkResponse func(*poolrpc.RenewAccountResponse) error
mockSetter func(*poolrpc.RenewAccountRequest,
*account.MockManager, *MockMarshaler,
)
Copy link
Contributor

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.

Copy link
Member

@bhandras bhandras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🍔

// 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: checksumSize/hashSize?


// Manager is responsible for the management of accounts on-chain.
type Manager struct {
type manager struct {
Copy link
Member

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)
Copy link
Member

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@positiveblue positiveblue force-pushed the add-rpc-test branch 2 times, most recently from 0a6e92e to 9195066 Compare December 15, 2021 04:32
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.
@positiveblue positiveblue merged commit e87226d into master Dec 15, 2021
@guggero guggero deleted the add-rpc-test branch December 15, 2021 11:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants