-
Notifications
You must be signed in to change notification settings - Fork 118
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 block encoding param to block index api #1752
base: main
Are you sure you want to change the base?
Conversation
"github.com/ava-labs/hypersdk/codec" | ||
) | ||
|
||
func TestEncodingValidate(t *testing.T) { |
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.
👍
api/indexer/server_test.go
Outdated
} | ||
} | ||
|
||
func TestBlockResponse(t *testing.T) { |
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.
Could we test via httptest
rather than calling individual functions this way?
ex of setting up httptest server: https://github.com/ava-labs/hypersdk/blob/main/tests/integration/integration.go#L236
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.
There are two ways we could go testing here imo:
- test the server/client w/ mocked indexer using httptest and test the indexer separately
- test the indexer + server/client solely through the server/client
Could we add basic tests here that use httptest
and the server/client with a mocked of the indexer (will need to change indexer to interface as it's used in server/client)?
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.
We should also avoid this pattern of using t.Run
within a single unit test. I think this makes sense for table tests (and this could be changed to a table test imo), but less so in this pattern with mostly independent test logic:
t.Run("JSON", func(t *testing.T) {
// independent test logic
})
t.Run("Hex", func(t *testing.T) {
// independent test logic
})
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.
Good idea by the way! The client was broken because of my changes !
} | ||
|
||
func (c *Client) GetBlock(ctx context.Context, blkID ids.ID, parser chain.Parser) (*chain.ExecutedBlock, error) { | ||
resp := GetBlockClientResponse{} | ||
err := c.requester.SendRequest( | ||
ctx, | ||
"getBlock", | ||
&GetBlockRequest{BlockID: blkID}, | ||
&GetBlockRequest{BlockID: blkID, Encoding: Hex}, |
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.
I did not provide the Encoding through variable, I think this client will continue to always use Hex
.
Closes #1632