test: Force named args for RPCOverloadWrapper optional args #32360

1. 
   maflcko commented at 8:41 am on April 28, 2025: member

   This can avoid bugs and makes the test code easier to read, because the order of positional args does not have to be known or assumed.

   Also, contains two commits to remove dead code.

2. test: Remove unused RPCOverloadWrapper is_cli field cccc1f4e91
3. 
   DrahtBot commented at 8:41 am on April 28, 2025: contributor

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

   Code Coverage & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32360.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	rkrux, janb84, achow101

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

4. DrahtBot renamed this:
   test: Force named args in RPCOverloadWrapper optional args
   test: Force named args in RPCOverloadWrapper optional args
   on Apr 28, 2025
5. DrahtBot added the label Tests on Apr 28, 2025
6. maflcko force-pushed on Apr 28, 2025
7. bitcoin deleted a comment on Apr 28, 2025
8. bitcoin deleted a comment on Apr 28, 2025
9. maflcko renamed this:
   test: Force named args in RPCOverloadWrapper optional args
   test: Force named args for RPCOverloadWrapper optional args
   on Apr 28, 2025
10. in test/functional/test_framework/test_node.py:936 in fa588c43a2 outdated

    930@@ -931,8 +931,9 @@ def __init__(self, rpc):
    931     def __getattr__(self, name):
    932         return getattr(self.rpc, name)
    933 
    934-    def createwallet_passthrough(self, *args, **kwargs):
    935-        return self.__getattr__("createwallet")(*args, **kwargs)
    936+    def rpc_passthrough(self, name, *args, **kwargs):
    937+        """Directly pass through the RPC, avoiding any wrapped method."""
    938+        return self.__getattr__(name)(*args, **kwargs)
    

    ---

    ---

    

    rkrux commented at 1:10 pm on April 28, 2025:

    avoiding any wrapped method.

    Avoiding any of the below 4 wrapped methods?

    add a more general (currently unused) method rpc_passthrough, which could be used in the future, if there really is need for it.

    It seems to be effectively same as calling the instance returned by __getattr__ above, while improving readability. I don’t think it hurts to add this but I sense it will not end up being used if there are not already usages of it in the framework.

    ---

    

    maflcko commented at 1:15 pm on April 28, 2025:

    thx, removed
11. test: Remove unused createwallet_passthrough aaaa45399c
12. test: Force named args for RPCOverloadWrapper optional args

    This can avoid bugs and makes the test code easier to read, because the
    order of positional args does not have to be known or assumed.

    fa48be3ba4
13. maflcko force-pushed on Apr 28, 2025
14. in test/functional/wallet_labels.py:52 in fa48be3ba4

    48@@ -49,7 +49,7 @@ def invalid_label_name_test(self):
    49         assert_equal(response[0]['error']['message'], "Invalid label name")
    50 
    51         for rpc_call in rpc_calls:
    52-            assert_raises_rpc_error(-11, "Invalid label name", *rpc_call, "*")
    53+            assert_raises_rpc_error(-11, "Invalid label name", *rpc_call, label="*")
    

    ---

    ---

    

    rkrux commented at 1:39 pm on April 28, 2025:

    Note: This named arg here is passed down to the non-wrapped methods of RPCOverloadWrapper class as well because those are present in rpc_calls list, but after the positional args.
15. rkrux approved
16. 
    rkrux commented at 1:40 pm on April 28, 2025: contributor

    tACK fa48be3ba443d2436f754265b86331f84b866130

    This pull makes the optional looking arguments as named args instead of being positional in the 4 wrapped methods of RPCOverloadWrapper class in the functional test framework. I, too, have a preference for named args over positional as I feel it makes the function call sites easier on the eyes.

    There are couple removals of unused fields and methods of RPCOverloadWrapper class. Some formatting changes are also there that distracts a bit but are small enough to ignore.

17. 
    janb84 commented at 6:44 pm on April 28, 2025: contributor

    tACK fa48be3

    PR makes the optional arguments, named arguments in the RPCOverloadWrapper in the functional test framework. The named arguments are clearer and makes the code easier to understand (imho)

    • tested ✅
    • code-review ✅
18. 
    achow101 commented at 7:23 pm on April 28, 2025: member

    ACK fa48be3ba443d2436f754265b86331f84b866130
19. achow101 merged this on Apr 28, 2025
20. achow101 closed this on Apr 28, 2025

    ---

21. maflcko deleted the branch on Apr 29, 2025

Contributors
maflcko DrahtBot rkrux janb84 achow101

Labels
Tests