test: detect OS in functional tests consistently using platform.system() #29034

pull theStack wants to merge 3 commits into bitcoin:master from theStack:202312-test-consolidate_os_detection_in_python changing 12 files +35 −34
  1. theStack commented at 5:12 pm on December 8, 2023: contributor

    There are at least three ways to detect the operating system in Python3:

    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.platform is 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.name is still useful to detect whether we are running a POSIX system (see BitcoinTestFramework.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

    0$ python3 -c "import os; import sys; import platform; print(os.name, sys.platform, platform.system())"
    
    OS os.name sys.platform platform.system()
    Linux 6.2.0 posix linux Linux
    MacOS* posix darwin Darwin
    OpenBSD 7.4 posix openbsd7 OpenBSD
    Windows* nt win32 Windows

    * = 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.

  2. DrahtBot commented at 5:12 pm on December 8, 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.

    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:

    • #29182 (test: randomize perturbed files excluding ldb by L0laL33tz)
    • #22729 (Make it possible to disable Tor binds and abort startup on bind failure by vasild)

    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.

  3. DrahtBot added the label Tests on Dec 8, 2023
  4. test: use `skip_if_platform_not_linux` helper where possible
    Rather than re-implementing these checks, we can use this test
    framework's helper (introduced in commit
    c934087b627f7d368458781944f990b0eb479634, PR #24358) called in a test's
    `skip_test_if_missing_module` method instead.
    37324ae3df
  5. test: detect OS consistently using `platform.system()` 4c65ac96f8
  6. doc: test: mention OS detection preferences in style guideline 878d914777
  7. theStack force-pushed on Dec 8, 2023
  8. DrahtBot added the label CI failed on Dec 8, 2023
  9. DrahtBot removed the label CI failed on Dec 8, 2023
  10. kevkevinpal commented at 7:25 pm on December 8, 2023: contributor

    ACK 878d914


    Ran this command to make sure we were not missing any, only seeing os.name show up for posix in test_framework.py

    0$ grep -nr "os\.name" ./test && grep -nr "sys\.platform" ./test
    1
    2./test/functional/README.md:40:- Use `platform.system()` for detecting the running operating system and `os.name` to
    3./test/functional/test_framework/test_framework.py:918:        if os.name != 'posix':
    

    I am running on MacOs 14.1.2 (23B92) and got the following from running the code provided

    0$ python3 -c "import os; import sys; import platform; print(os.name, sys.platform, platform.system())"
    1
    2posix darwin Darwin
    
  11. hebasto commented at 2:51 pm on December 10, 2023: member
    Concept ACK.
  12. hebasto commented at 4:17 pm on December 10, 2023: member

    The following table shows values for common operating systems, found via

    0$ python3 -c "import os; import sys; import platform; print(os.name, sys.platform, platform.system())"
    
    OS os.name sys.platform platform.system()
    MacOS* posix darwin Darwin
    Windows* nt win32 Windows
    • = 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.

    Confirming outputs for macOS and Windows.

  13. hebasto approved
  14. hebasto commented at 5:57 pm on December 10, 2023: member
    ACK 878d914777a03a04ecb84217152e8b7fd73a5062, I have reviewed the code and it looks OK.
  15. pablomartin4btc commented at 6:55 pm on January 10, 2024: member

    tACK 878d914777a03a04ecb84217152e8b7fd73a5062

    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.

  16. achow101 commented at 5:16 pm on January 11, 2024: member
    ACK 878d914777a03a04ecb84217152e8b7fd73a5062
  17. achow101 merged this on Jan 11, 2024
  18. achow101 closed this on Jan 11, 2024

  19. theStack deleted the branch on Jan 11, 2024

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-22 00:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me