test: detect OS in functional tests consistently using platform.system()
#29034
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, 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. |
Rather than re-implementing these checks, we can use this test framework's helper (introduced in commit c934087, PR bitcoin#24358) called in a test's `skip_test_if_missing_module` method instead.
7bdbd38 to
878d914
Compare
|
ACK 878d914 Ran this command to make sure we were not missing any, only seeing os.name show up for posix in I am running on |
|
Concept ACK. |
Confirming outputs for macOS and Windows. |
There was a problem hiding this comment.
tACK 878d914
Tested on both Linux (Ubuntu 22.04 & WSL) and Windows.
As @kevkevinpal pointed it out, there's still usage of os.name in test_framework.py, not sure if you want to cover that as well.
|
ACK 878d914 |
There are at least three ways to detect the operating system in Python3:
os.name(https://docs.python.org/3.9/library/os.html#os.name)sys.platform(https://docs.python.org/3.9/library/sys.html#sys.platform)platform.system()(https://docs.python.org/3.9/library/platform.html#platform.system)We are currently using all of them in functional tests (both in individual tests and shared test framework code), which seems a bit messy. This PR consolidates into using
platform.system(), as it appears to be one most consistent and easy to read (see also IRC discussion and table below).sys.platformis inconsistent as it has the major version number encoded for BSD systems, which doesn't make much sense for e.g. OpenBSD, where there is no concept of major versions, but instead the version is simply increased by 0.1 on each release.Note that
os.nameis still useful to detect whether we are running a POSIX system (seeBitcoinTestFramework.skip_if_platform_not_posix), so for this use-case it is kept as only exception. The following table shows values for common operating systems, found via* = I neither have a MacOS nor a Windows machine available, so I extracted the values from documentation and our current code. Also I'm relying on CI for testing the relevant code-paths. Having reviewers to this this locally would be very appreciated, if this gets Concept ACKed.