test: Update wait_until usage in tests not to use the one from utils

slmtpz commented at 6:19 pm on August 17, 2020: contributor

Replace global (from test_framework/util.py) wait_until() usages with the ones provided by BitcoinTestFramework and P2PInterface classes.

The motivation behind this change is that the util.wait_until() expects a timeout, timeout_factor and lock and it is not aware of the context of the test framework. BitcoinTestFramework offers a wait_until() which has an understandable amount of default timeout and a shared timeout_factor. Moreover, on top of these, mininode.wait_until() also has a shared lock.

closes #19080

DrahtBot added the label Tests on Aug 17, 2020

MarcoFalke commented at 2:26 pm on August 18, 2020: member

Very nice. Concept ACK

practicalswift commented at 5:32 pm on August 18, 2020: contributor

Nice first-time contribution! Warm welcome as a contributor @slmtpz :)

Concept ACK

slmtpz commented at 5:35 pm on August 18, 2020: contributor

Nice first-time contribution! Warm welcome as a contributor @slmtpz :)

Concept ACK

Thank you, looking forward to more contributions in future!

DrahtBot added the label Needs rebase on Aug 20, 2020

slmtpz force-pushed on Aug 20, 2020

DrahtBot removed the label Needs rebase on Aug 20, 2020

DrahtBot commented at 8:08 pm on August 20, 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:

#19700 (wallet: Replace -zapwallettxes with wallet tool command by achow101)
#19671 (wallet: Remove -zapwallettxes by achow101)

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.

in test/functional/p2p_leak.py:22 in c3e1a30741 outdated

18@@ -19,11 +19,7 @@
19 )
20 from test_framework.mininode import mininode_lock, P2PInterface
21 from test_framework.test_framework import BitcoinTestFramework
22-from test_framework.util import (

glozow commented at 2:57 am on August 21, 2020:

Style nit: I think it’s preferred to keep the list on newlines with commas at the end, otherwise we have a situation like this where the diff goes from 1 line to 6. Is there a reason you’re changing it?

slmtpz commented at 6:09 pm on August 25, 2020:

I was considering line lengths. It seemed a bit awkward to keep this structure even if there is only one import. However, I totally agree with you on separating with new lines so git history stays clean. What about the imports where this structure is not used? While I’m here, should I put them in a list?

MarcoFalke commented at 6:21 pm on August 25, 2020:

When the list is longer than one item and it was previously on multiple lines, it probably makes sense to keep it on multiple lines

in test/functional/p2p_compactblocks.py:92 in c3e1a30741 outdated

90 
91         This is used when we want to send a message into the node that we expect
92         will get us disconnected, eg an invalid block."""
93         self.send_message(message)
94-        wait_until(lambda: not self.is_connected, timeout=timeout, lock=mininode_lock)
95+        self.wait_until(lambda: not self.is_connected, timeout=timeout, check_connected=False)

glozow commented at 3:12 am on August 21, 2020:

Hm, is there a reason for removing mininode_lock and adding check_connected=False?

glozow commented at 3:33 am on August 21, 2020:

Also, wait_for_disconnect would be better.

slmtpz commented at 7:35 pm on August 25, 2020:

Used wait_for_disconnect instead, thanks!

in test/functional/p2p_sendheaders.py:176 in c3e1a30741 outdated

170@@ -174,7 +171,7 @@ def check_last_headers_announcement(self, headers):
171         """Test whether the last headers announcements received are right.
172            Headers may be announced across more than one message."""
173         test_function = lambda: (len(self.recent_headers_announced) >= len(headers))
174-        wait_until(test_function, timeout=60, lock=mininode_lock)
175+        self.wait_until(test_function)

glozow commented at 3:13 am on August 21, 2020:

why are you removing the timeout and lock here?

slmtpz commented at 6:58 pm on August 25, 2020:

This class extends P2PInterface not BitcoinTestFramework. mininode one already has a lock. As for timeout, it already has a default value of 60.

in test/functional/p2p_sendheaders.py:188 in c3e1a30741 outdated

182@@ -186,7 +183,7 @@ def check_last_inv_announcement(self, inv):
183         inv should be a list of block hashes."""
184 
185         test_function = lambda: self.block_announced
186-        wait_until(test_function, timeout=60, lock=mininode_lock)
187+        self.wait_until(test_function)

glozow commented at 3:13 am on August 21, 2020:

same

slmtpz commented at 6:59 pm on August 25, 2020:

This class extends P2PInterface not BitcoinTestFramework. mininode one already has a lock. As for timeout, it already has a default value of 60.

in test/functional/p2p_compactblocks.py:297 in c3e1a30741 outdated

293@@ -294,7 +294,7 @@ def test_compactblock_construction(self, test_node, use_witness_address=True):
294         block.rehash()
295 
296         # Wait until the block was announced (via compact blocks)
297-        wait_until(lambda: "cmpctblock" in test_node.last_message, timeout=30, lock=mininode_lock)
298+        self.wait_until(lambda: "cmpctblock" in test_node.last_message, timeout=30, lock=mininode_lock)

glozow commented at 3:26 am on August 21, 2020:

I’m not sure if this is one of the goals of this PR, but in this situation I think it’s best to use test_node.wait_until().

Same with the rest of the test.

slmtpz commented at 6:47 pm on August 25, 2020:

I was inclined to use the one from BitcoinTestFramework since I thought it would be good to keep consistency. However, in this case, since we are waiting for a change in test_node, using that one makes more sense, did I get it right? :)

slmtpz commented at 7:35 pm on August 25, 2020:

Updated.

in test/functional/wallet_resendwallettransactions.py:36 in c3e1a30741 outdated

32@@ -33,7 +33,7 @@ def run_test(self):
33         time.sleep(1.1)
34 
35         # Can take a few seconds due to transaction trickling
36-        wait_until(lambda: node.p2p.tx_invs_received[txid] >= 1, lock=mininode_lock)
37+        self.wait_until(lambda: node.p2p.tx_invs_received[txid] >= 1, lock=mininode_lock)

glozow commented at 3:30 am on August 21, 2020:

Here, wait_for_broadcast(txid) would be more appropriate

(txid needs to not be converted to int though)

slmtpz commented at 7:36 pm on August 25, 2020:

It looks cleaner now! Please have a look.

in test/functional/wallet_resendwallettransactions.py:67 in c3e1a30741 outdated

63@@ -64,7 +64,7 @@ def run_test(self):
64         # Transaction should be rebroadcast approximately 24 hours in the future,
65         # but can range from 12-36. So bump 36 hours to be sure.
66         node.setmocktime(now + 36 * 60 * 60)
67-        wait_until(lambda: node.p2ps[1].tx_invs_received[txid] >= 1, lock=mininode_lock)
68+        self.wait_until(lambda: node.p2ps[1].tx_invs_received[txid] >= 1, lock=mininode_lock)

glozow commented at 3:30 am on August 21, 2020:

same

slmtpz commented at 7:36 pm on August 25, 2020:

Updated

glozow commented at 3:37 am on August 21, 2020: member

Concept ACK, welcome! 😊 this is awesome!

I complain about this pretty often 😅 The util wait_until function doesn’t have any context from the TestFramework instance (i.e. timeout and timeout_factor). Sometimes when it’s used, the test writer forgets to pass in the mininode_lock which is even worse. I think it would be a good idea to add a comment to the wait_until function in test_framework.util that recommends using something else, because it’s almost never the best option - this is just my opinion though. (Btw, I recommend adding something along these lines to the PR description to explain what motivates these changes).

We actually have more options than just TestFramework wait_until. There are a few places where one of the mininode helper functions (wait_for_disconnect, wait_for_broadcast, etc) or the mininode wait_until would be more appropriate.

There seem to be some changes that are unrelated to changing which wait_until is used. I didn’t look into them too closely, but could you give an explanation and/or put them in a separate PR?

Also a style nit: there’s a few places where you’re refactoring the imports in addition to removing wait_until, which I don’t think is the style convention and makes the diff larger than it needs to be.

slmtpz force-pushed on Aug 25, 2020

in test/functional/rpc_invalidateblock.py:15 in 7a66c78b4d outdated

 8@@ -9,10 +9,8 @@
 9 from test_framework.util import (
10     assert_equal,
11     connect_nodes,
12-    wait_until,
13 )
14 
15-

MarcoFalke commented at 6:22 pm on August 25, 2020:

nit: accidental change

slmtpz commented at 6:25 pm on August 25, 2020:

Corrected.

slmtpz force-pushed on Aug 25, 2020

slmtpz commented at 7:40 pm on August 25, 2020: contributor

Concept ACK, welcome! 😊 this is awesome!

I complain about this pretty often 😅 The util wait_until function doesn’t have any context from the TestFramework instance (i.e. timeout and timeout_factor). Sometimes when it’s used, the test writer forgets to pass in the mininode_lock which is even worse. I think it would be a good idea to add a comment to the wait_until function in test_framework.util that recommends using something else, because it’s almost never the best option - this is just my opinion though. (Btw, I recommend adding something along these lines to the PR description to explain what motivates these changes).

We actually have more options than just TestFramework wait_until. There are a few places where one of the mininode helper functions (wait_for_disconnect, wait_for_broadcast, etc) or the mininode wait_until would be more appropriate.

There seem to be some changes that are unrelated to changing which wait_until is used. I didn’t look into them too closely, but could you give an explanation and/or put them in a separate PR?

Also a style nit: there’s a few places where you’re refactoring the imports in addition to removing wait_until, which I don’t think is the style convention and makes the diff larger than it needs to be.

Thanks for the thorough review and double thanks for your suggestions! Going deeper, things had started to build up in my mind. Rebase + force pushed the changes you suggested. As for your other proposal to add a comment in util.wait_until(), please look at this second commit. PR description is also updated. Should I update commit body as well (after your approval of the motivation for this change stated in the PR description) ?

slmtpz requested review from MarcoFalke on Aug 25, 2020

slmtpz force-pushed on Aug 25, 2020

kallewoof commented at 2:28 am on August 26, 2020: member

Big concept ACK: much cleaner!

utACK 0a2d50714f056955ac92f01346bbdc8d5c3019c7

One thing that tripped me up when reviewing is that there are two separate wait_until methods, one (in P2PInterface) which has a default lock (mininode_lock) and one (in BitcoinTestFramework) which doesn’t. This is why you see the lock=mininode_lock in a few places, while it’s removed in a bunch of others. (Check super class.)

in test/functional/example_test.py:205 in 0a2d50714f outdated

201@@ -203,7 +202,7 @@ def run_test(self):
202 
203         # wait_until() will loop until a predicate condition is met. Use it to test properties of the
204         # P2PInterface objects.
205-        wait_until(lambda: sorted(blocks) == sorted(list(self.nodes[2].p2p.block_receive_map.keys())), timeout=5, lock=mininode_lock)
206+        self.wait_until(lambda: sorted(blocks) == sorted(list(self.nodes[2].p2p.block_receive_map.keys())), timeout=5, lock=mininode_lock)

MarcoFalke commented at 6:25 am on August 26, 2020:

0        self.nodes[2].p2p.wait_until(lambda: sorted(blocks) == sorted(list(self.nodes[2].p2p.block_receive_map.keys())), timeout=5)

Shouldn’t this be

slmtpz commented at 6:38 am on August 26, 2020:

Is there a big difference between two? I was inclined to use the one from framework wherever possible.

MarcoFalke commented at 6:42 am on August 26, 2020:

It seems a bit convolved to import the lock, which needs to be taken, then call a method, which passes the imported lock to be taken.

Simply calling a member function that takes the lock when needed without importing or passing it seems less error prone, as there is no way it can be missed. Also, it makes the calling code shorter and more readable

slmtpz commented at 6:51 am on August 26, 2020:

Updated, thanks for clear explanation.

in test/functional/feature_versionbits_warning.py:93 in 0a2d50714f outdated

89@@ -91,14 +90,14 @@ def run_test(self):
90 
91         # Generating one block guarantees that we'll get out of IBD
92         node.generatetoaddress(1, node_deterministic_address)
93-        wait_until(lambda: not node.getblockchaininfo()['initialblockdownload'], timeout=10, lock=mininode_lock)
94+        self.wait_until(lambda: not node.getblockchaininfo()['initialblockdownload'], timeout=10, lock=mininode_lock)

MarcoFalke commented at 6:26 am on August 26, 2020:

0        self.wait_until(lambda: not node.getblockchaininfo()['initialblockdownload'], timeout=10)

why the lock?

slmtpz commented at 6:39 am on August 26, 2020:

The class of this method inherits from BitcoinTestFramework which does not have a default lock.

MarcoFalke commented at 6:43 am on August 26, 2020:

but why is the lock needed? Why does it need to be taken here?

slmtpz commented at 6:44 am on August 26, 2020:

I frankly don’t know. It was there before, so I didn’t touch it.

in test/functional/p2p_segwit.py:2116 in 0a2d50714f outdated

2112@@ -2114,7 +2113,7 @@ def test_wtxid_relay(self):
2113         # Check wtxidrelay feature negotiation message through connecting a new peer
2114         def received_wtxidrelay():
2115             return (len(self.wtx_node.last_wtxidrelay) > 0)
2116-        wait_until(received_wtxidrelay, timeout=60, lock=mininode_lock)
2117+        self.wait_until(received_wtxidrelay, lock=mininode_lock)

MarcoFalke commented at 6:28 am on August 26, 2020:

0        self.wtx_node.wait_until(received_wtxidrelay)