Skip to content
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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

najeal
Copy link
Contributor

@najeal najeal commented Nov 7, 2024

Closes #1632

"github.com/ava-labs/hypersdk/codec"
)

func TestEncodingValidate(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

}
}

func TestBlockResponse(t *testing.T) {
Copy link
Collaborator

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

Copy link
Collaborator

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)?

Copy link
Collaborator

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
})

Copy link
Contributor Author

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},
Copy link
Contributor Author

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.

@najeal najeal requested a review from aaronbuchwald November 21, 2024 19:46
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.

Add Block Encoding Param to Block Index API
2 participants