Add an argparse abbreviated mode to --failfast #23153

katesalazar commented at 10:52 AM on October 1, 2021: contributor

Happy Hacktoberfest, Bitcoin!

(this PR doesn't really pursue Hacktoberfest)

fanquake commented at 10:58 AM on October 1, 2021: member

Please follow the contributing guidelines. i.e write a proper commit message. Don't use the default GitHub message, it's not useful at all.

katesalazar force-pushed on Oct 1, 2021

DrahtBot added the label Tests on Oct 1, 2021

in test/functional/test_runner.py:347 in 36a0922c29 outdated

 343 | @@ -344,7 +344,7 @@ def main():
 344 |      parser.add_argument('--keepcache', '-k', action='store_true', help='the default behavior is to flush the cache directory on startup. --keepcache retains the cache from the previous testrun.')
 345 |      parser.add_argument('--quiet', '-q', action='store_true', help='only print dots, results summary and failure logs')
 346 |      parser.add_argument('--tmpdirprefix', '-t', default=tempfile.gettempdir(), help="Root directory for datadirs")
 347 | -    parser.add_argument('--failfast', action='store_true', help='stop execution after the first test failure')
 348 | +    parser.add_argument('--failfast', '-ff', action='store_true', help='stop execution after the first test failure')

luke-jr commented at 6:58 PM on October 3, 2021:

Short options should only be a single character.

luke-jr changes_requested

luke-jr approved

luke-jr commented at 9:37 PM on October 3, 2021: member

utACK

meshcollider commented at 2:08 AM on October 4, 2021: contributor

utACK

Please squash your commits (see here if you haven't done this before)

Btw, is this PR for hacktoberfest? (based on your PR description)

in test/functional/test_runner.py:347 in e792dfb176 outdated

 343 | @@ -344,7 +344,7 @@ def main():
 344 |      parser.add_argument('--keepcache', '-k', action='store_true', help='the default behavior is to flush the cache directory on startup. --keepcache retains the cache from the previous testrun.')
 345 |      parser.add_argument('--quiet', '-q', action='store_true', help='only print dots, results summary and failure logs')
 346 |      parser.add_argument('--tmpdirprefix', '-t', default=tempfile.gettempdir(), help="Root directory for datadirs")
 347 | -    parser.add_argument('--failfast', action='store_true', help='stop execution after the first test failure')
 348 | +    parser.add_argument('--failfast', '-F', action='store_true', help='stop execution after the first test failure')

fanquake commented at 2:13 AM on October 4, 2021:

Any reason this isn't lowercase, like all the other options?

luke-jr commented at 4:32 AM on October 4, 2021:

Lowercase -f would typically be reserved for "force", though... :/

katesalazar commented at 5:08 AM on October 4, 2021:

I'm seeing test/functional/wallet_hd.py help (in which the lowercase f is being used as short option) being automatically printed after requesting test/functional/test_runner.py -h.

Using an uppercase F here has been a precaution, but in this occasion I would be keen to switch it to lowercase here and deal with a potential arguments conflict later, once and if I'm sure it can happen.

BTW, it would appeal me to change this lowercase f in test_framework.py to an uppercase D (reserving lowercase d for --descriptors): https://github.com/bitcoin/bitcoin/blob/446b706696451ae1a66ac416f347d734c5741d7c/test/functional/test_framework/test_framework.py#L203

katesalazar commented at 5:49 AM on October 4, 2021:

Note https://stackoverflow.com/a/48798075.

katesalazar commented at 5:56 AM on October 4, 2021:

CC @josibake (168b6c317ca054c1287c36be532964e861f44266)

katesalazar commented at 8:52 AM on October 4, 2021:

I would see useful using an "s" for "stop" instead.

katesalazar commented at 4:42 AM on October 4, 2021: contributor

meshcollider wrote:

Btw, is this PR for hacktoberfest? (based on your PR description)

Not really, no. I have updated the text accordingly.

Add an argparse abbreviated mode to --failfast

Short options should only be a single character. If not, they can't be
concatenated in a single "-word" (from review by luke-jr).

F is chosen instead of f, because f could be reserved to the nested
wallet_hd.py (test_framework/test_framework.py) arguments parser.

2198f79e87

katesalazar force-pushed on Oct 4, 2021

theStack approved

theStack commented at 10:56 PM on November 10, 2021: member

Tested ACK 2198f79e87b3a8bfcda59e51225ab4cb789ccc53

MarcoFalke merged this on Nov 15, 2021

MarcoFalke closed this on Nov 15, 2021

sidhujag referenced this in commit e168f481a1 on Nov 15, 2021

PastaPastaPasta referenced this in commit d8c975ebc1 on Apr 3, 2022

DrahtBot locked this on Nov 15, 2022