From d8f05e7bf3ec838abb96b3ffcc456892ec9f52f2 Mon Sep 17 00:00:00 2001 From: Hodlinator <172445034+hodlinator@users.noreply.github.com> Date: Wed, 14 May 2025 14:33:29 +0200 Subject: [PATCH 1/4] qa: Fix dormant bug caused by multiple --tmpdir We would only modify the parent process' first --tmpdir arg. Now we tack on an additional --tmpdir after the parent's arguments. Also simplifies the code. Co-authored-by: Ryan Ofsky --- test/functional/feature_framework_startup_failures.py | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/test/functional/feature_framework_startup_failures.py b/test/functional/feature_framework_startup_failures.py index d6da7f713bd..f84fcdc38f0 100755 --- a/test/functional/feature_framework_startup_failures.py +++ b/test/functional/feature_framework_startup_failures.py @@ -36,15 +36,11 @@ class FeatureFrameworkStartupFailures(BitcoinTestFramework): # Launches a child test process that runs this same file, but instantiates # a child test. Verifies that it raises only the expected exception, once. def _verify_startup_failure(self, test, internal_args, expected_exception): - # Inherit args from parent, only modifying tmpdir so children don't fail - # as a cause of colliding with the parent dir. - parent_args = sys.argv.copy() + # Inherit sys.argv from parent, only overriding tmpdir to a subdirectory + # so children don't fail due to colliding with the parent dir. assert self.options.tmpdir, "Framework should always set tmpdir." - i, path = next(((i, m[1]) for i, arg in enumerate(parent_args) if (m := re.match(r'--tm(?:p(?:d(?:ir?)?)?)?=(.+)', arg))), - (len(parent_args), self.options.tmpdir)) subdir = md5(expected_exception.encode('utf-8')).hexdigest()[:8] - parent_args[i:i + 1] = [f"--tmpdir={path}/{subdir}"] - args = [sys.executable] + parent_args + [f"--internal_test={test.__name__}"] + internal_args + args = [sys.executable] + sys.argv + [f"--tmpdir={self.options.tmpdir}/{subdir}", f"--internal_test={test.__name__}"] + internal_args try: # The subprocess encoding argument gives different results for e.output From bd8ebbc4ab6a218ad783755524cd0d138e44a305 Mon Sep 17 00:00:00 2001 From: Hodlinator <172445034+hodlinator@users.noreply.github.com> Date: Fri, 16 May 2025 08:59:26 +0200 Subject: [PATCH 2/4] qa: Make --timeout-factor=0 result in a smaller factor Would otherwise cause an OverflowError in feature_framework_startup_failures.py when calling subprocess.run() with 60 * factor. Fixes #32506 Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> --- test/functional/test_framework/test_framework.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py index 75a0cb6f112..0f91a381542 100755 --- a/test/functional/test_framework/test_framework.py +++ b/test/functional/test_framework/test_framework.py @@ -261,7 +261,7 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass): parser.add_argument("-f", "--fff", help="a dummy argument to fool ipython", default="1") self.options = parser.parse_args() if self.options.timeout_factor == 0: - self.options.timeout_factor = 99999 + self.options.timeout_factor = 999 self.options.timeout_factor = self.options.timeout_factor or (4 if self.options.valgrind else 1) self.options.previous_releases_path = previous_releases_path From 075352ec8e573c0dd819202d223eed5209c9e4bf Mon Sep 17 00:00:00 2001 From: Hodlinator <172445034+hodlinator@users.noreply.github.com> Date: Wed, 14 May 2025 16:19:23 +0200 Subject: [PATCH 3/4] qa: assert_raises_message() - search in str(e) repr() is annoying because it requires escaping special characters, use str() instead. Original discussion: https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2080292165 --- test/functional/feature_framework_startup_failures.py | 8 ++++---- test/functional/test_framework/util.py | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/test/functional/feature_framework_startup_failures.py b/test/functional/feature_framework_startup_failures.py index f84fcdc38f0..f092f07e0ed 100755 --- a/test/functional/feature_framework_startup_failures.py +++ b/test/functional/feature_framework_startup_failures.py @@ -69,10 +69,10 @@ class FeatureFrameworkStartupFailures(BitcoinTestFramework): self.log.info("Verifying _verify_startup_failure() functionality (self-check).") assert_raises_message( AssertionError, - ("Child test didn't contain (only) expected errors:\n" + - linesep.join(["Found 0/1 tracebacks - expecting exactly one with no knock-on exceptions.", - "Found 0/1 occurrences of the specific exception: NonExistentError", - "Found 0/1 test failure output messages."])).encode("unicode_escape").decode("utf-8"), + ( "Child test didn't contain (only) expected errors:\n" + f"Found 0/1 tracebacks - expecting exactly one with no knock-on exceptions.{linesep}" + f"Found 0/1 occurrences of the specific exception: NonExistentError{linesep}" + "Found 0/1 test failure output messages."), self._verify_startup_failure, TestSuccess, [], "NonExistentError", diff --git a/test/functional/test_framework/util.py b/test/functional/test_framework/util.py index b2f765e4d12..70d68ea7303 100644 --- a/test/functional/test_framework/util.py +++ b/test/functional/test_framework/util.py @@ -101,9 +101,9 @@ def assert_raises_message(exc, message, fun, *args, **kwds): except JSONRPCException: raise AssertionError("Use assert_raises_rpc_error() to test RPC failures") except exc as e: - if message is not None and message not in repr(e): + if message is not None and message not in str(e): raise AssertionError("Expected substring not found in exception:\n" - f"substring: '{message}'\nexception: {repr(e)}.") + f"substring: '{message}'\nexception: {e!r}.") except Exception as e: raise AssertionError("Unexpected exception raised: " + type(e).__name__) else: From bf950c4544d3a8478b46faf0b93c0dc647274c9b Mon Sep 17 00:00:00 2001 From: Hodlinator <172445034+hodlinator@users.noreply.github.com> Date: Wed, 14 May 2025 14:40:15 +0200 Subject: [PATCH 4/4] qa: Improve suppressed errors output Original discussion: https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2080132673 --- .../feature_framework_startup_failures.py | 2 +- test/functional/test_framework/test_node.py | 18 ++++++++++-------- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/test/functional/feature_framework_startup_failures.py b/test/functional/feature_framework_startup_failures.py index f092f07e0ed..84a7d9d8374 100755 --- a/test/functional/feature_framework_startup_failures.py +++ b/test/functional/feature_framework_startup_failures.py @@ -89,7 +89,7 @@ class FeatureFrameworkStartupFailures(BitcoinTestFramework): self.log.info("Verifying inability to connect to bitcoind's RPC interface due to wrong port results in one exception containing at least one OSError.") self._verify_startup_failure( TestWrongRpcPortStartupFailure, [f"--internal_node_start_duration={node_start_duration}"], - r"AssertionError: \[node 0\] Unable to connect to bitcoind after \d+s \(ignored errors: {[^}]*'OSError \w+'?: \d+[^}]*}, latest error: \w+\([^)]+\)\)" + r"AssertionError: \[node 0\] Unable to connect to bitcoind after \d+s \(ignored errors: {[^}]*'OSError \w+'?: \d+[^}]*}, latest: '[\w ]+'/\w+\([^)]+\)\)" ) self.log.info("Verifying startup failure due to invalid arg results in only one exception.") diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py index 449c0628753..e753d260db3 100755 --- a/test/functional/test_framework/test_node.py +++ b/test/functional/test_framework/test_node.py @@ -263,8 +263,13 @@ class TestNode(): """Sets up an RPC connection to the bitcoind process. Returns False if unable to connect.""" # Poll at a rate of four times per second poll_per_s = 4 + suppressed_errors = collections.defaultdict(int) - latest_error = "" + latest_error = None + def suppress_error(category: str, e: Exception): + suppressed_errors[category] += 1 + return (category, repr(e)) + for _ in range(poll_per_s * self.rpc_timeout): if self.process.poll() is not None: # Attach abrupt shutdown error/s to the exception message @@ -318,8 +323,7 @@ class TestNode(): # -342 Service unavailable, could be starting up or shutting down if e.error['code'] not in [-28, -342]: raise # unknown JSON RPC exception - suppressed_errors[f"JSONRPCException {e.error['code']}"] += 1 - latest_error = repr(e) + latest_error = suppress_error(f"JSONRPCException {e.error['code']}", e) except OSError as e: error_num = e.errno # Work around issue where socket timeouts don't have errno set. @@ -335,16 +339,14 @@ class TestNode(): errno.ECONNREFUSED # Port not yet open? ]: raise # unknown OS error - suppressed_errors[f"OSError {errno.errorcode[error_num]}"] += 1 - latest_error = repr(e) + latest_error = suppress_error(f"OSError {errno.errorcode[error_num]}", e) except ValueError as e: # Suppress if cookie file isn't generated yet and no rpcuser or rpcpassword; bitcoind may be starting. if "No RPC credentials" not in str(e): raise - suppressed_errors["missing_credentials"] += 1 - latest_error = repr(e) + latest_error = suppress_error("missing_credentials", e) time.sleep(1.0 / poll_per_s) - self._raise_assertion_error(f"Unable to connect to bitcoind after {self.rpc_timeout}s (ignored errors: {str(dict(suppressed_errors))}, latest error: {latest_error})") + self._raise_assertion_error(f"Unable to connect to bitcoind after {self.rpc_timeout}s (ignored errors: {dict(suppressed_errors)!s}{'' if latest_error is None else f', latest: {latest_error[0]!r}/{latest_error[1]}'})") def wait_for_cookie_credentials(self): """Ensures auth cookie credentials can be read, e.g. for testing CLI with -rpcwait before RPC connection is up."""