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.

Sign up for GitHub

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 indexer client unit tests #2000

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
+134 −52
Open

add indexer client unit tests #2000

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
1015319
part 1
tsachiherman Mar 24, 2025
d728c86
Merge branch 'main' into tsachi/add-indexer-client-unit-tests
tsachiherman Mar 24, 2025
325be3d
factor unit test into a test case.
tsachiherman Mar 24, 2025
0706f55
Merge branch 'tsachi/add-indexer-client-unit-tests' of github.com:ava…
tsachiherman Mar 24, 2025
093f7ba
update
tsachiherman Mar 24, 2025
b598b52
update per feedback.
tsachiherman Mar 25, 2025
d504aaa
Merge branch 'main' into tsachi/add-indexer-client-unit-tests
tsachiherman Mar 25, 2025
3e185f7
update per CR
tsachiherman Mar 25, 2025
8e17b92
Merge branch 'tsachi/add-indexer-client-unit-tests' of github.com:ava…
tsachiherman Mar 25, 2025
890c656
update per CR
tsachiherman Mar 26, 2025
5d5edd8
Merge branch 'main' into tsachi/add-indexer-client-unit-tests
tsachiherman Mar 26, 2025
c96fbae
Merge branch 'main' into tsachi/add-indexer-client-unit-tests
tsachiherman Mar 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion api/indexer/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ import (
"github.com/ava-labs/hypersdk/requester"
)

var errTxUnmarshalingFailed = errors.New("transaction unmarshaling failed")

func NewClient(uri string) *Client {
uri = strings.TrimSuffix(uri, "/")
uri += Endpoint
Expand Down Expand Up @@ -122,7 +124,7 @@ func (c *Client) GetTx(ctx context.Context, txID ids.ID, parser chain.Parser) (G
var tx *chain.Transaction
tx, err = chain.UnmarshalTx(resp.TxBytes, parser)
if err != nil {
return GetTxResponse{}, nil, false, fmt.Errorf("failed to unmarshal tx %s: %w", txID, err)
return GetTxResponse{}, nil, false, fmt.Errorf("%w: tx %s: %w", errTxUnmarshalingFailed, txID, err)
}

resp.Result.CalculateCanotoCache()
Expand Down
195 changes: 136 additions & 59 deletions api/indexer/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,43 +9,54 @@ import (
"testing"
"time"

"github.com/ava-labs/avalanchego/database"
"github.com/ava-labs/avalanchego/ids"
"github.com/ava-labs/avalanchego/trace"
"github.com/stretchr/testify/require"

"github.com/ava-labs/hypersdk/api"
"github.com/ava-labs/hypersdk/chain"
"github.com/ava-labs/hypersdk/chain/chaintest"
"github.com/ava-labs/hypersdk/codec"
)

func TestIndexerClientBlocks(t *testing.T) {
const (
numExecutedBlocks = 4
blockWindow = 2
numTxs = 3
blockWindow = 2
numTxs = 3
)

testCases := []struct {
name string
blkHeight uint64
err error
name string
blkHeight uint64
blkHeightErr error
numExecutedBlocks int
}{
{
name: "success",
blkHeight: numExecutedBlocks - 1,
err: nil,
name: "success",
blkHeight: 3,
blkHeightErr: nil,
numExecutedBlocks: 4,
},
{
name: "missing block",
blkHeight: 0,
blkHeightErr: errBlockNotFound,
numExecutedBlocks: 4,
},
{
name: "missing block",
blkHeight: 0,
err: errBlockNotFound,
name: "no blocks",
blkHeight: 0,
Copy link
Collaborator

Choose a reason for hiding this comment

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

In production, we should be able to serve the genesis block, should we generate a sequence of blocks that includes a genesis block?

blkHeightErr: errBlockNotFound,
numExecutedBlocks: 0,
},
}

for _, tt := range testCases {
t.Run(tt.name, func(t *testing.T) {
r := require.New(t)
ctx := context.Background()
indexer, executedBlocks, _ := createTestIndexer(t, ctx, numExecutedBlocks, blockWindow, numTxs)
indexer, executedBlocks, _ := createTestIndexer(t, ctx, tt.numExecutedBlocks, blockWindow, numTxs)

jsonHandler, err := api.NewJSONRPCHandler(Name, NewServer(trace.Noop, indexer))
r.NoError(err)
Expand All @@ -59,26 +70,36 @@ func TestIndexerClientBlocks(t *testing.T) {

client := NewClient(httpServer.URL)

expectedBlock := executedBlocks[tt.blkHeight]
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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah understood, thanks for the clarification!

r.NoError(err)
expectedBlock := executedBlocks[tt.blkHeight-1]
r.Equal(expectedBlock.Block, executedBlock.Block)
} else {
r.ErrorContains(err, tt.err.Error())
r.ErrorContains(err, tt.blkHeightErr.Error())
}

expectedBlkID := ids.GenerateTestID()
Copy link
Contributor

Choose a reason for hiding this comment

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

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

if tt.blkHeight > 0 {
expectedBlock := executedBlocks[tt.blkHeight-1]
expectedBlkID = expectedBlock.Block.GetID()
}

executedBlock, err = client.GetBlock(ctx, expectedBlock.Block.GetID(), parser)
if tt.err == nil {
executedBlock, err = client.GetBlock(ctx, expectedBlkID, parser)
if tt.blkHeightErr == nil {
r.NoError(err)
r.Equal(expectedBlock.Block, executedBlock.Block)
r.Equal(executedBlocks[tt.blkHeight-1].Block, executedBlock.Block)
} else {
r.ErrorContains(err, tt.err.Error())
r.ErrorContains(err, tt.blkHeightErr.Error())
}

executedBlock, err = client.GetLatestBlock(ctx, parser)
r.NoError(err)
r.Equal(executedBlocks[numExecutedBlocks-1].Block, executedBlock.Block)
if tt.numExecutedBlocks == 0 {
r.ErrorContains(err, database.ErrNotFound.Error())
Comment on lines +97 to +98
Copy link
Collaborator

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

} else {
r.NoError(err)
r.Equal(executedBlocks[tt.numExecutedBlocks-1].Block, executedBlock.Block)
}
})
}
}
Expand All @@ -90,26 +111,61 @@ func TestIndexerClientTransactions(t *testing.T) {
numTxs = 3
)

parser := chaintest.NewTestParser()
badparser := &chain.TxTypeParser{
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: badParser

ActionRegistry: codec.NewTypeParser[chain.Action](),
AuthRegistry: codec.NewTypeParser[chain.Auth](),
}

testCases := []struct {
name string
blockIndex int
txIndex int
err error
found bool
name string
blockIndex int
txIndex int
getTxResultsErr error
getTxErr error
found bool
parser *chain.TxTypeParser
malformedBlock bool
}{
{
name: "success",
blockIndex: numExecutedBlocks - 1,
txIndex: 0,
found: true,
err: nil,
name: "success",
blockIndex: numExecutedBlocks - 1,
txIndex: 0,
found: true,
getTxResultsErr: nil,
getTxErr: nil,
parser: parser,
malformedBlock: false,
},
{
name: "missing transaction",
blockIndex: 0,
txIndex: 0,
found: false,
err: nil,
name: "missing transaction",
blockIndex: 0,
txIndex: 0,
found: false,
getTxResultsErr: nil,
getTxErr: nil,
parser: parser,
malformedBlock: false,
},
{
name: "badparser",
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: "bad parser"

blockIndex: numExecutedBlocks - 1,
txIndex: numTxs - 1,
found: true,
getTxResultsErr: nil,
getTxErr: errTxUnmarshalingFailed,
parser: badparser,
malformedBlock: false,
},
{
name: "malformed block",
blockIndex: numExecutedBlocks - 1,
txIndex: numTxs - 1,
found: true,
getTxResultsErr: errTxResultNotFound,
getTxErr: errTxResultNotFound,
parser: parser,
malformedBlock: true,
},
}

Expand All @@ -127,33 +183,45 @@ func TestIndexerClientTransactions(t *testing.T) {
httpServer.Close()
})

parser := chaintest.NewTestParser()

client := NewClient(httpServer.URL)

executedBlock := executedBlocks[tt.blockIndex]
executedTx := executedBlock.Block.Txs[tt.txIndex]

if tt.malformedBlock {
// "damage" the last executed block by removing the last result.
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 fix this comment? This removes the first result not the last result

executedBlock.ExecutionResults.Results = executedBlock.ExecutionResults.Results[1:]
r.NoError(indexer.Notify(context.Background(), executedBlock))
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we use the ctx already declared at the beginning of the subtest?

}

txResponse, found, err := client.GetTxResults(ctx, executedTx.GetID())
r.Equal(tt.err, err)
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)
if tt.getTxResultsErr != nil {
r.ErrorContains(err, tt.getTxResultsErr.Error())
} else {
r.NoError(err)
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)
}
}

txResponse, tx, found, err := client.GetTx(ctx, executedTx.GetID(), parser)
r.Equal(tt.err, err)
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)
txResponse, tx, found, err := client.GetTx(ctx, executedTx.GetID(), tt.parser)
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)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

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:

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

}
})
}
Expand All @@ -167,7 +235,7 @@ func TestIndexerClientWaitForTransaction(t *testing.T) {
)
r := require.New(t)
ctx := context.Background()
indexer, _, _ := createTestIndexer(t, ctx, numExecutedBlocks, blockWindow, numTxs)
indexer, executedBlocks, _ := createTestIndexer(t, ctx, numExecutedBlocks, blockWindow, numTxs)

jsonHandler, err := api.NewJSONRPCHandler(Name, NewServer(trace.Noop, indexer))
r.NoError(err)
Expand All @@ -183,4 +251,13 @@ func TestIndexerClientWaitForTransaction(t *testing.T) {
defer ctxCancel()
_, _, err = client.WaitForTransaction(timeoutCtx, 1*time.Millisecond, ids.GenerateTestID())
r.ErrorIs(err, context.DeadlineExceeded)

// wait for a past transaction.
lastExecutedBlock := executedBlocks[numExecutedBlocks-1]
lastTx := lastExecutedBlock.Block.Txs[numTxs-1]
lastTxResult := lastExecutedBlock.ExecutionResults.Results[numTxs-1]
success, fee, err := client.WaitForTransaction(context.Background(), 1*time.Millisecond, lastTx.GetID())
r.NoError(err)
r.Equal(lastTxResult.Success, success)
r.Equal(lastTxResult.Fee, fee)
}
Loading