rpc: Throw more user friendly arg type check error (1.5/2) #26929
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
|
https://github.com/bitcoin/bitcoin/pull/26929/checks?check_run_id=10776272066 /usr/bin/ccache g++ -m32 -std=c++17 -DHAVE_CONFIG_H -I. -I../src/config -fmacro-prefix-map=/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-i686-pc-linux-gnu=. -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -DHAVE_BUILD_INFO -D_FILE_OFFSET_BITS=64 -DPROVIDE_FUZZ_MAIN_FUNCTION -I. -I./minisketch/include -I./secp256k1/include -I./univalue/include -I./leveldb/include -isystem /tmp/cirrus-ci-build/depends/i686-pc-linux-gnu/include -DBOOST_MULTI_INDEX_DISABLE_SERIALIZATION -DBOOST_NO_CXX98_FUNCTION_BASE -isystem /tmp/cirrus-ci-build/depends/i686-pc-linux-gnu/include -I/tmp/cirrus-ci-build/depends/i686-pc-linux-gnu/include/ -fdebug-prefix-map=/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-i686-pc-linux-gnu=. -fstack-reuse=none -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection -Werror -fno-extended-identifiers -fvisibility=hidden -fPIE -pipe -std=c++17 -O2 -c -o rpc/libbitcoin_common_a-util.o `test -f 'rpc/util.cpp' || echo './'`rpc/util.cpp
rpc/util.cpp: In function ‘std::optional<UniValue::VType> ExpectedType(RPCArg::Type)’:
rpc/util.cpp:717:1: error: control reaches end of non-void function [-Werror=return-type]
}
^ |
fa63790 to
fa7562e
Compare
stickies-v
left a comment
There was a problem hiding this comment.
Concept ACK for better error feedback
fa7562e to
fa8fd49
Compare
fa8fd49 to
fafeddf
Compare
stickies-v
left a comment
There was a problem hiding this comment.
ACK fafeddf - although I think the functional test isn't in a logical place (but not blocking)
| assert_raises_rpc_error( | ||
| -3, | ||
| textwrap.dedent(""" | ||
| Wrong type passed: | ||
| { | ||
| "Position 1 (nblocks)": "JSON value of type string is not of expected type number", | ||
| "Position 2 (height)": "JSON value of type array is not of expected type number" | ||
| } | ||
| """).strip(), | ||
| lambda: self.nodes[0].getnetworkhashps("a", []), | ||
| ) |
There was a problem hiding this comment.
I don't think this is an appropriate place for this test?
E.g. renaming rpc_named_arguments.py -> rpc_arguments.py and adding it there seems more logical to me.
There was a problem hiding this comment.
Why not? Every other RPC that has missing-arg or wrong-type checks is also called in the rpc-specific test file?
Examples:
git grep 'is not of expected type' test
For example in this file:
test/functional/rpc_blockchain.py: assert_raises_rpc_error(-3, "JSON value of type string is not of expected type number", self.nodes[0].getchaintxstats, '')
test/functional/rpc_blockchain.py: assert_raises_rpc_error(-3, "JSON value of type number is not of expected type string", self.nodes[0].getchaintxstats, blockhash=0)
test/functional/rpc_blockchain.py: assert_raises_rpc_error(-3, "JSON value of type string is not of expected type number", node.getblock, blockhash, "2")
There was a problem hiding this comment.
Oh yes, I'm not arguing against checking for an RPC's specific argument types within the RPC-specific test file in general, that can help with regressions etc.
Since this PR is about type checking in general (and not about getnetworkhashps), I think it'd be good to have a test that checks the correct error message is raised - and that should be a test dedicated to just that? If for some unrelated reason in the future _test_getnetworkhashps() gets deleted because the method is deprecated, we'd unintentionally remove test coverage on this PR too.
There was a problem hiding this comment.
Ok, will add more tests in the follow-up
…or (1.5/2) fafeddf rpc: Throw more user friendly arg type check error (MarcoFalke) Pull request description: The arg type check error doesn't list which arg (position or name) failed. Fix that. ACKs for top commit: stickies-v: ACK fafeddf - although I think the functional test isn't in a logical place (but not blocking) Tree-SHA512: 17425aa145aab5045940ec74fff28f0e3b2b17ae55f91c4bb5cbcdff0ef13732f8e31621d85964dc2c04333ea37dbe528296ac61be27541384b44e37957555c8
The arg type check error doesn't list which arg (position or name) failed. Fix that.