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

878d914777a03a04ecb84217152e8b7fd73a5062 doc: test: mention OS detection preferences in style guideline (Sebastian Falbesoner)
4c65ac96f8b021c107783adce3e8afe4f8edee6e test: detect OS consistently using `platform.system()` (Sebastian Falbesoner)
37324ae3dfb0e50daaf752dc863a880559fa4637 test: use `skip_if_platform_not_linux` helper where possible (Sebastian Falbesoner)

Pull request description:

  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](https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2023-12-08#989301;) 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
  ```
  $ 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.

ACKs for top commit:
  kevkevinpal:
    ACK [878d914](https://github.com/bitcoin/bitcoin/pull/29034/commits/878d914777a03a04ecb84217152e8b7fd73a5062)
  achow101:
    ACK 878d914777a03a04ecb84217152e8b7fd73a5062
  hebasto:
    ACK 878d914777a03a04ecb84217152e8b7fd73a5062, I have reviewed the code and it looks OK.
  pablomartin4btc:
    tACK 878d914777a03a04ecb84217152e8b7fd73a5062

Tree-SHA512: 24513d493e47f572028c843260b81c47c2c29bfb701991050255c9f9529cd19065ecbc7b3b6e15619da7f3f608b4825c345ce6fee30d8fd1eaadbd08cff400fc

12 files changed, 35 insertions, 34 deletions

diff --git a/test/functional/README.md b/test/functional/README.md
index 1bd618a0c3..a4994f2e7c 100644
--- a/test/functional/README.md
+++ b/test/functional/README.md
@@ -37,6 +37,10 @@ don't have test cases for.
   `set_test_params()`, `add_options()` and `setup_xxxx()` methods at the top of
   the subclass, then locally-defined helper methods, then the `run_test()` method.
 - Use `f'{x}'` for string formatting in preference to `'{}'.format(x)` or `'%s' % x`.
+- Use `platform.system()` for detecting the running operating system and `os.name` to
+  check whether it's a POSIX system (see also the `skip_if_platform_not_{linux,posix}`
+  methods in the `BitcoinTestFramework` class, which can be used to skip a whole test
+  depending on the platform).
 
 #### Naming guidelines
 
diff --git a/test/functional/feature_bind_extra.py b/test/functional/feature_bind_extra.py
index 4a94d2ce7b..5cd031f852 100755
--- a/test/functional/feature_bind_extra.py
+++ b/test/functional/feature_bind_extra.py
@@ -7,15 +7,12 @@ Test starting bitcoind with -bind and/or -bind=...=onion and confirm
 that bind happens on the expected ports.
 """
 
-import sys
-
 from test_framework.netutil import (
     addr_to_hex,
     get_bind_addrs,
 )
 from test_framework.test_framework import (
     BitcoinTestFramework,
-    SkipTest,
 )
 from test_framework.util import (
     assert_equal,
@@ -32,12 +29,11 @@ class BindExtraTest(BitcoinTestFramework):
         self.bind_to_localhost_only = False
         self.num_nodes = 2
 
-    def setup_network(self):
+    def skip_test_if_missing_module(self):
         # Due to OS-specific network stats queries, we only run on Linux.
-        self.log.info("Checking for Linux")
-        if not sys.platform.startswith('linux'):
-            raise SkipTest("This test can only be run on Linux.")
+        self.skip_if_platform_not_linux()
 
+    def setup_network(self):
         loopback_ipv4 = addr_to_hex("127.0.0.1")
 
         # Start custom ports by reusing unused p2p ports
diff --git a/test/functional/feature_config_args.py b/test/functional/feature_config_args.py
index dcea662089..9e13a3deef 100755
--- a/test/functional/feature_config_args.py
+++ b/test/functional/feature_config_args.py
@@ -6,8 +6,8 @@
 
 import os
 from pathlib import Path
+import platform
 import re
-import sys
 import tempfile
 import time
 
@@ -116,7 +116,7 @@ class ConfArgsTest(BitcoinTestFramework):
     def test_config_file_log(self):
         # Disable this test for windows currently because trying to override
         # the default datadir through the environment does not seem to work.
-        if sys.platform == "win32":
+        if platform.system() == "Windows":
             return
 
         self.log.info('Test that correct configuration path is changed when configuration file changes the datadir')
@@ -339,7 +339,7 @@ class ConfArgsTest(BitcoinTestFramework):
     def test_ignored_default_conf(self):
         # Disable this test for windows currently because trying to override
         # the default datadir through the environment does not seem to work.
-        if sys.platform == "win32":
+        if platform.system() == "Windows":
             return
 
         self.log.info('Test error is triggered when bitcoin.conf in the default data directory sets another datadir '
diff --git a/test/functional/feature_init.py b/test/functional/feature_init.py
index 142d75a851..268009b0f4 100755
--- a/test/functional/feature_init.py
+++ b/test/functional/feature_init.py
@@ -3,8 +3,8 @@
 # Distributed under the MIT software license, see the accompanying
 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
 """Stress tests related to node initialization."""
-import os
 from pathlib import Path
+import platform
 import shutil
 
 from test_framework.test_framework import BitcoinTestFramework, SkipTest
@@ -36,7 +36,7 @@ class InitStressTest(BitcoinTestFramework):
         # and other approaches (like below) don't work:
         #
         #   os.kill(node.process.pid, signal.CTRL_C_EVENT)
-        if os.name == 'nt':
+        if platform.system() == 'Windows':
             raise SkipTest("can't SIGTERM on Windows")
 
         self.stop_node(0)
diff --git a/test/functional/feature_notifications.py b/test/functional/feature_notifications.py
index adf6c13973..d2b5315d31 100755
--- a/test/functional/feature_notifications.py
+++ b/test/functional/feature_notifications.py
@@ -4,6 +4,7 @@
 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
 """Test the -alertnotify, -blocknotify and -walletnotify options."""
 import os
+import platform
 
 from test_framework.address import ADDRESS_BCRT1_UNSPENDABLE
 from test_framework.descriptors import descsum_create
@@ -14,13 +15,13 @@ from test_framework.util import (
 
 # Linux allow all characters other than \x00
 # Windows disallow control characters (0-31) and /\?%:|"<>
-FILE_CHAR_START = 32 if os.name == 'nt' else 1
+FILE_CHAR_START = 32 if platform.system() == 'Windows' else 1
 FILE_CHAR_END = 128
-FILE_CHARS_DISALLOWED = '/\\?%*:|"<>' if os.name == 'nt' else '/'
+FILE_CHARS_DISALLOWED = '/\\?%*:|"<>' if platform.system() == 'Windows' else '/'
 UNCONFIRMED_HASH_STRING = 'unconfirmed'
 
 def notify_outputname(walletname, txid):
-    return txid if os.name == 'nt' else f'{walletname}_{txid}'
+    return txid if platform.system() == 'Windows' else f'{walletname}_{txid}'
 
 
 class NotificationsTest(BitcoinTestFramework):
@@ -181,7 +182,7 @@ class NotificationsTest(BitcoinTestFramework):
                 # Universal newline ensures '\n' on 'nt'
                 assert_equal(text[-1], '\n')
                 text = text[:-1]
-                if os.name == 'nt':
+                if platform.system() == 'Windows':
                     # On Windows, echo as above will append a whitespace
                     assert_equal(text[-1], ' ')
                     text = text[:-1]
diff --git a/test/functional/feature_remove_pruned_files_on_startup.py b/test/functional/feature_remove_pruned_files_on_startup.py
index c128587949..4ee653142a 100755
--- a/test/functional/feature_remove_pruned_files_on_startup.py
+++ b/test/functional/feature_remove_pruned_files_on_startup.py
@@ -4,6 +4,7 @@
 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
 """Test removing undeleted pruned blk files on startup."""
 
+import platform
 import os
 from test_framework.test_framework import BitcoinTestFramework
 
@@ -32,7 +33,7 @@ class FeatureRemovePrunedFilesOnStartupTest(BitcoinTestFramework):
         self.nodes[0].pruneblockchain(600)
 
         # Windows systems will not remove files with an open fd
-        if os.name != 'nt':
+        if platform.system() != 'Windows':
             assert not os.path.exists(blk0)
             assert not os.path.exists(rev0)
             assert not os.path.exists(blk1)
diff --git a/test/functional/rpc_bind.py b/test/functional/rpc_bind.py
index 43cd23fc7a..3106419e5c 100755
--- a/test/functional/rpc_bind.py
+++ b/test/functional/rpc_bind.py
@@ -4,8 +4,6 @@
 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
 """Test running bitcoind with the -rpcbind and -rpcallowip options."""
 
-import sys
-
 from test_framework.netutil import all_interfaces, addr_to_hex, get_bind_addrs, test_ipv6_local
 from test_framework.test_framework import BitcoinTestFramework, SkipTest
 from test_framework.util import assert_equal, assert_raises_rpc_error, get_rpc_proxy, rpc_port, rpc_url
@@ -17,6 +15,10 @@ class RPCBindTest(BitcoinTestFramework):
         self.num_nodes = 1
         self.supports_cli = False
 
+    def skip_test_if_missing_module(self):
+        # due to OS-specific network stats queries, this test works only on Linux
+        self.skip_if_platform_not_linux()
+
     def setup_network(self):
         self.add_nodes(self.num_nodes, None)
 
@@ -61,14 +63,9 @@ class RPCBindTest(BitcoinTestFramework):
         self.stop_nodes()
 
     def run_test(self):
-        # due to OS-specific network stats queries, this test works only on Linux
         if sum([self.options.run_ipv4, self.options.run_ipv6, self.options.run_nonloopback]) > 1:
             raise AssertionError("Only one of --ipv4, --ipv6 and --nonloopback can be set")
 
-        self.log.info("Check for linux")
-        if not sys.platform.startswith('linux'):
-            raise SkipTest("This test can only be run on linux.")
-
         self.log.info("Check for ipv6")
         have_ipv6 = test_ipv6_local()
         if not have_ipv6 and not (self.options.run_ipv4 or self.options.run_nonloopback):
diff --git a/test/functional/test_framework/p2p.py b/test/functional/test_framework/p2p.py
index b1ed97b794..34fe467d23 100755
--- a/test/functional/test_framework/p2p.py
+++ b/test/functional/test_framework/p2p.py
@@ -24,6 +24,7 @@ import asyncio
 from collections import defaultdict
 from io import BytesIO
 import logging
+import platform
 import struct
 import sys
 import threading
@@ -592,7 +593,7 @@ class NetworkThread(threading.Thread):
 
         NetworkThread.listeners = {}
         NetworkThread.protos = {}
-        if sys.platform == 'win32':
+        if platform.system() == 'Windows':
             asyncio.set_event_loop_policy(asyncio.WindowsSelectorEventLoopPolicy())
         NetworkThread.network_event_loop = asyncio.new_event_loop()
 
diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py
index b3f777b9df..77f6e69e98 100755
--- a/test/functional/test_framework/test_node.py
+++ b/test/functional/test_framework/test_node.py
@@ -12,6 +12,7 @@ import http.client
 import json
 import logging
 import os
+import platform
 import re
 import subprocess
 import tempfile
@@ -19,7 +20,6 @@ import time
 import urllib.parse
 import collections
 import shlex
-import sys
 from pathlib import Path
 
 from .authproxy import (
@@ -566,7 +566,7 @@ class TestNode():
                 cmd, shell=True,
                 stderr=subprocess.DEVNULL, stdout=subprocess.DEVNULL) == 0
 
-        if not sys.platform.startswith('linux'):
+        if platform.system() != 'Linux':
             self.log.warning("Can't profile with perf; only available on Linux platforms")
             return None
 
diff --git a/test/functional/test_framework/util.py b/test/functional/test_framework/util.py
index c65e3e38e6..b4b05b1597 100644
--- a/test/functional/test_framework/util.py
+++ b/test/functional/test_framework/util.py
@@ -13,8 +13,8 @@ import json
 import logging
 import os
 import pathlib
+import platform
 import re
-import sys
 import time
 
 from . import coverage
@@ -414,12 +414,12 @@ def get_temp_default_datadir(temp_dir: pathlib.Path) -> tuple[dict, pathlib.Path
     """Return os-specific environment variables that can be set to make the
     GetDefaultDataDir() function return a datadir path under the provided
     temp_dir, as well as the complete path it would return."""
-    if sys.platform == "win32":
+    if platform.system() == "Windows":
         env = dict(APPDATA=str(temp_dir))
         datadir = temp_dir / "Bitcoin"
     else:
         env = dict(HOME=str(temp_dir))
-        if sys.platform == "darwin":
+        if platform.system() == "Darwin":
             datadir = temp_dir / "Library/Application Support/Bitcoin"
         else:
             datadir = temp_dir / ".bitcoin"
diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py
index fb1e3594bb..826705598f 100755
--- a/test/functional/test_runner.py
+++ b/test/functional/test_runner.py
@@ -17,6 +17,7 @@ from collections import deque
 import configparser
 import datetime
 import os
+import platform
 import time
 import shutil
 import signal
@@ -42,8 +43,8 @@ except UnicodeDecodeError:
     CROSS = "x "
     CIRCLE = "o "
 
-if os.name != 'nt' or sys.getwindowsversion() >= (10, 0, 14393): #type:ignore
-    if os.name == 'nt':
+if platform.system() != 'Windows' or sys.getwindowsversion() >= (10, 0, 14393): #type:ignore
+    if platform.system() == 'Windows':
         import ctypes
         kernel32 = ctypes.windll.kernel32  # type: ignore
         ENABLE_VIRTUAL_TERMINAL_PROCESSING = 4
diff --git a/test/functional/wallet_multiwallet.py b/test/functional/wallet_multiwallet.py
index 10bc516d8f..ee866ee59b 100755
--- a/test/functional/wallet_multiwallet.py
+++ b/test/functional/wallet_multiwallet.py
@@ -9,9 +9,9 @@ Verify that a bitcoind node can load multiple wallet files
 from decimal import Decimal
 from threading import Thread
 import os
+import platform
 import shutil
 import stat
-import sys
 import time
 
 from test_framework.authproxy import JSONRPCException
@@ -143,7 +143,7 @@ class MultiWalletTest(BitcoinTestFramework):
 
         # should raise rpc error if wallet path can't be created
         err_code = -4 if self.options.descriptors else -1
-        assert_raises_rpc_error(err_code, "filesystem error:" if sys.platform != 'win32' else "create_directories:", self.nodes[0].createwallet, "w8/bad")
+        assert_raises_rpc_error(err_code, "filesystem error:" if platform.system() != 'Windows' else "create_directories:", self.nodes[0].createwallet, "w8/bad")
 
         # check that all requested wallets were created
         self.stop_node(0)