From fad441fba07877ea78ed6020fde11828307273a6 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Wed, 8 Jan 2025 11:00:58 +0100 Subject: [PATCH 1/4] test: Treat leftover process as error Printing to stderr instead of stdout makes the test_runner.py fail on leftover processes. This is desired and fine, because a leftover process should only happen on a test failure anyway. --- test/functional/test_framework/test_node.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py index d896a421d4e..da5660842c1 100755 --- a/test/functional/test_framework/test_node.py +++ b/test/functional/test_framework/test_node.py @@ -20,6 +20,7 @@ import time import urllib.parse import collections import shlex +import sys from pathlib import Path from .authproxy import ( @@ -204,7 +205,7 @@ class TestNode(): # Should only happen on test failure # Avoid using logger, as that may have already been shutdown when # this destructor is called. - print(self._node_msg("Cleaning up leftover process")) + print(self._node_msg("Cleaning up leftover process"), file=sys.stderr) self.process.kill() def __getattr__(self, name): From fa0dc09b9002f0bcae63af6af8d37fb3e0040ef4 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Wed, 8 Jan 2025 11:02:46 +0100 Subject: [PATCH 2/4] test: Remove --noshutdown flag --- test/functional/test_framework/test_framework.py | 14 +++----------- test/functional/test_framework/test_node.py | 3 +-- 2 files changed, 4 insertions(+), 13 deletions(-) diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py index 7e8c40cf162..2cc7e15e7e2 100755 --- a/test/functional/test_framework/test_framework.py +++ b/test/functional/test_framework/test_framework.py @@ -171,8 +171,6 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass): parser = argparse.ArgumentParser(usage="%(prog)s [options]") parser.add_argument("--nocleanup", dest="nocleanup", default=False, action="store_true", help="Leave bitcoinds and test.* datadir on exit or error") - parser.add_argument("--noshutdown", dest="noshutdown", default=False, action="store_true", - help="Don't stop bitcoinds after the test execution") parser.add_argument("--cachedir", dest="cachedir", default=os.path.abspath(os.path.dirname(test_file) + "/../cache"), help="Directory for caching pregenerated datadirs (default: %(default)s)") parser.add_argument("--tmpdir", dest="tmpdir", help="Root directory for datadirs (must not exist)") @@ -325,18 +323,12 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass): self.log.debug('Closing down network thread') self.network_thread.close() - if not self.options.noshutdown: - self.log.info("Stopping nodes") - if self.nodes: - self.stop_nodes() - else: - for node in self.nodes: - node.cleanup_on_exit = False - self.log.info("Note: bitcoinds were not stopped and may still be running") + self.log.info("Stopping nodes") + if self.nodes: + self.stop_nodes() should_clean_up = ( not self.options.nocleanup and - not self.options.noshutdown and self.success != TestStatus.FAILED and not self.options.perf ) diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py index da5660842c1..5c903a28862 100755 --- a/test/functional/test_framework/test_node.py +++ b/test/functional/test_framework/test_node.py @@ -159,7 +159,6 @@ class TestNode(): self.rpc = None self.url = None self.log = logging.getLogger('TestFramework.node%d' % i) - self.cleanup_on_exit = True # Whether to kill the node when this object goes away # Cache perf subprocesses here by their data output filename. self.perf_subprocesses = {} @@ -201,7 +200,7 @@ class TestNode(): def __del__(self): # Ensure that we don't leave any bitcoind processes lying around after # the test ends - if self.process and self.cleanup_on_exit: + if self.process: # Should only happen on test failure # Avoid using logger, as that may have already been shutdown when # this destructor is called. From fae3bf6b870eb0f9cddd1adac82ba72890806ae3 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Wed, 8 Jan 2025 11:06:56 +0100 Subject: [PATCH 3/4] test: Avoid redundant stop and error spam on startup failure Trying to immediately shut down a node after a startup failure without waiting for the RPC to be fully up will in most cases just fail and lead to an RPC error. Also, it is confusing to sidestep the existing fallback to kill any leftover nodes on a test failure. So just rely on the fallback. --- test/functional/test_framework/test_framework.py | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py index 2cc7e15e7e2..e437bde76bb 100755 --- a/test/functional/test_framework/test_framework.py +++ b/test/functional/test_framework/test_framework.py @@ -567,15 +567,10 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass): if extra_args is None: extra_args = [None] * self.num_nodes assert_equal(len(extra_args), self.num_nodes) - try: - for i, node in enumerate(self.nodes): - node.start(extra_args[i], *args, **kwargs) - for node in self.nodes: - node.wait_for_rpc_connection() - except Exception: - # If one node failed to start, stop the others - self.stop_nodes() - raise + for i, node in enumerate(self.nodes): + node.start(extra_args[i], *args, **kwargs) + for node in self.nodes: + node.wait_for_rpc_connection() if self.options.coveragedir is not None: for node in self.nodes: From faf2f2c654d9aa18b2f49a157956f9ab0fce302a Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Thu, 23 Jan 2025 14:39:59 +0100 Subject: [PATCH 4/4] test: Avoid redundant stop and error spam on shutdown Trying to shut down a node after a test failure may fail and lead to an RPC error. Also, it is confusing to sidestep the existing fallback to kill any leftover nodes on a test failure. So just rely on the fallback. Idea by Hodlinator. Co-Authored-By: Hodlinator <172445034+hodlinator@users.noreply.github.com> --- test/functional/test_framework/test_framework.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py index e437bde76bb..f77e03dd225 100755 --- a/test/functional/test_framework/test_framework.py +++ b/test/functional/test_framework/test_framework.py @@ -323,9 +323,12 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass): self.log.debug('Closing down network thread') self.network_thread.close() - self.log.info("Stopping nodes") - if self.nodes: - self.stop_nodes() + if self.success == TestStatus.FAILED: + self.log.info("Not stopping nodes as test failed. The dangling processes will be cleaned up later.") + else: + self.log.info("Stopping nodes") + if self.nodes: + self.stop_nodes() should_clean_up = ( not self.options.nocleanup and