Fix RPC failure testing #9707

jnewbery · 2017-02-07T15:22:21Z

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:

try:
    nodes.rpcfunction(paramters,...) 
except JSONRPCException as exp: #(1) must only catch JSONRPCExceptions
    assert_equal(exp.error["code"], EXPECTED_ERROR_CODE)  #(2) must verify error code
    assert_equal(exp.error["message"], "Expected error message")  #(3) must verify error message
else:
    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:

        try:
            self.nodes[0].generatetoaddress(1, 'mneYUmWYsuk7kySiURxCi3AGxrAqZxLgPZ')
        except JSONRPCException as e:
            assert("Invalid address" not in e.error['message'])
            assert("ProcessNewBlock, block not accepted" not in e.error['message'])
            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:

        try:
            tmpl = self.nodes[0].getblocktemplate({})
            assert(len(tmpl['transactions']) == 1)  # Doesn't include witness tx
            assert(tmpl['sigoplimit'] == 20000)
            assert(tmpl['transactions'][0]['hash'] == txid)
            assert(tmpl['transactions'][0]['sigops'] == 2)
            assert(('!segwit' in tmpl['rules']) or ('segwit' not in tmpl['rules']))
        except JSONRPCException:
            # This is an acceptable outcome
            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:

assert_raises(JSONRPCException, self.nodes[0].gettransaction, [txid3])
#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)

paveljanik · 2017-02-07T17:11:58Z

qa/rpc-tests/test_framework/util.py

+            RPC call. Set to None if checking the error string is not required
+        fun (function): the function to call. This should be the name of an RPC.
+        args*: positional argumemts for the function.
+        kwargs**: named arguments for the function.


paveljanik · 2017-02-07T17:12:19Z

qa/rpc-tests/test_framework/util.py

+def assert_raises_jsonrpc(code, message, fun, *args, **kwds):
+    """Run an RPC and verify that a specific JSONRPC exception code and message is raised.
+
+    Calls function `fun` with arguments `args` and `kwargs`. Catches a JSONRPCException


paveljanik · 2017-02-07T17:13:23Z

qa/rpc-tests/test_framework/util.py

+        message (string), optional: [a substring of] the error string returned by the
+            RPC call. Set to None if checking the error string is not required
+        fun (function): the function to call. This should be the name of an RPC.
+        args*: positional argumemts for the function.


paveljanik · 2017-02-07T17:15:02Z

Concept ACK

maflcko · 2017-02-07T17:23:25Z

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

Concept ACK

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)

Hmm, it would be still easy to get it wrong by ignoring (or not knowing) that assert_raises* exists. You can always type in a useless `try stuff() except: pass`.

…

On Tue, Feb 7, 2017 at 4:22 PM, John Newbery ***@***.***> wrote: 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: try: nodes.rpcfunction(paramters,...) except JSONRPCException as exp: #(1) must only catch JSONRPCExceptions assert_equal(exp.error["code"], EXPECTED_ERROR_CODE) #(2) must verify error code assert_equal(exp.error["message"], "Expected error message") #(3) must verify error message else: 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: try: self.nodes[0].generatetoaddress(1, 'mneYUmWYsuk7kySiURxCi3AGxrAqZxLgPZ') except JSONRPCException as e: assert("Invalid address" not in e.error['message']) assert("ProcessNewBlock, block not accepted" not in e.error['message']) 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: try: tmpl = self.nodes[0].getblocktemplate({}) assert(len(tmpl['transactions']) == 1) # Doesn't include witness tx assert(tmpl['sigoplimit'] == 20000) assert(tmpl['transactions'][0]['hash'] == txid) assert(tmpl['transactions'][0]['sigops'] == 2) assert(('!segwit' in tmpl['rules']) or ('segwit' not in tmpl['rules'])) except JSONRPCException: # This is an acceptable outcome 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: assert_raises(JSONRPCException, self.nodes[0].gettransaction, [txid3]) #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) ________________________________ You can view, comment on, or merge this pull request online at: #9707 Commit Summary Fix RPC failure testing File Changes M qa/rpc-tests/rpcnamedargs.py (2) M qa/rpc-tests/test_framework/util.py (23) Patch Links: https://github.com/bitcoin/bitcoin/pull/9707.patch https://github.com/bitcoin/bitcoin/pull/9707.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

jnewbery · 2017-02-07T17:34:01Z

it would be still easy to get it wrong by ignoring (or not knowing) that assert_raises* exists

Indeed. There's no way to stop people from doing this again in future. I think the best we can hope for is to remove all existing cases where try/except has been used explicitly in individual testcases for JSON RPC error testing. When people come to write new testcases and copy the existing ones, they see the assert_raises* functions being used.

jnewbery · 2017-02-07T17:44:09Z

Thanks @paveljanik - all typos fixed

maflcko · 2017-02-07T17:52:30Z

ACK, but could you replace it with `if code is not None`, so that 0, False, etc does not equal a wild card for code or message.

…

On Tue, Feb 7, 2017 at 6:44 PM, John Newbery ***@***.***> wrote: Thanks @paveljanik <https://github.com/paveljanik> - all typos fixed — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#9707 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGGmv-JGLqS-4fvVmfE4CJE7nti8UutGks5raK17gaJpZM4L5oD0> .

jnewbery · 2017-02-07T18:03:20Z

@MarcoFalke done. It now explicitly tests code and message for not being None.

Make sure that RPC tests are actually checking failures correctly by: - Catching JSON RPC exceptions and verifying the error codes and messages. - Failing the test case if the JSON RPC exception isn't raised.

jnewbery · 2017-02-08T15:25:11Z

On @TheBlueMatt's suggestion in #9713, I've added a new commit that removes the assert_raises() and assert_raises_message() functions.

jnewbery · 2017-02-08T18:18:08Z

Actually, on second thoughts I'll save removing assert_raises() and assert_raises_message() for a future PR.

TheBlueMatt · 2017-02-10T16:49:49Z

OK, sounds good. utACK 9db8eec

jnewbery · 2017-02-10T16:53:50Z

Thanks @TheBlueMatt.

@MarcoFalke any chance of merging this small PR? I have a lot of fixes that build off this so it'd be good to have this merged into master.

maflcko · 2017-02-10T17:00:47Z

utACK 9db8eec. This is refactoring, so can go into 0.14.

9db8eec Fix RPC failure testing (John Newbery)

jnewbery · 2017-02-10T17:42:39Z

awsome. Thanks @MarcoFalke

jtimon · 2017-03-07T17:54:34Z

late utACK 9db8eec

9db8eec Fix RPC failure testing (John Newbery)

…9707) c9bd0f6 Fix RPC failure testing (2 of 2) (John Newbery) Tree-SHA512: df30e6e85abe8c4e12910dc60699f1201e9c243457abd738c1fdeac45f0ff05c674f68619ad9a47c847ec557954007d672cd89d3a9a3b2398dd188d9ffa6dcc9

9db8eec Fix RPC failure testing (John Newbery)

…9707) c9bd0f6 Fix RPC failure testing (2 of 2) (John Newbery) Tree-SHA512: df30e6e85abe8c4e12910dc60699f1201e9c243457abd738c1fdeac45f0ff05c674f68619ad9a47c847ec557954007d672cd89d3a9a3b2398dd188d9ffa6dcc9

…9707) c9bd0f6 Fix RPC failure testing (2 of 2) (John Newbery) Tree-SHA512: df30e6e85abe8c4e12910dc60699f1201e9c243457abd738c1fdeac45f0ff05c674f68619ad9a47c847ec557954007d672cd89d3a9a3b2398dd188d9ffa6dcc9 s/serialize_with_witness/serialize/ miss a tx_submit -> sendrawtransaction dropped dip4 fix prioritise_transaction.py and nulldummy.py

…9707) c9bd0f6 Fix RPC failure testing (2 of 2) (John Newbery) Tree-SHA512: df30e6e85abe8c4e12910dc60699f1201e9c243457abd738c1fdeac45f0ff05c674f68619ad9a47c847ec557954007d672cd89d3a9a3b2398dd188d9ffa6dcc9 s/serialize_with_witness/serialize/ miss a tx_submit -> sendrawtransaction dropped dip4

…9707) c9bd0f6 Fix RPC failure testing (2 of 2) (John Newbery) Tree-SHA512: df30e6e85abe8c4e12910dc60699f1201e9c243457abd738c1fdeac45f0ff05c674f68619ad9a47c847ec557954007d672cd89d3a9a3b2398dd188d9ffa6dcc9 s/serialize_with_witness/serialize/ miss a tx_submit -> sendrawtransaction dropped dip4 Fix tests Signed-off-by: Pasta <Pasta@dash.org>

…9707) c9bd0f6 Fix RPC failure testing (2 of 2) (John Newbery) Tree-SHA512: df30e6e85abe8c4e12910dc60699f1201e9c243457abd738c1fdeac45f0ff05c674f68619ad9a47c847ec557954007d672cd89d3a9a3b2398dd188d9ffa6dcc9 s/serialize_with_witness/serialize/ miss a tx_submit -> sendrawtransaction dropped dip4 Fix tests Signed-off-by: Pasta <Pasta@dash.org> Fix file permissions

…9707) c9bd0f6 Fix RPC failure testing (2 of 2) (John Newbery) Tree-SHA512: df30e6e85abe8c4e12910dc60699f1201e9c243457abd738c1fdeac45f0ff05c674f68619ad9a47c847ec557954007d672cd89d3a9a3b2398dd188d9ffa6dcc9 s/serialize_with_witness/serialize/ miss a tx_submit -> sendrawtransaction dropped dip4 Fix tests Signed-off-by: Pasta <Pasta@dash.org>

…9707) c9bd0f6 Fix RPC failure testing (2 of 2) (John Newbery) Tree-SHA512: df30e6e85abe8c4e12910dc60699f1201e9c243457abd738c1fdeac45f0ff05c674f68619ad9a47c847ec557954007d672cd89d3a9a3b2398dd188d9ffa6dcc9 s/serialize_with_witness/serialize/ miss a tx_submit -> sendrawtransaction dropped dip4 Fix tests Signed-off-by: Pasta <Pasta@dash.org> Fix file permissions

fanquake added the Tests label Feb 7, 2017

paveljanik reviewed Feb 7, 2017

View reviewed changes

jnewbery force-pushed the rpctestassert branch from cd1fb36 to 1100ca0 Compare February 7, 2017 17:43

jnewbery force-pushed the rpctestassert branch from 1100ca0 to 4ce8d57 Compare February 7, 2017 18:02

This was referenced Feb 7, 2017

Fix error causes and messages in rpc/net.cpp #9713

Closed

Return correct error codes from bumpfee() #9714

Closed

Fix RPC failure testing

9db8eec

Make sure that RPC tests are actually checking failures correctly by: - Catching JSON RPC exceptions and verifying the error codes and messages. - Failing the test case if the JSON RPC exception isn't raised.

jnewbery force-pushed the rpctestassert branch from 4245b3f to 106eb88 Compare February 8, 2017 15:24

jnewbery force-pushed the rpctestassert branch from 106eb88 to 9db8eec Compare February 8, 2017 18:17

jnewbery mentioned this pull request Feb 10, 2017

gettxoutproof() should return consistent result #9738

Merged

maflcko added the Refactoring label Feb 10, 2017

maflcko merged commit 9db8eec into bitcoin:master Feb 10, 2017

maflcko pushed a commit that referenced this pull request Feb 10, 2017

Merge #9707: Fix RPC failure testing

b860915

9db8eec Fix RPC failure testing (John Newbery)

jnewbery deleted the rpctestassert branch February 10, 2017 17:48

jnewbery mentioned this pull request Feb 24, 2017

Fix error codes from various RPCs #9853

Merged

str4d mentioned this pull request Dec 11, 2017

Add assert_raises_message to the python test framework. zcash/zcash#2778

Merged

codablock pushed a commit to codablock/dash that referenced this pull request Jan 23, 2018

Merge bitcoin#9707: Fix RPC failure testing

8043468

9db8eec Fix RPC failure testing (John Newbery)

dagurval mentioned this pull request Feb 13, 2018

rpc: Add support for JSON-RPC named arguments bitcoinxt/bitcoinxt#334

Merged

andvgal pushed a commit to energicryptocurrency/gen2-energi that referenced this pull request Jan 6, 2019

Merge bitcoin#9707: Fix RPC failure testing

b9282d8

9db8eec Fix RPC failure testing (John Newbery)

CryptoCentric pushed a commit to absolute-community/absolute that referenced this pull request Feb 27, 2019

Merge bitcoin#9707: Fix RPC failure testing

36e8042

9db8eec Fix RPC failure testing (John Newbery)

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Fix RPC failure testing #9707

Fix RPC failure testing #9707

Uh oh!

Conversation

jnewbery commented Feb 7, 2017

Uh oh!

paveljanik Feb 7, 2017

Choose a reason for hiding this comment

Uh oh!

paveljanik Feb 7, 2017

Choose a reason for hiding this comment

Uh oh!

paveljanik Feb 7, 2017

Choose a reason for hiding this comment

Uh oh!

paveljanik commented Feb 7, 2017

Uh oh!

maflcko commented Feb 7, 2017 via email

Uh oh!

jnewbery commented Feb 7, 2017

Uh oh!

jnewbery commented Feb 7, 2017

Uh oh!

maflcko commented Feb 7, 2017 via email

Uh oh!

jnewbery commented Feb 7, 2017

Uh oh!

jnewbery commented Feb 8, 2017

Uh oh!

jnewbery commented Feb 8, 2017

Uh oh!

TheBlueMatt commented Feb 10, 2017

Uh oh!

jnewbery commented Feb 10, 2017

Uh oh!

maflcko commented Feb 10, 2017

Uh oh!

jnewbery commented Feb 10, 2017

Uh oh!

jtimon commented Mar 7, 2017

Uh oh!

Uh oh!