[tests] Remove func test code duplication #10169

jnewbery commented at 9:40 PM on April 7, 2017: member

Test cases that use bitcoind's p2p interface use NodeConnCB or a subclass to access the p2p interface. A lot of the test cases use a subclass of NodeConnCB and implement their own callbacks or helper methods. In many cases those local implementations are duplicates.

This PR adds sensible callbacks and helper functions to the base NodeConnCB class, and removes the subclass override methods. Some tests still require custom behaviour and so need to define or override some of the class methods.

The first commit adds the sensible default callbacks and helper methods to NodeConnCB. The second commit removes any duplicate implementations in the individual testcsases.

Net code change is ~ minus 300 lines

fanquake added the label Tests on Apr 7, 2017

in test/functional/test_framework/mininode.py:1593 in e74cf5028b outdated

1591 | +        test_function = lambda: not self.connected
1592 | +        assert wait_until(test_function, timeout=timeout)
1593 | +
1594 | +    # Message receiving helper methods
1595 | +
1596 | +    def sync(self, test_function, timeout=60):

jimmysong commented at 7:11 PM on April 17, 2017:

This function seems to duplicate a lot of what wait_until above does. Maybe it makes sense to do something like:

def sync(self, test_function, timeout=60):
    if not wait_until(test_function, timeout=timeout):
        AssertionError("Sync failed to complete")

in test/functional/test_framework/mininode.py:1638 in e74cf5028b outdated

1640 | -        def received_pong():
1641 | -            return (self.last_pong.nonce == self.ping_counter)
1642 |          self.send_message(msg_ping(nonce=self.ping_counter))
1643 | -        success = wait_until(received_pong, timeout=timeout)
1644 | +        test_function = lambda: self.last_message.get("pong") and self.last_message["pong"].nonce == self.ping_counter
1645 | +        success = wait_until(test_function, timeout = timeout)

jimmysong commented at 7:17 PM on April 17, 2017:

Also looks like it should use self.sync instead and maybe get rid of the if not success below.

in test/functional/test_framework/comptool.py:195 in e74cf5028b outdated

 191 | @@ -192,9 +192,7 @@ def disconnected():
 192 |          return wait_until(disconnected, timeout=10)
 193 |  
 194 |      def wait_for_verack(self):
 195 | -        def veracked():
 196 | -            return all(node.verack_received for node in self.test_nodes)
 197 | -        return wait_until(veracked, timeout=10)
 198 | +        [node.wait_for_verack() for node in self.test_nodes]

jimmysong commented at 7:22 PM on April 17, 2017:

Any reason not to return the value like this?

return all(node.wait_for_verack() for node in self.test_nodes)

jnewbery commented at 6:18 PM on April 18, 2017:

yes, that's much better. Changed, thanks.

in test/functional/sendheaders.py:590 in e74cf5028b outdated

 541 |              test_node.send_header_for_blocks([blocks[i%len(blocks)]])
 542 |              test_node.wait_for_getheaders(timeout=1)
 543 |  
 544 |          # Eventually this stops working.
 545 | -        with mininode_lock:
 546 | -            self.last_getheaders = None

jimmysong commented at 7:34 PM on April 17, 2017:

Any reason to get rid of this and the one 5 lines below? I expected self.last_message.pop.

jnewbery commented at 6:16 PM on April 18, 2017:

because we're never testing last_getheaders after this, so these lines are just setting a variable that never gets used.

jimmysong commented at 9:03 PM on April 18, 2017:

makes sense.

in test/functional/p2p-segwit.py:52 in e74cf5028b outdated

 107 |  
 108 |      def announce_block_and_wait_for_getdata(self, block, use_header, timeout=60):
 109 |          with mininode_lock:
 110 | -            self.last_getdata = None
 111 | -            self.last_getheaders = None
 112 | +            self.last_message.pop("getdata", None)

jimmysong commented at 7:36 PM on April 17, 2017:

how about getheaders?

jnewbery commented at 6:12 PM on April 18, 2017:

Yes. My mistake. Fixed

in test/functional/p2p-feefilter.py:75 in e74cf5028b outdated

  71 | @@ -78,7 +72,7 @@ def run_test(self):
  72 |          test_node.clear_invs()
  73 |  
  74 |          # Set a filter of 15 sat/byte
  75 | -        test_node.send_filter(15000)
  76 | +        test_node.send_and_ping(msg_feefilter(15001))

jimmysong commented at 7:39 PM on April 17, 2017:

Why did the constant change from 15000 to 15001?

jnewbery commented at 6:09 PM on April 18, 2017:

bah. My mistake. Fixed

jimmysong commented at 9:08 PM on April 18, 2017:

I don't see this yet? Maybe missing a changeset or I'm missing something?

jnewbery commented at 9:20 PM on April 18, 2017:

argh! Fixed now

in test/functional/p2p-compactblocks.py:41 in e74cf5028b outdated

  42 | -        self.last_cmpctblock = message
  43 |          self.block_announced = True
  44 | -        self.last_cmpctblock.header_and_shortids.header.calc_sha256()
  45 | -        self.set_announced_blockhashes.add(self.last_cmpctblock.header_and_shortids.header.sha256)
  46 | +        self.last_message["cmpctblock"].header_and_shortids.header.calc_sha256()
  47 | +        self.set_announced_blockhashes.add(self.last_message["cmpctblock"].header_and_shortids.header.sha256)

jimmysong commented at 7:41 PM on April 17, 2017:

Please let me know if this is inappropriate, but the object attribute set_announced_blockhashes is a bit confusing. Would changing the name to something like announced_blockhashes_set be appropriate here?

jnewbery commented at 6:08 PM on April 18, 2017:

Yes, agreed. Absolutely fine to point this out even though the variable name wasn't changed by this PR. I've gone ahead and changed it to announced_blockhashes. No need for the variable name to contain the name of the data structure.

in test/functional/p2p-compactblocks.py:190 in e74cf5028b outdated

 186 | @@ -214,14 +187,14 @@ def check_announcement_of_new_block(node, peer, predicate):
 187 |              with mininode_lock:
 188 |                  assert predicate(peer), (
 189 |                      "block_hash={!r}, cmpctblock={!r}, inv={!r}".format(
 190 | -                        block_hash, peer.last_cmpctblock, peer.last_inv))
 191 | +                        block_hash, peer.last_message.get("cmpctblock", None), peer.last_message.pop("inv", None)))

jimmysong commented at 7:42 PM on April 17, 2017:

Why pop instead of get for the inv message?

jnewbery commented at 6:09 PM on April 18, 2017:

bad search-and-replace I think. It doesn't really matter, since this block would only get executed if the assert failed, in which case the test case has failed. I've fixed it anyway.

in test/functional/maxuploadtarget.py:202 in e74cf5028b outdated

 158 | -        connections = []
 159 | -
 160 | -        for i in range(3):
 161 | -            test_nodes.append(TestNode())
 162 | -            connections.append(NodeConn('127.0.0.1', p2p_port(0), self.nodes[0], test_nodes[i]))
 163 | -            test_nodes[i].add_connection(connections[i])

jimmysong commented at 7:46 PM on April 17, 2017:

Am I correct in assuming you're creating 1 node instead of 3 because the other two aren't being used?

jnewbery commented at 6:05 PM on April 18, 2017:

correct. I was altering this line anyway, so I thought I'd optimize while I was here.

jimmysong commented at 7:50 PM on April 17, 2017: contributor

ACK e74cf5028bbe6bc7fe520647860590f855eb8b3e

This is my first review, so please feel free to let me know if I'm doing something wrong. Overall, the storing of the last network messages in NodeConnCB.last_message makes everything a lot simpler. I have tested the code by running the tests locally, and everything seems to work. There are a few trivial issues that I've commented inline.

Adds helper functions to NodeConnCB

This commit adds some helper functions to NodeConnCB which are useful
for many tests:

- NodeConnCB now keeps track of the number of each message type that
it's received and the most recent message of each type. Many tests
assert on the most recent block, tx or reject message.
- NodeConnCB now keeps track of its connection state by setting a
connected boolean in on_open() and on_close()
- NodeConnCB now has wait_for_block, wait_for_getdata,
wait_for_getheaders, wait_for_inv and wait_for_verack methods

I have updated the individual test cases to make sure that there are no
namespace problems that cause them to fail with these new definitions.
Future commits will remove the duplicate code.

52e15aa4d0

jnewbery commented at 8:18 PM on April 18, 2017: member

Thanks for the excellent review @jimmysong . I've responded to most of your points. I'm just running the extended test suite locally before pushing up to github.

I've decided to remove the sync() method from NodeConnCB entirely for now, since it's just duplicating the functionality of wait_until() and doesn't actually need to be a method. I'm still not entirely happy with wait_until(), but I think it's better to not duplicate code for no good reason in this PR.

jnewbery force-pushed on Apr 18, 2017

jnewbery commented at 8:34 PM on April 18, 2017: member

rebased, review comments addressed and pushed

jimmysong commented at 9:08 PM on April 18, 2017: contributor

utACK

I'll test after the 15001 is addressed.

Remove duplicate method definitions in NodeConnCB subclasses

All Node classes in individual test cases subclass from NodeConnCB. Many
have duplicate definitions for methods that are defined in the base
class. This commit removes those duplicate definitions.

This commit removes ~290 lines of duplicate code.

2a52ae63bf

jnewbery force-pushed on Apr 18, 2017

jimmysong commented at 9:45 PM on April 18, 2017: contributor

ACK 2a52ae63bfdde948250df1c876dcdd5af99f03b5

jimmysong approved

laanwj merged this on May 2, 2017

laanwj closed this on May 2, 2017

laanwj referenced this in commit 8f3e38477e on May 2, 2017

jnewbery deleted the branch on May 2, 2017

in test/functional/test_framework/mininode.py:1606 in 2a52ae63bf

1604 | +    def wait_for_getheaders(self, timeout=60):
1605 | +        test_function = lambda: self.last_message.get("getheaders")
1606 | +        assert wait_until(test_function, timeout=timeout)
1607 | +
1608 | +    def wait_for_inv(self, expected_inv, timeout=60):
1609 | +        test_function = lambda: self.last_message.get("inv") and self.last_message["inv"] != expected_inv

MarcoFalke commented at 6:26 PM on May 2, 2017:

@jnewbery Can you explain why the second operand is != and not ==?

jnewbery commented at 8:37 PM on May 2, 2017:

This was a code move from p2p-segwit.py to here, but you're right that this is a bug.

Fix here: #10318

MarcoFalke referenced this in commit aeb3175627 on Aug 12, 2017

PastaPastaPasta referenced this in commit 94f566fba8 on Jun 10, 2019

PastaPastaPasta referenced this in commit 585e58e814 on Jun 10, 2019

PastaPastaPasta referenced this in commit a1ead91c49 on Jun 10, 2019

PastaPastaPasta referenced this in commit dcdbc3e8fd on Jun 10, 2019

PastaPastaPasta referenced this in commit 383ed29f5a on Jun 10, 2019

PastaPastaPasta referenced this in commit 17654e36f9 on Jun 10, 2019

PastaPastaPasta referenced this in commit e42f78a872 on Jun 11, 2019

PastaPastaPasta referenced this in commit 88f5607d17 on Jun 11, 2019

PastaPastaPasta referenced this in commit 1d1061570c on Jun 12, 2019

PastaPastaPasta referenced this in commit 992d34c431 on Jun 14, 2019

PastaPastaPasta referenced this in commit a3dcbcd2f8 on Jun 14, 2019

PastaPastaPasta referenced this in commit d3802bcf3b on Jun 14, 2019

PastaPastaPasta referenced this in commit 7b5744751d on Jun 14, 2019

PastaPastaPasta referenced this in commit 2d771c95a1 on Jun 14, 2019

PastaPastaPasta referenced this in commit 97b0883205 on Jun 15, 2019

PastaPastaPasta referenced this in commit 6244de1e55 on Jun 19, 2019

PastaPastaPasta referenced this in commit de30da3a55 on Aug 6, 2019

PastaPastaPasta referenced this in commit b48a5a2d8a on Aug 6, 2019

PastaPastaPasta referenced this in commit 034b6f5124 on Aug 6, 2019

PastaPastaPasta referenced this in commit ba1f724983 on Aug 7, 2019

PastaPastaPasta referenced this in commit 6e26b48aee on Aug 8, 2019

PastaPastaPasta referenced this in commit 7f3d3deda7 on Aug 12, 2019

barrystyle referenced this in commit 29c20a0aca on Jan 22, 2020

barrystyle referenced this in commit 1260977d97 on Jan 22, 2020

MarcoFalke locked this on Sep 8, 2021