It seems inconsistent, fragile and verbose to:
get_datadir_path to recreate the path that already exists as field in TestNodeos.path.join with the hardcoded chain name or self.chain to recreate the TestNode chain_path property"node0")Fix all issues by using the TestNode properties.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | theStack, willcl-ark |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
<!--174a7506f384e20aa4161008e828411d-->
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.
Concept ACK
Here's a few more instances which I think could be simplified by using chain_path (found via $ git grep join.*regtest ./test):
https://github.com/bitcoin/bitcoin/blob/1ecdf6ea8fb6dc1d64f41323a1e337beb174c25b/test/functional/wallet_backwards_compatibility.py#L75-L78 https://github.com/bitcoin/bitcoin/blob/1ecdf6ea8fb6dc1d64f41323a1e337beb174c25b/test/functional/wallet_backwards_compatibility.py#L160-L163 https://github.com/bitcoin/bitcoin/blob/1ecdf6ea8fb6dc1d64f41323a1e337beb174c25b/test/functional/wallet_inactive_hdchains.py#L133-L134 https://github.com/bitcoin/bitcoin/blob/1ecdf6ea8fb6dc1d64f41323a1e337beb174c25b/test/functional/wallet_migration.py#L37 https://github.com/bitcoin/bitcoin/blob/1ecdf6ea8fb6dc1d64f41323a1e337beb174c25b/test/functional/wallet_migration.py#L460 https://github.com/bitcoin/bitcoin/blob/1ecdf6ea8fb6dc1d64f41323a1e337beb174c25b/test/functional/wallet_upgradewallet.py#L141-L145
Concept ACK.
In addition to those highlighted by TheStack, there are also quite a few other places we could update to use this, e.g. some of these matches:
$ git grep -n "os.path" test/functional | rg -e '.*\.chain*'
test/functional/feature_addrman.py:56: peers_dat = os.path.join(self.nodes[0].datadir, self.chain, "peers.dat")
test/functional/feature_asmap.py:116: self.datadir = os.path.join(self.node.datadir, self.chain)
test/functional/feature_blocksdir.py:33: assert os.path.isfile(os.path.join(blocksdir_path, self.chain, "blocks", "blk00000.dat"))
test/functional/feature_blocksdir.py:34: assert os.path.isdir(os.path.join(self.nodes[0].datadir, self.chain, "blocks", "index"))
test/functional/feature_config_args.py:418: assert os.path.exists(os.path.join(new_data_dir, self.chain, 'blocks'))
test/functional/feature_config_args.py:424: assert os.path.exists(os.path.join(new_data_dir_2, self.chain, 'blocks'))
test/functional/feature_fee_estimation.py:323: fee_dat = os.path.join(self.nodes[0].datadir, self.chain, "fee_estimates.dat")
test/functional/feature_filelock.py:27: datadir = os.path.join(self.nodes[0].datadir, self.chain)
test/functional/feature_loadblock.py:40: blocks_dir = os.path.join(data_dir, self.chain, "blocks")
test/functional/feature_logging.py:19: return os.path.join(self.nodes[0].datadir, self.chain, name)
test/functional/feature_posix_fs_permissions.py:34: datadir = os.path.join(self.nodes[0].datadir, self.chain)
test/functional/feature_pruning.py:94: self.prunedir = os.path.join(self.nodes[2].datadir, self.chain, 'blocks', '')
test/functional/feature_pruning.py:293: return os.path.isfile(os.path.join(self.nodes[node_number].datadir, self.chain, "blocks", f"blk{index:05}.dat"))
test/functional/feature_reindex.py:42: blk0 = os.path.join(self.nodes[0].datadir, self.nodes[0].chain, 'blocks', 'blk00000.dat')
test/functional/feature_remove_pruned_files_on_startup.py:23: blk0 = os.path.join(self.nodes[0].datadir, self.nodes[0].chain, 'blocks', 'blk00000.dat')
test/functional/feature_remove_pruned_files_on_startup.py:24: rev0 = os.path.join(self.nodes[0].datadir, self.nodes[0].chain, 'blocks', 'rev00000.dat')
test/functional/feature_remove_pruned_files_on_startup.py:25: blk1 = os.path.join(self.nodes[0].datadir, self.nodes[0].chain, 'blocks', 'blk00001.dat')
test/functional/feature_remove_pruned_files_on_startup.py:26: rev1 = os.path.join(self.nodes[0].datadir, self.nodes[0].chain, 'blocks', 'rev00001.dat')
test/functional/feature_txindex_compatibility.py:53: legacy_chain_dir = os.path.join(self.nodes[0].datadir, self.chain)
test/functional/feature_txindex_compatibility.py:56: migrate_chain_dir = os.path.join(self.nodes[2].datadir, self.chain)
test/functional/feature_txindex_compatibility.py:67: drop_index_chain_dir = os.path.join(self.nodes[1].datadir, self.chain)
test/functional/interface_rpc.py:49: assert_equal(info['logpath'], os.path.join(self.nodes[0].datadir, self.chain, 'debug.log'))
test/functional/mempool_compatibility.py:58: old_node_mempool = os.path.join(old_node.datadir, self.chain, 'mempool.dat')
test/functional/mempool_compatibility.py:59: new_node_mempool = os.path.join(new_node.datadir, self.chain, 'mempool.dat')
test/functional/mempool_persist.py:146: mempooldat0 = os.path.join(self.nodes[0].datadir, self.chain, 'mempool.dat')
test/functional/mempool_persist.py:147: mempooldat1 = os.path.join(self.nodes[1].datadir, self.chain, 'mempool.dat')
test/functional/test_framework/test_framework.py:826: return os.path.join(cache_node_dir, self.chain, *paths)
test/functional/tool_wallet.py:404: self.wallet_path = os.path.join(self.nodes[0].datadir, self.chain, 'wallets', self.default_wallet_name, self.wallet_data_filename)
test/functional/wallet_backup.py:112: os.remove(os.path.join(self.nodes[0].datadir, self.chain, 'wallets', self.default_wallet_name, self.wallet_data_filename))
test/functional/wallet_backup.py:113: os.remove(os.path.join(self.nodes[1].datadir, self.chain, 'wallets', self.default_wallet_name, self.wallet_data_filename))
test/functional/wallet_backup.py:114: os.remove(os.path.join(self.nodes[2].datadir, self.chain, 'wallets', self.default_wallet_name, self.wallet_data_filename))
test/functional/wallet_backup.py:121: not_created_wallet_file = os.path.join(node.datadir, self.chain, 'wallets', wallet_name)
test/functional/wallet_backup.py:131: not_created_wallet_file = os.path.join(node.datadir, self.chain, 'wallets', wallet_name)
test/functional/wallet_backup.py:138: wallet_file = os.path.join(node.datadir, self.chain, 'wallets', wallet_name)
test/functional/wallet_backup.py:209: assert os.path.exists(os.path.join(self.nodes[3].datadir, self.chain, 'wallets', "res0"))
test/functional/wallet_backup.py:210: assert os.path.exists(os.path.join(self.nodes[3].datadir, self.chain, 'wallets', "res1"))
test/functional/wallet_backup.py:211: assert os.path.exists(os.path.join(self.nodes[3].datadir, self.chain, 'wallets', "res2"))
test/functional/wallet_backup.py:229: shutil.rmtree(os.path.join(self.nodes[2].datadir, self.chain, 'blocks'))
test/functional/wallet_backup.py:230: shutil.rmtree(os.path.join(self.nodes[2].datadir, self.chain, 'chainstate'))
test/functional/wallet_backup.py:251: os.path.join(self.nodes[0].datadir, self.chain, 'wallets', self.default_wallet_name, self.wallet_data_filename),
test/functional/wallet_backup.py:252: os.path.join(self.nodes[0].datadir, self.chain, '.', 'wallets', self.default_wallet_name, self.wallet_data_filename),
test/functional/wallet_backup.py:253: os.path.join(self.nodes[0].datadir, self.chain, 'wallets', self.default_wallet_name),
test/functional/wallet_backup.py:254: os.path.join(self.nodes[0].datadir, self.chain, 'wallets')]
test/functional/wallet_descriptor.py:237: wallet_db = os.path.join(self.nodes[0].datadir, self.chain, "wallets", "crashme", self.wallet_data_filename)
test/functional/wallet_hd.py:90: shutil.rmtree(os.path.join(self.nodes[1].datadir, self.chain, "blocks"))
test/functional/wallet_hd.py:91: shutil.rmtree(os.path.join(self.nodes[1].datadir, self.chain, "chainstate"))
test/functional/wallet_hd.py:94: os.path.join(self.nodes[1].datadir, self.chain, 'wallets', self.default_wallet_name, self.wallet_data_filename),
test/functional/wallet_hd.py:118: shutil.rmtree(os.path.join(self.nodes[1].datadir, self.chain, "blocks"))
test/functional/wallet_hd.py:119: shutil.rmtree(os.path.join(self.nodes[1].datadir, self.chain, "chainstate"))
test/functional/wallet_hd.py:122: os.path.join(self.nodes[1].datadir, self.chain, "wallets", self.default_wallet_name, self.wallet_data_filename),
test/functional/wallet_keypool_topup.py:36: wallet_path = os.path.join(self.nodes[1].datadir, self.chain, "wallets", self.default_wallet_name, self.wallet_data_filename)
test/functional/wallet_listtransactions.py:237: wallet0 = os.path.join(self.nodes[0].datadir, self.chain, self.default_wallet_name, "wallet.dat")
test/functional/wallet_listtransactions.py:238: wallet2 = os.path.join(self.nodes[2].datadir, self.chain, self.default_wallet_name, "wallet.dat")
test/functional/wallet_multiwallet.py:65: data_dir = lambda *p: os.path.join(node.datadir, self.chain, *p)
test/functional/wallet_pruning.py:109: return os.path.isfile(os.path.join(self.nodes[1].datadir, self.chain, "blocks", f"blk{block_index:05}.dat"))
test/functional/wallet_reorgsrestore.py:92: shutil.copyfile(os.path.join(self.nodes[0].datadir, 'wallet.bak'), os.path.join(self.nodes[1].datadir, self.chain, self.default_wallet_name, self.wallet_data_filename))
...although, changing some of these occurences does require additional changes for example when watching logs.
Are we looking to change all of these out and switch to the os.Path operator syntax throughout?
$ git grep -n "os.path" test/functional | rg -e '.*\.chain*'
Thanks, added a scripted-diff as second commit
$ git grep join.*regtest ./test
Thanks, modified the last commit
Light crACK fa7f1251f8
Looks good to me.
There are still instances in tests (outside of test_framework/) where we are using os.path.join on self.datadir which we could also switch to using self.datadir_path and the / operator. Is that something that we are looking to do here or in the future?
Is that something that we are looking to do here or in the future?
Thanks, fixed the remaining join calls that used self.chain on the datadir.
LGTM, found only one more instance where chain_path could be used:
diff --git a/test/functional/feature_anchors.py b/test/functional/feature_anchors.py
index 468ad1eafa..0961f21a40 100755
--- a/test/functional/feature_anchors.py
+++ b/test/functional/feature_anchors.py
@@ -20,9 +20,7 @@ class AnchorsTest(BitcoinTestFramework):
self.disable_autoconnect = False
def run_test(self):
- node_anchors_path = os.path.join(
- self.nodes[0].datadir, "regtest", "anchors.dat"
- )
+ node_anchors_path = self.nodes[0].chain_path / "anchors.dat"
self.log.info("When node starts, check if anchors.dat doesn't exist")
assert not os.path.exists(node_anchors_path)
As a follow-up, a new TestNode property wallets_path might also make sense, to deduplicate even more?
Same, looks good here. Will ack if you want to make the anchors change.
Question: Now that we use more Paths, would there be appetite to replace many of the os.path.join() calls with the / operator? It looks like ~100 lines or thereabouts, and not any performance wins, nor perfectly scripted-diff-able; so quite a lot of code churn (and might be considered "many grains of sand", and not a nice fat "rock"). But would make the tests more readable IMO?
As a follow-up, a new TestNode property wallets_path might also make sense, to deduplicate even more?
Thanks, added and updated the scripted-diff to avoid touching the same lines twice.
Seems odd to hardcode all parent directory names in the path for no good
reason.
Also, add wallet_path property to TestNode.
Also, rework wallet_backup.py test for scripted-diff in the next commit.
Instead of passing the datadir and chain name to os.path.join, just use
the existing properties, which are the same.
-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended 's|\.datadir, self\.chain, .wallets.|.wallets_path|g' $(git grep -l '\.datadir, self\.chain,')
sed -i --regexp-extended 's|\.datadir, self\.chain,|.chain_path,|g' $(git grep -l '\.datadir, self\.chain,')
-END VERIFY SCRIPT-
Also, add datadir_path property to TestNode
Seems odd to place the burden on test writers to hardcode the chain or
datadir path for the nodes under test.
62 | @@ -62,6 +63,8 @@ def __init__(self, rpc_error, http_status=None): 63 | def EncodeDecimal(o): 64 | if isinstance(o, decimal.Decimal): 65 | return str(o) 66 | + if isinstance(o, pathlib.Path): 67 | + return str(o)
nit (feel free to ignore for now, probably follow-up material): Now that this function is not handling decimals only anymore, I think it should be renamed from EncodeDecimal to something generic?
Yeah, it should have been called serialization_fallback? May do on the next push or leave for a follow-up.
Code-review ACK aaaa3aefbdfca1c9243057eeefdc19940e60bf18 🌊
Re-reviewed the changes since my previous review via $ git range-diff faf830e3...aaaa3aef
Is this rfm or does it need more review?
re-ACK aaaa3aefbdfca1c9243057eeefdc19940e60bf18
LGTM with the wallets_path added.
