[test] Add test for getaddednodeinfo #10224

jimmysong commented at 1:47 AM on April 18, 2017: contributor

Test adds a node and sees if it's in the getaddednodeinfo call

jonasschnelli commented at 8:09 AM on April 18, 2017: contributor

Instead of creating a new test (which slightly slows down the CI/test process), you could add this to an existing test?

MarcoFalke commented at 8:22 AM on April 18, 2017: member

Agree, we already have two scripts [1] for net.cpp tests. Choose one of them.

fanquake added the label Tests on Apr 18, 2017

laanwj commented at 12:46 PM on April 18, 2017: member

Instead of creating a new test (which slightly slows down the CI/test process), you could add this to an existing test?

Yes it's sometimes hard to decide. On one hand, a small test granularity is better to quickly find specific problems based on what fails, on the other hand grouping tests together is better for performance.

Thanks for helping add tests anyhow!

jnewbery commented at 1:31 PM on April 18, 2017: member

On one hand, a small test granularity is better to quickly find specific problems based on what fails

This can also be achieved by structuring the sub-tests intelligently and making sure there are no dependencies between them (ie trying to avoid anything that carries state between the sub-tests), and adding plenty of logging so it's immediately obvious where the test failed.

+1 for adding this to an existing test script. Even better (in my opinion) if nodehandling.py and net.py can be merged into a single test script, but that's possibly more controversial.

jimmysong commented at 2:14 PM on April 18, 2017: contributor

@laanwj @jonasschnelli @jnewbery @MarcoFalke Thanks for the feedback. I'll put this into another test.

jimmysong force-pushed on Apr 18, 2017

jimmysong commented at 2:33 PM on April 18, 2017: contributor

Test was put into net.py. Please let me know if there's anything else I need to do.

in test/functional/net.py:16 in de73334ec8 outdated

  12 | @@ -13,9 +13,7 @@
  13 |  from test_framework.test_framework import BitcoinTestFramework
  14 |  from test_framework.authproxy import JSONRPCException
  15 |  from test_framework.util import (
  16 | -    assert_equal,
  17 | -    start_nodes,
  18 | -    connect_nodes_bi,
  19 | +    assert_equal, connect_nodes_bi, p2p_port, start_nodes,

MarcoFalke commented at 3:01 PM on April 18, 2017:

git solves conflicts on a line basis, not word basis, so I'd prefer to keep those in separate lines to make it easier to solve merge conflicts (in case they pop up).

jimmysong commented at 3:07 PM on April 18, 2017:

Fixed.

jimmysong commented at 3:33 PM on April 18, 2017: contributor

@MarcoFalke Fixed the code review item.

in test/functional/net.py:56 in 61d949fa30 outdated

  49 | @@ -49,6 +50,15 @@ def run_test(self):
  50 |          assert_equal(self.nodes[0].getnetworkinfo()['networkactive'], True)
  51 |          assert_equal(self.nodes[0].getnetworkinfo()['connections'], 2)
  52 |  
  53 | +        # test getaddednodeinfo
  54 | +        # add a node (node2) to node0
  55 | +        ip_port = "127.0.0.1:{}".format(p2p_port(2))
  56 | +        peers = self.nodes[0].addnode(ip_port, 'add')

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

nit: no need to assign this unused peers variable.

I recommend you run flake8 over PRs to catch nits like this.

jimmysong commented at 5:02 PM on April 18, 2017:

Good idea. Let me fix this and all the stuff you mentioned.

jnewbery approved

jnewbery commented at 4:14 PM on April 18, 2017: member

Tested ACK 61d949fa308f556aca0dda45eb38196164dfdbc5. It'd be great if you could add test cases for:

running getaddednodeinfo before any nodes are added and asserting that an empty array is returned
running getaddednodeinfo with an address that isn't an added node and asserting that bitcoind raises a JSONRPC error (use assert_raises_jsonrpc())

Even without those tests, this improves coverage, so ACK.

jimmysong commented at 5:54 PM on April 18, 2017: contributor

@jnewbery changed per your suggestions.

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

Tested ACK https://github.com/bitcoin/bitcoin/pull/10224/commits/7c8babebf24bc5b1000035f2c9d1fc73d40a5a15. Please squash commits before merge.

Tests: Add simple test for getaddednodeinfo

* net.py test adds a node and sees if it's in the getaddednodeinfo call
* flake8 fixes

bc53752616

jimmysong force-pushed on Apr 20, 2017

jimmysong commented at 6:30 PM on April 20, 2017: contributor

squashed and rebased

jimmysong renamed this:
~~Tests: Add simple test for getaddednodeinfo~~
[test] Add test for getaddednodeinfo
on Apr 22, 2017

MarcoFalke commented at 3:03 PM on April 23, 2017: member

utACK bc53752

in test/functional/net.py:63 in bc53752616

  58 | +        added_nodes = self.nodes[0].getaddednodeinfo(ip_port)
  59 | +        assert_equal(len(added_nodes), 1)
  60 | +        assert_equal(added_nodes[0]['addednode'], ip_port)
  61 | +        # check that a non-existant node returns an error
  62 | +        assert_raises_jsonrpc(-24, "Node has not been added",
  63 | +                              self.nodes[0].getaddednodeinfo, '1.1.1.1')

MarcoFalke commented at 3:03 PM on April 23, 2017:

nit: No need to limit lines at 80 chars because some tool tells you so. We don't have a strict guideline about the maximum length, so feel free to use any length that makes sense. 120 might be a rough rule of thumb.

MarcoFalke merged this on Apr 23, 2017

MarcoFalke closed this on Apr 23, 2017

MarcoFalke referenced this in commit 2723bcdce3 on Apr 23, 2017

PastaPastaPasta referenced this in commit 4d3b8bd142 on Jun 14, 2019

PastaPastaPasta referenced this in commit 64fad719e3 on Jun 14, 2019

PastaPastaPasta referenced this in commit b124581c64 on Jun 14, 2019

PastaPastaPasta referenced this in commit 4daceb9fee on Jun 14, 2019

barrystyle referenced this in commit fd6796192e on Jan 22, 2020

MarcoFalke locked this on Sep 8, 2021