test: fix 'unknown named parameter' test in `wallet

brunoerg commented at 7:04 PM on August 17, 2023: contributor

This PR removes loop when testing an unknown named parameter. They don't have any effect.

DrahtBot commented at 7:04 PM on August 17, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	theStack, jonatack
Stale ACK	kevkevinpal

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

DrahtBot added the label Tests on Aug 17, 2023

brunoerg force-pushed on Aug 17, 2023

DrahtBot added the label CI failed on Aug 17, 2023

kevkevinpal commented at 5:33 AM on August 18, 2023: contributor

ACK d9ef627

maflcko requested review from jonatack on Aug 18, 2023

DrahtBot removed the label CI failed on Aug 18, 2023

in test/functional/wallet_basic.py:313 in d9ef627af7 outdated

 309 | @@ -310,8 +310,7 @@ def run_test(self):
 310 |          node_0_bal += amount
 311 |          assert_equal(self.nodes[0].getbalance(), node_0_bal)
 312 |  
 313 | -        for key in ["totalFee", "feeRate"]:
 314 | -            assert_raises_rpc_error(-8, "Unknown named parameter key", self.nodes[2].sendtoaddress, address=address, amount=1, fee_rate=1, key=1)
 315 | +        assert_raises_rpc_error(-8, "Unknown named parameter key", self.nodes[2].sendtoaddress, address=address, amount=1, fee_rate=1, key=1)

jonatack commented at 6:44 PM on August 18, 2023:

Suggestion for both sites.

        assert_raises_rpc_error(-8, "Unknown named parameter feeRate", self.nodes[2].sendtoaddress, address=address, amount=1, fee_rate=1, feeRate=1)

jonatack commented at 7:12 PM on August 18, 2023: member

ACK, good catch. These tests were part of #20305, and I suppose the idea was to ensure totalFee and feeRate kwargs weren't valid. totalFee was then deprecated and subsequently removed from the codebase (there is a similar test in wallet_bumpfee.py). Testing for feeRate might be more interesting than for key.

brunoerg force-pushed on Aug 18, 2023

brunoerg commented at 7:51 PM on August 18, 2023: contributor

Thanks, @jonatack. Force-pushed changing it to feeRate instead of key.

jonatack commented at 8:09 PM on August 18, 2023: member

ACK 551c4e036dc18e6b4a917d854c9785170898aff7

theStack commented at 7:37 PM on August 19, 2023: contributor

Concept ACK

Could keep the loop and test for both invalid parameters (what was seemingly intended), as also done in e.g. wallet_bumpfee.py? https://github.com/bitcoin/bitcoin/blob/9b066da8af77f971046a42118fd65046321bed1b/test/functional/wallet_bumpfee.py#L126-L127 (I think you'd need the dict unpacking operator ** in this case though, as the parameters before are not positional, see https://stackoverflow.com/questions/47907268/how-do-i-pass-a-string-as-an-argument-name)

Also, IIUC the second loop could be removed completely, there is no value in having the exact same test executed twice for legacy wallets.

brunoerg force-pushed on Aug 21, 2023

brunoerg commented at 11:36 AM on August 21, 2023: contributor

Thanks, @theStack for the review. It seems better to keep the loop, just pushed changing it. Also, I checked that for both legacy and descriptor the first occurance is enough, so there is no need to have both. Removed the second one.

theStack approved

theStack commented at 12:05 PM on August 21, 2023: contributor

ACK 9ad950031bc487a089ada5aeb9d0bef467b74dd3

jonatack commented at 10:27 PM on August 21, 2023: member

Could keep the loop and test for both invalid parameters (what was seemingly intended), as also done in e.g. wallet_bumpfee.py?

I'm not sure that's necessary. totalFee was deprecated and then later removed from the codebase completely. It was only ever an argument in RPC bumpfee. I don't recall why I tested for it in this file. It may have been an inadvertent copy-paste as that PR was fairly large and by the time it was done the diff had become a blur.

in test/functional/wallet_basic.py:314 in 9ad950031b outdated

 310 | @@ -311,7 +311,7 @@ def run_test(self):
 311 |          assert_equal(self.nodes[0].getbalance(), node_0_bal)
 312 |  
 313 |          for key in ["totalFee", "feeRate"]:
 314 | -            assert_raises_rpc_error(-8, "Unknown named parameter key", self.nodes[2].sendtoaddress, address=address, amount=1, fee_rate=1, key=1)
 315 | +             assert_raises_rpc_error(-8, f"Unknown named parameter {key}", self.nodes[2].sendtoaddress, address=address, amount=1, fee_rate=1, **{key: 1})

jonatack commented at 10:32 PM on August 21, 2023:

nit, indentation

            assert_raises_rpc_error(-8, f"Unknown named parameter {key}", self.nodes[2].sendtoaddress, address=address, amount=1, fee_rate=1, **{key: 1})

test: fix 'unknown named parameter' test in `wallet_basic`

Fixes loop when testing an unknown named parameter.

452c094449

brunoerg force-pushed on Aug 22, 2023

brunoerg commented at 12:16 AM on August 22, 2023: contributor

I preferred to remove totalFee since it never had any relation to sendtoaddress, left only feeRate. I kept the removal of the second (and redundant) assert.

DrahtBot added the label CI failed on Aug 22, 2023

theStack approved

theStack commented at 1:48 AM on August 22, 2023: contributor

re-ACK 452c094449de00f3640e6e0763366e7603375825

(CI fail is in a different functional test and hence unrelated)

jonatack commented at 1:56 AM on August 22, 2023: member

ACK 452c094449de00f3640e6e0763366e7603375825

Thanks for fixing my oversights here.

jonatack commented at 2:01 AM on August 22, 2023: member

(CI fail is in a different functional test and hence unrelated)

Filed an issue to report it.

DrahtBot removed the label CI failed on Aug 22, 2023

fanquake merged this on Aug 22, 2023

fanquake closed this on Aug 22, 2023

Frank-GER referenced this in commit 159c10f9a5 on Sep 8, 2023

bitcoin locked this on Aug 21, 2024

test: fix 'unknown named parameter' test in `wallet_basic` #28288

Reviews