test: add addr-fetch peer connection state and timeout coverage #22568

pull jonatack wants to merge 2 commits into bitcoin:master from jonatack:p2p_addrfetch_test_enhancements changing 1 files +25 −8

jonatack commented at 4:22 PM on July 28, 2021: member

This patch adds additional addr-fetch peer connection state and timeout coverage as a follow-up to #22096.
test: add assert_getpeerinfo method and coverage in p2p_addrfetch.py 9321086af7
test: add addr-fetch timeout connection coverage in p2p_addrfetch.py f8d8eb5fda
DrahtBot added the label Tests on Jul 28, 2021
ShubhamPalriwala commented at 4:33 AM on August 10, 2021: contributor

tAck f8d8eb5fdaa93b6e5b77fd901b94927dc3a0473e and 9321086af7b75be536767d25abef4d7e02ca416a

Anything that increases Test Coverage is a yayy!

Tested on Ubuntu 21.04
mzumsande commented at 11:08 PM on August 15, 2021: member

Code review ACK f8d8eb5fdaa93b6e5b77fd901b94927dc3a0473e
fanquake requested review from amitiuttarwar on Aug 16, 2021
in test/functional/p2p_addrfetch.py:28 in f8d8eb5fda
```
  20 | @@ -21,18 +21,24 @@
  21 |  
  22 |  
  23 |  class P2PAddrFetch(BitcoinTestFramework):
  24 | -
  25 |      def set_test_params(self):
  26 |          self.setup_clean_chain = True
  27 |          self.num_nodes = 1
  28 |  
  29 | +    def assert_getpeerinfo(self, *, peer_ids):
```
amitiuttarwar commented at 5:35 AM on August 16, 2021:

why did you opt for having peer_ids be an array when no call sites pass in >1 id?

jonatack commented at 10:19 AM on August 17, 2021:

Right, the idea was that if a caller wants to pass more than one peer id, the helper function and all the existing callers don't need to be refactored.

amitiuttarwar commented at 3:56 PM on August 17, 2021:

hm, since this is a helper within a specific test, this feels like a premature optimization to me- I would prefer simplifying the logic & increasing readability of the test.

jonatack commented at 4:05 PM on August 17, 2021:

If it decreases the cost of future change at essentially no cost, it's good design. Leaving as it is and resolving.
in test/functional/p2p_addrfetch.py:75 in f8d8eb5fda
```
  73 | +        self.assert_getpeerinfo(peer_ids=[peer_id])
  74 | +
  75 | +        # Expect addr-fetch peer connection to be disconnected after 5 minutes.
  76 | +        node.setmocktime(time_now + 301)
  77 |          peer.wait_for_disconnect(timeout=5)
  78 | +        self.assert_getpeerinfo(peer_ids=[])
```
amitiuttarwar commented at 5:38 AM on August 16, 2021:

I don't think this line is adding much value since wait_for_disconnect would fail if the peer was still connected, but if you want to keep it you could consider updating to:

assert_equal(len(node.getpeerinfo()), 0)

since that's the only check that would run with an empty array for peer_ids
jonatack commented at 10:20 AM on August 17, 2021:

The idea was for it to work with only a change to the value of the peer_ids argument if the number of peers changes.

jonatack commented at 10:21 AM on August 17, 2021:

(Thanks for having a look!)

amitiuttarwar commented at 3:57 PM on August 17, 2021:

same as above (https://github.com/bitcoin/bitcoin/pull/22568#discussion_r690508132), I'd prefer if this was the simplest version that captured current expectations.

jonatack commented at 4:09 PM on August 17, 2021:

For the essentially the same amount of code, this suggestion could increase the cost of future change (and is also less symmetric with the other changes) so it doesn't seem worth updating for. Prefer to leave as-is.
Saviour1001 commented at 2:50 PM on August 19, 2021: none

Tested ACK <code>f8d8eb5</code>
MarcoFalke merged this on Aug 19, 2021
MarcoFalke closed this on Aug 19, 2021
jonatack deleted the branch on Aug 19, 2021
sidhujag referenced this in commit 7d2caa8553 on Aug 20, 2021
Fabcien referenced this in commit 1b081107e0 on Jan 28, 2022
DrahtBot locked this on Aug 19, 2022

Contributors

Labels

Linked (view graph)

#22096 p2p: AddrFetch - don't disconnect on self-announcements