Having the binary related utils sit in the test_framework.py is fine. However, they are mostly stand-alone utils, which may be used externally.
So move them to utils.py, to allow easier external use. The diff is trivial and can be reviewed via the git options --color-moved=dimmed-zebra --color-moved-ws=ignore-all-space.
Can be reviewed with the git options
--color-moved=dimmed-zebra --color-moved-ws=ignore-all-space
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33633.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | kevkevinpal, Sjors, janb84, musaHaruna, yuvicc, enirox001 |
| Concept ACK | l0rinc |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
253
254 def setup(self):
255 """Call this method to start up the test framework object with options set."""
256
257 check_json_precision()
258+ export_env_build_path(self.config)
is there a reason why you moved this logic above this line of code?
self.options.cachedir = os.path.abspath(self.options.cachedir)
ACK fa75ef4
Looks straightforward to me and makes sense to move those functions outside of the test framework to be used externally
ACK fa75ef4328f638221bcf85fcbefa885122084622
Pr moves Helper class for information about bitcoin binaries to util.py.
To move the a helper class outside Test framework makes sense to me, this cleans up the test_framework.py code
Not completely convinced that these functions in the helper class will be used externally due to the tight coupling with these config settings config["environment"]["BUILDDIR"], bin", (this could be a good follow-up, if needed by external testers)
Code review ACK fa75ef4328f638221bcf85fcbefa885122084622
The Binaries class and get_binary_paths() are logically separate from TestFramework orchestration, so moving them to util.py makes sense for modularity.
As mentioned by @janb84, the tight coupling with config["environment"]["BUILDDIR"] and related settings means external usage still requires test framework. Not blocking for this PR, but worth considering for future refactoring if there’s the requirement.
TestFramework class itself.
Code Review ACK fa75ef4
PR looks straight forward, moving binaries to util.py files, help make them reusable and not bound to the Test framework.
The config is written by the build system, so I don’t think it is coupled with the TestFramework class itself.
Also confirmed that the config is written by the build system located at bitcoin/build/test/config.ini.
From my understanding, bitcoin/test/config.ini.in is a blueprint that gets filled in with real build info like [environment] and [components] after compilation.
Example after building:
0[environment]
1CLIENT_NAME=Bitcoin Core
2CLIENT_BUGREPORT=https://github.com/bitcoin/bitcoin/issues
3SRCDIR=/home/user-name/bitcoin
4BUILDDIR=/home/user-name/bitcoin/build
5EXEEXT=
6RPCAUTH=/home/user-name/bitcoin/share/rpcauth/rpcauth.py
7
8[components]
9ENABLE_WALLET=true
10ENABLE_CLI=true
313+ default_filename = os.path.join(
314+ config["environment"]["BUILDDIR"],
315+ "bin",
316+ binary + config["environment"]["EXEEXT"],
317+ )
318+ setattr(paths, env_variable_name.lower(), os.getenv(env_variable_name, default=default_filename))
Just to clarify, i’m not saying that it is coupled to the test_framework, but that it is coupled to the config. For a followup I could see something like the code below, for this PR I like that it is more or less a code move.
Rough example; not ment to be a nit:
0def get_binary_paths(bin_path, exe_ext):
1 """Get paths of all binaries from environment variables or their default values"""
2
3 paths = types.SimpleNamespace()
4 binaries = {
5 "bitcoin": "BITCOIN_BIN",
6 "bitcoind": "BITCOIND",
7 "bitcoin-cli": "BITCOINCLI",
8 "bitcoin-util": "BITCOINUTIL",
9 "bitcoin-tx": "BITCOINTX",
10 "bitcoin-chainstate": "BITCOINCHAINSTATE",
11 "bitcoin-wallet": "BITCOINWALLET",
12 }
13 # Set paths to bitcoin core binaries allowing overrides with environment
14 # variables.
15 for binary, env_variable_name in binaries.items():
16 default_filename = os.path.join(
17 bin_path,
18 binary + exe_ext,
19 )
20 setattr(paths, env_variable_name.lower(), os.getenv(env_variable_name, default=default_filename))
321@@ -322,6 +322,13 @@ def get_binary_paths(config):
322 return paths
323
324
325+def export_env_build_path(config):
nano nit:
consider adding a docstring to the new export_env_build_path function. Its purpose was clear in the old context, but a comment would be helpful now that it’s a standalone utility.