[test] Add test for getaddednodeinfo #10224
Conversation
|
Instead of creating a new test (which slightly slows down the CI/test process), you could add this to an existing test? |
|
Agree, we already have two scripts [1] for net.cpp tests. Choose one of them. |
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! |
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. |
|
@laanwj @jonasschnelli @jnewbery @MarcoFalke Thanks for the feedback. I'll put this into another test. |
29e0c94 to
de73334
Compare
|
Test was put into net.py. Please let me know if there's anything else I need to do. |
test/functional/net.py
Outdated
| assert_equal, | ||
| start_nodes, | ||
| connect_nodes_bi, | ||
| assert_equal, connect_nodes_bi, p2p_port, start_nodes, |
There was a problem hiding this comment.
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).
|
@MarcoFalke Fixed the code review item. |
There was a problem hiding this comment.
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.
test/functional/net.py
Outdated
| # test getaddednodeinfo | ||
| # add a node (node2) to node0 | ||
| ip_port = "127.0.0.1:{}".format(p2p_port(2)) | ||
| peers = self.nodes[0].addnode(ip_port, 'add') |
There was a problem hiding this comment.
nit: no need to assign this unused peers variable.
I recommend you run flake8 over PRs to catch nits like this.
There was a problem hiding this comment.
Good idea. Let me fix this and all the stuff you mentioned.
|
@jnewbery changed per your suggestions. |
|
Tested ACK 7c8babe. Please squash commits before merge. |
* net.py test adds a node and sees if it's in the getaddednodeinfo call * flake8 fixes
7c8babe to
bc53752
Compare
|
squashed and rebased |
|
utACK bc53752 |
| assert_equal(added_nodes[0]['addednode'], ip_port) | ||
| # check that a non-existant node returns an error | ||
| assert_raises_jsonrpc(-24, "Node has not been added", | ||
| self.nodes[0].getaddednodeinfo, '1.1.1.1') |
There was a problem hiding this comment.
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.
bc53752 Tests: Add simple test for getaddednodeinfo (Jimmy Song) Tree-SHA512: d2f8a384fb994b83f881b585db70e7cf57980821074126d254960cb67988a40672ffde065d0ccd9a90f9e3b395d8cd33695c796ecb4b54f69fe74ee2bf7497a6
bc53752 Tests: Add simple test for getaddednodeinfo (Jimmy Song) Tree-SHA512: d2f8a384fb994b83f881b585db70e7cf57980821074126d254960cb67988a40672ffde065d0ccd9a90f9e3b395d8cd33695c796ecb4b54f69fe74ee2bf7497a6
bc53752 Tests: Add simple test for getaddednodeinfo (Jimmy Song) Tree-SHA512: d2f8a384fb994b83f881b585db70e7cf57980821074126d254960cb67988a40672ffde065d0ccd9a90f9e3b395d8cd33695c796ecb4b54f69fe74ee2bf7497a6
bc53752 Tests: Add simple test for getaddednodeinfo (Jimmy Song) Tree-SHA512: d2f8a384fb994b83f881b585db70e7cf57980821074126d254960cb67988a40672ffde065d0ccd9a90f9e3b395d8cd33695c796ecb4b54f69fe74ee2bf7497a6
bc53752 Tests: Add simple test for getaddednodeinfo (Jimmy Song) Tree-SHA512: d2f8a384fb994b83f881b585db70e7cf57980821074126d254960cb67988a40672ffde065d0ccd9a90f9e3b395d8cd33695c796ecb4b54f69fe74ee2bf7497a6
bc53752 Tests: Add simple test for getaddednodeinfo (Jimmy Song) Tree-SHA512: d2f8a384fb994b83f881b585db70e7cf57980821074126d254960cb67988a40672ffde065d0ccd9a90f9e3b395d8cd33695c796ecb4b54f69fe74ee2bf7497a6
Test adds a node and sees if it's in the getaddednodeinfo call