Skip to content

Conversation

jnewbery
Copy link
Contributor

I did this a few months ago (here: #9707), but a few new examples have crept back in.

When testing RPC failures, the test case should always assert the error value and message, to ensure that the failure was for the correct reason. Not doing that can hide bugs in the test code and mean that the test is not testing the correct behaviour.

RPC failure testing should use the utility function assert_raises_jsonrpc() (renamed in the final commit of this PR to assert_raises_rpc_error().

This PR does the following:

  • changes all remaining instances of tests directly testing on JSONRPCException to calls to assert_raises_jsonrpc()
  • prevents assert_raises_message() from being called with JSONRPCException
  • scripted-diff changes assert_raises_jsonrpc() to assert_raises_rpc_error()

@laanwj laanwj added the Tests label Jul 17, 2017
@jnewbery jnewbery force-pushed the cleanup_jsonrpc_asserts branch 2 times, most recently from 0bb80b7 to 9ebed01 Compare July 24, 2017 15:54
@jnewbery
Copy link
Contributor Author

Rebased and scripted-diff Travis check fixed.

@maflcko
Copy link
Member

maflcko commented Jul 30, 2017

Not sure if it is worth to go through the rebases due to renaming of the method. Mind to drop the last commit for now?

@jnewbery
Copy link
Contributor Author

I wanted the name change for a couple of reasons:

  • assert_raises_jsonrpc() doesn't make much sense. It's raising an error, so it should include that in the function name. Hopefully changing it to assert_raises_rpc_error() will mean people notice the function and use it instead of assert_raises_message() or try to roll their own.
  • with --usecli (see [tests] [utils] test bitcoin-cli #10798), I'm adding a new mode to the test framework, where RPCs are sent via bitcoin-cli rather than using JSON over HTTP. In that context, assert_raises_rpc_error() makes more sense.

It's a scripted diff changing one function name, so rebasing shouldn't be too difficult.

@maflcko maflcko added this to the 0.16.0 milestone Jul 31, 2017
@jnewbery jnewbery force-pushed the cleanup_jsonrpc_asserts branch 2 times, most recently from 6f4b7aa to befcccc Compare August 14, 2017 17:07
@jnewbery jnewbery force-pushed the cleanup_jsonrpc_asserts branch 2 times, most recently from be79245 to cc5c3e4 Compare September 1, 2017 18:21
@jnewbery jnewbery force-pushed the cleanup_jsonrpc_asserts branch from cc5c3e4 to 9a7512c Compare September 12, 2017 18:25
@jnewbery
Copy link
Contributor Author

rebased

@jnewbery jnewbery force-pushed the cleanup_jsonrpc_asserts branch from 9a7512c to 13836c7 Compare September 14, 2017 15:03
@jnewbery
Copy link
Contributor Author

rebased

@maflcko
Copy link
Member

maflcko commented Oct 2, 2017

Needs rebase

@jnewbery jnewbery force-pushed the cleanup_jsonrpc_asserts branch 2 times, most recently from 9827bee to 7093b9a Compare October 2, 2017 20:34
@jnewbery
Copy link
Contributor Author

jnewbery commented Oct 2, 2017

rebased

@maflcko
Copy link
Member

maflcko commented Oct 5, 2017

Needs silent merge conflicts solved.

-BEGIN VERIFY SCRIPT-
sed -i 's/assert_raises_jsonrpc/assert_raises_rpc_error/g' test/functional/*py test/functional/test_framework/*py
-END VERIFY SCRIPT-
@jnewbery jnewbery force-pushed the cleanup_jsonrpc_asserts branch from 7093b9a to 47ba8cf Compare October 5, 2017 13:59
@jnewbery
Copy link
Contributor Author

jnewbery commented Oct 5, 2017

Thanks. scripted-diff rerun and repushed.

@maflcko
Copy link
Member

maflcko commented Oct 9, 2017

utACK 47ba8cf

@maflcko maflcko merged commit 47ba8cf into bitcoin:master Oct 9, 2017
maflcko pushed a commit that referenced this pull request Oct 9, 2017
47ba8cf scripted-diff: rename assert_raises_jsonrpc to assert_raises_rpc error (John Newbery)
677d893 [tests] do not allow assert_raises_message to be called with JSONRPCException (John Newbery)
5864e9c [tests] remove direct testing on JSONRPCException from individual test cases (John Newbery)

Pull request description:

  I did this a few months ago (here: #9707), but a few new examples have crept back in.

  When testing RPC failures, the test case should always assert the error value and message, to ensure that the failure was for the correct reason. Not doing that can hide bugs in the test code and mean that the test is not testing the correct behaviour.

  RPC failure testing should use the utility function `assert_raises_jsonrpc()` (renamed in the final commit of this PR to `assert_raises_rpc_error()`.

  This PR does the following:
  - changes all remaining instances of tests directly testing on `JSONRPCException` to calls to `assert_raises_jsonrpc()`
  - prevents `assert_raises_message()` from being called with `JSONRPCException`
  - scripted-diff changes `assert_raises_jsonrpc()` to `assert_raises_rpc_error()`

Tree-SHA512: 2cc5e320704ec623a6e5a27d3c2c81cea86b502e29896f03bb5bf92cc36725132c1144410aecdf49e90d4577d512ee467d50d8184e9d5c5d0870bfc931316a5a
@jnewbery jnewbery deleted the cleanup_jsonrpc_asserts branch October 9, 2017 19:55
maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Nov 1, 2017
maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Nov 1, 2017
maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Nov 1, 2017
-BEGIN VERIFY SCRIPT-
sed -i 's/assert_raises_jsonrpc/assert_raises_rpc_error/g' test/functional/*py test/functional/test_framework/*py
-END VERIFY SCRIPT-

Github-Pull: bitcoin#10853
Rebased-From: 47ba8cf
@morcos morcos mentioned this pull request Nov 2, 2017
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants