[test] Add getblockchaininfo functional test by promag · Pull Request #11370 · bitcoin/bitcoin

promag · 2017-09-19T23:50:11Z

Adds functional test for getblockchaininfo. Also deals with the fact that pruneheight is only in the response when pruning is enabled (related to #11366).

promag · 2017-09-19T23:51:41Z

test/functional/blockchain.py

Should assert values or types of each field?

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.

jnewbery

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.

jnewbery · 2017-09-20T13:32:26Z

test/functional/blockchain.py

suggestion: perhaps add `self.log.info("test getblockchaininfo")

jnewbery · 2017-09-20T13:35:10Z

test/functional/blockchain.py

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.

jnewbery · 2017-09-20T13:37:41Z

test/functional/blockchain.py

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.

jnewbery · 2017-09-20T13:39:41Z

test/functional/blockchain.py

I think you can just use self.start_node(0). The default will use self.extra_args[0] for its args.

promag · 2017-09-20T14:02:28Z

test/functional/blockchain.py

promag · 2017-09-20T14:03:31Z

test/functional/test_framework/test_framework.py

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.

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.

jnewbery

I like the new restart_node() helper function. A couple of comments inline.

jnewbery · 2017-09-20T14:26:30Z

test/functional/blockchain.py

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()

jnewbery · 2017-09-20T14:28:28Z

test/functional/test_framework/test_framework.py

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.

promag · 2017-09-20T14:39:54Z

test/functional/test_framework/test_framework.py

            # Wait for nodes to stop
            node.wait_until_stopped()

+    def restart_node(self, i, extra_args=None):


@jnewbery added the option to override node.extra_args. I wonder if that this new extra_arg should be saved in node.

Looks good. I don't think you need to update node.extra_args

esotericnonsense · 2017-09-20T15:33:16Z

Noted for #11367, can update to include relevant checks (or vice versa, whichever goes in first I guess)

promag · 2017-09-20T15:59:24Z

I guess this could go in first since #11367 is a new feature. But if #11367 goes first then I'm happy to update this.

jnewbery · 2017-09-20T19:44:24Z

Tested ACK f6ffb14

jnewbery · 2017-09-22T20:22:11Z

@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)

esotericnonsense · 2017-09-22T21:46:51Z

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

promag commented Sep 19, 2017

View reviewed changes

fanquake added the Tests label Sep 19, 2017

promag mentioned this pull request Sep 20, 2017

[rpc] getblockchaininfo: add size_on_disk, prune_target_size #11367

Merged

jnewbery reviewed Sep 20, 2017

View reviewed changes

promag force-pushed the 2017-09-add-getblockchaininfo-functional-test branch 4 times, most recently from 11724d3 to 3eb13b6 Compare September 20, 2017 14:05

promag commented Sep 20, 2017

View reviewed changes

jnewbery reviewed Sep 20, 2017

View reviewed changes

promag added 2 commits September 20, 2017 15:38

[test] Add restart_node to BitcoinTestFramework

fd8f45f

[test] Add getblockchaininfo functional test

f6ffb14

promag force-pushed the 2017-09-add-getblockchaininfo-functional-test branch from 3eb13b6 to f6ffb14 Compare September 20, 2017 14:38

promag commented Sep 20, 2017

View reviewed changes

laanwj merged commit f6ffb14 into bitcoin:master Sep 25, 2017

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Nov 11, 2017

[test] Add restart_node to BitcoinTestFramework

b2194eb

Github-Pull: bitcoin#11370 Rebased-From: fd8f45f

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Nov 11, 2017

[test] Add getblockchaininfo functional test

2bb5a01

Github-Pull: bitcoin#11370 Rebased-From: f6ffb14

Eirik0 mentioned this pull request Oct 31, 2018

Port getblockchaininfo.size_on_disk from BTC master zcash/zcash#3639

Merged

UdjinM6 mentioned this pull request Nov 5, 2018

[rpc] getblockchaininfo: add size_on_disk, prune_target_size, automat… dashpay/dash#2416

Closed

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Conversation

promag commented Sep 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

esotericnonsense commented Sep 20, 2017

Uh oh!

promag commented Sep 20, 2017

Uh oh!

jnewbery commented Sep 20, 2017

Uh oh!

jnewbery commented Sep 22, 2017

Uh oh!

esotericnonsense commented Sep 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

promag commented Sep 19, 2017 •

edited

Loading

esotericnonsense commented Sep 22, 2017 •

edited

Loading