-
Notifications
You must be signed in to change notification settings - Fork 11
fix: tests for graphql #788
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
Conversation
b13165e to
29b6891
Compare
e4b4a6c to
0630144
Compare
| avgDuration /= int64(len(durations)) | ||
| require.Greater(t, avgDuration, int64(50)) | ||
| require.Less(t, avgDuration, int64(100)) | ||
| require.Less(t, avgDuration, int64(120)) |
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.
it takes longer time now
7be2017 to
134ed18
Compare
| run: make test-short | ||
| run: | | ||
| TEST_SHORT_PKGS="$(go list ./... \ | ||
| | grep -v '/packages/vm/core/testcore' \ |
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.
All these triggered timeout
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.
Let's not forget to reenable or fix them. These are important tests.
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.
#790 is solving the timeout issues too, so we will enable soon in the future
| t.Run("callTracer", func(t *testing.T) { | ||
| _, err := env.traceTransactionWithCallTracer(tx.Hash()) | ||
| require.ErrorContains(t, err, "expected exactly one top-level call") | ||
| require.ErrorContains(t, err, "transaction not found") |
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.
From what I remember, expected exactly one top-level call has been an annoying issue with the calltracer. Why is it suddenly "transaction not found"?
It seems like the transaction wasn't executed, therefore you get transaction not found.
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.
+1
Also, the change in graphql should not affect the EVM tests? Unless I'm missing something.
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.
fixed this. It was caused by the amount coin returned by faucet is different now. I have added a test to ensure the const we used match the real amount
| current *iotago.ObjectRef, | ||
| timeout time.Duration, | ||
| ) (*iotago.ObjectRef, error) { | ||
| ticker := time.NewTicker(200 * time.Millisecond) |
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.
what is the ticker for?
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 use it as a retry here, because the time for the node to index the TXs is too slow now. Or I should remove it?
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.
Ah ok... is polling like this the "official" way? I mean, is there something like a subscription API to be notified of the updated object ref? Just trying to avoid hardcoded delays and timeouts.
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 are trying to implement the subscription of graphql. However, I think UpdateObjectRef() is more like a one-off call. If we don't do retry here, we add to add it to all the tests. Not sure what is the best way to do it
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 don't know, we should try to avoid hardcoded delays if possible. I would think there must be a proper way to subscribe to updates, but I'm not familiarized with the graphql api.
Anyway, if the retry delay is unavoidable, then at least maybe it should be a parameter or a const?
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 added the tickingTime in GraphQLClient
| run: make test-short | ||
| run: | | ||
| TEST_SHORT_PKGS="$(go list ./... \ | ||
| | grep -v '/packages/vm/core/testcore' \ |
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.
Let's not forget to reenable or fix them. These are important tests.
| func (c *GraphQLClient) waitForNewerObjectRef( | ||
| ctx context.Context, | ||
| current *iotago.ObjectRef, | ||
| timeout time.Duration, |
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.
If we already receive a context, then maybe the timeout is redundant, because the caller could pass a ContextWithTimeout
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.
done
| var txnResponse *IotaTransactionBlockResponse | ||
| var gasPayments []*iotago.ObjectRef | ||
|
|
||
| for i := 0; i < 5; i++ { |
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.
maybe the amount of retries and the delay should be parameters, or consts
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.
done
| t.Run("callTracer", func(t *testing.T) { | ||
| _, err := env.traceTransactionWithCallTracer(tx.Hash()) | ||
| require.ErrorContains(t, err, "expected exactly one top-level call") | ||
| require.ErrorContains(t, err, "transaction not found") |
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.
+1
Also, the change in graphql should not affect the EVM tests? Unless I'm missing something.
packages/solo/solo.go
Outdated
| ctx, cancel := context.WithTimeout(env.ctx, 60*time.Second) | ||
| defer cancel() | ||
|
|
||
| ticker := time.NewTicker(250 * time.Millisecond) |
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.
Is this "permanent"? Time delays make tests unhappy.
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 think we need it until we discard grahpql in solo
0a3fa86 to
c48f7c8
Compare
|
|
||
| func getBalance(ctx context.Context, client *GraphQLClient, address *iotago.Address) *big.Int { | ||
| balance, err := client.GetBalance(ctx, GetBalanceRequest{Owner: address}) | ||
| if err != nil || balance.TotalBalance == nil { |
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.
if err != nil then maybe this should panic or return error
| current *iotago.ObjectRef, | ||
| timeout time.Duration, | ||
| ) (*iotago.ObjectRef, error) { | ||
| ticker := time.NewTicker(200 * time.Millisecond) |
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 don't know, we should try to avoid hardcoded delays if possible. I would think there must be a proper way to subscribe to updates, but I'm not familiarized with the graphql api.
Anyway, if the retry delay is unavoidable, then at least maybe it should be a parameter or a const?
| var txnResponse *IotaTransactionBlockResponse | ||
| var gasPayments []*iotago.ObjectRef | ||
|
|
||
| for i := 0; i < maxRetries; i++ { |
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.
Having to retry operations is suspicious... is the graphql api unreliable? Is it expected to have to retry? Perhaps we are using the api wrong somehow?
Maybe this is fine as a temporary measure but IMO we should at least leave TODOs to remove all delays and retries at some point.
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.
The problem was caused by the indexer performs really slow in the test scenario. Once we switch to Simulacrum, then the problem may be gone
No description provided.