test: Introduce ensure_for helper #30893

pull fjahr wants to merge 3 commits into bitcoin:master from fjahr:2024-09-shouldnt-happen changing 11 files +59 −51
  1. fjahr commented at 9:42 pm on September 12, 2024: contributor

    A repeating pattern in the functional tests is that the test sleeps for a while to ensure that a certain condition is still true after some amount of time has elapsed. Most recently a new case of this was added in #30807. This PR here introduces an ensure helper to streamline this functionality.

    Some approach considerations:

    • It is possible to construct this by reusing wait_until and wrapping it in try internally. However, the logger output of the failing wait would still be printed which seems irritating. So I opted for simplified but similar internals to wait_until.
    • This implementation starts for a failure in the condition right away which has the nice side-effect that it might give feedback on a failure earlier than is currently the case. However, in some cases, it may be expected that the condition may still be false at the beginning and then turns true until time has run out, something that would work when the test sleeps without checking in a loop. I decided against this design (and even against adding it as an option) because such a test design seems like it would be racy either way.
    • I have also been going back and forth on naming. To me ensure works well but I am also not a native speaker, happy consider a different name if others don’t think it’s clear enough.
  2. DrahtBot commented at 9:42 pm on September 12, 2024: 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/30893.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK maflcko, tdb3, furszy, achow101
    Concept ACK jonatack, BrandonOdiwuor, pablomartin4btc

    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:

    • #31250 (wallet: Disable creating and loading legacy wallets by achow101)
    • #29500 (test: create assert_not_equal util by kevkevinpal)

    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.

  3. DrahtBot added the label Tests on Sep 12, 2024
  4. in test/functional/feature_assumeutxo.py:307 in 0908a5776c outdated 
    303@@ -305,8 +304,7 @@ def test_sync_from_assumeutxo_node(self, snapshot):
304         # If it does request such blocks, the snapshot_node will ignore requests it cannot fulfill, causing the ibd_node
305         # to stall. This stall could last for up to 10 min, ultimately resulting in an abrupt disconnection due to the
306         # ibd_node's perceived unresponsiveness.
307-        time.sleep(3)  # Sleep here because we can't detect when a node avoids requesting blocks from other peer.
308-        assert_equal(len(ibd_node.getpeerinfo()[0]['inflight']), 0)
309+        ibd_node.ensure(lambda: len(ibd_node.getpeerinfo()[0]['inflight']) == 0, 3)
    jonatack commented at 9:49 pm on September 12, 2024:
    For clarity/readability, suggest using a named argument in the ensure invocations where you pass a timeout value (edit: maybe enforce that with a * parameter)
    fjahr commented at 10:27 am on September 13, 2024:
    Done and renamed to duration.
  5. jonatack commented at 9:50 pm on September 12, 2024: member
    Concept ACK
  6. in test/functional/test_framework/p2p.py:591 in 0908a5776c outdated 
    587@@ -587,6 +588,9 @@ def test_function():
588 
589         wait_until_helper_internal(test_function, timeout=timeout, lock=p2p_lock, timeout_factor=self.timeout_factor)
590 
591+    def ensure(self, test_function, timeout=5):
    maflcko commented at 9:51 pm on September 12, 2024: 
    0    def ensure(self, test_function, *, duration):

    I think calling this “timeout” may be a bit confusing, because it will always “timeout”. Maybe “duration” can be used to communicate that this always takes a fixed span of time?

    fjahr commented at 10:28 am on September 13, 2024:
    sounds good, done
  7. in test/functional/test_framework/util.py:271 in 0908a5776c outdated 
    266@@ -267,6 +267,14 @@ def satoshi_round(amount: Union[int, float, str], *, rounding: str) -> Decimal:
267     """Rounds a Decimal amount to the nearest satoshi using the specified rounding mode."""
268     return Decimal(amount).quantize(SATOSHI_PRECISION, rounding=rounding)
269 
270+def ensure_helper_internal(predicate, timeout, timeout_factor=1.0):
271+    timeout = timeout * timeout_factor
    maflcko commented at 9:54 pm on September 12, 2024:

    Why should this be scaled by the timeout factor? If someone increased that sufficiently to not have to deal with timeouts, they will be bitten by tests that never terminate. IIRC the CI has a large factor, so it may lead to timeouts there.

    I think just making the duration parameter a required named arg would be cleaner and also better document that the test will spend that amount of time “sleeping”.

    fjahr commented at 10:27 am on September 13, 2024:
    My thought here was that we set a higher factor when things are generally expected to run slower, so in that case it seems natural to me that we will also want to wait longer. But yeah, I didn’t take into account that people will set this factor unreasonably high to not see timeouts at all. I have removed it for now but I think an alternative could be to set a reasonable upper bound like 10x.
  8. DrahtBot added the label CI failed on Sep 13, 2024
  9. maflcko commented at 9:09 am on September 13, 2024: member 
     0 test  2024-09-13T08:23:13.856000Z TestFramework (ERROR): Unexpected exception caught during testing 
 1                                   Traceback (most recent call last):
 2                                     File "/ci_container_base/test/functional/test_framework/test_framework.py", line 132, in main
 3                                       self.run_test()
 4                                     File "/ci_container_base/ci/scratch/build-i686-pc-linux-gnu/test/functional/feature_minchainwork.py", line 68, in run_test
 5                                       self.nodes[2].ensure(len(self.nodes[2].getchaintips()) == 1, 3)
 6                                     File "/ci_container_base/test/functional/test_framework/test_node.py", line 846, in ensure
 7                                       return ensure_helper_internal(test_function, timeout, timeout_factor=self.timeout_factor)
 8                                     File "/ci_container_base/test/functional/test_framework/util.py", line 273, in ensure_helper_internal
 9                                       predicate_source = "''''\n" + inspect.getsource(predicate) + "'''"
10                                     File "/usr/lib64/python3.9/inspect.py", line 1024, in getsource
11                                       lines, lnum = getsourcelines(object)
12                                     File "/usr/lib64/python3.9/inspect.py", line 1006, in getsourcelines
13                                       lines, lnum = findsource(object)
14                                     File "/usr/lib64/python3.9/inspect.py", line 817, in findsource
15                                       file = getsourcefile(object)
16                                     File "/usr/lib64/python3.9/inspect.py", line 697, in getsourcefile
17                                       filename = getfile(object)
18                                     File "/usr/lib64/python3.9/inspect.py", line 677, in getfile
19                                       raise TypeError('module, class, method, function, traceback, frame, or '
20                                   TypeError: module, class, method, function, traceback, frame, or code object was expected, got bool
  10. fjahr force-pushed on Sep 13, 2024
  11. fjahr commented at 10:29 am on September 13, 2024: contributor
    Addressed feedback from @maflcko and @jonatack and fixed the missing lambda: causing the CI failure.
  12. in test/functional/test_framework/p2p.py:591 in f6161632f3 outdated 
    587@@ -587,6 +588,9 @@ def test_function():
588 
589         wait_until_helper_internal(test_function, timeout=timeout, lock=p2p_lock, timeout_factor=self.timeout_factor)
590 
591+    def ensure(self, test_function, *, duration=5):
    maflcko commented at 10:36 am on September 13, 2024: 
    0    def ensure(self, test_function, *, **kwargs):

    I am not sure about a default value here. I feel like time.sleep(), or similar constructs in tests should be easily visible. Otherwise, this may be used as a drop-in replacement for assert, because someone may think this is just an assert that accepts a lambda.

    fjahr commented at 10:46 am on September 13, 2024:
    Ok, that might be more of a naming issue with ensure then which I am happy to reconsider if someone has a better idea but I removed the default value for now.
    theStack commented at 11:53 am on September 13, 2024:
    One idea for improved readability might be to name it ensure_after, and reorder the (mandatory) duration argument to be before the test function lambda? Then the call-site would look like e.g. ensure_after(duration=3, lambda: some_condition == True)
    fjahr commented at 2:01 pm on September 13, 2024:
    Maybe rather ensure_for(duration=X...? “after” implies that the condition will only be checked once after time is up IMO…
  13. in test/functional/test_framework/util.py:270 in f6161632f3 outdated 
    266@@ -267,6 +267,13 @@ def satoshi_round(amount: Union[int, float, str], *, rounding: str) -> Decimal:
267     """Rounds a Decimal amount to the nearest satoshi using the specified rounding mode."""
268     return Decimal(amount).quantize(SATOSHI_PRECISION, rounding=rounding)
269 
270+def ensure_helper_internal(predicate, timeout):
    maflcko commented at 10:36 am on September 13, 2024: 
    0def ensure_helper_internal(predicate, *, duration):

    forgot to rename this one?

    fjahr commented at 10:43 am on September 13, 2024:
    I didn’t think it matters much because this is internal anyway, but I renamed it now as well.
  14. fjahr force-pushed on Sep 13, 2024
  15. fjahr force-pushed on Sep 13, 2024
  16. BrandonOdiwuor commented at 1:37 pm on September 13, 2024: contributor
    Concept ACK
  17. pablomartin4btc commented at 2:42 pm on September 13, 2024: member

    Concept ACK

    nit:

    • I have also been going back and forth on naming.

    What about ensure_during? (another alternatives: ensure_within_duration/ ensure_over_duration) It adds a notion of time, making it more specific to the context of waiting or checking a condition over a period and it’s less generic and more intuitive.

  18. DrahtBot removed the label CI failed on Sep 13, 2024
  19. fjahr force-pushed on Sep 15, 2024
  20. fjahr commented at 10:56 am on September 15, 2024: contributor
    Since a few reviewers seem to prefer a clearer naming, I have now changed it to ensure_for(duration=..., f=...).
  21. in test/functional/test_framework/util.py:276 in dcd6647260 outdated 
    271+    time_end = time.time() + duration
272+    predicate_source = "''''\n" + inspect.getsource(predicate) + "'''"
273+    while time.time() < time_end:
274+        if not predicate():
275+            raise AssertionError("Predicate {} became false within {} seconds".format(predicate_source, duration))
276+        time.sleep(0.05)
    furszy commented at 3:09 pm on September 15, 2024:

    Isn’t it a bit aggressive to execute the provided predicate every 0.05 seconds for the entire duration? Beyond flooding the logs, the predicate likely calls an RPC command that locks cs_main or some other important mutex, which slows down the node.

    For instance, the feature_assumeutxo.py one, will call getpeerinfo() 60 times prior to failing.

    fjahr commented at 4:55 pm on September 15, 2024:

    I simply took the same interval as wait_until uses. If the slowdown was an issue it should be much more apparent from the use there, so I did a simple benchmark and increasing the sleep from 0.05 to 0.2 lead to a considerable slowdown of the test suite overall (8min to 10:45min), so I don’t think this would be an issue here either.

    We could still lower the interval. Since we generally don’t expect the predicate to become false it shouldn’t hurt performance as much as changing it in wait_until. It means we would have fewer logs and if a failure occurs it would take a bit longer to detect it.

    furszy commented at 1:53 pm on September 16, 2024:

    I think we should target the expected scenario, not the failure, in both cases because that’s how tests will usually execute. ensure_for plays the opposite role of wait_until. We want to verify that a certain condition does not hold for a period of time (it is the timeout condition of wait_until). And, in fact, we could implement it using the wait_until function too (quick and dirty - not saying to do it in this way):

    0def ensure_for(self, wait_until_negated_condition, duration):
1    try:
2        self.wait_until(test_function_in=wait_until_negated_condition, timeout=duration)
3    except AssertionError:
4        return  # timeout, all good.
5    raise AssertionError("Predicate {} true after {} duration".format(negated_condition, duration))

    So, I’d suggest simply waiting for the entire duration without intermediate checks. If we think the unexpected failure could be triggered before the provided duration, we should decrease the duration. However, the idea is that the failure shouldn’t happen under normal conditions.

    fjahr commented at 1:59 pm on September 16, 2024:

    And, in fact, we could implement it using the wait_until function too

    Yes, I tried this and decided against it, see the description of the PR.

    I’d suggest simply waiting for the entire duration without intermediate checks.

    I have also considered this and decided against it because I don’t think it’s better. I have also described this in the PR description already. Since the polling doesn’t seem to lead to a real performance issue I think it’s better to fail faster if we fail. This also uncovers potential races, as mentioned in the description.

    maflcko commented at 2:01 pm on September 16, 2024:

    So, I’d suggest simply waiting for the entire duration without intermediate checks.

    I’d say doing the check once more at the beginning should help, to catch misuse of the function, where a predicate is expected to trigger after some time.

    (I think this may allow to remove time.sleep from within tests completely, because all code could be changed to use either (1) ensure_for, (2) mocktime, or (3) wait_until?)

    furszy commented at 7:54 pm on September 16, 2024:

    And, in fact, we could implement it using the wait_until function too

    Yes, I tried this and decided against it, see the description of the PR.

    Ah, missed it. Sorry.

    I’d suggest simply waiting for the entire duration without intermediate checks.

    I have also considered this and decided against it because I don’t think it’s better. I have also described this in the PR description already. Since the polling doesn’t seem to lead to a real performance issue I think it’s better to fail faster if we fail. This also uncovers potential races, as mentioned in the description.

    hmm, so you are thinking about a condition that passes at the beginning, does not passes somewhere during the test sleep time and ends up passing at the end. Maybe the test condition should be changed if such scenario exist?

    I’m not sure we can really ensure that this won’t lead to performance issues. It’s abnormal to fail here, as it’s something we don’t expect during the test. We would be making 59 additional RPC calls in feature_assumeutxo.py, which would flood the active logs (logs we may need if the test fails later for some other reason after wise), all while looking for a failure that will rarely occur. I mean, for current use cases, it probably doesn’t matter. But could this be an issue for future uses? It seems to be something it could be misused very easily.

    Still, if you still think thats ok to continue with the PR in the current form, it is ok for me too. If it ends up being a performance issue or logs ends up too flooded, we will notice it.

    fjahr commented at 8:35 pm on September 21, 2024:

    hmm, so you are thinking about a condition that passes at the beginning, does not passes somewhere during the test sleep time and ends up passing at the end. Maybe the test condition should be changed if such scenario exist?

    Yes, exactly, and in order to know that it should be changed the person writing the test will see that ensure_for fails. This is the protection against introducing flakiness that I mentioned.

    which would flood the active logs

    I don’t know if that’s a problem because I always only grep the logs anyway.

    I’m not sure we can really ensure that this won’t lead to performance issues.

    The benchmarks say no but if you have another way to show that there is a performance impact. Maybe that’s not a guarantee but if there is any it appears to be very small.

    However, I have made the delay configurable now so this can be adjusted and I have set the default to 0.2 instead of 0.05. That should keep impact on the logs (and performance) small. I hope that works for everyone.

  22. tdb3 approved
  23. tdb3 commented at 4:53 pm on September 19, 2024: contributor

    ACK dcd66472600f1676e2e450245924ab6e1a837d39

    Nice addition. Compliments wait_until nicely.

    (As expected) saw little difference in both individual and total runtimes of tests on master vs pr branch (at least within typical run-to-run difference).

    Master (e46bebb444dfff89813333dc1926a57c9a3e8aab):

    test status duration(seconds)
    feature_assumeutxo.py Passed 32
    feature_minchainwork.py Passed 10
    mempool_unbroadcast.py Passed 8
    p2p_segwit.py Passed 67

    Total runtime: 141s

    PR branch:

    test status duration(seconds)
    feature_assumeutxo.py Passed 32
    feature_minchainwork.py Passed 10
    mempool_unbroadcast.py Passed 7
    p2p_segwit.py Passed 67

    Total runtime: 141s

  24. DrahtBot requested review from BrandonOdiwuor on Sep 19, 2024
  25. DrahtBot requested review from pablomartin4btc on Sep 19, 2024
  26. DrahtBot requested review from jonatack on Sep 19, 2024
  27. fjahr force-pushed on Sep 21, 2024
  28. fjahr commented at 8:41 pm on September 21, 2024: contributor

    I have introduced a delay parameter that allows to adjust the delay between checking the predicate. The default is set at 0.2 instead of previously 0.05.

    I have also added a second commit adding the delay parameter to wait_until and using it in places where a time.sleep is currently used and a high frequency of checking does not seem appropriate.

    There are still a few sleeps spread across the tests, I think some of them require some further thought on how to remove them and some of them could probably be left alone. See for example #30942 that I found while working on this here but I am keeping it separate since we can get rid of the sleep there for different reasons without a replacement.

  29. fjahr force-pushed on Sep 21, 2024
  30. DrahtBot added the label CI failed on Sep 21, 2024
  31. DrahtBot commented at 8:56 pm on September 21, 2024: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/30475201416

    Make sure to run all tests locally, according to the documentation.

    The failure may 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.

  32. fjahr force-pushed on Sep 21, 2024
  33. DrahtBot removed the label CI failed on Sep 21, 2024
  34. in test/functional/test_framework/util.py:276 in facb80151d outdated 
    271+    time_end = time.time() + duration
272+    predicate_source = "''''\n" + inspect.getsource(predicate) + "'''"
273+    while time.time() < time_end:
274+        if not predicate():
275+            raise AssertionError("Predicate {} became false within {} seconds".format(predicate_source, duration))
276+        time.sleep(delay)
    furszy commented at 2:37 pm on September 22, 2024:
    tiny nit: if duration < delay, then delay should be set to duration?
    fjahr commented at 5:08 pm on September 22, 2024:
    Hm, just doing that would have an unexpected effect, I think. With the code unchanged if the user would today pass the parameters with delay=duration, then I think the predicate would only be checked once in the beginning and not at the end because at the second conditional check the time passed should be the runtime of predicate + the sleep of duration, so the loop wouldn’t run a second time. That’s not what a user expects when they pass a high delay probably. So I have done the following: If delay is 0, negative or higher than duration, we set it equal to duration. But also the predicate is now check once in the beginning explicitly and in the loop it now runs after the sleep instead of before. This means that if the user sets delay to 0 explicitly (or set a value that is weird, negative or above duration) they will have the behavior or check once in the beginning and once at the end. This should be the minimum of checks, as discussed previously.
  35. in test/functional/test_framework/p2p.py:583 in 44bbb331c5 outdated 
    579@@ -580,13 +580,13 @@ def on_version(self, message):
580 
581     # Connection helper methods
582 
583-    def wait_until(self, test_function_in, *, timeout=60, check_connected=True):
584+    def wait_until(self, test_function_in, *, timeout=60, check_connected=True, delay=0.05):
    furszy commented at 2:43 pm on September 22, 2024:
    maybe rename “delay” to “check_interval” or another similar term?
    fjahr commented at 5:08 pm on September 22, 2024:
    renamed to check_interval
  36. furszy commented at 2:45 pm on September 22, 2024: member
    looking good, left few nits.
  37. fjahr force-pushed on Sep 22, 2024
  38. fjahr commented at 5:08 pm on September 22, 2024: contributor
    Addressed feedback from @furszy and added some better documentation.
  39. in test/functional/test_framework/util.py:271 in aa4a0090a2 outdated 
    266@@ -267,6 +267,28 @@ def satoshi_round(amount: Union[int, float, str], *, rounding: str) -> Decimal:
267     """Rounds a Decimal amount to the nearest satoshi using the specified rounding mode."""
268     return Decimal(amount).quantize(SATOSHI_PRECISION, rounding=rounding)
269 
270+def ensure_for_helper_internal(*, duration, predicate, check_interval=0.2):
    furszy commented at 11:39 am on September 23, 2024:

    nit: PEP 8: E302 expected 2 blank lines, found 1

    Should add another blank line above and below this function.

    fjahr commented at 3:34 pm on September 23, 2024:
    fixed
  40. in test/functional/test_framework/util.py:291 in aa4a0090a2 outdated 
    286+        raise error
287+    # Repeat checks until timeout
288+    while time.time() < time_end:
289+        time.sleep(check_interval)
290+        if not predicate():
291+            raise error
    furszy commented at 11:43 am on September 23, 2024:

    ultra tiny nit: same code, a bit shorter.

    0predicate_source = "''''\n" + inspect.getsource(predicate) + "'''"
1while True:
2    if not predicate():
3        raise AssertionError("Predicate {} became false within {} seconds".format(predicate_source, duration))
4     if time.time() > time_end:
5        return
6    time.sleep(check_interval)
    fjahr commented at 3:34 pm on September 23, 2024:
    taken
  41. furszy commented at 12:35 pm on September 23, 2024: member

    Code review ACK

    Commit 19bc71caf6a43702946f1bc15696fe14afa24fda description says “delay” when it should say “check_interval”.

  42. in test/functional/wallet_keypool.py:139 in 19bc71caf6 outdated 
    135@@ -137,8 +136,7 @@ def run_test(self):
136         nodes[0].keypoolrefill(3)
137 
138         # test walletpassphrase timeout
139-        time.sleep(1.1)
140-        assert_equal(nodes[0].getwalletinfo()["unlocked_until"], 0)
141+        self.nodes[0].wait_until(lambda: nodes[0].getwalletinfo()["unlocked_until"] == 0, timeout=1.1, check_interval=0.5)
    maflcko commented at 12:54 pm on September 23, 2024:

    Just for context, the timeout inside wait_until is scaled by the timeout-factor, but the test really should measure real (wall-clock) time here, not something scaled.

    I know I suggested #30893 (review), and this looks more like (2) mocktime should ideally be used. However, that requires changing real code. I’d say it would also be fine to leave this as-is for now, but up to you.

    fjahr commented at 9:13 pm on September 29, 2024:
    Yeah, looking at it again, this doesn’t make sense because the test may fail if someone actually scales the wait time down. I changed it back and will leave the mocktime approach for a follow-up.
  43. fjahr force-pushed on Sep 23, 2024
  44. fjahr commented at 3:35 pm on September 23, 2024: contributor

    Commit https://github.com/bitcoin/bitcoin/commit/19bc71caf6a43702946f1bc15696fe14afa24fda description says “delay” when it should say “check_interval”.

    Also fixed, thanks!

  45. furszy commented at 5:34 pm on September 23, 2024: member
    ACK 5bcc578b7f5b34bd4c3a6ee1e54681c56e3db450
  46. DrahtBot requested review from tdb3 on Sep 23, 2024
  47. fjahr renamed this:
    test: Introduce ensure helper
    test: Introduce ensure_for helper
    on Sep 23, 2024
  48. in test/functional/test_framework/test_node.py:846 in 5bcc578b7f outdated 
    843-        return wait_until_helper_internal(test_function, timeout=timeout, timeout_factor=self.timeout_factor)
844+    def wait_until(self, test_function, timeout=60, check_interval=0.05):
845+        return wait_until_helper_internal(test_function, timeout=timeout, timeout_factor=self.timeout_factor, check_interval=check_interval)
846 
847+    def ensure_for(self, *, duration, f, check_interval=0.2):
848+        return ensure_for_helper_internal(duration=duration, predicate=f, check_interval=check_interval)
    maflcko commented at 9:30 am on September 24, 2024: 
    0    def ensure_for(self, *, duration, f, **kwargs):
1        return ensure_for_helper_internal(duration=duration, predicate=f, **kwargs)

    style-nit: Seems fine to have the default arg only in one place, since it is supposed to be the same in all places.

    (Same for all other places)

    fjahr commented at 9:13 pm on September 29, 2024:
    Changed to use **kwargs in both ensure_for impls
  49. in test/functional/test_framework/util.py:292 in 5bcc578b7f outdated 
    288+        if time.time() > time_end:
289+            return
290+        time.sleep(check_interval)
291+
292+
293+def wait_until_helper_internal(predicate, *, attempts=float('inf'), timeout=float('inf'), lock=None, timeout_factor=1.0, check_interval=0.05):
    maflcko commented at 9:32 am on September 24, 2024:
    attempts= is unused and I fail to see a use-case, so it may be best to remove
    fjahr commented at 9:13 pm on September 29, 2024:
    I agree and couldn’t find any good use cases for it. It can be re-added when there is a use case for it so I have removed it for now and simplified the code.
  50. fjahr force-pushed on Sep 29, 2024
  51. fjahr commented at 9:16 pm on September 29, 2024: contributor
    Addressed feedback from @maflcko , thanks!
  52. tdb3 approved
  53. tdb3 commented at 9:47 pm on September 29, 2024: contributor

    re ACK 352b8209aa5327f7d369e2acc4d87f9767389a6b

    Also sanity checked testing timing (https://github.com/bitcoin/bitcoin/pull/30893?notification_referrer_id=NT_kwDOBljilbUxMjM4MjY5Nzc4OToxMDY0ODg0Njk#pullrequestreview-2316167311)

  54. DrahtBot requested review from furszy on Sep 29, 2024
  55. furszy commented at 10:04 pm on October 15, 2024: member
    ACK 352b8209aa5
  56. DrahtBot added the label CI failed on Oct 29, 2024
  57. DrahtBot commented at 8:37 pm on October 29, 2024: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/30825233774

    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.

  58. fjahr force-pushed on Nov 5, 2024
  59. fjahr commented at 3:51 pm on November 5, 2024: contributor
    I could remove another sleep import line after #30942 was merged which was what the linter complained about. Should be trivial to re-ACK: https://github.com/bitcoin/bitcoin/compare/352b8209aa5327f7d369e2acc4d87f9767389a6b..f04529fc40f2f7c8e4e63d892cdcbe6c91dfbf96
  60. DrahtBot removed the label CI failed on Nov 5, 2024
  61. in test/functional/test_framework/test_node.py:845 in 6462ccfec7 outdated 
    841@@ -841,6 +842,8 @@ def bumpmocktime(self, seconds):
842     def wait_until(self, test_function, timeout=60):
843         return wait_until_helper_internal(test_function, timeout=timeout, timeout_factor=self.timeout_factor)
844 
845+    def ensure_for(self, *, duration, f, **kwargs):
    maflcko commented at 11:29 am on November 8, 2024:

    f04529fc40f2f7c8e4e63d892cdcbe6c91dfbf96: Any reason to make this a member method, when it doesn’t use any member fields? I’d say it would be fine to either make this a global, standalone, util function. Otherwise, if you want to make it a member, it would be good to also add it to the other classes, like test_framework. Then, always use it from the outer-most scope.

    For example, it is a bit confusing to call self.nodes[0].p2ps[0].ensure_for(...), when self.ensure_for(...) is identical, or even just ensure_for(...).

    fjahr commented at 10:56 pm on November 10, 2024:
    Initially it had the timeout_factor and also it seemed nice to have it next to wait_until but sure, I have made it a util function now.
  62. tdb3 approved
  63. tdb3 commented at 5:40 pm on November 9, 2024: contributor

    Code Review re ACK f04529fc40f2f7c8e4e63d892cdcbe6c91dfbf96

    Would also review again if the suggestion in #30893 (review) was implemented.

  64. DrahtBot requested review from furszy on Nov 9, 2024
  65. fjahr force-pushed on Nov 10, 2024
  66. fjahr commented at 10:56 pm on November 10, 2024: contributor
    Addressed feedback from @maflcko and made ensure_for a util function.
  67. tdb3 approved
  68. tdb3 commented at 0:14 am on November 11, 2024: contributor

    code review re ACK dde84e2b2b6a64f5f67d4dff6e3fb2fc7d2c5a77

    Changes are focused on the movement of ensure_for() to util (with appropriate adjustments to imports and calls to it).

  69. in test/functional/test_framework/util.py:271 in b2ac698ce1 outdated 
    267@@ -268,6 +268,27 @@ def satoshi_round(amount: Union[int, float, str], *, rounding: str) -> Decimal:
268     return Decimal(amount).quantize(SATOSHI_PRECISION, rounding=rounding)
269 
270 
271+def ensure_for(*, duration, f, check_interval=0.2, **kwargs):
    maflcko commented at 8:26 am on November 11, 2024:
    nit in b2ac698ce102edaf8039f42b8793ca10deaa0a53: Can remove unused kwargs?
    fjahr commented at 11:03 am on November 13, 2024:
    done
  70. in test/functional/test_framework/util.py:286 in b2ac698ce1 outdated 
    281+        check_interval = duration
282+    time_end = time.time() + duration
283+    predicate_source = "''''\n" + inspect.getsource(f) + "'''"
284+    while True:
285+        if not f():
286+            raise AssertionError("Predicate {} became false within {} seconds".format(predicate_source, duration))
    maflcko commented at 8:26 am on November 11, 2024:
    nit in b2ac698ce102edaf8039f42b8793ca10deaa0a53: Can use f-strings?
    fjahr commented at 11:03 am on November 13, 2024:
    done
  71. in test/functional/wallet_encryption.py:57 in 8362ec6ebb outdated 
    53@@ -53,8 +54,7 @@ def run_test(self):
54         assert self.nodes[0].verifymessage(address, sig, msg)
55 
56         # Check that the timeout is right
57-        time.sleep(3)
58-        assert_raises_rpc_error(-13, "Please enter the wallet passphrase with walletpassphrase first", self.nodes[0].signmessage, address, msg)
59+        self.nodes[0].wait_until(lambda: try_rpc(-13, "Please enter the wallet passphrase with walletpassphrase first", self.nodes[0].signmessage, address, msg), timeout=3, check_interval=0.5)
    maflcko commented at 8:35 am on November 11, 2024:
    8362ec6ebbe2410c551c4d6550438426c425ce3c: I am not sure this is accurate. The time can now be scaled, but in reality it is really a fixed time.
    fjahr commented at 1:11 am on November 12, 2024:
    What do you mean by “accurate”? Is the concern that someone scales this by <0.5 and then the test might fail?
    maflcko commented at 5:26 pm on November 12, 2024:

    Yeah, looking at it again, this doesn’t make sense because the test may fail if someone actually scales the wait time down. I changed it back and will leave the mocktime approach for a follow-up.

    Source: #30893 (review)

    fjahr commented at 11:03 am on November 13, 2024:
    reverted it
  72. maflcko commented at 8:39 am on November 11, 2024: member

    almost ack dde84e2b2b6a64f5f67d4dff6e3fb2fc7d2c5a77 🚿

    Signature:

    0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: almost ack dde84e2b2b6a64f5f67d4dff6e3fb2fc7d2c5a77 🚿
3nzWmeb5moWyuyuQLS3fgHHElryIVOHlOhCoY+A+Ny0e4Eo+J09cUfv98NBfJtN3Huhk9sqVRYf7s8+PEHS/zCw==
  73. test: Introduce ensure_for helper 16c87d91fd
  74. test: Add check_interval parameter to wait_until 
    This also replaces two sleep calls in functional tests with wait_until
    5468a23eb9
  75. test: Remove unused attempts parameter from wait_until 111465d72d
  76. fjahr force-pushed on Nov 13, 2024
  77. fjahr commented at 11:06 am on November 13, 2024: contributor
    Addressed latest feedback from @maflcko
  78. maflcko commented at 3:43 pm on November 13, 2024: member

    re-ACK 111465d72dd35e42361fc2a089036f652417ed37 🍋

    Signature:

    0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: re-ACK 111465d72dd35e42361fc2a089036f652417ed37 🍋
3ynbpyXRp6ASf5EA21q9pVCnfI2uyNU3+D86HmBtstsppUQZRtGEv9GKT65LJbkhI8wwIbV5w+cVm4VGr0SnsAQ==
  79. DrahtBot requested review from tdb3 on Nov 13, 2024
  80. tdb3 approved
  81. tdb3 commented at 9:11 pm on November 15, 2024: contributor
    code review re ACK 111465d72dd35e42361fc2a089036f652417ed37
  82. furszy commented at 8:43 pm on November 18, 2024: member
    utACK 111465d72dd35e42361fc2a089036f652417ed37
  83. achow101 commented at 6:41 pm on November 19, 2024: member
    ACK 111465d72dd35e42361fc2a089036f652417ed37
  84. achow101 merged this on Nov 19, 2024
  85. achow101 closed this on Nov 19, 2024


fjahr DrahtBot jonatack maflcko theStack BrandonOdiwuor pablomartin4btc furszy tdb3 achow101


jonatack BrandonOdiwuor pablomartin4btc

Labels
Tests

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-21 09:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me