test: Added coverage to the waitfornewblock rpc

kevkevinpal commented at 1:17 pm on January 28, 2025: contributor

Added a test for the Negative timeout error if the rpc is given a negative value for its timeout arg

This adds coverage to the waitfornewblock rpc

you can check to see there is no coverage for this error by doing grep -nri "Negative timeout" ./test/

and nothing shows up, you can also see by manually checking where we call waitfornewblock in the functional tests

DrahtBot commented at 1:17 pm on January 28, 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/31746.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	brunoerg, Sjors, tdb3, achow101

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#30635 (rpc: add optional blockhash to waitfornewblock, unhide in help by Sjors)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Tests on Jan 28, 2025

Sjors commented at 2:16 pm on January 28, 2025: member

Concept ACK. Always good to test error handling.

CI failure seems unrelated.

kevkevinpal commented at 2:20 pm on January 28, 2025: contributor

looks like an unrelated ci failure p2p_orphan_handling.py | ✖ Failed | 437 s

 02025-01-28T13:25:25.716000Z TestFramework (INFO): Test that, if a parent goes missing during orphan reso, it is requested
 12025-01-28T13:32:07.160000Z TestFramework.utils (ERROR): wait_until() failed. Predicate: ''''
 2        def test_function():
 3            if check_connected:
 4                assert self.is_connected
 5            return test_function_in()
 6'''
 72025-01-28T13:32:07.386000Z TestFramework (ERROR): Assertion failed
 8Traceback (most recent call last):
 9  File "/Users/runner/work/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 135, in main
10    self.run_test()
11    ~~~~~~~~~~~~~^^
12  File "/Users/runner/work/bitcoin/bitcoin/ci/scratch/build-aarch64-apple-darwin23.6.0/test/functional/p2p_orphan_handling.py", line 820, in run_test
13    self.test_parents_change()
14    ~~~~~~~~~~~~~~~~~~~~~~~~^^
15  File "/Users/runner/work/bitcoin/bitcoin/ci/scratch/build-aarch64-apple-darwin23.6.0/test/functional/p2p_orphan_handling.py", line 55, in wrapper
16    func(self)
17    ~~~~^^^^^^
18  File "/Users/runner/work/bitcoin/bitcoin/ci/scratch/build-aarch64-apple-darwin23.6.0/test/functional/p2p_orphan_handling.py", line 791, in test_parents_change
19    peer2.wait_for_parent_requests([int(parent_peekaboo_AB["txid"], 16), int(parent_missing["txid"], 16)])
20    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
21  File "/Users/runner/work/bitcoin/bitcoin/ci/scratch/build-aarch64-apple-darwin23.6.0/test/functional/p2p_orphan_handling.py", line 99, in wait_for_parent_requests
22    self.wait_until(test_function, timeout=10)
23    ~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^
24  File "/Users/runner/work/bitcoin/bitcoin/test/functional/test_framework/p2p.py", line 594, in wait_until
25    wait_until_helper_internal(test_function, timeout=timeout, lock=p2p_lock, timeout_factor=self.timeout_factor, check_interval=check_interval)
26    ~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
27  File "/Users/runner/work/bitcoin/bitcoin/test/functional/test_framework/util.py", line 317, in wait_until_helper_internal
28    raise AssertionError("Predicate {} not true after {} seconds".format(predicate_source, timeout))
29AssertionError: Predicate ''''
30        def test_function():
31            if check_connected:
32                assert self.is_connected
33            return test_function_in()
34''' not true after 400.0 seconds
352025-01-28T13:32:07.505000Z TestFramework (INFO): Not stopping nodes as test failed. The dangling processes will be cleaned up later.
362025-01-28T13:32:07.505000Z TestFramework (WARNING): Not cleaning up dir /Users/runner/work/bitcoin/bitcoin/ci/scratch/test_runner/test_runner_₿_🏃_20250128_132322/p2p_orphan_handling_282
372025-01-28T13:32:07.505000Z TestFramework (ERROR): Test failed. Test logging available at /Users/runner/work/bitcoin/bitcoin/ci/scratch/test_runner/test_runner_₿_🏃_20250128_132322/p2p_orphan_handling_282/test_framework.log
382025-01-28T13:32:07.506000Z TestFramework (ERROR): 
392025-01-28T13:32:07.506000Z TestFramework (ERROR): Hint: Call /Users/runner/work/bitcoin/bitcoin/test/functional/combine_logs.py '/Users/runner/work/bitcoin/bitcoin/ci/scratch/test_runner/test_runner_₿_🏃_20250128_132322/p2p_orphan_handling_282' to consolidate all logs
402025-01-28T13:32:07.506000Z TestFramework (ERROR): 
412025-01-28T13:32:07.506000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log.
422025-01-28T13:32:07.506000Z TestFramework (ERROR): https://github.com/bitcoin/bitcoin/issues
432025-01-28T13:32:07.506000Z TestFramework (ERROR):

brunoerg commented at 2:34 pm on January 28, 2025: contributor

I did not check the code just noted that https://maflcko.github.io/b-c-cov/total.coverage/src/rpc/blockchain.cpp.gcov.html shows that this error is already covered and https://corecheck.dev/bitcoin/bitcoin/pulls/31746 does not show any gained coverage for it based on this PR.

Sjors commented at 2:41 pm on January 28, 2025: member

@brunoerg the line is covered, but I don’t think that means both branches of the if statement are.

You could change it to:

0    if (timeout < 0) {
1        throw JSONRPCError(RPC_MISC_ERROR, "Negative timeout");
2    }

In which case coverage would go up, but that seems unnecessary.

brunoerg commented at 2:45 pm on January 28, 2025: contributor

@brunoerg the line is covered, but I don’t think that means both branches of the if statement are.

Sure, I did not pay much attention to it. Anyway, this kind of coverage the corecheck should be able to catch, maybe it just checks the lines?

test: Added coverage to the waitfornewblock rpc

Added a test for the Negative timeout error if the rpc is given a
negative value for its timeout arg

93747d934b

in test/functional/rpc_blockchain.py:552 in 954f768d4f outdated

548@@ -549,6 +549,7 @@ def _test_waitforblock(self):
549         # The chain has probably already been restored by the time reconsiderblock returns,
550         # but poll anyway.
551         self.wait_until(lambda: node.waitfornewblock(timeout=100)['hash'] == current_hash)
552+        assert_raises_rpc_error(-1, "Negative timeout", lambda: node.waitfornewblock(timeout=-1))

brunoerg commented at 2:49 pm on January 28, 2025:

Why do we need the lambda here? It will throw the error right at the beginning of the execution.

Sjors commented at 3:08 pm on January 28, 2025:

Could try:

0assert_raises_rpc_error(-1, "Negative timeout", node.waitfornewblock, -1)

kevkevinpal commented at 3:14 pm on January 28, 2025:

thanks! that works updated in 93747d9

kevkevinpal force-pushed on Jan 28, 2025

brunoerg commented at 3:26 pm on January 28, 2025: contributor

code review ACK 93747d934b8a5a1732732a958c0d7e2f5dd0b8c2

DrahtBot requested review from Sjors on Jan 28, 2025

Sjors commented at 3:44 pm on January 28, 2025: member

tACK 93747d934b8a5a1732732a958c0d7e2f5dd0b8c2

tdb3 commented at 1:32 am on January 31, 2025: contributor

You could change it to:
0    if (timeout < 0) {
1        throw JSONRPCError(RPC_MISC_ERROR, "Negative timeout");
2    }
In which case coverage would go up, but that seems unnecessary.

Maybe my preference for multi line if-statements is biasing me, but in general this seems like it could be an argument for preferring multi line if-statements over single line ones.

The current developer notes provides the optionality, but I would support updating it to avoid single line ifs. Thoughts?

tdb3 approved

tdb3 commented at 1:37 am on January 31, 2025: contributor

ACK 93747d934b8a5a1732732a958c0d7e2f5dd0b8c2

Thanks for adding coverage. Induced test failure by commenting out https://github.com/bitcoin/bitcoin/blob/8fa10edcd1706a1f0dc9d8c3adbc8efa3c7755bf/src/rpc/blockchain.cpp#L284

Sjors commented at 7:52 am on January 31, 2025: member

it could be an argument for preferring multi line if-statements over single line ones

Maybe, but it wouldn’t catch ? branches either. You could propose a linter change that insists on multi line if-statements. Though I expect … strong opinions :-)

achow101 commented at 7:39 pm on January 31, 2025: member

ACK 93747d934b8a5a1732732a958c0d7e2f5dd0b8c2

achow101 merged this on Jan 31, 2025

achow101 closed this on Jan 31, 2025

test: Added coverage to the waitfornewblock rpc #31746

Code Coverage & Benchmarks

Reviews

Conflicts