test: add coverage for `node` field of `getaddednodeinfo` RPC

brunoerg commented at 7:06 pm on June 25, 2024: contributor

We currently do not test a successful call to getaddednodeinfo filtering by node, we only test it with an unknown address and checks whether it fails. This PR adds coverage to it.

DrahtBot commented at 7:06 pm on June 25, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	kevkevinpal, tdb3, BrandonOdiwuor, rkrux

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

DrahtBot added the label Tests on Jun 25, 2024

in test/functional/rpc_net.py:254 in 13a871f8e3 outdated

248@@ -249,11 +249,17 @@ def test_addnode_getaddednodeinfo(self):
249         added_nodes = self.nodes[0].getaddednodeinfo()
250         assert_equal(len(added_nodes), 1)
251         assert_equal(added_nodes[0]['addednode'], ip_port)
252+        # check that filtering by node works
253+        self.nodes[0].addnode(node="11.22.33.44", command='add')
254+        added_node = self.nodes[0].getaddednodeinfo(node=ip_port)

tdb3 commented at 0:41 am on June 26, 2024:

nit: If this file is touched, would recommend using first_added_node or added_node_0 instead of added_node to make it clearer that the node being referred to is the one added before 11.22.33.44. Alternatively, could update the comment/log message instead.

brunoerg commented at 9:11 am on June 26, 2024:

added_node_0 seems weird because it can be confused with nodes[0], I prefer first_added_node. I will address it.

in test/functional/rpc_net.py:252 in 13a871f8e3 outdated

248@@ -249,11 +249,17 @@ def test_addnode_getaddednodeinfo(self):
249         added_nodes = self.nodes[0].getaddednodeinfo()
250         assert_equal(len(added_nodes), 1)
251         assert_equal(added_nodes[0]['addednode'], ip_port)
252+        # check that filtering by node works

tdb3 commented at 0:42 am on June 26, 2024:

nit: How about making this comment a log message instead?

https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md#general-test-writing-advice

Instead of inline comments or no test documentation at all, log the comments to the test log, e.g. self.log.info(‘Create enough transactions to fill a block’). Logs make the test code easier to read and the test logic easier to debug.

Also, since we’re touching this function, it might be reasonable to add a commit to convert the surrounding comments of this function to log messages. Any objections from others?

brunoerg commented at 9:18 am on June 26, 2024:

It seems good to me, I’ll address it.

brunoerg commented at 11:10 am on June 26, 2024:

Done.

in test/functional/rpc_net.py:255 in 13a871f8e3 outdated

248@@ -249,11 +249,17 @@ def test_addnode_getaddednodeinfo(self):
249         added_nodes = self.nodes[0].getaddednodeinfo()
250         assert_equal(len(added_nodes), 1)
251         assert_equal(added_nodes[0]['addednode'], ip_port)
252+        # check that filtering by node works
253+        self.nodes[0].addnode(node="11.22.33.44", command='add')
254+        added_node = self.nodes[0].getaddednodeinfo(node=ip_port)
255+        assert_equal(added_nodes, added_node)

tdb3 commented at 0:53 am on June 26, 2024:

Manually induced a failure here by using getaddednodeinfo(node="192.168.99.99"). Test failed as expected.

kevkevinpal commented at 4:09 am on June 26, 2024:

I’m not sure if we test for any list of added_nodes greater than a length of 1

brunoerg commented at 9:18 am on June 26, 2024:

Yes, I can add this verification.

brunoerg commented at 11:10 am on June 26, 2024:

Done.

tdb3 commented at 0:54 am on June 26, 2024: contributor

Approach ACK

Good update to exercise the filtering feature of the RPC. Left a suggestion and nit.

in test/functional/rpc_net.py:263 in 13a871f8e3 outdated

258         # check that node can be removed
259         self.nodes[0].addnode(node=ip_port, command='remove')
260-        assert_equal(self.nodes[0].getaddednodeinfo(), [])
261+        added_nodes = self.nodes[0].getaddednodeinfo()
262+        assert_equal(len(added_nodes), 1)
263+        assert_equal(added_nodes[0]['addednode'], "11.22.33.44")

kevkevinpal commented at 4:07 am on June 26, 2024:

instead of modifying the code here why not use self.nodes[0].addnode(node="11.22.33.44", command='remove') instead?

brunoerg commented at 9:12 am on June 26, 2024:

Because I wanted to remove exactly the first added node.

kevkevinpal commented at 4:12 am on June 26, 2024: contributor

Concept ACK 13a871f

ran locally and it passed for me

EarlTZ approved

test: add coverage for `node` field of `getaddednodeinfo` RPC c838e3b610

test: change comments to `self.log.info` for `test_addnode_getaddednodeinfo` e38eadb2c2

brunoerg force-pushed on Jun 26, 2024

brunoerg commented at 11:12 am on June 26, 2024: contributor

Force-pushed:

I added a commit to change some comments to self.log.info
Addressed: #30339 (review) and #30339 (review)

Thanks, @kevkevinpal and @tdb3.

kevkevinpal commented at 12:18 pm on June 26, 2024: contributor

ACK e38eadb e38eadb

DrahtBot requested review from tdb3 on Jun 26, 2024

tdb3 approved

tdb3 commented at 12:26 pm on June 26, 2024: contributor

re ACK e38eadb2c2d93d2ee3c9accb649b2de144b3732e Thanks for implementing the changes.

BrandonOdiwuor approved

BrandonOdiwuor commented at 2:47 pm on June 26, 2024: contributor

Code Review ACK e38eadb2c2d93d2ee3c9accb649b2de144b3732e

rkrux approved

rkrux commented at 9:16 am on June 27, 2024: none

tACK e38eadb

make, test/functional are successful. Thanks for testing the filtering by node functionality, and replacing the comments with info/debug logs as suggested by @tdb3.

test: add coverage for node field of getaddednodeinfo RPC #30339

Code Coverage

Reviews

test: add coverage for `node` field of `getaddednodeinfo` RPC #30339