Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

qa: Make TestNodeCLI command optional in send_cli #12089

Merged
merged 2 commits into from Jan 24, 2018

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Jan 4, 2018

Makes the command optional, since there are valid bitcoin-cli calls that have no command:

  • bitcoin-cli -?
  • bitcoin-cli -getinfo
  • ...

Also, rename self.args to self.options, since that is the name in the bitcoin-cli -help documentation.

@maflcko maflcko added the Tests label Jan 4, 2018
@maflcko maflcko changed the title qa: Don't remember TestNodeCLI options between calls qa: TestNodeCLI Rework output parsing, Prevent options mixups, Make command optional Jan 4, 2018
@maflcko maflcko closed this Jan 4, 2018
@maflcko maflcko deleted the Mf1801-qaCliOptions branch January 4, 2018 21:48
@maflcko maflcko changed the title qa: TestNodeCLI Rework output parsing, Prevent options mixups, Make command optional qa: make command optional Jan 12, 2018
@maflcko maflcko changed the title qa: make command optional qa: Make command optional Jan 12, 2018
@maflcko maflcko restored the Mf1801-qaCliOptions branch January 12, 2018 23:01
@maflcko maflcko changed the title qa: Make command optional qa: Make TestNodeCLI command optional in send_cli Jan 12, 2018
@maflcko maflcko reopened this Jan 12, 2018
@jnewbery
Copy link
Contributor

Tested ACK fab04c3f55715f8d63ae88d087e9ea83f525a670

Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK fab04c3f55715f8d63ae88d087e9ea83f525a670, but could be cleaned up slightly.

if named_args:
p_args += ["-named"]
p_args += [command] + pos_args + named_args
if command:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest s/command/command is not None/ so if 0 or False or [] or "" is passed, it will trigger appropriate python type errors or "method not found" error instead of "too few parameters" error.

@@ -213,16 +213,16 @@ class TestNodeCLI():
"""Interface to bitcoin-cli for an individual node"""

def __init__(self, binary, datadir):
self.args = []
self.options = []
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename seems appropriate, but ideally it would be in a separate commit, since it is bigger than the real change, and doesn't even overlap with it.

That is the name in bitcoin-cli -help
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK fae7b14. Thanks for update! Only changes since last review were what was suggested.

@jnewbery
Copy link
Contributor

Tested ACK fae7b14

@maflcko maflcko merged commit fae7b14 into bitcoin:master Jan 24, 2018
maflcko pushed a commit that referenced this pull request Jan 24, 2018
fae7b14 qa: Make TestNodeCLI command optional in send_cli (MarcoFalke)
ffffb10 qa: Rename cli.args to cli.options (MarcoFalke)

Pull request description:

  Makes the `command` optional, since there are valid bitcoin-cli calls that have no `command`:

  * `bitcoin-cli -?`
  * `bitcoin-cli -getinfo`
  * ...

  Also, rename self.args to self.options, since that is the name in the `bitcoin-cli -help` documentation.

Tree-SHA512: f49c06024e78423301d70782946d47c0fb97a26876afba0a1f71ed329f5d7124aee4c2df520c7af74079bf9937851902f7be9c54abecc28dc29274584804d46c
@maflcko maflcko deleted the Mf1801-qaCliOptions branch January 24, 2018 13:59
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Apr 7, 2020
fae7b14 qa: Make TestNodeCLI command optional in send_cli (MarcoFalke)
ffffb10 qa: Rename cli.args to cli.options (MarcoFalke)

Pull request description:

  Makes the `command` optional, since there are valid bitcoin-cli calls that have no `command`:

  * `bitcoin-cli -?`
  * `bitcoin-cli -getinfo`
  * ...

  Also, rename self.args to self.options, since that is the name in the `bitcoin-cli -help` documentation.

Tree-SHA512: f49c06024e78423301d70782946d47c0fb97a26876afba0a1f71ed329f5d7124aee4c2df520c7af74079bf9937851902f7be9c54abecc28dc29274584804d46c
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Apr 8, 2020
fae7b14 qa: Make TestNodeCLI command optional in send_cli (MarcoFalke)
ffffb10 qa: Rename cli.args to cli.options (MarcoFalke)

Pull request description:

  Makes the `command` optional, since there are valid bitcoin-cli calls that have no `command`:

  * `bitcoin-cli -?`
  * `bitcoin-cli -getinfo`
  * ...

  Also, rename self.args to self.options, since that is the name in the `bitcoin-cli -help` documentation.

Tree-SHA512: f49c06024e78423301d70782946d47c0fb97a26876afba0a1f71ed329f5d7124aee4c2df520c7af74079bf9937851902f7be9c54abecc28dc29274584804d46c
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants