test: Use pathlib over os path by nsvrn · Pull Request #28392 · bitcoin/bitcoin

nsvrn · 2023-09-02T12:40:05Z

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 · 2023-09-02T12:40:08Z

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.

hebasto · 2023-09-02T12:51:41Z

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

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

nsvrn · 2023-09-02T13:05:19Z

https://cirrus-ci.com/task/5520063122374656:
AttributeError: '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 · 2023-09-02T13:10:22Z

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.

hebasto · 2023-09-02T13:13:38Z

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

FYI: #28211

maflcko

Left some questions on the closed pull and here.

maflcko · 2023-09-03T08:50:47Z

test/functional/wallet_backup.py

Where is 'x' from?

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.

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

maflcko · 2023-09-03T08:51:21Z

test/functional/wallet_backup.py

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

you're correct, removed

maflcko · 2023-09-03T08:54:10Z

test/functional/feature_addrman.py

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

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

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

maflcko · 2023-09-03T08:54:44Z

test/functional/feature_config_args.py

agreed, will revert

maflcko · 2023-09-04T07:32:54Z

test/functional/combine_logs.py

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.

maflcko

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.

maflcko · 2023-09-04T07:33:40Z

test/functional/combine_logs.py

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

maflcko · 2023-09-04T07:38:08Z

test/functional/wallet_signer.py

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 · 2023-09-04T12:58:18Z

test/functional/feature_settings.py

Suggested change

conf = Path(node.datadir_path, "bitcoin.conf")

conf = node.datadir_path / "bitcoin.conf"

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

maflcko · 2023-09-04T12:58:32Z

test/functional/feature_settings.py

Same, etc ...

maflcko

lgtm

maflcko · 2023-09-04T16:09:21Z

test/functional/test_framework/test_node.py

Suggested change

"-datadir=" + str(self.datadir_path),

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

maflcko · 2023-09-04T16:10:46Z

test/functional/test_framework/util.py

Suggested change

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

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

maflcko · 2023-09-04T16:12:01Z

test/functional/test_framework/test_node.py

May be better to just fixup the send_cli method?

maflcko · 2023-09-04T16:14:15Z

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

fanquake · 2023-09-05T09:36:30Z

@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.

maflcko

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

Nice, lgtm ACK 12c8a210fe8a9e1e4cce95e719da0728a53866ef 🔊

Show signature

Signature:

untrusted 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}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: Nice, lgtm ACK 12c8a210fe8a9e1e4cce95e719da0728a53866ef  🔊
nRLN+xBFTudmjfgANKjDSiWaCvK/pfswAHKlWzf8NyiteLNzZhGP6l2wiZzab1v8twadrnY1ETOCUR7oAGDtDA==

fanquake · 2023-10-10T12:48:31Z

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

revert netutil chgs py3.8 compliant fixes based on PR review

maflcko · 2023-10-10T13:48:43Z

re-ACK bfa0bd6 🐨

Show signature

Signature:

untrusted 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}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK  bfa0bd632a7ce5d04005e20cba79abb32aec8da8  🐨
FCtv08fhfrsUvxCN8vUpfczHjg0LVrjbVd650I6AYDIutlzNfRs64msHcc0ZaQ19nw2GIxSuGaAg1iIt/cwnAw==

willcl-ark

ACK bfa0bd6

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.

DrahtBot added the Tests label Sep 2, 2023

nsvrn marked this pull request as draft September 2, 2023 13:13

DrahtBot added the CI failed label Sep 2, 2023

maflcko reviewed Sep 3, 2023

View reviewed changes

maflcko reviewed Sep 4, 2023

View reviewed changes

DrahtBot removed the CI failed label Sep 4, 2023

maflcko approved these changes Sep 4, 2023

View reviewed changes

nsvrn marked this pull request as ready for review September 4, 2023 17:15

nsvrn marked this pull request as draft September 4, 2023 17:17

nsvrn marked this pull request as ready for review September 4, 2023 20:07

fanquake linked an issue Sep 5, 2023 that may be closed by this pull request

test: Use pathlib over os.path #28362

Closed

DrahtBot added the Needs rebase label Sep 5, 2023

DrahtBot removed the Needs rebase label Sep 5, 2023

This was referenced Sep 20, 2023

qa: Fix intermittent "Unable to connect to bitcoind" errors on Windows #28509

Closed

wallet: Fix wallet directory initialization #28514

Closed

DrahtBot mentioned this pull request Oct 3, 2023

AssumeUTXO follow-ups #28562

Merged

DrahtBot added the CI failed label Oct 7, 2023

maflcko approved these changes Oct 10, 2023

View reviewed changes

test: Use pathlib over os.path #28362

bfa0bd6

revert netutil chgs py3.8 compliant fixes based on PR review

maflcko added this to the 26.0 milestone Oct 10, 2023

DrahtBot removed the CI failed label Oct 10, 2023

willcl-ark approved these changes Oct 11, 2023

View reviewed changes

fanquake merged commit d98d88c into bitcoin:master Oct 11, 2023

Frank-GER pushed a commit to syscoin/syscoin that referenced this pull request Oct 13, 2023

Merge bitcoin#28392: test: Use pathlib over os path

9490565

jonatack mentioned this pull request Oct 16, 2023

test: update tool_wallet coverage for unexpected writes to wallet #28116

Closed

bitcoin locked and limited conversation to collaborators Oct 10, 2024

	conf = Path(node.datadir_path, "bitcoin.conf")
	conf = node.datadir_path / "bitcoin.conf"

	"-datadir=" + str(self.datadir_path),
	f"-datadir={self.datadir_path}",

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

Conversation

nsvrn commented Sep 2, 2023

Uh oh!

DrahtBot commented Sep 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

hebasto commented Sep 2, 2023

Uh oh!

nsvrn commented Sep 2, 2023

Uh oh!

hebasto commented Sep 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Sep 2, 2023

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nsvrn Sep 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Sep 4, 2023

Uh oh!

fanquake commented Sep 5, 2023

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

fanquake commented Oct 10, 2023

Uh oh!

maflcko commented Oct 10, 2023

Uh oh!

willcl-ark left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

DrahtBot commented Sep 2, 2023 •

edited

Loading

hebasto commented Sep 2, 2023 •

edited

Loading

nsvrn Sep 3, 2023 •

edited

Loading