qa: Remove need to handle the network thread in tests

MarcoFalke commented at 1:26 pm on June 21, 2018: member

This simplifies test writing by removing the need to handle the network thread in tests. E.g. start thread, join thread, restart thread mid-test, adding p2p connections at the “right” time, …

MarcoFalke added the label Tests on Jun 21, 2018

MarcoFalke commented at 1:38 pm on June 21, 2018: member

Please review the refactoring in #13512 first.

jnewbery commented at 2:47 pm on June 21, 2018: member

Big concept ACK!

feature_block.py currently failing on Travis.

in test/functional/README.md:74 in 563faf7d50 outdated

70@@ -71,23 +71,23 @@ P2P messages. These can be found in the following source files:
71 
72 #### Using the P2P interface
73 
74+- Set `setup_mininode_event_loop = True` in `set_test_params()` to spawn a thread

jnewbery commented at 2:10 pm on June 22, 2018:

Would it be possible to start the network thread for all tests, so it’s not necessary for the test writer to even perform this step?

MarcoFalke commented at 2:16 pm on June 22, 2018:

Good point. I guess there is no overhead in running an event loop with no events.

in test/functional/README.md:83 in 563faf7d50 outdated

79 over the network (`CBlock`, `CTransaction`, etc, along with the network-level
80 wrappers for them, `msg_block`, `msg_tx`, etc).
81 
82 - P2P tests have two threads. One thread handles all network communication
83-with the bitcoind(s) being tested (using python's asyncore package); the other
84+with the bitcoind(s) being tested; the other

jnewbery commented at 2:10 pm on June 22, 2018:

nit: Join lines

MarcoFalke commented at 2:17 pm on June 22, 2018:

md renderer will take care of this ;)

in test/functional/feature_nulldummy.py:18 in 563faf7d50 outdated

14@@ -15,7 +15,7 @@
15 
16 from test_framework.test_framework import BitcoinTestFramework
17 from test_framework.util import *
18-from test_framework.mininode import CTransaction, network_thread_start
19+from test_framework.mininode import CTransaction

jnewbery commented at 2:11 pm on June 22, 2018:

Can be changed to from test_framework.messages import CTransaction :)

MarcoFalke commented at 2:16 pm on June 22, 2018:

;)

in test/functional/test_framework/mininode.py:16 in 563faf7d50 outdated

12@@ -13,11 +13,10 @@
13 P2PInterface: A high-level interface object for communicating to a node over P2P
14 P2PDataStore: A p2p interface class that keeps a store of transactions and blocks
15               and can respond correctly to getdata and getheaders messages"""
16-import asyncore
17+import asyncio

jnewbery commented at 2:12 pm on June 22, 2018:

:tada:

in test/functional/rpc_blockchain.py:43 in 563faf7d50 outdated

40@@ -41,14 +41,14 @@
41 )
42 from test_framework.mininode import (
43     P2PInterface,

jnewbery commented at 2:13 pm on June 22, 2018:

nit: Can join to a single line

jnewbery commented at 2:13 pm on June 22, 2018: member

Cursory review. A few nits inline.

Will do full review once #13512 is merged.

MarcoFalke force-pushed on Jun 24, 2018

MarcoFalke commented at 6:30 pm on June 24, 2018: member

aaaaddressed @jnewbery feedback in aaaa4a4b17506f526bebee65b0ea930289f79016

in test/functional/feature_assumevalid.py:161 in aaaa4a4b17 outdated

157@@ -160,9 +158,8 @@ def run_test(self):
158             self.block_time += 1
159             height += 1
160 
161-        # We're adding new connections so terminate the network thread
162+        # We're adding new connections and terminate the old ones

jnewbery commented at 4:11 pm on June 25, 2018:

nit: just remove this comment entirely. It adds no information to the code.

in test/functional/test_framework/mininode.py:80 in aaaa4a4b17 outdated

83+        self._transport = None
84 
85     @property
86     def is_connected(self):
87-        return self._conn_open
88+        return bool(self._transport)

jnewbery commented at 4:13 pm on June 25, 2018:

nit: I think return self._transport is not None is preferable

in test/functional/test_framework/mininode.py:387 in aaaa4a4b17 outdated

391     def __init__(self):
392         super().__init__(name="NetworkThread")
393+        # There is only one event loop and no more than one thread must be created
394+        assert not self._network_event_loop
395+
396+        NetworkThread._network_event_loop = asyncio.new_event_loop()

jnewbery commented at 4:14 pm on June 25, 2018:

This is accessed outside the class, so it shouldn’t be marked as private. Prefer network_event_loop

in test/functional/test_framework/mininode.py:103 in aaaa4a4b17 outdated

128-            self._asyncore_pre_connection = False
129-            self.on_open()
130-
131-    def handle_close(self):
132-        """asyncore callback when a connection is closed."""
133+    def connection_made(self, _transport):

jnewbery commented at 4:15 pm on June 25, 2018:

nit: prefer transport as the argument name

in test/functional/test_framework/mininode.py:117 in aaaa4a4b17 outdated

142+
143+    def connection_lost(self, exc):
144+        """asyncio callback when a connection is closed."""
145+        if exc:
146+            logger.warning("Connection lost to {}:{} due to {}".format(self.dstaddr, self.dstport, exc))
147         logger.debug("Closing connection to: %s:%d" % (self.dstaddr, self.dstport))

jnewbery commented at 4:16 pm on June 25, 2018:

nit: this should be in an else branch to avoid log repetition

in test/functional/test_framework/test_framework.py:35 in aaaa4a4b17 outdated

31@@ -32,6 +32,8 @@
32     sync_blocks,
33     sync_mempools,
34 )
35+from .mininode import NetworkThread

jnewbery commented at 4:20 pm on June 25, 2018:

nit: sort imports

jnewbery commented at 5:32 pm on June 25, 2018: member

Tested ACK aaaa4a4b17506f526bebee65b0ea930289f79016.

Looks great Marco. Thanks for doing this. A few nits inline - feel free to ignore.

qa: Avoid start/stop of the network thread mid-test fa87da2f17

MarcoFalke force-pushed on Jun 25, 2018

MarcoFalke commented at 6:14 pm on June 25, 2018: member

Force pushed to address feedback

jnewbery commented at 6:47 pm on June 25, 2018: member

Tested ACK fa87da2f172ae2e6dc15e9ed156a3564a8ecfbdd

DrahtBot commented at 11:19 am on June 26, 2018: member

#13467 ([Tests] Make p2p_segwit easier to debug by jnewbery)
#13054 (tests: Enable automatic detection of undefined names in Python tests scripts. Remove wildcard imports. by practicalswift)

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.

laanwj commented at 4:10 pm on June 29, 2018: member

utACK fa87da2f172ae2e6dc15e9ed156a3564a8ecfbdd

laanwj merged this on Jun 29, 2018

laanwj closed this on Jun 29, 2018

laanwj referenced this in commit a6ed99a1e6 on Jun 29, 2018

MarcoFalke deleted the branch on Jun 30, 2018

random-zebra referenced this in commit bffe509aed on Jun 28, 2021

UdjinM6 referenced this in commit 0e016eb41d on Jun 29, 2021

UdjinM6 referenced this in commit ef4cb96b6a on Jun 29, 2021

UdjinM6 referenced this in commit cf48b073a6 on Jul 1, 2021

UdjinM6 referenced this in commit 1e65e39aed on Jul 2, 2021

UdjinM6 referenced this in commit c02337e1b0 on Jul 2, 2021

MarcoFalke locked this on Sep 8, 2021

qa: Remove need to handle the network thread in tests #13517