Yes, makes sense to check that. utACK fac3af7 (also for 0.14)

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

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 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.

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.

@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