Using a hard-coded time.sleep in the tests is usually confusing and brittle. For example, the one in break_wait_test:
Fix all issues by replacing it with the proper condition to wait on.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34137.
See the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.
🚧 At least one of the CI tasks failed.
Task lint: https://github.com/bitcoin/bitcoin/actions/runs/20433712257/job/58709973844
LLM reason (✨ experimental): Lint Python code failed due to unused import (time) in test/functional/feature_init.py, causing lint check to exit with error.
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
270@@ -272,7 +271,14 @@ def break_wait_test(self):
271 # returns early it will return the current block height.
272 self.log.debug(f"Calling waitforblockheight with {self.rpc_timeout} sec RPC timeout")
No? The rpc timeout is half in reality, since it is never re-set after the init loop, which sets it to half.
edit: Also, this seems unrelated to the changes here. I am not touching this line.
270@@ -272,7 +271,14 @@ def break_wait_test(self):
271 # returns early it will return the current block height.
272 self.log.debug(f"Calling waitforblockheight with {self.rpc_timeout} sec RPC timeout")
273 fut = ex.submit(node.waitforblockheight, height=current_height+1, timeout=self.rpc_timeout*1000*2)
274- time.sleep(1)
275+
276+ def waitforblockheight_active():
So IIUC this checks if there’s any active commands. If not, it throws KeyError so the wait ends. If there is one, it ensure the first running command is waitforblockheight and then ends the wait.
A bit hard to follow. Maybe instead check whether any of the active_commands is waitforblockheight?
So IIUC this checks if there’s any active commands. If not, it throws
KeyErrorso the wait ends.
No it does not. It waits until the condition is true.
In any case, I am happy to push any diff or commit, if someone writes one.
In any case, I am happy to push any diff or commit, if someone writes one.
0diff --git a/test/functional/feature_init.py b/test/functional/feature_init.py
1index eb45340da7..56f3e61d21 100755
2--- a/test/functional/feature_init.py
3+++ b/test/functional/feature_init.py
4@@ -274,7 +274,7 @@ class InitTest(BitcoinTestFramework):
5
6 def waitforblockheight_active():
7 try:
8- return node.cli.getrpcinfo()["active_commands"][0]["method"] == "waitforblockheight"
9+ return any(active_command["method"] == "waitforblockheight" for active_command in node.cli.getrpcinfo()["active_commands"])
10 except KeyError:
11 return False
KeyError exception need not be looked for now.
tACK fa727e3
Seems like an esoteric change but I will have it because I like it.
utACK fa727e3ec984106371eeedb34d7bbbbc3dcce4ff
Thanks, this is easier to read than before: #34137 (review)
It’s also an improvement over sleep(1), because the code specifically says what it’s waiting for.
tACK fa727e3ec984106371eeedb34d7bbbbc3dcce4ff
PR removes arbitrary sleep(1) for a “readiness” check, this improves reliability of test.
(on my machine there is a 1 sec reduction on test duration; Master: Feature-init.py (15s) - this PR Feature-init.py (14s) )
