add indexer client unit tests #2000
Conversation
…-labs/hypersdk into tsachi/add-indexer-client-unit-tests
| executedBlock, err := client.GetBlockByHeight(ctx, expectedBlock.Block.Hght, parser) | ||
| if tt.err == nil { | ||
| executedBlock, err := client.GetBlockByHeight(ctx, tt.blkHeight, parser) | ||
| if tt.blkHeightErr == nil { |
There was a problem hiding this comment.
Could we simplify this if-condition? I think it's sufficient to check that the errors are "equal" before invoking the if-condition rather than doing an if-else:
r.ErrorIs(tt.blkHeightErr, err)
if tt.blkHeightErr == nil {
expectedBlock := executedBlocks[tt.blkHeight-1]
r.Equal(expectedBlock.Block, executedBlock.Block)
}There was a problem hiding this comment.
Unfortunately, that won't work ( as is.. ). The returned error is an rpc error that contains a string representing the internal server error. Hence, we need to compare it by string (ErrorContains).
We do have a PR to update this and create RPC specific error codes, which would resolve this issue (#1942)
There was a problem hiding this comment.
Ah understood, thanks for the clarification!
api/indexer/client_test.go
Outdated
| @@ -90,26 +111,61 @@ func TestIndexerClientTransactions(t *testing.T) { | |||
| numTxs = 3 | |||
| ) | |||
|
|
|||
| parser := chaintest.NewTestParser() | |||
| badparser := &chain.TxTypeParser{ | |||
api/indexer/client_test.go
Outdated
| malformedBlock: false, | ||
| }, | ||
| { | ||
| name: "badparser", |
There was a problem hiding this comment.
Left two comments but afterwards, LGTM.
Could we create an issue afterwards to address #2000 (comment)? With the current if-else usage, the table tests are hard to read (ref) but am willing to defer this to a future PR.
api/indexer/client_test.go
Outdated
| if tt.malformedBlock { | ||
| // "damage" the last executed block by removing the last result. | ||
| executedBlock.ExecutionResults.Results = executedBlock.ExecutionResults.Results[1:] | ||
| r.NoError(indexer.Notify(context.Background(), executedBlock)) |
There was a problem hiding this comment.
Could we use the ctx already declared at the beginning of the subtest?
api/indexer/client_test.go
Outdated
| r.ErrorContains(err, tt.blkHeightErr.Error()) | ||
| } | ||
|
|
||
| expectedBlkID := ids.GenerateTestID() |
There was a problem hiding this comment.
Could we use ids.Empty{} here? This way, it's clear that if we're not targeting a particular block (i.e. numExecutedBlocks >= height > 0), then it's clear that we're querying with a invalid block ID (since a valid block cannot have id = ids.Empty{}).
I think that updating the tests would be inevitable once the error codes are refactored. I'll add that to the issue. |
…-labs/hypersdk into tsachi/add-indexer-client-unit-tests
| blkHeight: 0, | ||
| err: errBlockNotFound, | ||
| name: "no blocks", | ||
| blkHeight: 0, |
There was a problem hiding this comment.
In production, we should be able to serve the genesis block, should we generate a sequence of blocks that includes a genesis block?
| if tt.numExecutedBlocks == 0 { | ||
| r.ErrorContains(err, database.ErrNotFound.Error()) |
There was a problem hiding this comment.
Handling the special case this way makes it seem like we default to not including the genesis. Could we include at least the genesis block (we should receive a notification of at least the genesis block on startup)
There was a problem hiding this comment.
this is not intended to cover the genesis block; it's really about covering the no-blocks-of-any-kind usecaswe.
Adding genesis is ok of course, but I don't feel that it's needed since it wouldn't be treated any differently from the indexer perspective.
api/indexer/client_test.go
Outdated
| executedBlock := executedBlocks[tt.blockIndex] | ||
| executedTx := executedBlock.Block.Txs[tt.txIndex] | ||
|
|
||
| if tt.malformedBlock { | ||
| // "damage" the last executed block by removing the last result. |
There was a problem hiding this comment.
Could we fix this comment? This removes the first result not the last result
api/indexer/client_test.go
Outdated
| if tt.getTxErr != nil { | ||
| r.ErrorContains(err, tt.getTxErr.Error()) | ||
| } else { | ||
| r.Equal(tt.found, found) | ||
| if tt.found { | ||
| r.Equal(GetTxResponse{ | ||
| TxBytes: executedTx.Bytes(), | ||
| Timestamp: executedBlock.Block.Tmstmp, | ||
| Result: executedBlock.ExecutionResults.Results[tt.txIndex], | ||
| }, txResponse) | ||
| r.Equal(executedTx, tx) | ||
| } |
There was a problem hiding this comment.
There's a lot of conditional nesting in these table tests. Could we try and simplify them so that they have narrow scope and reduce unnecessary nesting as much as possible?
For example, this section could be re-written as:
txResponse, tx, found, err := client.GetTx(ctx, executedTx.GetID(), tt.parser)
if tt.getTxErr != nil {
r.ErrorContains(err, tt.getTxErr.Error())
}
r.Equal(tt.found, found)
if !tt.found {
return
}
r.Equal(GetTxResponse{
TxBytes: executedTx.Bytes(),
Timestamp: executedBlock.Block.Tmstmp,
Result: executedBlock.ExecutionResults.Results[tt.txIndex],
}, txResponse)
r.Equal(executedTx, tx)style guide refs:
There was a problem hiding this comment.
Hmm putting this in place fails on the tests: badParser and malformed_block
it seems this is because found is re-used across both GetTxResults and GetTx, so we require the error in GetTx to short circuit the test at the error.
What ?
This PR extends the indexer client api unit tests.