Merge #18828: test: Strip down previous releases boilerplate

fa359d14c09c6b139dead5da17c5a1c02f68393c test: Strip down previous releases boilerplate (MarcoFalke)

Pull request description:

  Reduces code bloat and mental load to write compatibility tests

ACKs for top commit:
  Sjors:
    tACK fa359d14c09c6b139dead5da17c5a1c02f68393c on macOS

Tree-SHA512: dc66286b24b2f137e5bca99412850ec7eee8cc61cf9cdc7ab532d529220808189baea8d1b077f8b7f40d3e8881d981e1ffc5a877adb394816f1225b1186253e4

3 files changed, 56 insertions, 50 deletions

diff --git a/test/functional/feature_backwards_compatibility.py b/test/functional/feature_backwards_compatibility.py
index 166c28d376..e69a217fad 100755
--- a/test/functional/feature_backwards_compatibility.py
+++ b/test/functional/feature_backwards_compatibility.py
@@ -18,15 +18,16 @@ needs an older patch version.
 import os
 import shutil
 
-from test_framework.test_framework import BitcoinTestFramework, SkipTest
+from test_framework.test_framework import BitcoinTestFramework
 from test_framework.descriptors import descsum_create
 
 from test_framework.util import (
     assert_equal,
     sync_blocks,
-    sync_mempools
+    sync_mempools,
 )
 
+
 class BackwardsCompatibilityTest(BitcoinTestFramework):
     def set_test_params(self):
         self.setup_clean_chain = True
@@ -42,35 +43,15 @@ class BackwardsCompatibilityTest(BitcoinTestFramework):
 
     def skip_test_if_missing_module(self):
         self.skip_if_no_wallet()
+        self.skip_if_no_previous_releases()
 
     def setup_nodes(self):
-        if os.getenv("TEST_PREVIOUS_RELEASES") == "false":
-            raise SkipTest("backwards compatibility tests")
-
-        releases_path = os.getenv("PREVIOUS_RELEASES_DIR") or os.getcwd() + "/releases"
-        if not os.path.isdir(releases_path):
-            if os.getenv("TEST_PREVIOUS_RELEASES") == "true":
-                raise AssertionError("TEST_PREVIOUS_RELEASES=1 but releases missing: " + releases_path)
-            raise SkipTest("This test requires binaries for previous releases")
-
         self.add_nodes(self.num_nodes, extra_args=self.extra_args, versions=[
             None,
             None,
-            190000,
+            190001,
             180100,
-            170100
-        ], binary=[
-            self.options.bitcoind,
-            self.options.bitcoind,
-            releases_path + "/v0.19.0.1/bin/bitcoind",
-            releases_path + "/v0.18.1/bin/bitcoind",
-            releases_path + "/v0.17.1/bin/bitcoind"
-        ], binary_cli=[
-            self.options.bitcoincli,
-            self.options.bitcoincli,
-            releases_path + "/v0.19.0.1/bin/bitcoin-cli",
-            releases_path + "/v0.18.1/bin/bitcoin-cli",
-            releases_path + "/v0.17.1/bin/bitcoin-cli"
+            170100,
         ])
 
         self.start_nodes()
diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py
index 8719bd0d39..81d0d66d66 100755
--- a/test/functional/test_framework/test_framework.py
+++ b/test/functional/test_framework/test_framework.py
@@ -6,11 +6,12 @@
 
 import configparser
 from enum import Enum
-import logging
 import argparse
+import logging
 import os
 import pdb
 import random
+import re
 import shutil
 import subprocess
 import sys
@@ -185,10 +186,11 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
         self.options.bitcoind = os.getenv("BITCOIND", default=config["environment"]["BUILDDIR"] + '/src/bitcoind' + config["environment"]["EXEEXT"])
         self.options.bitcoincli = os.getenv("BITCOINCLI", default=config["environment"]["BUILDDIR"] + '/src/bitcoin-cli' + config["environment"]["EXEEXT"])
 
+        self.options.previous_releases_path = os.getenv("PREVIOUS_RELEASES_DIR") or os.getcwd() + "/releases"
+
         os.environ['PATH'] = os.pathsep.join([
             os.path.join(config['environment']['BUILDDIR'], 'src'),
-            os.path.join(config['environment']['BUILDDIR'], 'src', 'qt'),
-            os.environ['PATH']
+            os.path.join(config['environment']['BUILDDIR'], 'src', 'qt'), os.environ['PATH']
         ])
 
         # Set up temp directory and start logging
@@ -388,6 +390,25 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
 
         Should only be called once after the nodes have been specified in
         set_test_params()."""
+        def get_bin_from_version(version, bin_name, bin_default):
+            if not version:
+                return bin_default
+            return os.path.join(
+                self.options.previous_releases_path,
+                re.sub(
+                    r'\.0$',
+                    '',  # remove trailing .0 for point releases
+                    'v{}.{}.{}.{}'.format(
+                        (version % 100000000) // 1000000,
+                        (version % 1000000) // 10000,
+                        (version % 10000) // 100,
+                        (version % 100) // 1,
+                    ),
+                ),
+                'bin',
+                bin_name,
+            )
+
         if self.bind_to_localhost_only:
             extra_confs = [["bind=127.0.0.1"]] * num_nodes
         else:
@@ -397,9 +418,9 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
         if versions is None:
             versions = [None] * num_nodes
         if binary is None:
-            binary = [self.options.bitcoind] * num_nodes
+            binary = [get_bin_from_version(v, 'bitcoind', self.options.bitcoind) for v in versions]
         if binary_cli is None:
-            binary_cli = [self.options.bitcoincli] * num_nodes
+            binary_cli = [get_bin_from_version(v, 'bitcoin-cli', self.options.bitcoincli) for v in versions]
         assert_equal(len(extra_confs), num_nodes)
         assert_equal(len(extra_args), num_nodes)
         assert_equal(len(versions), num_nodes)
@@ -640,6 +661,25 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
         if not self.is_cli_compiled():
             raise SkipTest("bitcoin-cli has not been compiled.")
 
+    def skip_if_no_previous_releases(self):
+        """Skip the running test if previous releases are not available."""
+        if not self.has_previous_releases():
+            raise SkipTest("previous releases not available or disabled")
+
+    def has_previous_releases(self):
+        """Checks whether previous releases are present and enabled."""
+        if os.getenv("TEST_PREVIOUS_RELEASES") == "false":
+            # disabled
+            return False
+
+        if not os.path.isdir(self.options.previous_releases_path):
+            if os.getenv("TEST_PREVIOUS_RELEASES") == "true":
+                raise AssertionError("TEST_PREVIOUS_RELEASES=true but releases missing: {}".format(
+                    self.options.previous_releases_path))
+            # missing
+            return False
+        return True
+
     def is_cli_compiled(self):
         """Checks whether bitcoin-cli was compiled."""
         return self.config["components"].getboolean("ENABLE_CLI")
diff --git a/test/functional/wallet_upgradewallet.py b/test/functional/wallet_upgradewallet.py
index d04bc4ce44..e7e71bf3f6 100755
--- a/test/functional/wallet_upgradewallet.py
+++ b/test/functional/wallet_upgradewallet.py
@@ -12,14 +12,15 @@ contrib/devtools/previous_release.sh -b v0.15.2 v0.16.3
 import os
 import shutil
 
-from test_framework.test_framework import BitcoinTestFramework, SkipTest
+from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
     adjust_bitcoin_conf_for_pre_17,
     assert_equal,
     assert_greater_than,
-    assert_is_hex_string
+    assert_is_hex_string,
 )
 
+
 class UpgradeWalletTest(BitcoinTestFramework):
     def set_test_params(self):
         self.setup_clean_chain = True
@@ -32,32 +33,16 @@ class UpgradeWalletTest(BitcoinTestFramework):
 
     def skip_test_if_missing_module(self):
         self.skip_if_no_wallet()
+        self.skip_if_no_previous_releases()
 
     def setup_network(self):
         self.setup_nodes()
 
     def setup_nodes(self):
-        if os.getenv("TEST_PREVIOUS_RELEASES") == "false":
-            raise SkipTest("upgradewallet RPC tests")
-
-        releases_path = os.getenv("PREVIOUS_RELEASES_DIR") or os.getcwd() + "/releases"
-        if not os.path.isdir(releases_path):
-            if os.getenv("TEST_PREVIOUS_RELEASES") == "true":
-                raise AssertionError("TEST_PREVIOUS_RELEASES=1 but releases missing: " + releases_path)
-            raise SkipTest("This test requires binaries for previous releases")
-
         self.add_nodes(self.num_nodes, extra_args=self.extra_args, versions=[
             None,
             160300,
-            150200
-        ], binary=[
-            self.options.bitcoind,
-            releases_path + "/v0.16.3/bin/bitcoind",
-            releases_path + "/v0.15.2/bin/bitcoind",
-        ], binary_cli=[
-            self.options.bitcoincli,
-            releases_path + "/v0.16.3/bin/bitcoin-cli",
-            releases_path + "/v0.15.2/bin/bitcoin-cli",
+            150200,
         ])
         # adapt bitcoin.conf, because older bitcoind's don't recognize config sections
         adjust_bitcoin_conf_for_pre_17(self.nodes[1].bitcoinconf)