qa: Check return code when stopping nodes #9824

MarcoFalke commented at 10:22 AM on February 22, 2017: member

We had a segfault slip through recently (#9817) because the test framework just ignores the return code.

laanwj added the label Tests on Feb 22, 2017

laanwj commented at 10:25 AM on February 22, 2017: member

Yes, makes sense to check that. utACK https://github.com/bitcoin/bitcoin/pull/9824/commits/fac3af7f1c6a1d2ec25ed93201c9dec7bf9bb7bf (also for 0.14)

laanwj added this to the milestone 0.14.0 on Feb 22, 2017

shyii approved

shyii commented at 10:55 AM on February 22, 2017: none

Hey

laanwj commented at 11:13 AM on February 22, 2017: member

@shyii I'm not sure what you're trying to do, but please don't abuse the code review system for sending idle messages.

jnewbery commented at 7:49 PM on February 22, 2017: member

Concept ACK. We should definitely assert the return code on shutdown.

This causes at least the following two tests to fail:

fundrawtransaction.py
rpcbind_test.py

fundrawtransaction is failing because it's calling stop_nodes() on a subset of the running nodes:

        self.nodes[1].encryptwallet("test")
        self.nodes.pop(1)
        stop_nodes(self.nodes)

The encryptwallet() RPC actually stops bitcoind. That means that node 1 is still in the util module's global bitcoind_processes dictionary, so a call to stop_nodes() hits your assert.

It can be fixed by just calling stop_node() for each of the nodes individually.

The failure in rpcbind_test.py is due to this:

        try:
            # connect to node through non-loopback interface
            node = get_rpc_proxy(rpc_url(0, "%s:%d" % (rpchost, rpcport)), 0)
            node.getnetworkinfo()
        finally:
            node = None # make sure connection will be garbage collected and closed
            stop_nodes(self.nodes)

I don't think we need to wrap those calls in try/finallys since the BitcoinTestFramework.main() method takes care of cleaning up the nodes for us.

I've pushed fixes for both of those failures to https://github.com/jnewbery/bitcoin/commit/cbebbcf010e2f7986e349b1a21121b6cdaf182c2. Feel free to squash into your commit if you're happy with the changes.

Generally, tracking bitcoin_processes as a global dictionary in util.py smells (any global variables in util.py smells). The nodes should be members of TestFramework, and it should be the test framework's responsibility to clean them up (and assert return codes) at the end of the test run.

And one question on code style: I know that assert_equal() is used all over the qa test directory. Why? Why is assert_equal(ret, 0) preferred over assert ret == 0, "unexpected return code %s" % ret?

Why is assert_equal(len(bitcoind_processes.values()), 0) preferred over assert not bitcoind_processes, "bitcoind processes still running: %s" % str(bitcoind_processes)?

In generally, assert seems to be treated as a function throughout the qa directory instead of a statement. It would be nice if we could change that.

laanwj commented at 8:28 PM on February 22, 2017: member

In generally, assert seems to be treated as a function throughout the qa directory instead of a statement. > It would be nice if we could change that.

I think the idea on the longer run is to move away from using bare assert everywhere and to something that resembles a real testing framework (maybe unittest). Testing frameworks provide helper functions such as assert_equal because those can automatically print the expected and passed-in value, without having to provide a custom message everywhere.

MarcoFalke commented at 8:31 PM on February 22, 2017: member

Thanks for the fixups, I will include them in this pull. I will also adjust the code style of the second assertion.

In regard to your question:

assert_equal(a, b) is the lazy way to assert equality but still have a somewhat useful assert message which includes both values that are unequal.

assert as a statement should only be used with a single variable of type boolean as expression or with an additional string that provides further debug infos (such as the ones you mentioned above). I assume no one uses the hand crafted strings because assertions are expected to fire off basically never and the additional effort required to put into crafting the string is not considered worthy. (In the rare case a assertion fires, a developer is usually required to jump into the code at that location anyway...)

On Wed, Feb 22, 2017 at 8:49 PM, John Newbery notifications@github.com wrote:

Concept ACK. We should definitely assert the return code on shutdown.

This causes at least the following two tests to fail:

fundrawtransaction.py rpcbind_test.py

fundrawtransaction is failing because it's calling stop_nodes() on a subset of the running nodes:
    self.nodes[1].encryptwallet("test")
    self.nodes.pop(1)
    stop_nodes(self.nodes)
The encryptwallet() RPC actually stops bitcoind. That means that node 1 is still in the util module's global bitcoind_processes dictionary, so a call to stop_nodes() hits your assert.

It can be fixed by just calling stop_node() for each of the nodes individually.

The failure in rpcbind_test.py is due to this:
    try:
        # connect to node through non-loopback interface
        node = get_rpc_proxy(rpc_url(0, "%s:%d" % (rpchost, rpcport)),
     node.getnetworkinfo()
 finally:
     node = None # make sure connection will be garbage collected and
closed stop_nodes(self.nodes)
I don't think we need to wrap those calls in try/finallys since the BitcoinTestFramework.main() method takes care of cleaning up the nodes for us.

I've pushed fixes for both of those failures to jnewbery@cbebbcf. Feel free to squash into your commit if you're happy with the changes.

Generally, tracking bitcoin_processes as a global dictionary in util.py smells (any global variables in util.py smells). The nodes should be members of TestFramework, and it should be the test framework's responsibility to clean them up (and assert return codes) at the end of the test run.

And one question on code style: I know that assert_equal() is used all over the qa test directory. Why? Why is assert_equal(ret, 0) preferred over assert ret == 0, "unexpected return code %s" % ret?

Why is assert_equal(len(bitcoind_processes.values()), 0) preferred over assert not bitcoind_processes, "bitcoind processes still running: %s" % str(bitcoind_processes)?

In generally, assert seems to be treated as a function throughout the qa directory instead of a statement. It would be nice if we could change that.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

jnewbery commented at 10:51 PM on February 22, 2017: member

@laanwj @MarcoFalke thanks. Makes sense.

assert as a statement should only be used with a single variable of type boolean as expression or with an additional string that provides further debug infos

Sounds sensible. We're quite a long way off that though!

→ cat qa/rpc-tests/*py | grep -c "assert("
364

qa: Check return code when stopping nodes

This includes work by jnewbery

fa4cd2e998

MarcoFalke force-pushed on Feb 23, 2017

laanwj referenced this in commit 260c71cbb8 on Feb 23, 2017

laanwj merged this on Feb 23, 2017

laanwj closed this on Feb 23, 2017

laanwj referenced this in commit 1d2a57e9fd on Feb 23, 2017

MarcoFalke deleted the branch on Feb 23, 2017

jnewbery referenced this in commit dd4416ee89 on Mar 23, 2017

codablock referenced this in commit d88030845d on Jan 26, 2018

andvgal referenced this in commit 1342725b75 on Jan 6, 2019

CryptoCentric referenced this in commit 8bbb66f178 on Feb 27, 2019

DrahtBot locked this on Sep 8, 2021