test: Use pathlib over os path

nsvrn commented at 12:40 pm on September 2, 2023: contributor

In reference to issue #28362 refactoring of functional tests to use pathlib over os.path to reduce verbosity and increase the intuitiveness of managing file access.

DrahtBot commented at 12:40 pm on September 2, 2023: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	maflcko, willcl-ark

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28514 (wallet: Fix wallet directory initialization by BrandonOdiwuor)
#27228 (test: exempt previous release binaries from valgrind by Sjors)

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.

DrahtBot added the label Tests on Sep 2, 2023

hebasto commented at 12:51 pm on September 2, 2023: member

https://cirrus-ci.com/task/5520063122374656:

0AttributeError: 'PosixPath' object has no attribute 'readlink'

nsvrn commented at 1:05 pm on September 2, 2023: contributor

https://cirrus-ci.com/task/5520063122374656:
0AttributeError: 'PosixPath' object has no attribute 'readlink'

Thank you for pointing this out, seems like readlink was introduced in python 3.9 and we’re using 3.8 here. I will fix that shortly. Just quick question on how do you get the task ID for the cirrus-ci from the details or is it something only members can get to?

Secondly, it seems like this issue is causing 3 other failures as well but I will double check and once I fix this issue - is it ok for me to close this PR and open a new one because i will do a fresh fork and PR to make it cleaner (otherwise I can follow the squash commit instructions as per contribution guidelines).

hebasto commented at 1:10 pm on September 2, 2023: member

Just quick question on how do you get the task ID for the cirrus-ci from the details or is it something only members can get to?

Links should be available via the “Checks” tab in this PR on the GitHub web-interface. Perhaps, a Cirrus CI account is required as well.

Secondly, it seems like this issue is causing 3 other failures as well but I will double check and once I fix this issue - is it ok for me to close this PR and open a new one because i will do a fresh fork and PR to make it cleaner (otherwise I can follow the squash commit instructions as per contribution guidelines).

No need to close a PR and open another. The accepted routine is to update your local tests_use_pathlib branch and force push it at your convenience.

EDIT: While CI fails, you also might mark this PR as a draft.

nsvrn marked this as a draft on Sep 2, 2023

hebasto commented at 1:13 pm on September 2, 2023: member

Thank you for pointing this out, seems like readlink was introduced in python 3.9 and we’re using 3.8 here.

FYI: https://github.com/bitcoin/bitcoin/pull/28211

DrahtBot added the label CI failed on Sep 2, 2023

nsvrn force-pushed on Sep 2, 2023

in test/functional/wallet_backup.py:236 in c968d3ad91 outdated

231@@ -246,13 +232,13 @@ def run_test(self):
232 
233         # Backup to source wallet file must fail
234         sourcePaths = [
235-            os.path.join(self.nodes[0].wallets_path, self.default_wallet_name, self.wallet_data_filename),
236-            os.path.join(self.nodes[0].wallets_path, '.', self.default_wallet_name, self.wallet_data_filename),
237-            os.path.join(self.nodes[0].wallets_path, self.default_wallet_name),
238-            os.path.join(self.nodes[0].wallets_path)]
239+            self.nodes[0].wallets_path / self.default_wallet_name / self.wallet_data_filename,
240+            self.nodes[0].wallets_path / 'x' / self.default_wallet_name / self.wallet_data_filename,

maflcko commented at 8:50 am on September 3, 2023:

Where is 'x' from?

nsvrn commented at 1:19 pm on September 3, 2023:

the original . here pathlib was resolving it to remove it whereas os.path was keeping it and since it was passing as strings I mixed this up thinking we’re trying to fail one of these source paths but you’re right, I switched it back to . and it works.

maflcko commented at 1:54 pm on September 3, 2023:

If you are changing the behavior of a test, for example removing test coverage or changing it, it would be good to leave as-is for now. There is no need to remove all use of os.path.join. The main goal should be to remove the TestNode.datadir method

in test/functional/wallet_backup.py:241 in c968d3ad91 outdated

241+            self.nodes[0].wallets_path / self.default_wallet_name,
242+            self.nodes[0].wallets_path]
243 
244         for sourcePath in sourcePaths:
245-            assert_raises_rpc_error(-4, "backup failed", self.nodes[0].backupwallet, sourcePath)
246+            assert_raises_rpc_error(-4, "backup failed", self.nodes[0].backupwallet, str(sourcePath))

maflcko commented at 8:51 am on September 3, 2023:

Is this needed? RPC methods should be able to be passed pathlib paths.

nsvrn commented at 1:20 pm on September 3, 2023:

you’re correct, removed

in test/functional/feature_addrman.py:47 in c968d3ad91 outdated

43@@ -44,7 +44,7 @@ def serialize_addrman(
44 
45 
46 def write_addrman(peers_dat, **kwargs):
47-    with open(peers_dat, "wb") as f:
48+    with peers_dat.open(mode="wb") as f:

maflcko commented at 8:54 am on September 3, 2023:

nit: Is this change needed? Seems better to just leave as-is if there is no reason to change?

nsvrn commented at 1:23 pm on September 3, 2023:

ya i think in python3 both pathlib and os open use io.open so no difference, in general my thought was to remove import os from files so that there’s an extra step of adding the import is involved in using os.path.join in the code just as a deterrent but ya I will revert this one.

EDIT: never mind, open is available without import os so what I said i is incorrect

maflcko commented at 1:56 pm on September 3, 2023:

open is a built-in, not os.open, so I don’t think this affects os imports at all.

in test/functional/feature_config_args.py:96 in c968d3ad91 outdated

100+        with inc_conf_file_path.open(mode='w', encoding='utf-8') as conf:
101             conf.write('server=1\nrpcuser=someuser\nmain.rpcpassword=some#pass')
102         self.nodes[0].assert_start_raises_init_error(expected_msg='Error: Error reading configuration file: parse error on line 3, using # in rpcpassword can be ambiguous and should be avoided')
103 
104-        with open(inc_conf_file_path, 'w', encoding='utf-8') as conf:
105+        with inc_conf_file_path.open(mode='w', encoding='utf-8') as conf:

maflcko commented at 8:54 am on September 3, 2023:

Same here?

nsvrn commented at 1:23 pm on September 3, 2023:

agreed, will revert

maflcko commented at 8:55 am on September 3, 2023: member

Left some questions on the closed pull and here.

nsvrn force-pushed on Sep 3, 2023

in test/functional/combine_logs.py:94 in 925b7f479c outdated

90@@ -91,7 +91,7 @@ def read_logs(tmp_dir):
91     files = [("test", "%s/test_framework.log" % tmp_dir)]
92     for i in itertools.count():
93         logfile = "{}/node{}/{}/debug.log".format(tmp_dir, i, chain)
94-        if not os.path.isfile(logfile):
95+        if not Path(logfile).is_file():

maflcko commented at 7:32 am on September 4, 2023:

nit: Not sure if it makes sense to change this. Either logfile is a Path to begin with and is using the Path interface throughout, or it isn’t. Converting a string to Path just to call is_file on it doesn’t seem to add any value, does it? My preference for this pull would be to focus on removing the TestNode.datadir helper only.

in test/functional/combine_logs.py:132 in 925b7f479c outdated

131         fullpath = join_tmp(basename)
132         return (
133-            os.path.isdir(fullpath)
134-            and basename.startswith(TMPDIR_PREFIX)
135+            fullpath.is_dir()
136+            and str(basename).startswith(TMPDIR_PREFIX)

maflcko commented at 7:33 am on September 4, 2023:

basename is a str already, no? Seems odd to change this

in test/functional/wallet_signer.py:205 in 925b7f479c outdated

201@@ -202,7 +202,7 @@ def test_valid_signer(self):
202 
203         assert hww.testmempoolaccept([mock_tx])[0]["allowed"]
204 
205-        with open(os.path.join(self.nodes[1].cwd, "mock_psbt"), "w", encoding="utf8") as f:
206+        with open(Path(self.nodes[1].cwd) / "mock_psbt", "w", encoding="utf8") as f:

maflcko commented at 7:38 am on September 4, 2023:

Not sure if this makes sense. Either cwd is a Path to begin with, but wrapping a str into Path for no benefit, seems not worth it.

For now, my preference would be to focus on removing TestNode.datadir only.

maflcko commented at 7:39 am on September 4, 2023: member

Not sure about the random Path constructors inserted, which clutter the code. I think it would be easier to review if no new Path constructor was added.

nsvrn force-pushed on Sep 4, 2023

in test/functional/feature_settings.py:25 in 4c2ae1f773 outdated

21@@ -22,7 +22,7 @@ def set_test_params(self):
22     def run_test(self):
23         node, = self.nodes
24         settings = Path(node.chain_path, "settings.json")
25-        conf = Path(node.datadir, "bitcoin.conf")
26+        conf = Path(node.datadir_path, "bitcoin.conf")

maflcko commented at 12:58 pm on September 4, 2023:

0        conf = node.datadir_path / "bitcoin.conf"

nit: This is already a path, so no need to run a no-op on it again.

in test/functional/feature_settings.py:82 in 4c2ae1f773 outdated

78@@ -79,7 +79,7 @@ def run_test(self):
79             self.stop_node(0)
80 
81         # Test alternate settings path
82-        altsettings = Path(node.datadir, "altsettings.json")
83+        altsettings = Path(node.datadir_path, "altsettings.json")

maflcko commented at 12:58 pm on September 4, 2023:

Same, etc …

DrahtBot removed the label CI failed on Sep 4, 2023

nsvrn force-pushed on Sep 4, 2023

in test/functional/test_framework/test_node.py:102 in 819a994e77 outdated

 98@@ -99,7 +99,7 @@ def __init__(self, i, datadir, *, chain, rpchost, timewait, timeout_factor, bitc
 99         # spam debug.log.
100         self.args = [
101             self.binary,
102-            "-datadir=" + self.datadir,
103+            "-datadir=" + str(self.datadir_path),

maflcko commented at 4:09 pm on September 4, 2023:

0            f"-datadir={self.datadir_path}",

in test/functional/test_framework/util.py:415 in 819a994e77 outdated

411@@ -412,7 +412,7 @@ def write_config(config_path, *, n, chain, extra_config="", disable_autoconnect=
412 
413 
414 def get_datadir_path(dirname, n):
415-    return os.path.join(dirname, "node" + str(n))
416+        return pathlib.Path(dirname) / f"node{n}"

maflcko commented at 4:10 pm on September 4, 2023:

0    return pathlib.Path(dirname) / f"node{n}"

in test/functional/test_framework/test_node.py:128 in 819a994e77 outdated

124@@ -127,7 +125,7 @@ def __init__(self, i, datadir, *, chain, rpchost, timewait, timeout_factor, bitc
125         if self.version_is_at_least(239000):
126             self.args.append("-loglevel=trace")
127 
128-        self.cli = TestNodeCLI(bitcoin_cli, self.datadir)
129+        self.cli = TestNodeCLI(bitcoin_cli, str(self.datadir_path))

maflcko commented at 4:12 pm on September 4, 2023:

May be better to just fixup the send_cli method?

maflcko approved

maflcko commented at 4:13 pm on September 4, 2023: member

lgtm

maflcko commented at 4:14 pm on September 4, 2023: member

If you are done with the changes and confident in them, you can take this pull out of draft.

nsvrn marked this as ready for review on Sep 4, 2023

nsvrn marked this as a draft on Sep 4, 2023

nsvrn force-pushed on Sep 4, 2023

nsvrn marked this as ready for review on Sep 4, 2023

DrahtBot added the label Needs rebase on Sep 5, 2023

fanquake commented at 9:36 am on September 5, 2023: member

@ns-xvrn when you rebase this, can you update the commit message to drop all the now-redundant messages from your previously squashed commits? i.e we don’t things like “reversions after PR review” included in the commit message.

nsvrn force-pushed on Sep 5, 2023

DrahtBot removed the label Needs rebase on Sep 5, 2023

DrahtBot added the label CI failed on Oct 7, 2023

maflcko approved

maflcko commented at 11:16 am on October 10, 2023: member

I think you’ll have to rebase on current master. Otherwise:

Nice, lgtm ACK 12c8a210fe8a9e1e4cce95e719da0728a53866ef 🔊

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: Nice, lgtm ACK 12c8a210fe8a9e1e4cce95e719da0728a53866ef  🔊
3nRLN+xBFTudmjfgANKjDSiWaCvK/pfswAHKlWzf8NyiteLNzZhGP6l2wiZzab1v8twadrnY1ETOCUR7oAGDtDA==

nsvrn force-pushed on Oct 10, 2023

fanquake commented at 12:48 pm on October 10, 2023: member

Thanks, you’ll need to update has_blockfile in test_framework.py as well. Should fix the failing feature_assumeutxo.py.

test: Use pathlib over os.path #28362

revert netutil chgs py3.8 compliant

fixes based on PR review

bfa0bd632a

nsvrn force-pushed on Oct 10, 2023

maflcko commented at 1:48 pm on October 10, 2023: member

re-ACK bfa0bd632a7ce5d04005e20cba79abb32aec8da8 🐨

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: re-ACK  bfa0bd632a7ce5d04005e20cba79abb32aec8da8  🐨
3FCtv08fhfrsUvxCN8vUpfczHjg0LVrjbVd650I6AYDIutlzNfRs64msHcc0ZaQ19nw2GIxSuGaAg1iIt/cwnAw==

maflcko added this to the milestone 26.0 on Oct 10, 2023

DrahtBot removed the label CI failed on Oct 10, 2023

willcl-ark approved

willcl-ark commented at 9:41 am on October 11, 2023: member

ACK bfa0bd632a7ce5d04005e20cba79abb32aec8da8

Changes look good to me. I ran a few quick greps to try and find any places that this had been missed but couldn’t find any.

I did spot one or two changed lines where we could have taken the opportunity to switch to format strings from .format(), along with switching to double-quoted strings (which, if I am correct we are preferring and switching to?), but not relevant to the correctness of these changes.

fanquake merged this on Oct 11, 2023

fanquake closed this on Oct 11, 2023

Frank-GER referenced this in commit 9490565002 on Oct 13, 2023

bitcoin locked this on Oct 10, 2024

test: Use pathlib over os path #28392

Code Coverage

Reviews

Conflicts