Add tests for RPC help #14020

promag · 2018-08-22T00:41:08Z

At the moment the new test checks for:

invalid usages
expected output for unknown command
current RPC command titles (derived from command categories) — this prevents adding wrong RPC categories and new categories must be added to the test

promag · 2018-08-22T00:41:23Z

Follow up of #13945 (comment)

isghe · 2018-08-22T01:01:50Z

missing hidden category

isghe · 2018-08-22T01:04:14Z

missing test category (in Qt)

promag · 2018-08-22T01:07:00Z

missing hidden category

It's hidden to the RPC client, and has special treatment in

bitcoin/src/rpc/server.cpp

Line 172 in 2df1132

    
           if ((strCommand != "" || pcmd->category == "hidden") && strMethod != strCommand)

isghe · 2018-08-22T01:16:31Z

How can be sure, external scripts analysing C/C++ Bitcoin source code, are immune to injection?

isghe · 2018-08-22T01:18:34Z

I would prefer a C/C++ compile time security check, not an external script security check.

promag · 2018-08-22T01:23:47Z

missing test category (in Qt)

I'm not following, in Qt?

How can be sure, external scripts analysing C/C++ Bitcoin source code, are immune to injection?

What is subject to injection here?

I would prefer a C/C++ compile time security check, not an external script security check.

There is still room to that, this test file can check for other stuff, like output is sorted, formatting.

isghe · 2018-08-22T01:24:30Z

worst, I understood only now: it looks it is working on ./bitcoin-cli help output!

isghe · 2018-08-22T01:35:59Z

I'm not following, in Qt?

I found this (as I wrote here #13945 in the p.s: anyway I was able to catch the QT command rpcNestedTest.)

in file src/qt/test/rpcnestedtests.cpp line 32:

static const CRPCCommand vRPCCommands[] =
{
    { "test", "rpcNestedTest", &rpcNestedTest_rpc, {} },
};

Anyway take care, I always configure --without-gui, I usually (read it: ALWAYS) don't take care about QT.

promag · 2018-08-22T01:37:22Z

Oh, that doesn't matter IMO.

l2a5b1 · 2018-08-22T12:04:08Z

Thanks @promag!

Would it be an idea to assert that the actual titles are valid (i.e. not map the titles back to categories)?

I think this is more in spirit of a functional test and the test can then also assert that there are no true negatives (for example: "== Hidden ==", "== hidden ==", "== blockchain ==","== ==").

promag · 2018-08-22T12:09:18Z

@251labs I'll update.

promag · 2018-08-23T00:42:12Z

@251labs updated.

l2a5b1 · 2018-08-24T14:27:34Z

utACK 6af6d9b

Thanks @promag! Maybe a small nit would be to assert against the full category title to catch potentially missing title suffixes (==). Other than this nit LGTM.

isghe · 2018-08-25T01:04:54Z

I am not an expert about functional test, and their implementations; but I like to learn new things, that's why I am asking (maybe stupid) question.

In this kind of functional and deterministic test, wouldn't be easier simply to check the sha256 output?

$ ./src/bitcoin-cli help | sha256
2ae3a94e05bbb98323f72f19272bde01f075db16c7e4357bc46815a1393cd44a

thanks :-)

maflcko · 2018-08-25T13:55:56Z

test/functional/test_runner.py

@@ -152,6 +152,7 @@
    'p2p_node_network_limited.py',
    'feature_blocksdir.py',
    'feature_config_args.py',
+    'rpc_help.py',
    'feature_help.py',


Imo this could be combined into the feature_help test. Just adding a new function there def rpc_help(self): ... and moving the other code into def cli_help(self): ....

hmm not sure about this, my first intuition was to agree with you but now I think RPC help and options help are sufficiently different concepts (and in code) to have as separate tests
But I'll leave it to @promag

promag · 2018-08-29T12:49:05Z

I tend to prefer like this because

follows current convention
more little test files is more "readable" than less bigger test files
more files allows parallelization

6af6d9b test: Add tests for RPC help (João Barbosa) Pull request description: At the moment the new test checks for: - invalid usages - expected output for unknown command - current RPC command titles (derived from command categories) — this prevents adding wrong RPC categories and new categories must be added to the test Tree-SHA512: f987535d001b1cd300656588602b1634099ea68a1dd2282180c30fa56caf7f990be9e2dc86c7431dfcf7fd686d0299a8d4935df178a2c9f0fb6fbebcba748eb5

maflcko · 2018-08-30T11:58:22Z

test/functional/rpc_help.py

+
+        # command titles
+        titles = [line[3:-3] for line in node.help().splitlines() if line.startswith('==')]
+        assert_equal(titles, ['Blockchain', 'Control', 'Generating', 'Mining', 'Network', 'Rawtransactions', 'Util', 'Wallet', 'Zmq'])


zmq is only available when installed. Should check for that like the other zmq tests?

Github-Pull: bitcoin#14020 Rebased-From: 6af6d9b

6af6d9b test: Add tests for RPC help (João Barbosa) Pull request description: At the moment the new test checks for: - invalid usages - expected output for unknown command - current RPC command titles (derived from command categories) — this prevents adding wrong RPC categories and new categories must be added to the test Tree-SHA512: f987535d001b1cd300656588602b1634099ea68a1dd2282180c30fa56caf7f990be9e2dc86c7431dfcf7fd686d0299a8d4935df178a2c9f0fb6fbebcba748eb5

fanquake added the Tests label Aug 22, 2018

test: Add tests for RPC help

6af6d9b

promag force-pushed the 2018-08-test-help branch from 31e285d to 6af6d9b Compare August 23, 2018 00:39

promag changed the title ~~Assert RPC command categories~~ Add tests for RPC help Aug 23, 2018

maflcko reviewed Aug 25, 2018

View reviewed changes

maflcko merged commit 6af6d9b into bitcoin:master Aug 29, 2018

maflcko reviewed Aug 30, 2018

View reviewed changes

isghe mentioned this pull request Sep 2, 2018

Refactoring CRPCCommand with enum category #13945

Closed

promag deleted the 2018-08-test-help branch September 2, 2018 21:02

Bushstar mentioned this pull request Sep 4, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#359

Merged

maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Oct 25, 2018

test: Add tests for RPC help

24d796a

Github-Pull: bitcoin#14020 Rebased-From: 6af6d9b

toxeus pushed a commit to toxeus/bitcoin that referenced this pull request Nov 28, 2018

test: Add tests for RPC help

e07bb9f

Github-Pull: bitcoin#14020 Rebased-From: 6af6d9b

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

Add tests for RPC help #14020

Add tests for RPC help #14020

Uh oh!

Conversation

promag commented Aug 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Aug 22, 2018

Uh oh!

isghe commented Aug 22, 2018

Uh oh!

isghe commented Aug 22, 2018

Uh oh!

promag commented Aug 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

isghe commented Aug 22, 2018

Uh oh!

isghe commented Aug 22, 2018

Uh oh!

promag commented Aug 22, 2018

Uh oh!

isghe commented Aug 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

isghe commented Aug 22, 2018

Uh oh!

promag commented Aug 22, 2018

Uh oh!

l2a5b1 commented Aug 22, 2018

Uh oh!

promag commented Aug 22, 2018

Uh oh!

promag commented Aug 23, 2018

Uh oh!

l2a5b1 commented Aug 24, 2018

Uh oh!

isghe commented Aug 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko Aug 25, 2018

Choose a reason for hiding this comment

Uh oh!

laanwj Aug 29, 2018

Choose a reason for hiding this comment

Uh oh!

promag commented Aug 29, 2018

Uh oh!

maflcko Aug 30, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

promag commented Aug 22, 2018 •

edited

Loading

promag commented Aug 22, 2018 •

edited

Loading

isghe commented Aug 22, 2018 •

edited

Loading

isghe commented Aug 25, 2018 •

edited

Loading