test: Treat `bitcoin-wallet` binary in the same way as others #27554

pull hebasto wants to merge 2 commits into bitcoin:master from hebasto:230502-toolwallet changing 2 files +19 −20

hebasto commented at 2:36 PM on May 2, 2023: member

This PR makes the bitcoin-wallet binary path customizable in the same way how it can be done now with other ones, including bitcoind, bitcoin-cli and bitcoin-util.
hebasto added the label Tests on May 2, 2023
DrahtBot commented at 2:36 PM on May 2, 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 stickies-v

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

Type	Reviewers
ACK	stickies-v

in test/functional/test_framework/test_framework.py:263 in 018c8bc845 outdated

 258 | +            "bitcoin-wallet" + config["environment"]["EXEEXT"],
 259 | +        )
 260 |          self.options.bitcoind = os.getenv("BITCOIND", default=fname_bitcoind)
 261 |          self.options.bitcoincli = os.getenv("BITCOINCLI", default=fname_bitcoincli)
 262 |          self.options.bitcoinutil = os.getenv("BITCOINUTIL", default=fname_bitcoinutil)
 263 | +        self.options.bitcoinwallet = os.getenv("BITCOINWALLET", default=fname_bitcoinwallet)

maflcko commented at 3:07 PM on May 2, 2023:

        self.options.bitcoinwallet = os.getenv("BITCOINWALLET", default=fname("bitcoin-wallet"))

What about making this a function to reduce code bloat while touching this?

hebasto commented at 4:35 PM on May 2, 2023:

What about making this a function to reduce code bloat while touching this?

Done.

hebasto force-pushed on May 2, 2023

in test/functional/test_framework/test_framework.py:240 in 0079c55d7a outdated

 235 | +            "bitcoin-util": ("bitcoinutil", "BITCOINUTIL"),
 236 | +            "bitcoin-wallet": ("bitcoinwallet", "BITCOINWALLET"),
 237 | +        }
 238 | +        for binary in binaries.keys():
 239 | +            attribute_name = binaries[binary][0]
 240 | +            env_variable_name = binaries[binary][1]

maflcko commented at 4:39 PM on May 2, 2023:

            attribute_name, env_variable_name = binaries[binary]

nit

hebasto commented at 6:08 PM on May 2, 2023:

Thanks! Updated.

maflcko approved
maflcko commented at 4:39 PM on May 2, 2023: member

lgtm
hebasto force-pushed on May 2, 2023

in test/functional/test_framework/test_framework.py:239 in ef01337bbb outdated

 234 | +            "bitcoin-cli": ("bitcoincli", "BITCOINCLI"),
 235 | +            "bitcoin-util": ("bitcoinutil", "BITCOINUTIL"),
 236 | +            "bitcoin-wallet": ("bitcoinwallet", "BITCOINWALLET"),
 237 | +        }
 238 | +        for binary in binaries.keys():
 239 | +            attribute_name, env_variable_name = binaries[binary]

stickies-v commented at 11:29 AM on May 5, 2023:

nit

        for binary, [attribute_name, env_variable_name] in binaries.items():

in test/functional/test_framework/test_framework.py:231 in ef01337bbb outdated

 227 | @@ -228,6 +228,22 @@ def parse_args(self):
 228 |  
 229 |          PortSeed.n = self.options.port_seed
 230 |  
 231 | +    def set_binary_paths(self):

stickies-v commented at 11:35 AM on May 5, 2023:

nit

    """Update self.options with the paths of all binaries from environment variables or their default values"""
    def set_binary_paths(self):

stickies-v commented at 12:05 PM on May 5, 2023:

Oops sorry, the docstring is meant to go underneath the function def, not above. My bad.

hebasto commented at 12:36 PM on May 5, 2023:

Adjusted.

stickies-v approved
stickies-v commented at 11:39 AM on May 5, 2023: contributor

ACK ef01337bbb3d5269db6a26e42c7bbd238000ce2f

Nice little refactoring (modulo being able to now set BITCOINWALLET which is new behaviour), less code duplication and functions with smaller scope.
hebasto force-pushed on May 5, 2023
hebasto commented at 12:04 PM on May 5, 2023: member
Updated ef01337bbb3d5269db6a26e42c7bbd238000ce2f -> de8b7ab2e023e28168e870629c2423dab589085e (pr27554.03 -> pr27554.04, diff):
- addressed @stickies-v's comments

test, refactor: Add `set_binary_paths` function

This change factors out the repeated code into a new `set_binary_paths`
function.

dda961cec5

test: Treat `bitcoin-wallet` binary in the same way as others

This change makes the `bitcoin-wallet` binary path customizable in the
same way how it can be done now with other ones, including `bitcoind`,
`bitcoin-cli` and `bitcoin-util`.

f6d7636be4

hebasto force-pushed on May 5, 2023
stickies-v approved
stickies-v commented at 1:04 PM on May 5, 2023: contributor

re-ACK f6d7636be4eb0b19878428906dd5e394df7d07a2
maflcko approved
fanquake merged this on May 5, 2023
fanquake closed this on May 5, 2023
hebasto deleted the branch on May 5, 2023
sidhujag referenced this in commit e7c70491fd on May 5, 2023
fanquake referenced this in commit 5ef2c1ee7a on May 23, 2023
sidhujag referenced this in commit c2228804b5 on May 23, 2023
luke-jr referenced this in commit 61a6e0ba5f on Aug 16, 2023
bitcoin locked this on May 4, 2024

Contributors

Labels

Linked (view graph)

#27717 test: Make `util/test_runner.py` honor `BITCOINUTIL` and `BITCOINTX`

test: Treat `bitcoin-wallet` binary in the same way as others #27554

Reviews