tests: Convert selected tests to using named RPC arguments

laanwj commented at 8:53 am on March 13, 2017: member

Convert selected tests to use named arguments in RPC calls. This covers a few of the important ones:

assumevalid.py
blockchain.py
merkle_blocks.py
segwit.py

As well as the test framework itself, util.py.

This makes invocations easier to read especially if booleans or lots of arguments are involved.

Reviewing

To review I’d suggest using the command:

git diff --word-diff --word-diff-regex='[^[:space:],\(\)=]+'

This will regard the added argument names as one word, making it trivial to see what is added.

Process

As doing this is a bit of a finnicky process, I’m doing this a few tests at a time. I may add more to this PR later. In case people want to help, I created this list of RPC calls with argument names to avoid having to refer to the individual help all the time.

To make sure a test has no use of positional arguments left you can add these lines to __call__(self, *args, **argsn) in authproxy.py:

0        if args:
1            raise ValueError('TEST: supporting named arguments')

Every use of positional arguments will then raise that exception.

tests: Convert selected tests to named arguments

Convert selected tests to use named arguments in RPC calls.
This covers:

- `assumevalid.py`
- `blockchain.py`
- `merkle_blocks.py`
- `segwit.py`

As well as the test framework itself, `util.py`.

This makes invocations easier to read especially if booleans or lots of
arguments are involved.

To review I'd suggest using the command:

    git diff --word-diff --word-diff-regex='[^[:space:],\(\)=]+'

This will regard the added argument names as one words, making it easy
to see what is added.

01217188c9

laanwj added the label RPC/REST/ZMQ on Mar 13, 2017

MarcoFalke commented at 11:21 am on March 13, 2017: member

This makes invocations easier to read especially if booleans or lots of arguments are involved.

Concept ACK. Though I think we don’t need named args for calls that only have a single and obvious param, such as generate( $num ) or any single param call that starts with set*.

To aid reviewers even more, maybe in future pulls, a project wide search and replace of eligible calls makes sense. Potentially one s&r per commit, but no strong opinion on this.

On Mon, Mar 13, 2017 at 9:54 AM, Wladimir J. van der Laan notifications@github.com wrote:

Convert selected tests to use named arguments in RPC calls. This covers a few of the important ones:

assumevalid.py blockchain.py merkle_blocks.py segwit.py

As well as the test framework itself, util.py.

This makes invocations easier to read especially if booleans or lots of arguments are involved.

Reviewing

To review I’d suggest using the command:

git diff –word-diff –word-diff-regex=’[^[:space:],()=]+'

This will regard the added argument names as one word, making it trivial to see what is added.

Process

As doing this is a bit of a finnicky process, I’m doing this a few tests at a time. I may add more to this PR later. In case people want to help, I created this list of RPC calls with argument names to avoid having to refer to the individual help all the time.

To make sure a test has no use of positional arguments left you can add these lines to call(self, *args, **argsn) in authproxy.py:
    if args:
        raise ValueError('TEST: supporting named arguments')
Every use of positional arguments will then raise that exception.

You can view, comment on, or merge this pull request online at:

#9983

Commit Summary

tests: Convert selected tests to named arguments

File Changes

M qa/rpc-tests/assumevalid.py (8) M qa/rpc-tests/blockchain.py (8) M qa/rpc-tests/merkle_blocks.py (46) M qa/rpc-tests/segwit.py (176) M qa/rpc-tests/test_framework/util.py (14)

Patch Links:

#9983.patch #9983.diff

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

laanwj commented at 1:31 pm on March 13, 2017: member

Concept ACK. Though I think we don’t need named args for calls that only have a single and obvious param, such as generate( $num ) or any single param call that starts with set*.

I’ve thought about that. But some of the functions called with one argument are actually multi-arg functions of which only the first is used. And it may happen that more arguments get added to a function in the future. So I thought it better to be consistent and do all of them to move to a fully name-based API.

To aid reviewers even more, maybe in future pulls, a project wide search and replace of eligible calls makes sense. Potentially one s&r per commit, but no strong opinion on this.

Yeah, maybe. The advantage of doing it per test is that it is possible to verify that all calls have been converted without having to convert the entire project, which would conflict with every possible PR that changes tests. I do agree it makes it easier to review per commit.

MarcoFalke commented at 2:10 pm on March 13, 2017: member

And it may happen that more arguments get added to a function in the future. So I thought it better to be consistent and do all of them to move to a fully name-based API.

Hmm. As long as downstream projects use the interface with positional args, we can’t do that. Though, maybe we should require named args for new calls and whenever we extend an existing API call to encourage the name-based API and then switch completely at some point in the future?

On Mon, Mar 13, 2017 at 2:31 PM, Wladimir J. van der Laan notifications@github.com wrote:

Concept ACK. Though I think we don’t need named args for calls that only have a single and obvious param, such as generate( $num ) or any single param call that starts with set*.

I’ve thought about that. But some of the functions called with one argument are actually multi-arg functions of which only the first is used. And it may happen that more arguments get added to a function in the future. So I thought it better to be consistent and do all of them to move to a fully name-based API.

To aid reviewers even more, maybe in future pulls, a project wide search and replace of eligible calls makes sense. Potentially one s&r per commit, but no strong opinion on this.

Yeah, maybe. The advantage of doing it per test is that it is possible to verify that all calls have been converted without having to convert the entire project, which would conflict with every possible PR that changes tests.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

laanwj commented at 9:58 am on March 16, 2017: member

Hmm. As long as downstream projects use the interface with positional args, we can’t do that.

I don’t say we should deprecate the positional arguments any time soon, just try to use it as our own best practice to use named arguments. Mixing them is kind of messy, IMO.

laanwj commented at 12:44 pm on March 24, 2017: member

Closing, too much work to rebase all the time.

laanwj closed this on Mar 24, 2017

DrahtBot locked this on Sep 8, 2021

tests: Convert selected tests to using named RPC arguments #9983

Reviewing

Process