test: Do not pass tests on unhandled exceptions #33001

All these exception handlers (except SkipTest) have the same behaviour, except for making the logs less useful by hiding the actual error. Can't we just remove all (except SkipTest and BaseException) and let the exception speak for itself? + updates log to exception instead of warning.

<details> <summary>git diff on fad0cd510c</summary>

diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py
index 21007d9156..17e5a6b397 100755
--- a/test/functional/test_framework/test_framework.py
+++ b/test/functional/test_framework/test_framework.py
@@ -194,29 +194,11 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
             else:
                 self.run_test()
 
-        except JSONRPCException:
-            self.log.exception("JSONRPC error")
-            self.success = TestStatus.FAILED
         except SkipTest as e:
             self.log.warning("Test Skipped: %s" % e.message)
             self.success = TestStatus.SKIPPED
-        except AssertionError:
-            self.log.exception("Assertion failed")
-            self.success = TestStatus.FAILED
-        except KeyError:
-            self.log.exception("Key error")
-            self.success = TestStatus.FAILED
-        except subprocess.CalledProcessError as e:
-            self.log.exception("Called Process failed with '{}'".format(e.output))
-            self.success = TestStatus.FAILED
-        except Exception:
-            self.log.exception("Unexpected exception caught during testing")
-            self.success = TestStatus.FAILED
-        except KeyboardInterrupt:
-            self.log.warning("Exiting after keyboard interrupt")
-            self.success = TestStatus.FAILED
         except BaseException as e:
-            self.log.warning(f"Unhandled base exception {repr(e)}")
+            self.log.exception(repr(e))
             self.success = TestStatus.FAILED
         finally:
             exit_code = self.shutdown()

</details>

Agree; thx, done

Logging the exception e seems more useful than "Unexpected exception", and imo completely obviates the need for having the CalledProcessError and KeyboardInterrupt special case handling?

Logs with special case handling:

2025-07-17T15:07:39.774663Z TestFramework (ERROR): Called Process failed with 'None'
2025-07-17T15:18:22.039274Z TestFramework (WARNING): Exiting after keyboard interrupt

Logs without special case handling:

2025-07-17T15:19:09.169925Z TestFramework (ERROR): CalledProcessError(1, ['ls', '--invalid-flag'])
2025-07-17T15:19:26.639162Z TestFramework (ERROR): KeyboardInterrupt()

Imo, they're equally informative for KeyboardInterrupt, and more informative without special case handling for CalledProcessError, with less code.

<details> <summary>git diff on fa86ba62de</summary>

diff --git a/test/functional/feature_abortnode.py b/test/functional/feature_abortnode.py
index a5c8aa163a..748875b25d 100755
--- a/test/functional/feature_abortnode.py
+++ b/test/functional/feature_abortnode.py
@@ -22,6 +22,8 @@ class AbortNodeTest(BitcoinTestFramework):
         # We'll connect the nodes later
 
     def run_test(self):
+        import subprocess
+        subprocess.run(["ls", "--invalid-flag"], check=True)
         self.generate(self.nodes[0], 3, sync_fun=self.no_op)
 
         # Deleting the undo file will result in reorg failure
diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py
index 823d946cfe..17e5a6b397 100755
--- a/test/functional/test_framework/test_framework.py
+++ b/test/functional/test_framework/test_framework.py
@@ -197,14 +197,8 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
         except SkipTest as e:
             self.log.warning("Test Skipped: %s" % e.message)
             self.success = TestStatus.SKIPPED
-        except subprocess.CalledProcessError as e:
-            self.log.exception("Called Process failed with '{}'".format(e.output))
-            self.success = TestStatus.FAILED
-        except KeyboardInterrupt:
-            self.log.warning("Exiting after keyboard interrupt")
-            self.success = TestStatus.FAILED
         except BaseException as e:
-            self.log.exception(f"Unexpected exception")
+            self.log.exception(repr(e))
             self.success = TestStatus.FAILED
         finally:
             exit_code = self.shutdown()

</details>

more informative without special case handling for CalledProcessError, with less code.

I don't think this is true. It is missing the output. You can try this for any command that captures the output:

subprocess.run(['bash', '-c', 'echo aaaaa && false'], check=True, capture_output=True)

There could be an argument about stderr, but this seems better as a separate commit or pull.

KeyboardInterrupt

It currently uses self.log.warning, which is different in verbosity. I don't think it makes sense to show the traceback when the user pressed CTRL+C?

You can try this for any command that captures the output:

Ah cool, thanks for the example. Agreed that improving is out of scope for this PR then.

I don't think it makes sense to show the traceback when the user pressed CTRL+C?

No strong preference either way, I kinda like seeing where it got interrupted (and I'm already used to it because the KeyboardInterrupt exception handling doesn't work for test_runner.py which is my usual interface), but makes sense to not change this behaviour here and leave as-is.

Can be marked as resolved.

I kinda like seeing where it got interrupted

Yeah, I realize it is useful to debug timeouts. Pushed a fresh commit for KeyboardInterrupt.