-
Notifications
You must be signed in to change notification settings - Fork 38.6k
test: add coverage for bitcoin-cli -rpcwait #18653
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
in interface_bitcoin_cli.py
in interface_bitcoin_cli.py and improve/harmonize the test logging.
| # Start node without RPC connection. | ||
| self.nodes[0].start() | ||
| # Verify failure without -rpcwait. | ||
| assert_raises_process_error(1, "Could not connect to the server", self.nodes[0].cli('getblockcount').echo) |
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.
Isn't this racy?
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.
Running (for i in {1..1000}; do test/functional/interface_bitcoin_cli.py ; done) to see.
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 generally only see races in travis or valgrind
Logically, I don't see how the rpc connection is guaranteed to be down when the cli is called here.
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.
Makes sense: it ran 1000 times without fail for me in macOS and Debian, but yeah, travis and valgrind. I'll wait to see the travis result for the fun of it and then probably remove that line; we'll still have the assertion with -rpcwait
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.
Travis all green. Maybe leave it in and remove if we see issues later?
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
kristapsk
left a comment
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.
ACK c28c7b8
c28c7b8 test: add -getinfo "unlocked_until" and "headers" coverage (Jon Atack) bb13f46 test: verify cli.getwalletinfo in wallet section (Jon Atack) becc8b9 test: verify bitcoin-cli -version with node stopped (Jon Atack) 727b67e test: add coverage for bitcoin-cli -rpcwait (Jon Atack) Pull request description: and other coverage improvements in `interface-bitcoin_cli.py` ACKs for top commit: kristapsk: ACK c28c7b8 Tree-SHA512: c3ac73b673872cba05496e3f9debc2c4dfa4291c6426bf0be9bb0242035613b25c0aecebb39abb14a1d87b13e1d28a5f34b21d4b77d93de56c679a8be92bbe82
in interface_bitcoin_cli.py Github-Pull: bitcoin#18653 Rebased-From: 727b67e
in interface_bitcoin_cli.py and improve/harmonize the test logging. Github-Pull: bitcoin#18653 Rebased-From: becc8b9
Github-Pull: bitcoin#18653 Rebased-From: bb13f46
Github-Pull: bitcoin#18653 Rebased-From: c28c7b8
Summary: - add coverage for bitcoin-cli -rpcwait - verify bitcoin-cli -version with node stopped and improve/harmonize the test logging. - verify cli.getwalletinfo in wallet section - add -getinfo "unlocked_until" and "headers" coverage This is a backport of Core [[bitcoin/bitcoin#18653 | PR18653]] Test Plan: `test/functional/test_runner.py interface_bitcoin*` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D8931
and other coverage improvements in
interface-bitcoin_cli.py