RPC interface testing should include failure test cases (ie where we call an RPC with parameters that we expect to fail). The failure test cases should follow this pattern:
0try:
1 nodes.rpcfunction(paramters,...)
2except JSONRPCException as exp: #(1) must only catch JSONRPCExceptions
3 assert_equal(exp.error["code"], EXPECTED_ERROR_CODE) #(2) must verify error code
4 assert_equal(exp.error["message"], "Expected error message") #(3) must verify error message
5else:
6 assert(False) #(4) must fail the test if no JSONRPCException raised
Unfortunately, many of the test cases in qa/rpc-tests get this pattern wrong and don’t actually test what they’re supposed to.
Exhibit A:
0 try:
1 self.nodes[0].generatetoaddress(1, 'mneYUmWYsuk7kySiURxCi3AGxrAqZxLgPZ')
2 except JSONRPCException as e:
3 assert("Invalid address" not in e.error['message'])
4 assert("ProcessNewBlock, block not accepted" not in e.error['message'])
5 assert("Couldn't create new block" not in e.error['message'])
The call to generatetoaddress() actually succeeds here, but there isn’t an else: clause so the test continues (and none of the code in the except: branch is executing)
Exhibit B:
0 try:
1 tmpl = self.nodes[0].getblocktemplate({})
2 assert(len(tmpl['transactions']) == 1) # Doesn't include witness tx
3 assert(tmpl['sigoplimit'] == 20000)
4 assert(tmpl['transactions'][0]['hash'] == txid)
5 assert(tmpl['transactions'][0]['sigops'] == 2)
6 assert(('!segwit' in tmpl['rules']) or ('segwit' not in tmpl['rules']))
7 except JSONRPCException:
8 # This is an acceptable outcome
9 pass
The call to getblocktemplate() is raising an exception. Apparently This is an acceptable outcome, but there’s no explanation why. None of the asserts in the try: branch are being executed. There’s also no checking of the error code/message of the JSONRPCException.
Exhibit C:
0assert_raises(JSONRPCException, self.nodes[0].gettransaction, [txid3])
1#there must be a expection because the unconfirmed wallettx0 must be gone by now
This is using the helper function assert_raises() and the comment explains that the call should fail because the unconfirmed transaction is gone. There’s no testing of the error message or code. In fact, gettransaction() is failing because it’s being called with an array [txid3] instead of a string txid3.
This PR improves the assert_raises_jsonrpc() to verify error cause and error message and improves the commenting of that function. Future PRs will go over all the qa tests, remove the various broken implementations of this pattern and replace them with calls to assert_raises_jsonrpc().
This should also prevent the same bugs from being introduced in future because it should now be easier to get this right (by calling assert_raises_jsonrpc()) than get it wrong (by trying to reimplement the pattern and failing)
