test: Add resendwallettransactions functional tests#11000
test: Add resendwallettransactions functional tests#11000maflcko merged 1 commit intobitcoin:masterfrom
Conversation
promag
force-pushed
the
201708-resendwallettransactions-test
branch
from
ae384aa to
0dabe79
Compare
|
Thanks! Can you add a test to restart the node with walletbroadcast and check that it works? Maybe move the walletbroadcast tests from wallet.py? |
You mean restart with explicit arg (
Maybe only the one calling |
|
You mean restart with explicit arg (`=true`)?
Can do that too, just always a good idea to test failure and success conditions together.
Maybe only the one calling `resendwallettransactions`?
I just figured if you move all the walletbroadcasting stuff to one place (wallet.py is already big enough) but don't really care.
|
jnewbery
left a comment
jnewbery
left a comment
There was a problem hiding this comment.
Each test script incurs about 5 seconds of overhead (node startup and shutdown), so it's much better to group tests for related functionality together into a single script. I think it makes sense to include this test in wallet.py.
There was a problem hiding this comment.
nit: prefer explicit import if you're only using one function from test_framework.util
There was a problem hiding this comment.
Comment is incorrect. This isn't about transactions in the mempool, it's about transactions in the wallet.
There was a problem hiding this comment.
As above, this isn't about mempool
promag
force-pushed
the
201708-resendwallettransactions-test
branch
from
0dabe79 to
f985328
Compare
|
Thanks @jnewbery, comments addressed, and rebased with #11002. Regarding where to place the tests, IMO performance issues shouldn't lead to long-and-hard-to-follow-and-also-tedious-to-change tests. It also makes parallel tests hard to implement. Beside that, in this particular case, at least one restart must occur. |
|
@promag and I chatted about this. I still prefer combining this with I'm happy to defer to consensus opinion if other people disagree. |
…nsaction 055d95f [wallet] return correct error code from resendwallettransaction (John Newbery) Pull request description: New code in #10995 uses `RPC_INVALID_REQUEST`. According to the comment in rpc/protocol.h: ``` // RPC_INVALID_REQUEST is internally mapped to HTTP_BAD_REQUEST (400). // It should not be used for application-layer errors. ``` Change the returned error code to `RPC_WALLET_ERROR` #11000 will need to be updated to test for the correct error code. Tree-SHA512: 0201b3a2091adf17ad301825da5bd29f0ea7e284b5394cbef80483fc293a558acc849f74a0780bb8501acab324fc722e41ae049cffec7afb76884e26df4b809e
|
ACK f9853283623df7ab3972611005b9dd089b6985ca @jnewbery I don't like long test-suite run times either, but in this case merging a test with the already hard-to-read |
|
@sdaftuar - sure, this is very much a case-by-case judgement call. Perhaps Matt's suggestion of having a separate test for all the wallet broadcast functionality is the best way to organise these tests. In any case, I certainly won't NACK any new tests based on my personal taste, and we can always refactor in later PRs if it makes sense to group things differently. |
promag
force-pushed
the
201708-resendwallettransactions-test
branch
from
f985328 to
bdf607e
Compare
|
Rebased since #11002 is merged. |
bdf607e test: Add resendwallettransactions functional tests (João Barbosa) Pull request description: Adds functional tests to cover the behaviour introduced in #10995. Tree-SHA512: 3be337cbe51edab2cc0eb9ecfa68d00cd3c3a00aef0672cd841706802726c9cd4138626a8f209c3d9fbd207ce332daa062f270e4c94c1c2398bfc475e36423f6
bdf607e test: Add resendwallettransactions functional tests (João Barbosa) Pull request description: Adds functional tests to cover the behaviour introduced in bitcoin#10995. Tree-SHA512: 3be337cbe51edab2cc0eb9ecfa68d00cd3c3a00aef0672cd841706802726c9cd4138626a8f209c3d9fbd207ce332daa062f270e4c94c1c2398bfc475e36423f6
…llettransaction 055d95f [wallet] return correct error code from resendwallettransaction (John Newbery) Pull request description: New code in bitcoin#10995 uses `RPC_INVALID_REQUEST`. According to the comment in rpc/protocol.h: ``` // RPC_INVALID_REQUEST is internally mapped to HTTP_BAD_REQUEST (400). // It should not be used for application-layer errors. ``` Change the returned error code to `RPC_WALLET_ERROR` bitcoin#11000 will need to be updated to test for the correct error code. Tree-SHA512: 0201b3a2091adf17ad301825da5bd29f0ea7e284b5394cbef80483fc293a558acc849f74a0780bb8501acab324fc722e41ae049cffec7afb76884e26df4b809e
6 participants
Adds functional tests to cover the behaviour introduced in #10995.