test: Use pathlib over os.path #28362

1. 
   maflcko commented at 10:43 am on August 29, 2023: member

   Motivation

   os.path is quite verbose in the tests and is problematic, because strings can be appended to it, for example it is possible to type:

   0>>> os.path.join('a/') + '/../b.txt'
   1'a//../b.txt'
   

   This may lead to issues down the line when the tests are run on Windows and expect a different path separator.

   Possible solution

   While some uses of os.path are hard(er) to replace, with questionable benefits, I think removing the use of TestNode.datadir and replacing it with TestNode.datadir_path is a good first step.

   Useful Skills

   • Compile Bitcoin Core and run the python tests
   • Basic familiarity with the Bitcoin Core RPC, and the python tests
   • os.path and pathlib knowledge

   Guidance for new contributors

   Want to work on this issue?

   For guidance on contributing, please read CONTRIBUTING.md before opening your pull request.

2. maflcko added the label good first issue on Aug 29, 2023
3. maflcko added the label Tests on Aug 29, 2023
4. 
   miles170 commented at 7:27 am on August 30, 2023: contributor

   I would like to work on this.
5. 
   nsvrn commented at 0:08 am on August 31, 2023: contributor

   If @miles170 doesn’t mind, I would like to attempt this one. I’ve changed all the refs in functional tests (18 files) and test_node.py had some methods where I think it also makes sense to pass Path object instead of datadir str so that os.path.join can be avoided in those places as well. All functional tests passed on my local.

   If ok by you guys, I can spend sometime on replacing os.path refs in the next commit.

6. 
   maflcko commented at 6:58 am on August 31, 2023: member

   Ah yes, for all lines you’ve touched, you can (in the same commit) drop os.path.join and use /
7. 
   maflcko commented at 8:03 am on August 31, 2023: member

   See https://docs.python.org/3.8/library/pathlib.html#correspondence-to-tools-in-the-os-module for the correspondence table. Also, removing the datadir property, if possible, ensures that it isn’t used anymore.
8. 
   nsvrn commented at 12:00 pm on August 31, 2023: contributor

   Understood, will do. Just did another commit, but not completely done yet. A dozen more files remaining, I should’ve this completed in a day or two.
9. 
   nsvrn commented at 0:04 am on September 1, 2023: contributor

   I have made the changes and just did a single clean commit. I was able to remove most of the os.path.join references, there’s a handful remaining in the files where those are just using config strings to join in a single use pattern and since those use the string output using Path won’t be very much value add (will have to typecast as strings again). I will do a PR once CI/CD is successful (I saw it automatically kicked off last time on my changes but doesn’t seem like the latest one is - I assume it should pick up from workflow automatically).
10. nsvrn referenced this in commit bacd6844d5 on Sep 2, 2023
11. 
    nsvrn commented at 12:29 pm on September 2, 2023: contributor

    There were some issues when I opened the PR: https://github.com/bitcoin/bitcoin/pull/28388/checks

    I’ve fixed the linting issues from that list but I’m not sure about the rest of the issues so I would appreciate if anyone can advise on those so I can fix those and resubmit another PR.

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

    There were some issues when I opened the PR: #28388 (checks)

    I’ve fixed the linting issues from that list but I’m not sure about the rest of the issues so I would appreciate if anyone can advise on those so I can fix those and resubmit another PR.

    A PR itself is a proper place to review and discuss any issues. Just open it and follow reviewers’ comments.

13. nsvrn referenced this in commit c968d3ad91 on Sep 2, 2023
14. nsvrn referenced this in commit 9f8990a9d0 on Sep 3, 2023
15. nsvrn referenced this in commit 925b7f479c on Sep 3, 2023
16. nsvrn referenced this in commit 4c2ae1f773 on Sep 4, 2023
17. nsvrn referenced this in commit 819a994e77 on Sep 4, 2023
18. nsvrn referenced this in commit 854ccc5552 on Sep 4, 2023
19. nsvrn referenced this in commit 12c8a210fe on Sep 5, 2023
20. nsvrn referenced this in commit 9cc8389c40 on Oct 10, 2023
21. nsvrn referenced this in commit bfa0bd632a on Oct 10, 2023
22. fanquake referenced this in commit d98d88c779 on Oct 11, 2023
23. fanquake closed this on Oct 11, 2023

    ---

24. bitcoin locked this on Oct 10, 2024

Contributors
maflcko miles170 nsvrn hebasto

Labels
Tests good first issue