test: Replace global wait_until with BitcoinTestFramework.wait_until and mininode.wait_until #19134

pull dboures wants to merge 2 commits into bitcoin:master from dboures:master changing 26 files +81 −99
  1. dboures commented at 2:50 pm on June 1, 2020: none

    Closes: #19080

    This PR creates a wait_until method for the BitcoinTestFramework class, and replaces all uses of the global wait_until with the class method. If applicable, the mininode wait_until is used instead.

  2. DrahtBot added the label Tests on Jun 1, 2020
  3. in test/functional/p2p_node_network_limited.py:17 in e4e1292711 outdated 
    15 from test_framework.util import (
16     assert_equal,
17     disconnect_nodes,
18-    connect_nodes,
19-    wait_until,
20+    connect_nodes
    MarcoFalke commented at 4:19 pm on June 1, 2020:
    Any reason to remove the trailing comma in this line? (Same in the other changes)
    dboures commented at 5:15 pm on June 1, 2020:
    No reason, I have since added back the trailing commas.
  4. in test/functional/p2p_blockfilters.py:70 in e4e1292711 outdated 
    67+        node0.wait_until(lambda: self.nodes[0].getblockcount() == 1000)
68         stale_block_hash = self.nodes[0].getblockhash(1000)
69 
70         self.nodes[1].generate(1001)
71-        wait_until(lambda: self.nodes[1].getblockcount() == 2000)
72+        node1.wait_until(lambda: self.nodes[1].getblockcount() == 2000)
    MarcoFalke commented at 4:23 pm on June 1, 2020:

    node1 is a mininode. self.nodes[1] is a TestNode.

    It might be best to add a def wait_until(self, ...): to the test framework.

    0        self.wait_until(lambda: self.nodes[1].getblockcount() == 2000)
  5. MarcoFalke commented at 4:24 pm on June 1, 2020: member
    Concept ACK
  6. dboures marked this as a draft on Jun 1, 2020
  7. glozow commented at 0:47 am on June 2, 2020: member
    Concept ACK In keeping with the goal of this PR, I also recommend changing the wait in feature_abortnode.py here to use wait_until_node_stopped so that it uses the test_node timeout factor.
  8. DrahtBot commented at 10:47 pm on June 4, 2020: member

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #19653 (wallet: Replace -zapwallettxes with zapwallettxes RPC by achow101)
    • #19631 (test: Wait for ‘cmpctblock’ in p2p_compactblocks when it is expected by Empact)
    • #19489 (test: Fail wait_until early if connection is lost by MarcoFalke)

    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.

  9. dboures renamed this:
    test: Replace global wait_until with mininode.wait_until
    test: Replace global wait_until with BitcoinTestFramework.wait_until and mininode.wait_until
    on Jun 8, 2020
  10. dboures marked this as ready for review on Jun 8, 2020
  11. in test/functional/p2p_compactblocks.py:84 in cdc0cff667 outdated 
    82-    def wait_for_block_announcement(self, block_hash, timeout=30):
83+    def wait_for_block_announcement(self, block_hash):
84         def received_hash():
85             return (block_hash in self.announced_blockhashes)
86-        wait_until(received_hash, timeout=timeout, lock=mininode_lock)
87+        self.wait_until(received_hash,)
    adamjonas commented at 2:30 pm on June 11, 2020:
    Remove , if no other arguments being passed in.
  12. in test/functional/feature_versionbits_warning.py:100 in cdc0cff667 outdated 
     97         # Check that get*info() shows the versionbits unknown rules warning
 98         assert WARN_UNKNOWN_RULES_ACTIVE in node.getmininginfo()["warnings"]
 99         assert WARN_UNKNOWN_RULES_ACTIVE in node.getnetworkinfo()["warnings"]
100         # Check that the alert file shows the versionbits unknown rules warning
101-        wait_until(lambda: self.versionbits_in_alert_file(), timeout=60)
102+        self.wait_until(lambda: self.versionbits_in_alert_file(), timeout=60)
    adamjonas commented at 2:38 pm on June 11, 2020:
    You’ve defined the default as 60 seconds, so you can drop here and other places where you specify 60.
  13. in test/functional/p2p_compactblocks.py:76 in cdc0cff667 outdated 
    72@@ -73,23 +73,23 @@ def send_header_for_blocks(self, new_blocks):
73     def request_headers_and_sync(self, locator, hashstop=0):
74         self.clear_block_announcement()
75         self.get_headers(locator, hashstop)
76-        wait_until(self.received_block_announcement, timeout=30, lock=mininode_lock)
77+        self.wait_until(self.received_block_announcement)
    adamjonas commented at 2:39 pm on June 11, 2020:
    Also re the default timeout, are you intending to change these non-60 timeouts throughout?
    dboures commented at 7:57 pm on June 20, 2020:
    No I was not. Thank you for the careful review, it’s very helpful as I am still learning!
  14. adamjonas commented at 3:17 pm on June 11, 2020: member
    Concept ACK. A few questions on when you specify and omit timeouts.
  15. MarcoFalke added the label Waiting for author on Jun 16, 2020
  16. DrahtBot added the label Needs rebase on Jun 17, 2020
  17. MarcoFalke removed the label Waiting for author on Jun 17, 2020
  18. DrahtBot removed the label Needs rebase on Jun 20, 2020
  19. DrahtBot added the label Needs rebase on Jun 21, 2020
  20. DrahtBot removed the label Needs rebase on Jun 22, 2020
  21. MarcoFalke commented at 5:29 pm on July 10, 2020: member
    Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits
  22. DrahtBot added the label Needs rebase on Jul 15, 2020
  23. jonatack commented at 12:00 pm on July 19, 2020: member
    Concept ACK. Needs squash and rebase.
  24. DrahtBot removed the label Needs rebase on Jul 19, 2020
  25. fjahr commented at 7:46 pm on July 19, 2020: member

    Concept ACK

    I think you missed test/functional/p2p_ping.py. And in your current commit order you are using the TestFramework wait_until before it is added. But I am not sure if that is viewed as critical (so nit). Otherwise lgtm.

  26. dboures marked this as a draft on Jul 19, 2020
  27. dboures closed this on Jul 19, 2020
  28. dboures reopened this on Jul 19, 2020
  29. dboures marked this as ready for review on Jul 19, 2020
  30. MarcoFalke referenced this in commit edfeaf6836 on Jul 21, 2020
  31. DrahtBot commented at 8:15 pm on July 22, 2020: member

    🐙 This pull request conflicts with the target branch and needs rebase.

  32. DrahtBot added the label Needs rebase on Jul 22, 2020
  33. test: Replace global wait_until with mininode wait_until 92edbbfd86
  34. test: Use test framework wait_until 4dd014e0d4
  35. dboures marked this as a draft on Jul 23, 2020
  36. MarcoFalke commented at 9:29 am on August 4, 2020: member
    @dboures Are you still working on this?
  37. MarcoFalke commented at 12:04 pm on August 15, 2020: member
    Going to close this as “up for grabs”. @dboures Feel free to open a new pull request once you have all the code ready for review.
  38. MarcoFalke closed this on Aug 15, 2020
  39. glozow commented at 8:35 pm on August 16, 2020: member
    Aw man :( was so excited for this! Hope you come back soon @dboures
  40. DrahtBot locked this on Feb 15, 2022


dboures MarcoFalke glozow DrahtBot adamjonas jonatack fjahr

Labels
Tests Needs rebase

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-07-05 19:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me