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

Jump to bottom

e2e: Diagnose and fix flakes #1941

Merged
merged 11 commits into from
Sep 7, 2023
Merged

e2e: Diagnose and fix flakes #1941

merged 11 commits into from
Sep 7, 2023

Conversation

marun
Copy link
Contributor

@marun marun commented Aug 29, 2023 •
edited
Loading

Why this should be merged

The high incidence of e2e flakes suggests a need to increase the details logged in testing to aid in diagnosing failure. Also, tests are made more resilient to failure.

How this works

  • Enable the wallet to accept an optional function to log a transaction ID immediately after issuance and ensure the e2e wallet helper configures the wallet with the option.
  • Log the node ID used by a given test
    • Also ensure that tests use at most one node (where possible) to minimize the scope of log inspection
  • Resolve C-Chain flakes induced by (Suggested gas price may be too low coreth#314) by doubling the suggested gas price

How this was tested

CI on this PR and on PRs based on it:

@marun marun marked this pull request as ready for review August 29, 2023 17:55
@marun marun requested review from abi87 and gyuho as code owners August 29, 2023 17:55
@marun marun changed the title e2e: Log node uri to aid in troubleshooting e2e: Ensure tests use the same node uri Aug 29, 2023
@marun marun changed the title e2e: Ensure tests use the same node uri e2e: Ensure tests use the same node URI Aug 29, 2023
@marun marun force-pushed the e2e-log-node-uri branch 2 times, most recently from 282aca2 to 3fb1691 Compare August 30, 2023 16:30
This was referenced Aug 30, 2023
Merged
Merged
Merged
Merged
@marun marun force-pushed the e2e-log-node-uri branch 2 times, most recently from ee8d1f7 to 5c007d0 Compare August 31, 2023 15:43
@marun marun changed the title e2e: Ensure tests use the same node URI e2e: Increase logging detail to improve traceability Aug 31, 2023
@marun marun force-pushed the e2e-log-node-uri branch 4 times, most recently from 6d14ed1 to 0180602 Compare August 31, 2023 18:38
danlaine
danlaine suggested changes Aug 31, 2023
View reviewed changes
tests/e2e/e2e.go Outdated
func (te *TestEnvironment) NewWalletForURI(keychain *secp256k1fx.Keychain, uri string) primary.Wallet {
tests.Outf("{{blue}} initializing a new wallet {{/}}\n")
func (te *TestEnvironment) NewWallet(keychain *secp256k1fx.Keychain, uri string) primary.Wallet {
tests.Outf("{{blue}} initializing a new wallet for URI: %s {{/}}\n", uri)
Copy link

Choose a reason for hiding this comment

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

Should we log the node ID too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would involve passing the node ID everywhere. I think it's sufficient that the node ID is logged with the node URI so that it can be easily determined for a given test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On second thought, maybe it makes more sense to use a type with fields for both node id and uri.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to use a struct containing node ID and URI to facilitate logging both.

@danlaine
Copy link

Alternatively, maybe we could put this logging into the wallet itself?

@marun
Copy link
Contributor Author

marun commented Sep 1, 2023 •
edited
Loading

Alternatively, maybe we could put this logging into the wallet itself?

Edit: I think it might be preferable to get traceability in now and if/when we get something implemented in the wallet we can switch to that, but that's just me. Would you prefer to see a wallet implementation instead in this PR (e.g. by providing a writer via an option)?

@marun marun force-pushed the e2e-log-node-uri branch 3 times, most recently from bf6c33e to 6ad8b33 Compare September 1, 2023 16:26
@marun marun changed the title e2e: Increase logging detail to improve traceability e2e: Diagnose and fix flakes Sep 1, 2023
@marun
Copy link
Contributor Author

marun commented Sep 1, 2023

@danlaine I've pivoted to log the tx id in the wallet as per your suggestion.

@marun marun mentioned this pull request Sep 1, 2023
Merged
1 task
danlaine
danlaine reviewed Sep 5, 2023
View reviewed changes
wallet/subnet/primary/common/options.go Outdated
@@ -126,6 +130,10 @@ func (o *Options) PollFrequency() time.Duration {
return defaultPollFrequency
}

func (o *Options) LogTxIDFunc() LogTxIDFunc {
Copy link

Choose a reason for hiding this comment

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

Having this as on option feels kind of weird to me. Maybe we could have a more general OnIssued callback or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed to PostIssuanceFunc as per Stephen's suggestion.

@marun
Copy link
Contributor Author

marun commented Sep 5, 2023

Rebased

danlaine
danlaine reviewed Sep 5, 2023
View reviewed changes
This was referenced Sep 5, 2023
Merged
Merged
@marun marun mentioned this pull request Sep 6, 2023
Merged
1 task
@marun marun assigned StephenButtolph and unassigned marun and StephenButtolph Sep 6, 2023
StephenButtolph
StephenButtolph approved these changes Sep 7, 2023
View reviewed changes
tests/e2e/e2e.go Outdated
return true
}, DefaultTimeout, DefaultPollingInterval, "failed to see transaction acceptance before timeout")

// Retrieve the contract address
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment seems incorrect

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops... I factored this code out of the dynamic fees test and there it was retrieving the contract address from the receipt.

Done.

@StephenButtolph StephenButtolph added this to the v1.10.10 milestone Sep 7, 2023
@StephenButtolph StephenButtolph merged commit 0cf3d29 into dev Sep 7, 2023
@StephenButtolph StephenButtolph deleted the e2e-log-node-uri branch September 7, 2023 01:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danlaine danlaine danlaine approved these changes

@StephenButtolph StephenButtolph StephenButtolph approved these changes

@hexfusion hexfusion Awaiting requested review from hexfusion

@abi87 abi87 Awaiting requested review from abi87

@gyuho gyuho Awaiting requested review from gyuho

Assignees
No one assigned
Labels
bug Something isn't working ci This focuses on changes to the CI process testing This primarily focuses on testing
Projects
No open projects
Ski Patrol
Archived in project
Milestone
v1.10.10
Development

Successfully merging this pull request may close these issues.
4 participants
@marun @danlaine @StephenButtolph @maru-ava