[tests] Split NodeConn from NodeConnCB #11712

jnewbery · 2017-11-17T21:47:33Z

This is the final step in #11518, except for possibly renaming - for motivation, please see that PR.

If this is merged, then migrating the test framework from asyncore to asyncio should be easier (I say should because I haven't dug too deeply into what would be required).

Requesting review from @ryanofsky , since he always has good feedback on these refactor PRs, and I'd appreciate his take on this refactor. Note particularly that I've reverted the change suggested here: #11182 (comment) . The idea, as always, is to present a simple interface to the test writer.

ryanofsky

This basically looks good, but I really think it's important to stop creating peer objects via callback in add_p2p_connection. The change I think you should make is here a683588 (previously described in #11182 (comment)).

Advantages of this change:

Simpler control flow. Tests create their own peer objects. You can see when and where they get constructed and what arguments they are constructed with. No need to pass around callbacks. There's no advantage I can see to passing around callbacks unless saving two () characters is an advantage.
Less coupling and repetition. TestNode no longer needs need to know anything about peer objects other than that they have connect and disconnect methods. The connect arguments (dstaddr, dstport, net="regtest", services=NODE_NETWORK, send_version=True) are defined and used one place instead of repeated 12 times in testnode, mininode, and comptool code, and in individual test case code.
Lets you drop second commit, reduces overall diff from 502 lines to 457 lines.

ryanofsky · 2017-11-21T19:25:53Z

test/functional/p2p-segwit.py

+    - Submit the block over the p2p interface
+    - use the getbestblockhash rpc to check for acceptance."""
+    if with_witness:
+        node.p2p.send_message(msg_witness_block(block))


In commit "[tests] Remove rpc property from TestNode in p2p-segwit.py"

Did you mean to s/node.p2p/p2p/ here and below?

ryanofsky · 2017-11-21T19:30:04Z

test/functional/p2p-segwit.py

+    else:
+        node.p2p.send_message(msg_block(block))
+    p2p.sync_with_ping()
+    assert_equal(node.rpc.getbestblockhash() == block.hash, accepted)


In commit "[tests] Remove rpc property from TestNode in p2p-segwit.py"

Do you want to s/node.rpc/node/ here for consistency with test_transaction_acceptance? Another alternative which would also be nice would be to pass these functions just rpc objects instead of nodes.

ryanofsky · 2017-11-21T19:36:12Z

test/functional/p2p-segwit.py

-        self.std_node.test_transaction_acceptance(p2sh_txs[3], True, False, b'bad-witness-nonstandard')
-        self.test_node.test_transaction_acceptance(p2sh_txs[3], True, True)
+        test_transaction_acceptance(self.nodes[1], self.std_node, p2sh_txs[3], True, False, b'bad-witness-nonstandard')
+        test_transaction_acceptance(self.nodes[0], self.test_node, p2sh_txs[3], True, True)


[tests] Remove rpc property from TestNode in p2p-segwit.py

Not a comment on this PR but this whole test file is very repetitive. With a few lambdas / local helper functions it could probably be made a lot shorter and more readable.

ryanofsky · 2017-11-21T19:48:13Z

test/functional/test_framework/test_node.py

        self.wait_until_stopped()

-    def add_p2p_connection(self, p2p_conn, **kwargs):
+    def add_p2p_connection(self, p2p_conn_type, **kwargs):


In commit "[tests] instantiate NodeConnCB in TestNode"

Can similary control flow and revert this whole commit, see main comment.

ryanofsky · 2017-11-21T21:17:46Z

test/functional/example_test.py

 # override the on_*() methods if you need custom behaviour.
 class BaseNode(NodeConnCB):
-    def __init__(self):
+    def __init__(self, dstaddr, dstport, net="regtest", services=NODE_NETWORK, send_version=True):


In commit "[tests] Make NodeConnCB a subclass of NodeConn"

Can get rid of all these default parameters repeated so many times. See main comment.

Change the helper methods to functions which take a node and a p2p connection as arguments.

jnewbery · 2017-11-23T15:37:13Z

Thanks Russ. I much prefer your suggested way of passing the constructed NodeConnCB object to TestNode. I've incorporated your change and split the PR into smaller commits to aid reviewing.

maflcko

Concept ACK a328ab35888baaa481b5b59e41472f91e8802f8e. Going to review this week or so.

maflcko · 2017-11-27T15:50:42Z

test/functional/test_framework/test_node.py

stylte-nit: Any reason for this change? I think the previous is easier to read python code.

There's some discussion of the differences here: https://stackoverflow.com/questions/850795/different-ways-of-clearing-lists

If there was a different reference to this array, then self.p2ps = [] wouldn't clear that array, it would just change self.p2ps to point to a new, empty array.

Practically, there's not much difference, but I think I prefer del self.p2ps[:]. I actually got this from @ryanofsky's commit - perhaps he had a specific reason to make this change?

Ok, fine to keep, since it is ever-so-slightly safer and already used by this syntax in stop_node

Main reason for suggesting this is that having multiple instances of the same list can make things confusing when you are debugging.

ryanofsky

utACK a328ab35888baaa481b5b59e41472f91e8802f8e

ryanofsky · 2017-11-27T22:35:43Z

test/functional/test_framework/mininode.py

In commit "[tests] Tidy up mininode"

Think you missed the readble() function above. Would be good to note that is an asyncore callback like writeable().

We can actually just remove the readable() override entirely. From https://docs.python.org/3.6/library/asyncore.html#asyncore.dispatcher.readable : "The default method simply returns True, indicating that by default, all channels will be interested in read events." so this override isn't actually doing anything.

I'll update the commit.

ryanofsky · 2017-11-27T22:42:01Z

test/functional/test_framework/mininode.py

In commit "[tests] Tidy up mininode"

Seems like this method should begin with an underscore (like _log_message below), since this is part of the read handler, and not something that should be called externally. I also think a name like _got_data _on_data _handle_data _process_messages might be more descriptive since read_message suggests a method that blocks waiting for a message as opposed to a method handling incoming data.

Agree. Changed to _on_data.

ryanofsky · 2017-11-27T22:44:47Z

test/functional/test_framework/mininode.py

In commit "[tests] Tidy up mininode"

Would change "a message" to "messages" everywhere since this can parse more than just one message.

Yes. Fixed.

ryanofsky · 2017-11-27T23:06:08Z

test/functional/test_framework/mininode.py

In commit "[tests] Make NodeConnCB a subclass of NodeConn"

Maybe keep assigning nServices/messages_count/last_message/ping_counter members that don't depend on connection information in __init__ rather than peer_connect. This would mean you don't need to introduce a NodeConnCB.peer_connect method in this commit, and in the next commit it can be a shorter, more focused override. Just a suggestion, though, difference is minor.

Agree. Makes more sense. Thanks.

Add docstrings and renames some methods. Also removes the redundant NodeConn.readable() method override.

This is required since NodeConnCB will inherit from NodeConn after the next commit.

This makes NodeConnCB a subclass of NodeConn, and removes the need for the client code to know anything about the implementation details of NodeConnCB. NodeConn can now be swapped out for any other implementation of a low-level connection without changing client code.

This commit moves the logic that sends a version message on connection from NodeConn to NodeConnCB. NodeConn should not be aware of the semantics or meaning of the P2P payloads.

jnewbery · 2017-11-28T17:48:24Z

Addressed @ryanofsky's comments

ryanofsky

utACK e9dfa9b. Only changes since last review were the various things commented above.

maflcko

utACK e9dfa9b

maflcko · 2017-11-29T19:14:19Z

test/functional/test_framework/test_node.py

        self.wait_until_stopped()

-    def add_p2p_connection(self, p2p_conn, **kwargs):
+    def add_p2p_connection(self, p2p_conn, *args, **kwargs):


nit: I'd rather do the opposite and disallow passing through unnamed args here. Forcing code to be explicit should be safer and ease future review, imo.

def add_p2p_connection(self, p2p_conn, *, **kwargs)

If the goal is to disallow unnamed arguments, this place to do is where the arguments are actually declared (in the peer_connect method). If you make that change here instead of there, you will be introducing an inconsistency between add_p2p_connection and peer_connect. It is better for testnode to just forward arguments on to peernode, instead making unnecessary assumptions about how it works internally and creating unnecessary differences. Forwarding *args and **kwargs is also idiomatic in python while forwarding just **kwargs is more unusual.

I think @ryanofsky is right - the right place for this is in NodeConnCB.peer_connect(). Can we leave that for a future PR?

@jnewbery Sure, future PR sounds good.

@ryanofsky

e9dfa9b [tests] Move version message sending from NodeConn to NodeConnCB (John Newbery) dad596f [tests] Make NodeConnCB a subclass of NodeConn (John Newbery) e30d404 [tests] Move only: move NodeConnCB below NodeConn (John Newbery) 4d50598 [tests] Tidy up mininode (John Newbery) f2ae6f3 [tests] Remove mininode periodic (half-hour) ping messages (John Newbery) ec59523 [tests] Remove rpc property from TestNode in p2p-segwit.py. (John Newbery) Pull request description: This is the final step in #11518, except for possibly renaming - for motivation, please see that PR. If this is merged, then migrating the test framework from asyncore to asyncio should be easier (I say should because I haven't dug too deeply into what would be required). Requesting review from @ryanofsky , since he always has good feedback on these refactor PRs, and I'd appreciate his take on this refactor. Note particularly that I've reverted the change suggested here: #11182 (comment) . The idea, as always, is to present a simple interface to the test writer. Tree-SHA512: 94dd467a13ec799b101108cf47d4dccb6f6240b601e375e3d785313333bbb389c26072a50759aca663bbf3d6c8b867b99e36ae8800ab8ea115e0496c151926ce

* [tests] Remove mininode periodic (half-hour) ping messages * [tests] Tidy up mininode Add docstrings and renames some methods. Also removes the redundant NodeConn.readable() method override. * [tests] Move only: move NodeConnCB below NodeConn This is required since NodeConnCB will inherit from NodeConn after the next commit. * [tests] Make NodeConnCB a subclass of NodeConn This makes NodeConnCB a subclass of NodeConn, and removes the need for the client code to know anything about the implementation details of NodeConnCB. NodeConn can now be swapped out for any other implementation of a low-level connection without changing client code. * [tests] Move version message sending from NodeConn to NodeConnCB This commit moves the logic that sends a version message on connection from NodeConn to NodeConnCB. NodeConn should not be aware of the semantics or meaning of the P2P payloads. * remove witness Signed-off-by: Pasta <[email protected]> * Fix 11712 Co-authored-by: John Newbery <[email protected]> Co-authored-by: UdjinM6 <[email protected]>

…#3373) * [tests] Remove mininode periodic (half-hour) ping messages * [tests] Tidy up mininode Add docstrings and renames some methods. Also removes the redundant NodeConn.readable() method override. * [tests] Move only: move NodeConnCB below NodeConn This is required since NodeConnCB will inherit from NodeConn after the next commit. * [tests] Make NodeConnCB a subclass of NodeConn This makes NodeConnCB a subclass of NodeConn, and removes the need for the client code to know anything about the implementation details of NodeConnCB. NodeConn can now be swapped out for any other implementation of a low-level connection without changing client code. * [tests] Move version message sending from NodeConn to NodeConnCB This commit moves the logic that sends a version message on connection from NodeConn to NodeConnCB. NodeConn should not be aware of the semantics or meaning of the P2P payloads. * remove witness Signed-off-by: Pasta <[email protected]> * Fix 11712 Co-authored-by: John Newbery <[email protected]> Co-authored-by: UdjinM6 <[email protected]>

jnewbery mentioned this pull request Nov 17, 2017

[tests] [demonstration] Simplify/clarify the NodeConn/NodeConnCB mininode classes #11518

Closed

6 tasks

fanquake added the Tests label Nov 17, 2017

ryanofsky reviewed Nov 21, 2017

View reviewed changes

[tests] Remove rpc property from TestNode in p2p-segwit.py.

ec59523

Change the helper methods to functions which take a node and a p2p connection as arguments.

jnewbery force-pushed the split_nodeconn branch from 6090ef4 to f66ff10 Compare November 22, 2017 23:11

[tests] Remove mininode periodic (half-hour) ping messages

f2ae6f3

jnewbery force-pushed the split_nodeconn branch from f66ff10 to a328ab3 Compare November 23, 2017 15:34

maflcko reviewed Nov 27, 2017

View reviewed changes

maflcko self-assigned this Nov 27, 2017

ryanofsky reviewed Nov 27, 2017

View reviewed changes

jnewbery added 4 commits November 28, 2017 12:31

[tests] Tidy up mininode

4d50598

Add docstrings and renames some methods. Also removes the redundant NodeConn.readable() method override.

[tests] Move only: move NodeConnCB below NodeConn

e30d404

This is required since NodeConnCB will inherit from NodeConn after the next commit.

[tests] Move version message sending from NodeConn to NodeConnCB

e9dfa9b

This commit moves the logic that sends a version message on connection from NodeConn to NodeConnCB. NodeConn should not be aware of the semantics or meaning of the P2P payloads.

jnewbery force-pushed the split_nodeconn branch from a328ab3 to e9dfa9b Compare November 28, 2017 17:47

ryanofsky reviewed Nov 28, 2017

View reviewed changes

maflcko reviewed Nov 29, 2017

View reviewed changes

maflcko merged commit e9dfa9b into bitcoin:master Nov 29, 2017

jnewbery deleted the split_nodeconn branch November 29, 2017 22:12

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

[tests] Split NodeConn from NodeConnCB #11712

[tests] Split NodeConn from NodeConnCB #11712

Uh oh!

Conversation

jnewbery commented Nov 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Nov 23, 2017

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Nov 28, 2017

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jnewbery commented Nov 17, 2017 •

edited

Loading