Merge #1427: Test: refactor conftest.py

a9deacc Refactor `conftest.py`: (PulpCattel)

Pull request description:

  * Uses [shlex.split()](https://docs.python.org/3/library/shlex.html#shlex.split) to avoid unnecessary handling of command lists
  * Removes unnecessary globals
  * PEP8 fixes
  * Uses [`yield`](https://docs.pytest.org/en/7.2.x/how-to/fixtures.html#yield-fixtures-recommended) instead of `addfinalizer` teardown
  * Add docstrings
  * Clean up functions

  Tests pass for me, let me know if/what it breaks for you.

ACKs for top commit:
  kristapsk:
    ACK a9deacc

Tree-SHA512: 1a1e4056c06054afbe57405605961265c8629b553e82270204b30dc79b1267bf313403066369a3b446ebe0ccc487a56a00f58e49525d720f89c074f01b236a9c

conftest.py

@@ -1,51 +1,72 @@
-import pytest
-import re
 import os
-import time
+import re
 import subprocess
-from typing import Any
+from shlex import split
+from time import sleep
+from typing import Any, Tuple
+
+import pytest
 
-bitcoin_path = None
-bitcoin_conf = None
-bitcoin_rpcpassword = None
-bitcoin_rpcusername = None
-miniircd_procs = []
 
-def get_bitcoind_version(version_string: bytes) -> tuple:
-    # this utility function returns the version number
-    # as a tuple in the form (major, minor)
+def get_bitcoind_version(bitcoind_path: str) -> Tuple[int, int]:
+    """
+    This utility function returns the bitcoind version number
+    as a tuple in the form (major, minor)
+    """
+    version = local_command(f'{bitcoind_path} -version')
+    if version.returncode != 0:
+        raise RuntimeError(version.stdout.decode('utf-8'))
+    version_string = version.stdout.split(b'\n')[0]
     version_tuple = re.match(
-        br'.*v(?P<major>\d+)\.(?P<minor>\d+)',
-        version_string).groups()
-    return tuple(map(lambda x: int(x), version_tuple))
+        br'.*v(?P<major>\d+)\.(?P<minor>\d+)', version_string).groups()
+    major, minor = map(lambda x: int(x), version_tuple)
+    return major, minor
+
 
 def local_command(command: str, bg: bool = False):
+    """
+    Execute command in a new process.
+    """
+    command = split(command)
     if bg:
-        #using subprocess.PIPE seems to cause problems
+        # using subprocess.PIPE seems to cause problems
         FNULL = open(os.devnull, 'w')
         return subprocess.Popen(command,
                                 stdout=FNULL,
                                 stderr=subprocess.STDOUT,
                                 close_fds=True)
-    else:
-        #in case of foreground execution, we can use the output; if not
-        #it doesn't matter
-        return subprocess.run(command, stdout=subprocess.PIPE,
-            stderr=subprocess.STDOUT)
+    # in case of foreground execution, we can use the output; if not
+    # it doesn't matter
+    return subprocess.run(command, stdout=subprocess.PIPE,
+                          stderr=subprocess.STDOUT)
+
 
 def root_path() -> str:
-    # returns the directory in which this file is contained
+    """
+    Returns the directory in which this file is contained.
+    """
     return os.path.dirname(os.path.realpath(__file__))
 
+
+def btc_conf_test_path() -> str:
+    """
+    Returns default Bitcoin conf test path.
+    """
+    return os.path.join(root_path(), 'test/bitcoin.conf')
+
+
 def pytest_addoption(parser: Any) -> None:
+    """
+    Pytest initialization hook to register argparse-style options.
+    """
     parser.addoption("--btcroot", action="store", default='',
-                     help="the fully qualified path to the directory containing "+\
-                     "the bitcoin binaries, e.g. /home/user/bitcoin/bin/")
+                     help="the fully qualified path to the directory containing " +
+                          "the bitcoin binaries, e.g. /home/user/bitcoin/bin/")
     parser.addoption("--btcconf", action="store",
-                         default=os.path.join(root_path(), 'test/bitcoin.conf'),
-                         help="the fully qualified path to the location of the "+\
-                         "bitcoin configuration file you use for testing, e.g. "+\
-                         "/home/user/.bitcoin/bitcoin.conf")
+                     default=btc_conf_test_path(),
+                     help="the fully qualified path to the location of the " +
+                          "bitcoin configuration file you use for testing, e.g. " +
+                          "/home/user/.bitcoin/bitcoin.conf")
     parser.addoption("--btcpwd",
                      action="store",
                      help="the RPC password for your test bitcoin instance")
@@ -54,82 +75,73 @@ def pytest_addoption(parser: Any) -> None:
                      default='bitcoinrpc',
                      help="the RPC username for your test bitcoin instance (default=bitcoinrpc)")
     parser.addoption("--nirc",
-                         type="int",
-                         action="store",
-                         default=1,
-    help="the number of local miniircd instances")
-
-def teardown() -> None:
-    #didn't find a stop command in miniircd, so just kill
-    global miniircd_procs
-    for m in miniircd_procs:
-        m.kill()
-    #shut down bitcoin and remove the regtest dir
-    local_command([os.path.join(bitcoin_path, "bitcoin-cli"), "-regtest",
-                    "-rpcuser=" + bitcoin_rpcusername,
-                   "-rpcpassword=" + bitcoin_rpcpassword, "stop"])
-    #note, it is better to clean out ~/.bitcoin/regtest but too
-    #dangerous to automate it here perhaps
+                     type="int",
+                     action="store",
+                     default=1,
+                     help="the number of local miniircd instances")
 
 
 @pytest.fixture(scope="session", autouse=True)
-def setup(request) -> None:
-    request.addfinalizer(teardown)
-
-    global bitcoin_conf, bitcoin_path, bitcoin_rpcpassword, bitcoin_rpcusername
-    bitcoin_path = request.config.getoption("--btcroot")
-    bitcoin_conf = request.config.getoption("--btcconf")
-    print("Here is the bitcoin_conf path:")
-    print(bitcoin_conf)
-    bitcoin_rpcpassword = request.config.getoption("--btcpwd")
-    bitcoin_rpcusername = request.config.getoption("--btcuser")
-
-    #start up miniircd
-    #minor bug in miniircd (seems); need *full* unqualified path for motd file
+def setup_miniircd(pytestconfig):
+    """
+    Setup miniircd and handle its clean up.
+    """
+    miniircd_procs = []
     cwd = os.getcwd()
-    n_irc = request.config.getoption("--nirc")
-    global miniircd_procs
+    n_irc = pytestconfig.getoption("--nirc")
+    miniircd_path = os.path.join(root_path(), 'miniircd', 'miniircd')
+    # minor bug in miniircd (seems); need *full* unqualified path for motd file
+    motd_path = os.path.join(cwd, 'miniircd', 'testmotd')
     for i in range(n_irc):
-        miniircd_proc = local_command(
-            ["./miniircd/miniircd", "--ports=" + str(16667+i),
-             "--motd=" + cwd + "/miniircd/testmotd"],
-            bg=True)
+        command = f"{miniircd_path} --ports={16667 + i} --motd={motd_path}"
+        miniircd_proc = local_command(command, bg=True)
         miniircd_procs.append(miniircd_proc)
+    yield
+    # didn't find a stop command in miniircd, so just kill
+    for m in miniircd_procs:
+        m.kill()
 
-    # determine bitcoind version
-    bitcoind_version_string = subprocess.check_output([
-        os.path.join(bitcoin_path, "bitcoind"), "-version"]).split(b'\n')[0]
-    bitcoind_version = get_bitcoind_version(bitcoind_version_string)
-
-    #start up regtest blockchain
-    bitcoin_args = ["-regtest", "-daemon", "-conf=" + bitcoin_conf]
 
-    btc_proc = subprocess.call([os.path.join(bitcoin_path, "bitcoind")] +
-        bitcoin_args)
-    root_cmd = [os.path.join(bitcoin_path, "bitcoin-cli"), "-regtest",
-                       "-rpcuser=" + bitcoin_rpcusername,
-                       "-rpcpassword=" + bitcoin_rpcpassword]
+@pytest.fixture(scope="session", autouse=True)
+def setup_regtest_bitcoind(pytestconfig):
+    """
+    Setup regtest bitcoind and handle its clean up.
+    """
+    conf = pytestconfig.getoption("--btcconf")
+    rpcuser = pytestconfig.getoption("--btcuser")
+    rpcpassword = pytestconfig.getoption("--btcpwd")
+    bitcoin_path = pytestconfig.getoption("--btcroot")
+    bitcoind_path = os.path.join(bitcoin_path, "bitcoind")
+    bitcoincli_path = os.path.join(bitcoin_path, "bitcoin-cli")
+    start_cmd = f'{bitcoind_path} -regtest -daemon -conf={conf}'
+    stop_cmd = f'{bitcoincli_path} -regtest -rpcuser={rpcuser} -rpcpassword={rpcpassword} stop'
+    local_command(start_cmd, bg=True)
+    # determine bitcoind version
+    try:
+        bitcoind_version = get_bitcoind_version(bitcoind_path)
+    except RuntimeError as exc:
+        pytest.exit(f"Cannot setup tests, bitcoind failing.\n{exc}")
+    root_cmd = f'{bitcoincli_path} -regtest -rpcuser={rpcuser} -rpcpassword={rpcpassword}'
+    wallet_name = 'jm-test-wallet'
     # Bitcoin Core v0.21+ does not create default wallet
     # From Bitcoin Core 0.21.0 there is support for descriptor wallets, which
     # are default from 23.x+ (including 22.99.0 development versions).
     # We don't support descriptor wallets yet.
     if bitcoind_version[0] >= 22:
-        local_command(root_cmd + ["-rpcwait"] + ["-named"] +
-            ["createwallet",
-                "wallet_name=jm-test-wallet", "descriptors=false"])
+        create_wallet = f'{root_cmd} -rpcwait -named createwallet wallet_name={wallet_name} descriptors=false'
     else:
-        local_command(root_cmd + ["-rpcwait"] +
-            ["createwallet", "jm-test-wallet"])
-    local_command(root_cmd + ["loadwallet", "jm-test-wallet"])
+        create_wallet = f'{root_cmd} -rpcwait createwallet {wallet_name}'
+    local_command(create_wallet)
+    local_command(f'{root_cmd} loadwallet {wallet_name}')
     for i in range(2):
-        cpe = local_command(root_cmd + ["-rpcwallet=jm-test-wallet"] +
-            ["getnewaddress"])
-        if cpe.returncode == 0:
-            destn_addr = cpe.stdout[:-1].decode('utf-8')
-            local_command(root_cmd + ["-rpcwallet=jm-test-wallet"] +
-                ["generatetoaddress", "301", destn_addr])
-        else:
-            pytest.exit("Cannot setup tests, bitcoin-cli failing.\n" +
-                str(cpe.stdout))
-        time.sleep(1)
-
+        cpe = local_command(f'{root_cmd} -rpcwallet={wallet_name} getnewaddress')
+        if cpe.returncode != 0:
+            pytest.exit(f"Cannot setup tests, bitcoin-cli failing.\n{cpe.stdout.decode('utf-8')}")
+        destn_addr = cpe.stdout[:-1].decode('utf-8')
+        local_command(f'{root_cmd} -rpcwallet={wallet_name} generatetoaddress 301 {destn_addr}')
+        sleep(1)
+    yield
+    # shut down bitcoind
+    local_command(stop_cmd)
+    # note, it is better to clean out ~/.bitcoin/regtest but too
+    # dangerous to automate it here perhaps