test: check for invalid listtransactions RPC parameters #23835

pull theStack wants to merge 1 commits into bitcoin:master from theStack:202112-test-rpc-check_invalid_listtransactions_parameters changing 1 files +9 −0

theStack commented at 7:21 PM on December 21, 2021: member

This PR adds missing test coverage for RPC errors that are thrown if invalid parameters are passed to listtransactions:

https://github.com/bitcoin/bitcoin/blob/887796a5ffcbafcd281b920f8d55fcb6e8347584/src/wallet/rpc/transactions.cpp#L508

https://github.com/bitcoin/bitcoin/blob/887796a5ffcbafcd281b920f8d55fcb6e8347584/src/wallet/rpc/transactions.cpp#L524

https://github.com/bitcoin/bitcoin/blob/887796a5ffcbafcd281b920f8d55fcb6e8347584/src/wallet/rpc/transactions.cpp#L526
DrahtBot added the label Tests on Dec 21, 2021

in test/functional/wallet_listtransactions.py:282 in 5934b61d5b outdated

 276 | @@ -275,6 +277,13 @@ def normalize_list(txs):
 277 |          assert_equal(['pizza2'], self.nodes[0].getaddressinfo(addr2)['labels'])
 278 |          assert_equal(['pizza3'], self.nodes[0].getaddressinfo(addr3)['labels'])
 279 |  
 280 | +    def run_invalid_parameters_test(self):
 281 | +        self.log.info("Test listtransactions RPC parameter validity")
 282 | +        assert_raises_rpc_error(-8, 'Label argument must be a valid label name or "*"', self.nodes[0].listtransactions, label="")

shaavan commented at 10:16 AM on December 22, 2021:

nit:

        assert_raises_rpc_error(-8, 'Label argument must be a valid label name or "*".', self.nodes[0].listtransactions, label="")

theStack commented at 7:46 PM on December 22, 2021:

Thanks, fixed!

shaavan commented at 10:16 AM on December 22, 2021: contributor
Concept ACK

The added test looks suitable for the code given in the description. The lines are tested in the following way:
- Line 508: By giving "" as the label.
- Line 524: By giving -1 as count.
- Line 526: By giving -1 skip value.
The code given for RPC_INVALID_PARAMETER is also correct ( = -8).

However, there's one minor nit I caught.
test: check for invalid listtransactions RPC parameters c27bba9672

in test/functional/wallet_listtransactions.py:283 in 5934b61d5b outdated

 276 | @@ -275,6 +277,13 @@ def normalize_list(txs):
 277 |          assert_equal(['pizza2'], self.nodes[0].getaddressinfo(addr2)['labels'])
 278 |          assert_equal(['pizza3'], self.nodes[0].getaddressinfo(addr3)['labels'])
 279 |  
 280 | +    def run_invalid_parameters_test(self):
 281 | +        self.log.info("Test listtransactions RPC parameter validity")
 282 | +        assert_raises_rpc_error(-8, 'Label argument must be a valid label name or "*"', self.nodes[0].listtransactions, label="")
 283 | +        self.nodes[0].listtransactions(label="*")

brunoerg commented at 10:44 AM on December 22, 2021:

I didn't understand this line, I think you're not using the return of this listtransactions. Noticed that removing it, the test doesn't fail.

theStack commented at 7:45 PM on December 22, 2021:

The intention here is to ensure that passing "*" as label is considered valid; the asterisk is not passed in any other part of the tests, so I thought it would make sense to do it here. I agree though that it looks a bit odd.

theStack force-pushed on Dec 22, 2021
shaavan approved
shaavan commented at 10:33 AM on December 23, 2021: contributor
ACK c27bba96723384488fb6995e21bb43969d94b0f6

Change since my last review:
- Addressed suggestion under this comment
MarcoFalke merged this on Dec 24, 2021
MarcoFalke closed this on Dec 24, 2021
theStack deleted the branch on Dec 24, 2021
sidhujag referenced this in commit 373589c42f on Dec 27, 2021
DrahtBot locked this on Dec 24, 2022

Contributors

Labels