Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
test: Check RPC argument mapping #10753
Conversation
laanwj
added RPC/REST/ZMQ Tests
labels
Jul 6, 2017
|
Nice. |
|
Concept ACK. Will review later. |
| + m = re.search('{ *("[^"]*"), *("[^"]*"), *&([^,]*), *(true|false), *{([^}]*)} *},', line) | ||
| + if not m: | ||
| + print('No match: %s' % line) | ||
| + exit(1) |
practicalswift
Jul 6, 2017
•
Contributor
According to the Python documentation exit(…) should be used in the interactive interpreter shell but not in programs:
The site module (which is imported automatically during startup, except if the -S command-line option is given) adds several constants to the built-in namespace. They are useful for the interactive interpreter shell and should not be used in programs.
Links:
sys.exit(…): "Exit from Python."exit(…): "raise SystemExit with the specified exit code. […] should not be used in programs."
laanwj
referenced
this pull request
Jul 6, 2017
Merged
Be consistent in calling transactions "replaceable" for Opt-In RBF #10698
|
IMO we could somehow share the same data between the server table and the client table so that none of this would be necessary. Don't take me wrong, but parsing code feels bad practice. Beside that, keeping these tables in sync could very well be avoided. I could work in an alternative, unless I'm missing something. |
|
@promag That discussion is in #10751 (comment) And yes, in the long run there are absolutely better solutions (ideally the cli would have none of this information and requests it from the server), but in the short term I think adding this check is good. Anyhow I don't care deeply if this doesn't get merged, I use this script myself, and @TheBlueMatt requested some way to check this in #10751 so I thought it'd be useful for some other people too... |
|
Thanks for pointing that out. @laanwj could this be executed in pre-commit hook? |
Sure, would be a matter of adding the line to execute it to the |
|
@laanwj another PR or care to add it here? |
|
You have to do it locally. Hooks are not part of the repository (for
security reasons, among others.) Shipping some examples of hooks would work
but that's definitely out of scope here.
…
|
|
I think this might be simpler and more robust if it were written as a c++ unit test that just accesses the global variables directly, instead of a python tool that has to parse c++ with regexes. |
laanwj
closed this
Jul 8, 2017
|
@laanwj reason? |
|
@laanwj did you close this because you're planning to propose an alternative? |
|
@jnewbery No, I don't have the time nor motivation to work on this (it works for me), but I'm looking forward to a better alternative as no one seems to like this solution. |
|
I like it! At the very least I like it better than no tests at all. Running code beats suggestions of better ways to do it without any code. If you change your mind, I'm happy to review. |
laanwj
reopened this
Jul 10, 2017
jnewbery
reviewed
Jul 10, 2017
A few nits inline.
I think it'd simplify the implementation if you got rid of the RPCCommand and RPCArgument classes and returned tuples from the process_commands() and process_mappings() functions. The different format of the return values makes some of the logic in main() a little fiddly.
However, this does the job, and I don't think it's worth the time optimizing this, so ACK 6031595 (once #10747 is merged).
I also recommend running a linter such as flake8 over python modules before committing, since removing all warnings can sometimes uncover bugs. Rather than fill this review with nits, I've pushed a branch here that fixes them: https://github.com/jnewbery/bitcoin/tree/pr10753 . Feel free to take/leave/squash as you see fit.
If @ryanofsky or @promag want to implement a C++ unit test, I'm happy to review.
| + assert(s[-1] == '"') | ||
| + return s[1:-1] | ||
| + | ||
| +def make_string(s): |
| + self.names = names | ||
| + self.idx = idx | ||
| + | ||
| +def process_commands(fname): |
jnewbery
Jul 10, 2017
Member
I think it'd simplify implementation in main() if process_commands() and process_mappings() both returned a set of tuples (command, index, argname).
| + else: | ||
| + args = [] | ||
| + cmds.append(RPCCommand(name, args)) | ||
| + assert(not in_rpcs) # If this assertion is hit, something went wrong with parsing the C++ file: update the regexps |
jnewbery
Jul 10, 2017
Member
assert is a statement, not a function, so these parens are unnecessary. In fact, this would be better expressed as:
assert not in_rpcs, "Something went wrong with parsing the C++ file: update the regexps"since that delivers the error text to the user at the point of failure.
laanwj commentedJul 6, 2017
•
edited
Parse the dispatch tables from the server implementation files, and the conversion table from the client (see #10751).
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).
This test is added to travis and run when
CHECK_DOC. Currently fails with the following output:#10698 fixes the first ERRORUpdate: #10698 was merged, leaving: