Merge bitcoin/bitcoin#32697: test: Turn util/test_runner into functional test

fa21631595 test: Use self.log (MarcoFalke)
fa346f7797 test: Move error string into exception (MarcoFalke)
fa1986181f test: Remove useless catch-throw (MarcoFalke)
fa2f1c55b7 move-only util data to test/functional/data/util (MarcoFalke)
faa18bf287 test: Turn util/test_runner into functional test (MarcoFalke)
fa955154c7 test: Add missing skip_if_no_bitcoin_tx (MarcoFalke)
fac9db6eb0 test: Add missing tx util to Binaries (MarcoFalke)
fa91835ec6 test: Use lowercase env var as attribute name (MarcoFalke)
fac49094cd test: Remove duplicate ConfigParser (MarcoFalke)

Pull request description:

  The `test/util/test_runner.py` has many issues:

  * The boilerplate for the test runner is duplicate or inconsistent with the other (functional) tests. For example, logging options, `ConfigParser` handling, `Binaries` handling ...
  * The cmake/ci behavior is brittle and can silently fail, as explained in https://github.com/bitcoin/bitcoin/issues/31476
  * corecheck (and likely other places that manually run the tests) completely forget to run it
  * If the test is manually called, it runs single threaded, when it could just run in parallel with the other functional tests

  Fix all issues by removing the util test_runner and moving the test logic into a new functional test file.

ACKs for top commit:
  janb84:
    re ACK fa21631595
  brunoerg:
    re-ACK fa21631595
  hebasto:
    re-ACK fa21631595, additional feedback has been addressed since my previous [review](https://github.com/bitcoin/bitcoin/pull/32697#pullrequestreview-2940350432).

Tree-SHA512: 694e647887801f002843a74011035d5ed3dfed091d3f0ae18e812a16a4680e04e60e50de0a92af7e047e8ddd6ff5a7834c690f16fd42b74ebc1674bf9989406f

.github/workflows/ci.yml

@@ -260,6 +260,7 @@ jobs:
         env:
           BITCOIND: '${{ github.workspace }}\build\bin\Release\bitcoind.exe'
           BITCOINCLI: '${{ github.workspace }}\build\bin\Release\bitcoin-cli.exe'
+          BITCOINTX: '${{ github.workspace }}\build\bin\Release\bitcoin-tx.exe'
           BITCOINUTIL: '${{ github.workspace }}\build\bin\Release\bitcoin-util.exe'
           BITCOINWALLET: '${{ github.workspace }}\build\bin\Release\bitcoin-wallet.exe'
           TEST_RUNNER_EXTRA: ${{ github.event_name != 'pull_request' && '--extended' || '' }}
@@ -389,9 +390,6 @@ jobs:
           (Get-Content "test/config.ini") -replace '(?<=^SRCDIR=).*', '${{ github.workspace }}' -replace '(?<=^BUILDDIR=).*', '${{ github.workspace }}' -replace '(?<=^RPCAUTH=).*', '${{ github.workspace }}/share/rpcauth/rpcauth.py' | Set-Content "test/config.ini"
           Get-Content "test/config.ini"
 
-      - name: Run util tests
-        run: py -3 test/util/test_runner.py
-
       - name: Run rpcauth test
         run: py -3 test/util/rpcauth-test.py
 

CMakeLists.txt

@@ -594,7 +594,7 @@ if(Python3_EXECUTABLE)
   set(PYTHON_COMMAND ${Python3_EXECUTABLE})
 else()
   list(APPEND configure_warnings
-    "Minimum required Python not found. Utils and rpcauth tests are disabled."
+    "Minimum required Python not found. Rpcauth tests are disabled."
   )
 endif()
 

cmake/tests.cmake

@@ -2,12 +2,6 @@
 # Distributed under the MIT software license, see the accompanying
 # file COPYING or https://opensource.org/license/mit/.
 
-if(TARGET bitcoin-util AND TARGET bitcoin-tx AND PYTHON_COMMAND)
-  add_test(NAME util_test_runner
-    COMMAND ${CMAKE_COMMAND} -E env BITCOINUTIL=$<TARGET_FILE:bitcoin-util> BITCOINTX=$<TARGET_FILE:bitcoin-tx> ${PYTHON_COMMAND} ${PROJECT_BINARY_DIR}/test/util/test_runner.py
-  )
-endif()
-
 if(PYTHON_COMMAND)
   add_test(NAME util_rpcauth_test
     COMMAND ${PYTHON_COMMAND} ${PROJECT_BINARY_DIR}/test/util/rpcauth-test.py

test/CMakeLists.txt

@@ -17,6 +17,7 @@ function(create_test_config)
 
   set_configure_variable(ENABLE_WALLET ENABLE_WALLET)
   set_configure_variable(BUILD_CLI BUILD_BITCOIN_CLI)
+  set_configure_variable(BUILD_TX BUILD_BITCOIN_TX)
   set_configure_variable(BUILD_UTIL BUILD_BITCOIN_UTIL)
   set_configure_variable(BUILD_UTIL_CHAINSTATE BUILD_BITCOIN_CHAINSTATE)
   set_configure_variable(BUILD_WALLET_TOOL BUILD_BITCOIN_WALLET)
@@ -36,7 +37,7 @@ file(MAKE_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/fuzz)
 file(MAKE_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/util)
 
 file(GLOB_RECURSE functional_tests RELATIVE ${CMAKE_CURRENT_SOURCE_DIR} functional/*)
-foreach(script ${functional_tests} fuzz/test_runner.py util/rpcauth-test.py util/test_runner.py)
+foreach(script ${functional_tests} fuzz/test_runner.py util/rpcauth-test.py)
   if(CMAKE_HOST_WIN32)
     set(symlink)
   else()

test/README.md

@@ -10,10 +10,9 @@ This directory contains the following sets of tests:
 - [functional](/test/functional) which test the functionality of
 bitcoind and bitcoin-qt by interacting with them through the RPC and P2P
 interfaces.
-- [util](/test/util) which tests the utilities (bitcoin-util, bitcoin-tx, ...).
 - [lint](/test/lint/) which perform various static analysis checks.
 
-The util tests are run as part of `ctest` invocation. The fuzz tests, functional
+The fuzz tests, functional
 tests and lint scripts can be run as explained in the sections below.
 
 # Running tests locally
@@ -321,11 +320,6 @@ perf report -i /path/to/datadir/send-big-msgs.perf.data.xxxx --stdio | c++filt |
 For ways to generate more granular profiles, see the README in
 [test/functional](/test/functional).
 
-### Util tests
-
-Util tests can be run locally by running `build/test/util/test_runner.py`.
-Use the `-v` option for verbose output.
-
 ### Lint tests
 
 See the README in [test/lint](/test/lint).

test/config.ini.in

@@ -17,6 +17,7 @@ RPCAUTH=@abs_top_srcdir@/share/rpcauth/rpcauth.py
 # Which components are enabled. These are commented out by cmake if they were disabled during configuration.
 @ENABLE_WALLET_TRUE@ENABLE_WALLET=true
 @BUILD_BITCOIN_CLI_TRUE@ENABLE_CLI=true
+@BUILD_BITCOIN_TX_TRUE@BUILD_BITCOIN_TX=true
 @BUILD_BITCOIN_UTIL_TRUE@ENABLE_BITCOIN_UTIL=true
 @BUILD_BITCOIN_CHAINSTATE_TRUE@ENABLE_BITCOIN_CHAINSTATE=true
 @BUILD_BITCOIN_WALLET_TRUE@ENABLE_WALLET_TOOL=true

test/functional/rpc_users.py

@@ -17,7 +17,6 @@ import urllib.parse
 import subprocess
 from random import SystemRandom
 import string
-import configparser
 import sys
 from typing import Optional
 
@@ -47,9 +46,7 @@ class HTTPBasicsTest(BitcoinTestFramework):
         self.rpcuser = "rpcuser💻"
         self.rpcpassword = "rpcpassword🔑"
 
-        config = configparser.ConfigParser()
-        config.read_file(open(self.options.configfile))
-        gen_rpcauth = config['environment']['RPCAUTH']
+        gen_rpcauth = self.config["environment"]["RPCAUTH"]
 
         # Generate RPCAUTH with specified password
         self.rt2password = "8/F3uMDw4KSEbw96U3CA1C4X05dkHDN2BPFjTgZW4KI="

test/functional/test_framework/test_framework.py

@@ -84,6 +84,10 @@ class Binaries:
         # Add -nonamed because "bitcoin rpc" enables -named by default, but bitcoin-cli doesn't
         return self._argv("rpc", self.paths.bitcoincli) + ["-nonamed"]
 
+    def tx_argv(self):
+        "Return argv array that should be used to invoke bitcoin-tx"
+        return self._argv("tx", self.paths.bitcointx)
+
     def util_argv(self):
         "Return argv array that should be used to invoke bitcoin-util"
         return self._argv("util", self.paths.bitcoinutil)
@@ -272,9 +276,8 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
         self.options.timeout_factor = self.options.timeout_factor or (4 if self.options.valgrind else 1)
         self.options.previous_releases_path = previous_releases_path
 
-        config = configparser.ConfigParser()
-        config.read_file(open(self.options.configfile))
-        self.config = config
+        self.config = configparser.ConfigParser()
+        self.config.read_file(open(self.options.configfile))
         self.binary_paths = self.get_binary_paths()
         if self.options.v1transport:
             self.options.v2transport=False
@@ -286,19 +289,20 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
 
         paths = types.SimpleNamespace()
         binaries = {
-            "bitcoind": ("bitcoind", "BITCOIND"),
-            "bitcoin-cli": ("bitcoincli", "BITCOINCLI"),
-            "bitcoin-util": ("bitcoinutil", "BITCOINUTIL"),
-            "bitcoin-chainstate": ("bitcoinchainstate", "BITCOINCHAINSTATE"),
-            "bitcoin-wallet": ("bitcoinwallet", "BITCOINWALLET"),
+            "bitcoind": "BITCOIND",
+            "bitcoin-cli": "BITCOINCLI",
+            "bitcoin-util": "BITCOINUTIL",
+            "bitcoin-tx": "BITCOINTX",
+            "bitcoin-chainstate": "BITCOINCHAINSTATE",
+            "bitcoin-wallet": "BITCOINWALLET",
         }
-        for binary, [attribute_name, env_variable_name] in binaries.items():
+        for binary, env_variable_name in binaries.items():
             default_filename = os.path.join(
                 self.config["environment"]["BUILDDIR"],
                 "bin",
                 binary + self.config["environment"]["EXEEXT"],
             )
-            setattr(paths, attribute_name, os.getenv(env_variable_name, default=default_filename))
+            setattr(paths, env_variable_name.lower(), os.getenv(env_variable_name, default=default_filename))
         # BITCOIN_CMD environment variable can be specified to invoke bitcoin
         # wrapper binary instead of other executables.
         paths.bitcoin_cmd = shlex.split(os.getenv("BITCOIN_CMD", "")) or None
@@ -314,10 +318,8 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
 
         self.options.cachedir = os.path.abspath(self.options.cachedir)
 
-        config = self.config
-
         os.environ['PATH'] = os.pathsep.join([
-            os.path.join(config['environment']['BUILDDIR'], 'bin'),
+            os.path.join(self.config["environment"]["BUILDDIR"], "bin"),
             os.environ['PATH']
         ])
 
@@ -1000,6 +1002,11 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
         if not self.is_wallet_tool_compiled():
             raise SkipTest("bitcoin-wallet has not been compiled")
 
+    def skip_if_no_bitcoin_tx(self):
+        """Skip the running test if bitcoin-tx has not been compiled."""
+        if not self.is_bitcoin_tx_compiled():
+            raise SkipTest("bitcoin-tx has not been compiled")
+
     def skip_if_no_bitcoin_util(self):
         """Skip the running test if bitcoin-util has not been compiled."""
         if not self.is_bitcoin_util_compiled():
@@ -1054,6 +1061,10 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
         """Checks whether bitcoin-wallet was compiled."""
         return self.config["components"].getboolean("ENABLE_WALLET_TOOL")
 
+    def is_bitcoin_tx_compiled(self):
+        """Checks whether bitcoin-tx was compiled."""
+        return self.config["components"].getboolean("BUILD_BITCOIN_TX")
+
     def is_bitcoin_util_compiled(self):
         """Checks whether bitcoin-util was compiled."""
         return self.config["components"].getboolean("ENABLE_BITCOIN_UTIL")

test/functional/test_runner.py

@@ -170,6 +170,7 @@ BASE_SCRIPTS = [
     'wallet_txn_doublespend.py --mineblock',
     'tool_bitcoin_chainstate.py',
     'tool_wallet.py',
+    'tool_utils.py',
     'tool_signet_miner.py',
     'wallet_txn_clone.py',
     'wallet_txn_clone.py --segwit',

test/functional/tool_utils.py

@@ -0,0 +1,133 @@
+#!/usr/bin/env python3
+# Copyright 2014 BitPay Inc.
+# Copyright 2016-present The Bitcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or https://opensource.org/license/mit.
+"""Exercise the utils via json-defined tests."""
+
+from test_framework.test_framework import BitcoinTestFramework
+
+import difflib
+import json
+import os
+import subprocess
+from pathlib import Path
+
+
+class ToolUtils(BitcoinTestFramework):
+    def set_test_params(self):
+        self.num_nodes = 0  # No node/datadir needed
+
+    def setup_network(self):
+        pass
+
+    def skip_test_if_missing_module(self):
+        self.skip_if_no_bitcoin_tx()
+        self.skip_if_no_bitcoin_util()
+
+    def run_test(self):
+        self.testcase_dir = Path(self.config["environment"]["SRCDIR"]) / "test" / "functional" / "data" / "util"
+        self.bins = self.get_binaries()
+        with open(self.testcase_dir / "bitcoin-util-test.json", encoding="utf8") as f:
+            input_data = json.loads(f.read())
+
+        for i, test_obj in enumerate(input_data):
+            self.log.debug(f"Running [{i}]: " + test_obj["description"])
+            self.test_one(test_obj)
+
+    def test_one(self, testObj):
+        """Runs a single test, comparing output and RC to expected output and RC.
+
+        Raises an error if input can't be read, executable fails, or output/RC
+        are not as expected. Error is caught by bctester() and reported.
+        """
+        # Get the exec names and arguments
+        if testObj["exec"] == "./bitcoin-util":
+            execrun = self.bins.util_argv() + testObj["args"]
+        elif testObj["exec"] == "./bitcoin-tx":
+            execrun = self.bins.tx_argv() + testObj["args"]
+
+        # Read the input data (if there is any)
+        inputData = None
+        if "input" in testObj:
+            with open(self.testcase_dir / testObj["input"], encoding="utf8") as f:
+                inputData = f.read()
+
+        # Read the expected output data (if there is any)
+        outputFn = None
+        outputData = None
+        outputType = None
+        if "output_cmp" in testObj:
+            outputFn = testObj['output_cmp']
+            outputType = os.path.splitext(outputFn)[1][1:]  # output type from file extension (determines how to compare)
+            with open(self.testcase_dir / outputFn, encoding="utf8") as f:
+                outputData = f.read()
+            if not outputData:
+                raise Exception(f"Output data missing for {outputFn}")
+            if not outputType:
+                raise Exception(f"Output file {outputFn} does not have a file extension")
+
+        # Run the test
+        res = subprocess.run(execrun, capture_output=True, text=True, input=inputData)
+
+        if outputData:
+            data_mismatch, formatting_mismatch = False, False
+            # Parse command output and expected output
+            try:
+                a_parsed = parse_output(res.stdout, outputType)
+            except Exception as e:
+                self.log.error(f"Error parsing command output as {outputType}: '{str(e)}'; res: {str(res)}")
+                raise
+            try:
+                b_parsed = parse_output(outputData, outputType)
+            except Exception as e:
+                self.log.error('Error parsing expected output %s as %s: %s' % (outputFn, outputType, e))
+                raise
+            # Compare data
+            if a_parsed != b_parsed:
+                self.log.error(f"Output data mismatch for {outputFn} (format {outputType}); res: {str(res)}")
+                data_mismatch = True
+            # Compare formatting
+            if res.stdout != outputData:
+                error_message = f"Output formatting mismatch for {outputFn}:\nres: {str(res)}\n"
+                error_message += "".join(difflib.context_diff(outputData.splitlines(True),
+                                                              res.stdout.splitlines(True),
+                                                              fromfile=outputFn,
+                                                              tofile="returned"))
+                self.log.error(error_message)
+                formatting_mismatch = True
+
+            assert not data_mismatch and not formatting_mismatch
+
+        # Compare the return code to the expected return code
+        wantRC = 0
+        if "return_code" in testObj:
+            wantRC = testObj['return_code']
+        if res.returncode != wantRC:
+            raise Exception(f"Return code mismatch for {outputFn}; res: {str(res)}")
+
+        if "error_txt" in testObj:
+            want_error = testObj["error_txt"]
+            # A partial match instead of an exact match makes writing tests easier
+            # and should be sufficient.
+            if want_error not in res.stderr:
+                raise Exception(f"Error mismatch:\nExpected: {want_error}\nReceived: {res.stderr.rstrip()}\nres: {str(res)}")
+        else:
+            if res.stderr:
+                raise Exception(f"Unexpected error received: {res.stderr.rstrip()}\nres: {str(res)}")
+
+
+def parse_output(a, fmt):
+    """Parse the output according to specified format.
+
+    Raise an error if the output can't be parsed."""
+    if fmt == 'json':  # json: compare parsed data
+        return json.loads(a)
+    elif fmt == 'hex':  # hex: parse and compare binary data
+        return bytes.fromhex(a.strip())
+    else:
+        raise NotImplementedError("Don't know how to compare %s" % fmt)
+
+
+if __name__ == "__main__":
+    ToolUtils(__file__).main()

test/util/test_runner.py

@@ -1,181 +0,0 @@
-#!/usr/bin/env python3
-# Copyright 2014 BitPay Inc.
-# Copyright 2016-present The Bitcoin Core developers
-# Distributed under the MIT software license, see the accompanying
-# file COPYING or http://www.opensource.org/licenses/mit-license.php.
-"""Test framework for bitcoin utils.
-
-Runs automatically during `ctest --test-dir build/`.
-
-Can also be run manually."""
-
-import argparse
-import configparser
-import difflib
-import json
-import logging
-import os
-import pprint
-import subprocess
-import sys
-
-def main():
-    config = configparser.ConfigParser()
-    config.optionxform = str
-    with open(os.path.join(os.path.dirname(__file__), "../config.ini"), encoding="utf8") as f:
-        config.read_file(f)
-    env_conf = dict(config.items('environment'))
-
-    parser = argparse.ArgumentParser(description=__doc__)
-    parser.add_argument('-v', '--verbose', action='store_true')
-    args = parser.parse_args()
-    verbose = args.verbose
-
-    if verbose:
-        level = logging.DEBUG
-    else:
-        level = logging.ERROR
-    formatter = '%(asctime)s - %(levelname)s - %(message)s'
-    # Add the format/level to the logger
-    logging.basicConfig(format=formatter, level=level)
-
-    bctester(os.path.join(env_conf["SRCDIR"], "test", "util", "data"), "bitcoin-util-test.json", env_conf)
-
-def bctester(testDir, input_basename, buildenv):
-    """ Loads and parses the input file, runs all tests and reports results"""
-    input_filename = os.path.join(testDir, input_basename)
-    with open(input_filename, encoding="utf8") as f:
-        raw_data = f.read()
-    input_data = json.loads(raw_data)
-
-    failed_testcases = []
-
-    for testObj in input_data:
-        try:
-            bctest(testDir, testObj, buildenv)
-            logging.info("PASSED: " + testObj["description"])
-        except Exception:
-            logging.info("FAILED: " + testObj["description"])
-            failed_testcases.append(testObj["description"])
-
-    if failed_testcases:
-        error_message = "FAILED_TESTCASES:\n"
-        error_message += pprint.pformat(failed_testcases, width=400)
-        logging.error(error_message)
-        sys.exit(1)
-    else:
-        sys.exit(0)
-
-def bctest(testDir, testObj, buildenv):
-    """Runs a single test, comparing output and RC to expected output and RC.
-
-    Raises an error if input can't be read, executable fails, or output/RC
-    are not as expected. Error is caught by bctester() and reported.
-    """
-    # Get the exec names and arguments
-    execprog = os.path.join(buildenv["BUILDDIR"], "bin", testObj["exec"] + buildenv["EXEEXT"])
-    if testObj["exec"] == "./bitcoin-util":
-        execprog = os.getenv("BITCOINUTIL", default=execprog)
-    elif testObj["exec"] == "./bitcoin-tx":
-        execprog = os.getenv("BITCOINTX", default=execprog)
-
-    execargs = testObj['args']
-    execrun = [execprog] + execargs
-
-    # Read the input data (if there is any)
-    inputData = None
-    if "input" in testObj:
-        filename = os.path.join(testDir, testObj["input"])
-        with open(filename, encoding="utf8") as f:
-            inputData = f.read()
-
-    # Read the expected output data (if there is any)
-    outputFn = None
-    outputData = None
-    outputType = None
-    if "output_cmp" in testObj:
-        outputFn = testObj['output_cmp']
-        outputType = os.path.splitext(outputFn)[1][1:]  # output type from file extension (determines how to compare)
-        try:
-            with open(os.path.join(testDir, outputFn), encoding="utf8") as f:
-                outputData = f.read()
-        except Exception:
-            logging.error("Output file " + outputFn + " cannot be opened")
-            raise
-        if not outputData:
-            logging.error("Output data missing for " + outputFn)
-            raise Exception
-        if not outputType:
-            logging.error("Output file %s does not have a file extension" % outputFn)
-            raise Exception
-
-    # Run the test
-    try:
-        res = subprocess.run(execrun, capture_output=True, text=True, input=inputData)
-    except OSError:
-        logging.error("OSError, Failed to execute " + execprog)
-        raise
-
-    if outputData:
-        data_mismatch, formatting_mismatch = False, False
-        # Parse command output and expected output
-        try:
-            a_parsed = parse_output(res.stdout, outputType)
-        except Exception as e:
-            logging.error(f"Error parsing command output as {outputType}: '{str(e)}'; res: {str(res)}")
-            raise
-        try:
-            b_parsed = parse_output(outputData, outputType)
-        except Exception as e:
-            logging.error('Error parsing expected output %s as %s: %s' % (outputFn, outputType, e))
-            raise
-        # Compare data
-        if a_parsed != b_parsed:
-            logging.error(f"Output data mismatch for {outputFn} (format {outputType}); res: {str(res)}")
-            data_mismatch = True
-        # Compare formatting
-        if res.stdout != outputData:
-            error_message = f"Output formatting mismatch for {outputFn}:\nres: {str(res)}\n"
-            error_message += "".join(difflib.context_diff(outputData.splitlines(True),
-                                                          res.stdout.splitlines(True),
-                                                          fromfile=outputFn,
-                                                          tofile="returned"))
-            logging.error(error_message)
-            formatting_mismatch = True
-
-        assert not data_mismatch and not formatting_mismatch
-
-    # Compare the return code to the expected return code
-    wantRC = 0
-    if "return_code" in testObj:
-        wantRC = testObj['return_code']
-    if res.returncode != wantRC:
-        logging.error(f"Return code mismatch for {outputFn}; res: {str(res)}")
-        raise Exception
-
-    if "error_txt" in testObj:
-        want_error = testObj["error_txt"]
-        # A partial match instead of an exact match makes writing tests easier
-        # and should be sufficient.
-        if want_error not in res.stderr:
-            logging.error(f"Error mismatch:\nExpected: {want_error}\nReceived: {res.stderr.rstrip()}\nres: {str(res)}")
-            raise Exception
-    else:
-        if res.stderr:
-            logging.error(f"Unexpected error received: {res.stderr.rstrip()}\nres: {str(res)}")
-            raise Exception
-
-
-def parse_output(a, fmt):
-    """Parse the output according to specified format.
-
-    Raise an error if the output can't be parsed."""
-    if fmt == 'json':  # json: compare parsed data
-        return json.loads(a)
-    elif fmt == 'hex':  # hex: parse and compare binary data
-        return bytes.fromhex(a.strip())
-    else:
-        raise NotImplementedError("Don't know how to compare %s" % fmt)
-
-if __name__ == '__main__':
-    main()