test: Split rpc_signmessage test for disabled wallet#22641
test: Split rpc_signmessage test for disabled wallet#22641maflcko merged 1 commit intobitcoin:masterfrom Shubhankar-Gambhir:master
Conversation
maflcko
changed the title
maflcko
left a comment
maflcko
left a comment
There was a problem hiding this comment.
Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits
ShubhamPalriwala
left a comment
ShubhamPalriwala
left a comment
There was a problem hiding this comment.
tAck 17019494d9ae381fa75bf469ca10bdad95df7ae1
Decoupling the signing of messages with a private key and with an address opens up the doors for further individual testing in the future.
Tested on Ubuntu 21.04
There was a problem hiding this comment.
Is there a reason for running the loop here on L32 and L36 from 0 to 5?
There was a problem hiding this comment.
It skips 2 (signmessagewithprivkey has two required parameters). I think the intention here is to ensure the RPC returns an error if called with the wrong number of parameters.
There was a problem hiding this comment.
Alright! Makes sense then
There was a problem hiding this comment.
I don't think this argument makes any difference without wallet?
Edit: yes, it works fine without this line
vasild
left a comment
vasild
left a comment
There was a problem hiding this comment.
Approach ACK
I verified that this is just moving bits of a test to a separate new test (as intended).
Tested:
When compiled with --enable-wallet:
rpc_signmessagewithprivkey.py | ✓ Passed | 1 s
wallet_signmessagewithaddress.py | ✓ Passed | 1 s
When compiled with --disable-wallet:
rpc_signmessagewithprivkey.py | ✓ Passed | 1 s
wallet_signmessagewithaddress.py | ○ Skipped | 0 s
wallet_signmessagewithaddress.py fails as expected if skip_if_no_wallet() is removed from it and the wallet is not enabled.
Just a few minor suggestions:
- The first line in the commit message usually starts with a lowercase letter and has no space before the
::
$ git log --oneline --grep='^test:' origin/master |wc -l
829
$ git log --oneline --grep='^Test:' origin/master |wc -l
3
$ git log --oneline --grep='^Test :' origin/master |wc -l
0-
In the OP:
s/rpc_signmessagewithaddress.py/wallet_signmessagewithaddress.py/ -
The commit message is a bit scarce. In general, it is good if it contains a summary of the changes and more importantly, why are they being done. It would be best if one can make sense of it by just reading the commit message, if github.com is not available.
There was a problem hiding this comment.
s/signmessage(withprivkey) have/signmessagewithprivkey has/ because this test is now executing just one RPC - signmessagewithprivkey.
There was a problem hiding this comment.
s/signmessage(withprivkey) have/signmessage has/ because this test is now executing just one RPC - signmessage.
There was a problem hiding this comment.
This and lines 46,47,48 below exercise verifymessage which does not require a wallet, I think they can be removed from this file (wallet_signmessagewithaddress.py) since they are also present in rpc_signmessagewithprivkey.py.
Divided tests in rpc_signmessage.py into 2 files wallet_signmessagewithaddress.py and rpc_signmessagewithprivkey.py, latter one can run even when wallet is disabled.
Shubhankar-Gambhir
changed the title
maflcko
changed the title
theStack
left a comment
theStack
left a comment
There was a problem hiding this comment.
Code-review ACK a3b559c
(review was quite painless thanks to --color-moved=dimmed-zebra)
No big deal, but if you for any reason need to retouch, I'd suggest to also adapt the commit subject to the PR title -- "added test for disabled wallet" is very generic and doesn't say much.
7 participants
This PR enables a part of the non-wallet functional test (rpc_signmessage.py) to be run even with the Bitcoin Core wallet disabled, it is inspired by #20078.
Divided tests in rpc_signmessage.py into 2 files wallet_signmessagewithaddress.py and rpc_signmessagewithprivkey.py, latter one can run even when wallet is disabled that provides extra test which was not performed earlier.