-
Notifications
You must be signed in to change notification settings - Fork 10
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
Faucet Polling in E2E Tests #363
Conversation
Deploying agoric-dapp-inter-emerynet with Cloudflare Pages
|
Deploying dapp-inter-test with Cloudflare Pages
|
if (transactionStatus === TRANSACTION_STATUS.NOT_FOUND) | ||
// eslint-disable-next-line cypress/no-unnecessary-waiting | ||
return cy.wait(2000).then(() => getStatus(txHash)); | ||
else return cy.wrap(transactionStatus); |
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.
Should we use a maximum number of retires or some timeout to avoid an infinite loop? Is it possible that return cy.wait(2000).then(() => getStatus(txHash));
continues indefinitely?
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.
Cypress commands have a default timeout of 4 seconds so unless we have changed it, this shouldn't happen
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.
Command timeout is 6 minutes for testnets and 2 minutes with local chain. Would that be fine? I think we should have 4 or 5 retries. That would be enough to make the call invalid, I guess.
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 think we can confidentaly say that N
number of retries should be enough to include a transaction in a block. I would say let's keep trying until the Cypress command timeout exceeds instead to get maximum retires in. What do you think?
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.
Okies. Let's see how tests perform with this change.
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 just feel that waiting 6 min for transaction status with testnets seems like an overkill
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.
Yes you have a valid point. But I would like to see how often do we timeout waiting for the transaction to complete. Maybe we will need to do something about that if it is frequent.
Description
Updated test to poll for transaction completion