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

tests: Fix value display name in test_runner help text #14619

Merged
merged 1 commit into from Nov 6, 2018

Conversation

Projects
None yet
5 participants
@merland
Copy link
Contributor

commented Oct 31, 2018

The help text given by test/functional/test_runner.py -h refers to the value n, which is defined as COMBINEDLOGSLEN in the list of commands.

To make the help text consistent, this PR changes the display name COMBINEDLOGSLEN to n by setting the argparse metavar attribute. (metavar only changes the displayed name)

Alternatively: Do the opposite and change the help text to use COMBINEDLOGSLEN.


Before PR:

➜  bitcoin > test/functional/test_runner.py -h | grep -A 1 combinedlogslen
  --combinedlogslen COMBINEDLOGSLEN, -c COMBINEDLOGSLEN
                        print a combined log (of length n lines) from all test nodes and test framework to the console on failure.

After PR:

➜  bitcoin > test/functional/test_runner.py -h | grep -A 1 combinedlogslen
  --combinedlogslen n, -c n
                        print a combined log (of length n lines) from all test nodes and test frameworks to the console on failure.

Also, fixed pluralization typo.

@practicalswift

This comment has been minimized.

Copy link
Member

commented Oct 31, 2018

Concept ACK

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Nov 1, 2018

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

test/functional/test_runner.py Outdated
@@ -214,7 +214,7 @@ def main():
epilog='''
Help text and arguments for individual test script:''',
formatter_class=argparse.RawTextHelpFormatter)
parser.add_argument('--combinedlogslen', '-c', type=int, default=0, help='print a combined log (of length n lines) from all test nodes and test framework to the console on failure.')
parser.add_argument('--combinedlogslen', '-c', type=int, default=0, metavar='n', help='print a combined log (of length n lines) from all test nodes and test frameworks to the console on failure.')

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Nov 5, 2018

Member
Suggested change
parser.add_argument('--combinedlogslen', '-c', type=int, default=0, metavar='n', help='print a combined log (of length n lines) from all test nodes and test frameworks to the console on failure.')
parser.add_argument('--combinedlogslen', '-c', type=int, default=0, metavar='n', help='print a combined log (of length n lines) from all test nodes and test framework to the console on failure.')

There is only one test framework

This comment has been minimized.

Copy link
@merland

merland Nov 6, 2018

Author Contributor

Thanks. I think I misread due to the fact that there is no "the". For better readability, I suggest to apply your change and then switch the order:
from the test framework all test nodes
I'll update the commit.

This comment has been minimized.

Copy link
@merland

merland Nov 6, 2018

Author Contributor

Realized the whole sentence was a bit complicated, so I rephrased slightly. Hopefully more readable now.
@MarcoFalke

@merland merland force-pushed the merland:test-runner-fix branch to 5a05aa2 Nov 6, 2018

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Nov 6, 2018

Merge bitcoin#14619: tests: Fix value display name in test_runner hel…
…p text

5a05aa2 Add metavar to match var name in help text + Change wording for better readability (Martin Erlandsson)

Pull request description:

  The help text given by `test/functional/test_runner.py -h` refers to the value `n`, which is defined as `COMBINEDLOGSLEN` in the list of commands.

  To make the help text consistent, this PR changes the display name `COMBINEDLOGSLEN` to `n` by setting the argparse [`metavar`](https://docs.python.org/3/library/argparse.html#metavar) attribute. (`metavar` only changes the _displayed_ name)

  Alternatively: Do the opposite and change the help text to use `COMBINEDLOGSLEN`.

  ---

  Before PR:
  ```
  ➜  bitcoin > test/functional/test_runner.py -h | grep -A 1 combinedlogslen
    --combinedlogslen COMBINEDLOGSLEN, -c COMBINEDLOGSLEN
                          print a combined log (of length n lines) from all test nodes and test framework to the console on failure.
  ```

  After PR:
  ```
  ➜  bitcoin > test/functional/test_runner.py -h | grep -A 1 combinedlogslen
    --combinedlogslen n, -c n
                          print a combined log (of length n lines) from all test nodes and test frameworks to the console on failure.
  ```
  ---
  Also, fixed pluralization typo.

Tree-SHA512: a1124a4976d29fae1e8ecd7fa2ac523b7f05d541c611166532f44692995691a96faf797fa71582d78634f328b500cbee49c6ef296c8f1a898a57c050cc4e721d

@MarcoFalke MarcoFalke merged commit 5a05aa2 into bitcoin:master Nov 6, 2018

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@merland merland deleted the merland:test-runner-fix branch Nov 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.