Page MenuHomePhabricator

test: Check RPC argument mapping
ClosedPublic

Authored by Fabien on Jul 5 2019, 15:50.

Details

Summary
Parse the dispatch tables from the server implementation files,
and the conversion table from the client.

Perform the following consistency checks:

- Arguments defined in conversion table, must be present in dispatch
  table. If not, it was probably forgotten to add them to the
  dispatch table, and they will not work.

- Arguments defined in conversion table must have the same names as
  in the dispatch table. If not, they will not work.

- All aliases for an argument must either be present in the
  conversion table, or not. Anything in between means an oversight
  and some aliases won't work.

Any of these results in an error.

It also performs a consistency check to see if the same
named argument is sometimes converted, and sometimes not. E.g.
one RPC call might have a 'verbose' argument that is converted,
another RPC call might have one that is not converted. This is not
necessarily wrong, but points at a possible error (as well as
makes the API harder to memorize) - so it is emitted as a warning
(could upgrade this to error).

Backport of core PR10753
https://github.com/bitcoin/bitcoin/pull/10753/files

The script will be integrated into arcanist in a later diff.

Test Plan

Should report no error

./contrib/devtools/check-rpc-mappings.py .

Remove the line:

{"getblock", 1, "verbose"},

In the file src/rpc/client.cpp

./contrib/devtools/check-rpc-mappings.py .

Check the error is catched

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

From the test plan, sounds like this depends on D3564? Or am I misunderstanding...

Sounds like this should be added to the release checklist or to a linter.

@deadalnix As stated in the summary, this will be a linter.
It's not yet because there are some other backports (e.g. https://reviews.bitcoinabc.org/D3545) that build on top and I want to add them in between to make it easier to merge/review.

OK tried out the test plan on commit before and after D3564 laned -- yes it did depend on D3564 to pass :-)

This revision is now accepted and ready to land.Jul 5 2019, 18:38
This revision was automatically updated to reflect the committed changes.