New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[test] Add getblockchaininfo functional test #11370
[test] Add getblockchaininfo functional test #11370
Conversation
test/functional/blockchain.py
Outdated
res = self.nodes[0].getblockchaininfo() | ||
# should have exact fields | ||
assert_equal(sorted(res.keys()), keys) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should assert values or types of each field?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
value: only if you're testing a meaningful value
type: I don't think we do this for other RPC methods, so I wouldn't bother
Thinking about this some more, perhaps we can add some utility method check_rpc_return_object(dict)
which tests the presence and type of each field in the return object? Obviously not for this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
concept ACK. Thanks for improving coverage here!
A few nits inline. Nothing blocking this from being merged if you don't want to take them.
@@ -44,6 +45,38 @@ def run_test(self): | |||
self._test_stopatheight() | |||
assert self.nodes[0].verifychain(4, 0) | |||
|
|||
def _test_getblockchaininfo(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: perhaps add `self.log.info("test getblockchaininfo")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
test/functional/blockchain.py
Outdated
res = self.nodes[0].getblockchaininfo() | ||
# should have exact fields | ||
assert_equal(sorted(res.keys()), keys) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
value: only if you're testing a meaningful value
type: I don't think we do this for other RPC methods, so I wouldn't bother
Thinking about this some more, perhaps we can add some utility method check_rpc_return_object(dict)
which tests the presence and type of each field in the return object? Obviously not for this PR.
test/functional/blockchain.py
Outdated
assert_equal(sorted(res.keys()), keys) | ||
|
||
# restart node with pruning enabled | ||
self.stop_node(0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can speed this test up by starting with pruning the first time, and then restart without pruning (saves one stop-start which is a slow operation). Do that by setting self.extra_args = [['-stopatheight=207', '-prune=1']]
in set_test_params()
and then setting self.extra_args = [['-stopatheight=207']]
before restarting.
test/functional/blockchain.py
Outdated
|
||
# restore node | ||
self.stop_node(0) | ||
self.start_node(0, self.extra_args[0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can just use self.start_node(0)
. The default will use self.extra_args[0]
for its args.
11724d3
to
3eb13b6
Compare
@@ -44,6 +45,38 @@ def run_test(self): | |||
self._test_stopatheight() | |||
assert self.nodes[0].verifychain(4, 0) | |||
|
|||
def _test_getblockchaininfo(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -273,6 +273,11 @@ def stop_nodes(self): | |||
# Wait for nodes to stop | |||
node.wait_until_stopped() | |||
|
|||
def restart_node(self, i): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Convenient method to stop and start a node with the same arguments as before. If this gets ACK's I can push a commit to use where appropriate or submit this in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Ideally the stop and start methods in TestFramework
would be methods on the TestNode
class, and so would this, but we can clean that up later.
You can change this to:
def restart_node(self, i):
"""Stop and start a test node"""
self.stop_node(i)
self.start_node(i)
As long as you also take my change to blockchain.py above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the new restart_node()
helper function. A couple of comments inline.
test/functional/blockchain.py
Outdated
assert res['pruneheight'] >= 0 | ||
|
||
# restart node | ||
self.extra_args[0] = ['-stopatheight=207'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I misled you here. You should use self.nodes[0].extra_args = ...
. The self.extra_args
class variable is only used when instantiating the TestNode. That's important for restart_node()
@@ -273,6 +273,11 @@ def stop_nodes(self): | |||
# Wait for nodes to stop | |||
node.wait_until_stopped() | |||
|
|||
def restart_node(self, i): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Ideally the stop and start methods in TestFramework
would be methods on the TestNode
class, and so would this, but we can clean that up later.
You can change this to:
def restart_node(self, i):
"""Stop and start a test node"""
self.stop_node(i)
self.start_node(i)
As long as you also take my change to blockchain.py above.
3eb13b6
to
f6ffb14
Compare
@@ -273,6 +273,11 @@ def stop_nodes(self): | |||
# Wait for nodes to stop | |||
node.wait_until_stopped() | |||
|
|||
def restart_node(self, i, extra_args=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jnewbery added the option to override node.extra_args
. I wonder if that this new extra_arg
should be saved in node
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I don't think you need to update node.extra_args
Noted for #11367, can update to include relevant checks (or vice versa, whichever goes in first I guess) |
Tested ACK f6ffb14 |
@promag @esotericnonsense - I think it's a good idea if you decide between yourselves which should go in first and then rebase the other on top of it (otherwise there's a risk they both get merged independently and no tests are added for #11367) |
I'll rebase on top, given that there seem to be a few niggles with #11367 this one can go in first. |
f6ffb14 [test] Add getblockchaininfo functional test (João Barbosa) fd8f45f [test] Add restart_node to BitcoinTestFramework (João Barbosa) Pull request description: Adds functional test for `getblockchaininfo`. Also deals with the fact that `pruneheight` is only in the response when pruning is enabled (related to #11366). Tree-SHA512: 56cdec0921f572874f2fdded0990d1722d1435c3ff9979e6bff1afdccdca6f8b214dbe8d7490cdac07b5758909db085132d14340de2cce943241f7ebde7e5b6c
Github-Pull: bitcoin#11370 Rebased-From: fd8f45f
Github-Pull: bitcoin#11370 Rebased-From: f6ffb14
f6ffb14 [test] Add getblockchaininfo functional test (João Barbosa) fd8f45f [test] Add restart_node to BitcoinTestFramework (João Barbosa) Pull request description: Adds functional test for `getblockchaininfo`. Also deals with the fact that `pruneheight` is only in the response when pruning is enabled (related to bitcoin#11366). Tree-SHA512: 56cdec0921f572874f2fdded0990d1722d1435c3ff9979e6bff1afdccdca6f8b214dbe8d7490cdac07b5758909db085132d14340de2cce943241f7ebde7e5b6c
f6ffb14 [test] Add getblockchaininfo functional test (João Barbosa) fd8f45f [test] Add restart_node to BitcoinTestFramework (João Barbosa) Pull request description: Adds functional test for `getblockchaininfo`. Also deals with the fact that `pruneheight` is only in the response when pruning is enabled (related to bitcoin#11366). Tree-SHA512: 56cdec0921f572874f2fdded0990d1722d1435c3ff9979e6bff1afdccdca6f8b214dbe8d7490cdac07b5758909db085132d14340de2cce943241f7ebde7e5b6c
f6ffb14 [test] Add getblockchaininfo functional test (João Barbosa) fd8f45f [test] Add restart_node to BitcoinTestFramework (João Barbosa) Pull request description: Adds functional test for `getblockchaininfo`. Also deals with the fact that `pruneheight` is only in the response when pruning is enabled (related to bitcoin#11366). Tree-SHA512: 56cdec0921f572874f2fdded0990d1722d1435c3ff9979e6bff1afdccdca6f8b214dbe8d7490cdac07b5758909db085132d14340de2cce943241f7ebde7e5b6c
f6ffb14 [test] Add getblockchaininfo functional test (João Barbosa) fd8f45f [test] Add restart_node to BitcoinTestFramework (João Barbosa) Pull request description: Adds functional test for `getblockchaininfo`. Also deals with the fact that `pruneheight` is only in the response when pruning is enabled (related to bitcoin#11366). Tree-SHA512: 56cdec0921f572874f2fdded0990d1722d1435c3ff9979e6bff1afdccdca6f8b214dbe8d7490cdac07b5758909db085132d14340de2cce943241f7ebde7e5b6c
f6ffb14 [test] Add getblockchaininfo functional test (João Barbosa) fd8f45f [test] Add restart_node to BitcoinTestFramework (João Barbosa) Pull request description: Adds functional test for `getblockchaininfo`. Also deals with the fact that `pruneheight` is only in the response when pruning is enabled (related to bitcoin#11366). Tree-SHA512: 56cdec0921f572874f2fdded0990d1722d1435c3ff9979e6bff1afdccdca6f8b214dbe8d7490cdac07b5758909db085132d14340de2cce943241f7ebde7e5b6c
f6ffb14 [test] Add getblockchaininfo functional test (João Barbosa) fd8f45f [test] Add restart_node to BitcoinTestFramework (João Barbosa) Pull request description: Adds functional test for `getblockchaininfo`. Also deals with the fact that `pruneheight` is only in the response when pruning is enabled (related to bitcoin#11366). Tree-SHA512: 56cdec0921f572874f2fdded0990d1722d1435c3ff9979e6bff1afdccdca6f8b214dbe8d7490cdac07b5758909db085132d14340de2cce943241f7ebde7e5b6c
f6ffb14 [test] Add getblockchaininfo functional test (João Barbosa) fd8f45f [test] Add restart_node to BitcoinTestFramework (João Barbosa) Pull request description: Adds functional test for `getblockchaininfo`. Also deals with the fact that `pruneheight` is only in the response when pruning is enabled (related to bitcoin#11366). Tree-SHA512: 56cdec0921f572874f2fdded0990d1722d1435c3ff9979e6bff1afdccdca6f8b214dbe8d7490cdac07b5758909db085132d14340de2cce943241f7ebde7e5b6c
f6ffb14 [test] Add getblockchaininfo functional test (João Barbosa) fd8f45f [test] Add restart_node to BitcoinTestFramework (João Barbosa) Pull request description: Adds functional test for `getblockchaininfo`. Also deals with the fact that `pruneheight` is only in the response when pruning is enabled (related to bitcoin#11366). Tree-SHA512: 56cdec0921f572874f2fdded0990d1722d1435c3ff9979e6bff1afdccdca6f8b214dbe8d7490cdac07b5758909db085132d14340de2cce943241f7ebde7e5b6c
f6ffb14 [test] Add getblockchaininfo functional test (João Barbosa) fd8f45f [test] Add restart_node to BitcoinTestFramework (João Barbosa) Pull request description: Adds functional test for `getblockchaininfo`. Also deals with the fact that `pruneheight` is only in the response when pruning is enabled (related to bitcoin#11366). Tree-SHA512: 56cdec0921f572874f2fdded0990d1722d1435c3ff9979e6bff1afdccdca6f8b214dbe8d7490cdac07b5758909db085132d14340de2cce943241f7ebde7e5b6c
Adds functional test for
getblockchaininfo
. Also deals with the fact thatpruneheight
is only in the response when pruning is enabled (related to #11366).