Electrum integration tests for mainnet servers #3599
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We add test vectors for mainnet and extract exisitng test data for testnet from integration test to the test data package.
We extract functions to assert the server messages, the responses depend not only on the electrum server implementation but a daemon running behind it. It's hard to maintain the exact response for the particular server, so we just define a list of responses that are fine.
We need to cancel the connections for electrum client so nothing is dangling in the background.
We want to execute the whole suite of tests for all the servers including the embedded ones. To do that we have to get the list from the config package, we use go:linkname for that, instead of exposing the readEmbeddedServers function publicly. We combine testnet and mainnet servers in the config, distinguishing it with by bitcoin network property. We renamed the package to `electrum_test` to overcome the cyclic dependency problem that we have between the config and electrum packages. We read the servers in init function and then remove duplicates (by URL), so we don't execute the test twice for the same server.
Handle testnet and mainnet, read testdata from common package, improve latest block check to confirm all servers are on the same page.
This file is no longer needed as we test the embedded servers in pkg/bitcoin/electrum/electrum_integration_test.go
2 tasks
Version 1.18.3 was causing tests execution to fail with panic, e.g. https://github.com/keep-network/keep-core/actions/runs/5210502339/jobs/9402113717?pr=3599 With the latest release for 1.18 everything works fine.
The server is unstable, we have other servers for testing TCP client.
The script helps to run the integration tests consistently with the CI job as it's based on the same solutions as defined in the workflow.
pdyraga
reviewed
Jun 9, 2023
pdyraga
reviewed
Jun 9, 2023
pdyraga
reviewed
Jun 9, 2023
pdyraga
reviewed
Jun 9, 2023
pdyraga
reviewed
Jun 9, 2023
pdyraga
reviewed
Jun 9, 2023
pdyraga
reviewed
Jun 9, 2023
| func assertConfirmationsCloseTo(t *testing.T, expected uint, actual uint) { | ||
| delta := uint(2) | ||
|
|
||
| func assertNumberCloseTo(t *testing.T, expected uint, actual uint, delta uint) { |
There was a problem hiding this comment.
We could make it a part of testutils. Non-blocking, it does not come from this PR.
pdyraga
reviewed
Jun 9, 2023
This was referenced Jun 12, 2023
|
I've executed several attempts locally and I was not able to get a green build. However, the problems that occurred are not caused by this code. This code reveals those problems which is great. One problem is a panic in Other problems are timeouts for individual tests that should lead to tweaks in the number of retries and timeouts. That being said, I would recommend:
And then, I am happy to merge. The last two items are already done based on what I know, but please confirm. |
These are details of transactions swept from actual mainnet wallet.
We want to workaround the strange panic in CI, so maybe this sleep will help.
We use consistent timeouts, with exception for blockstream's servers which need to set them longer.
We should remove entry from the test configs.
We need to capture `testName` and `testConfig` if we use `t.Parallel()`. See `TestGroupedParallel` example [here](https://pkg.go.dev/testing).
The tag got removed by accident.
We need the details for debugging, it will help.
pdyraga
reviewed
Jun 15, 2023
|
I was able to run tests locally. All of them but |
pdyraga
approved these changes
Jun 15, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In this PR we implement support for mainnet servers in the electrum client integration tests.
We also include the embedded electrum servers in the verification.
To run the tests execute:
go test -v -tags=integration ./pkg/bitcoin/electrum -count=1Closes: #3594