qa: feature_framework_startup_failures.py fixes & improvements (#30660 follow-up) #32509

1. 
   hodlinator commented at 11:58 am on May 15, 2025: contributor

   • Handle multiple --tmpdir args properly.
   • Handle --timeout-factor=0 properly (fixes #32506).
   • Improve readability of expected error message (assert_raises_message()).
   • Make suppressed error output less confusing (wait_for_rpc_connection()).
2. 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 <ryan@ofsky.org>

   d8f05e7bf3
3. 
   DrahtBot commented at 11:58 am on May 15, 2025: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Code Coverage & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32509.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	i-am-yuvi, ismaelsadeeq, janb84, achow101

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

4. DrahtBot added the label Tests on May 15, 2025
5. 
   i-am-yuvi commented at 7:56 pm on May 15, 2025: contributor

   Tested ACK c47f634718d4248fd2a30e51a57944f89da72a64

    0./build/test/functional/test_runner.py feature_framework_startup_failures.py --timeout-factor=0
    1Temporary test directory at /var/folders/jb/wlbrz0t95vl58wzxjt75wqmh0000gn/T/test_runner_₿_🏃_20250516_012813
    2Remaining jobs: [feature_framework_startup_failures.py]
    31/1 - feature_framework_startup_failures.py passed, Duration: 5 s
    4
    5TEST                                  | STATUS    | DURATION
    6
    7feature_framework_startup_failures.py | ✓ Passed  | 5 s
    8
    9ALL                                   | ✓ Passed  | 5 s (accumulated) 
   10Runtime: 5 s
   

6. in test/functional/feature_framework_startup_failures.py:46 in 1235a0df5f outdated

   41@@ -42,10 +42,13 @@ def _verify_startup_failure(self, test, internal_args, expected_exception):
   42         subdir = md5(expected_exception.encode('utf-8')).hexdigest()[:8]
   43         args = [sys.executable] + sys.argv + [f"--tmpdir={self.options.tmpdir}/{subdir}", f"--internal_test={test.__name__}"] + internal_args
   44 
   45+        # BitcoinTestFramework.parse_args converts --timeout-factor=0 to 99999.
   46+        timeout = None if self.options.timeout_factor == 99999 else 60 * self.options.timeout_factor
   

   ---

   ---

   

   ismaelsadeeq commented at 10:37 pm on May 15, 2025:

   In “qa: Handle –timeout-factor=0” 1235a0df5ff6278d07dc95e7acc3ccb14f536286 whats the rationale behind multiplying by factor of 60?

   Wondering if this is the right fix? if I pass timeout-factor=99998 the test will fail, but on master I dont think running the functional test with timeout-factor=99998 will trigger any failure.

   ---

   

   maflcko commented at 6:28 am on May 16, 2025:

   I’d say an alternative fix could also be to reduce the magic value from 99'999 to 9'999 or 999. ( Any value of that should be more than sufficient in practise. And anyone really wanting a larger timeout factor, can just set it trivially)

   ---

   

   hodlinator commented at 7:08 am on May 16, 2025:

   whats the rationale behind multiplying by factor of 60?

   Default timeout-factor is 1, so we get 60 seconds.

   Here’s a similar pre-existing example:

   https://github.com/bitcoin/bitcoin/blob/c47f634718d4248fd2a30e51a57944f89da72a64/test/functional/test_framework/test_framework.py#L765-L773

   on master I dont think running the functional test with timeout-factor=99998 will trigger any failure.

   99998 does fail feature_framework_startup_failures.py on master, but skipping back before the test was added, it succeeded for me (except p2p_node_network_limited.py --v1transport, p2p_node_network_limited.py --v2transport “never” finishes). It appears no other test is specifying the timeout parameter to subprocess.run(). It feels more correct to do so, so I’d rather keep it.

   an alternative fix could also be to reduce the magic value from 99'999 to 9'999 or 999. ( Any value of that should be more than sufficient in practise. And anyone really wanting a larger timeout factor, can just set it trivially)

   Thanks! Set to 999 in test_framework.py in latest push and reverted the change to feature_framework_startup_failures.py.

7. in test/functional/test_framework/test_node.py:349 in c47f634718 outdated

   349-                suppressed_errors["missing_credentials"] += 1
   350-                latest_error = repr(e)
   351+                latest_error = suppress_error("missing_credentials", e)
   352             time.sleep(1.0 / poll_per_s)
   353-        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})")
   354+        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]}'})")
   

   ---

   ---

   

   ismaelsadeeq commented at 10:37 pm on May 15, 2025:

   Nice this is much better I think.
8. 
   ismaelsadeeq commented at 10:37 pm on May 15, 2025: member

   Concept ACK thanks for following up.
9. 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>

   bd8ebbc4ab
10. 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

    075352ec8e
11. qa: Improve suppressed errors output

    Original discussion: https://github.com/bitcoin/bitcoin/pull/30660#discussion_r2080132673

    bf950c4544
12. hodlinator force-pushed on May 16, 2025
13. 
    i-am-yuvi commented at 12:02 pm on May 16, 2025: contributor

    re-ACK bf950c4544d3a8478b46faf0b93c0dc647274c9b
14. DrahtBot requested review from ismaelsadeeq on May 16, 2025
15. 
    ismaelsadeeq commented at 1:51 pm on May 16, 2025: member

    Code review and tested ACK bf950c45

    This PR makes the test easier to read and also rightly fixes the issue in the OP.

16. 
    janb84 commented at 9:32 am on May 21, 2025: contributor

    LGTM ACK https://github.com/bitcoin/bitcoin/commit/bf950c4544d3a8478b46faf0b93c0dc647274c9b

    • code review ✅
    • build and tested ✅

    Improved code readability and addressed open issues from #30660

17. 
    achow101 commented at 9:39 pm on May 27, 2025: member

    ACK bf950c4544d3a8478b46faf0b93c0dc647274c9b
18. achow101 merged this on May 27, 2025
19. achow101 closed this on May 27, 2025

    ---

Contributors
hodlinator DrahtBot i-am-yuvi ismaelsadeeq maflcko janb84 achow101

Review Requested
ismaelsadeeq

Labels
Tests