* [PATCH] kunit: tool: more descriptive metavars/--help output
@ 2022-02-26 3:30 Daniel Latypov
2022-02-26 4:29 ` David Gow
0 siblings, 1 reply; 3+ messages in thread
From: Daniel Latypov @ 2022-02-26 3:30 UTC (permalink / raw)
To: brendanhiggins, davidgow
Cc: linux-kernel, kunit-dev, linux-kselftest, skhan, Daniel Latypov
Before, our help output contained lines like
--kconfig_add KCONFIG_ADD
--qemu_config qemu_config
--jobs jobs
They're not very helpful.
The former kind come from the automatic 'metavar' we get from argparse,
the uppsercase version of the flag name.
The latter are where we manually specified metavar as the flag name.
After:
--build_dir DIR
--make_options X=Y
--kunitconfig KUNITCONFIG
--kconfig_add CONFIG_X=Y
--arch ARCH
--cross_compile PREFIX
--qemu_config FILE
--jobs N
--timeout SECONDS
--raw_output [{all,kunit}]
--json [FILE]
This patch tries to make the code more clear by specifying the _type_ of
input we expect, e.g. --build_dir is a DIR, --qemu_config is a FILE.
I also switched it to uppercase since it looked more clearly like
placeholder text that way.
This patch also changes --raw_output to specify `choices` to make it
more clear what the options are, and this way argparse can validate it
for us, as shown by the added test case.
Signed-off-by: Daniel Latypov <dlatypov@google.com>
---
tools/testing/kunit/kunit.py | 26 ++++++++++++--------------
tools/testing/kunit/kunit_tool_test.py | 5 +++++
2 files changed, 17 insertions(+), 14 deletions(-)
diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py
index 9274c6355809..566404f5e42a 100755
--- a/tools/testing/kunit/kunit.py
+++ b/tools/testing/kunit/kunit.py
@@ -206,8 +206,6 @@ def parse_tests(request: KunitParseRequest, input_data: Iterable[str]) -> Tuple[
pass
elif request.raw_output == 'kunit':
output = kunit_parser.extract_tap_lines(output)
- else:
- print(f'Unknown --raw_output option "{request.raw_output}"', file=sys.stderr)
for line in output:
print(line.rstrip())
@@ -281,10 +279,10 @@ def add_common_opts(parser) -> None:
parser.add_argument('--build_dir',
help='As in the make command, it specifies the build '
'directory.',
- type=str, default='.kunit', metavar='build_dir')
+ type=str, default='.kunit', metavar='DIR')
parser.add_argument('--make_options',
help='X=Y make option, can be repeated.',
- action='append')
+ action='append', metavar='X=Y')
parser.add_argument('--alltests',
help='Run all KUnit tests through allyesconfig',
action='store_true')
@@ -292,11 +290,11 @@ def add_common_opts(parser) -> None:
help='Path to Kconfig fragment that enables KUnit tests.'
' If given a directory, (e.g. lib/kunit), "/.kunitconfig" '
'will get automatically appended.',
- metavar='kunitconfig')
+ metavar='KUNITCONFIG')
parser.add_argument('--kconfig_add',
help='Additional Kconfig options to append to the '
'.kunitconfig, e.g. CONFIG_KASAN=y. Can be repeated.',
- action='append')
+ action='append', metavar='CONFIG_X=Y')
parser.add_argument('--arch',
help=('Specifies the architecture to run tests under. '
@@ -304,7 +302,7 @@ def add_common_opts(parser) -> None:
'string passed to the ARCH make param, '
'e.g. i386, x86_64, arm, um, etc. Non-UML '
'architectures run on QEMU.'),
- type=str, default='um', metavar='arch')
+ type=str, default='um', metavar='ARCH')
parser.add_argument('--cross_compile',
help=('Sets make\'s CROSS_COMPILE variable; it should '
@@ -316,18 +314,18 @@ def add_common_opts(parser) -> None:
'if you have downloaded the microblaze toolchain '
'from the 0-day website to a directory in your '
'home directory called `toolchains`).'),
- metavar='cross_compile')
+ metavar='PREFIX')
parser.add_argument('--qemu_config',
help=('Takes a path to a path to a file containing '
'a QemuArchParams object.'),
- type=str, metavar='qemu_config')
+ type=str, metavar='FILE')
def add_build_opts(parser) -> None:
parser.add_argument('--jobs',
help='As in the make command, "Specifies the number of '
'jobs (commands) to run simultaneously."',
- type=int, default=get_default_jobs(), metavar='jobs')
+ type=int, default=get_default_jobs(), metavar='N')
def add_exec_opts(parser) -> None:
parser.add_argument('--timeout',
@@ -336,7 +334,7 @@ def add_exec_opts(parser) -> None:
'tests.',
type=int,
default=300,
- metavar='timeout')
+ metavar='SECONDS')
parser.add_argument('filter_glob',
help='Filter which KUnit test suites/tests run at '
'boot-time, e.g. list* or list*.*del_test',
@@ -346,7 +344,7 @@ def add_exec_opts(parser) -> None:
metavar='filter_glob')
parser.add_argument('--kernel_args',
help='Kernel command-line parameters. Maybe be repeated',
- action='append')
+ action='append', metavar='')
parser.add_argument('--run_isolated', help='If set, boot the kernel for each '
'individual suite/test. This is can be useful for debugging '
'a non-hermetic test, one that might pass/fail based on '
@@ -357,13 +355,13 @@ def add_exec_opts(parser) -> None:
def add_parse_opts(parser) -> None:
parser.add_argument('--raw_output', help='If set don\'t format output from kernel. '
'If set to --raw_output=kunit, filters to just KUnit output.',
- type=str, nargs='?', const='all', default=None)
+ type=str, nargs='?', const='all', default=None, choices=['all', 'kunit'])
parser.add_argument('--json',
nargs='?',
help='Stores test results in a JSON, and either '
'prints to stdout or saves to file if a '
'filename is specified',
- type=str, const='stdout', default=None)
+ type=str, const='stdout', default=None, metavar='FILE')
def main(argv, linux=None):
parser = argparse.ArgumentParser(
diff --git a/tools/testing/kunit/kunit_tool_test.py b/tools/testing/kunit/kunit_tool_test.py
index 352369dffbd9..eb2011d12c78 100755
--- a/tools/testing/kunit/kunit_tool_test.py
+++ b/tools/testing/kunit/kunit_tool_test.py
@@ -595,6 +595,11 @@ class KUnitMainTest(unittest.TestCase):
self.assertNotEqual(call, mock.call(StrContains('Testing complete.')))
self.assertNotEqual(call, mock.call(StrContains(' 0 tests run')))
+ def test_run_raw_output_invalid(self):
+ self.linux_source_mock.run_kernel = mock.Mock(return_value=[])
+ with self.assertRaises(SystemExit) as e:
+ kunit.main(['run', '--raw_output=invalid'], self.linux_source_mock)
+
def test_run_raw_output_does_not_take_positional_args(self):
# --raw_output is a string flag, but we don't want it to consume
# any positional arguments, only ones after an '='
base-commit: 5debe5bfa02c4c8922bd2d0f82c9c3a70bec8944
--
2.35.1.574.g5d30c73bfb-goog
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] kunit: tool: more descriptive metavars/--help output
2022-02-26 3:30 [PATCH] kunit: tool: more descriptive metavars/--help output Daniel Latypov
@ 2022-02-26 4:29 ` David Gow
2022-02-26 21:22 ` Daniel Latypov
0 siblings, 1 reply; 3+ messages in thread
From: David Gow @ 2022-02-26 4:29 UTC (permalink / raw)
To: Daniel Latypov
Cc: Brendan Higgins, Linux Kernel Mailing List, KUnit Development,
open list:KERNEL SELFTEST FRAMEWORK, Shuah Khan
[-- Attachment #1: Type: text/plain, Size: 9317 bytes --]
On Sat, Feb 26, 2022 at 11:31 AM Daniel Latypov <dlatypov@google.com> wrote:
>
> Before, our help output contained lines like
> --kconfig_add KCONFIG_ADD
> --qemu_config qemu_config
> --jobs jobs
>
> They're not very helpful.
>
> The former kind come from the automatic 'metavar' we get from argparse,
> the uppsercase version of the flag name.
Nit: "uppsercase" -> "uppercase"
> The latter are where we manually specified metavar as the flag name.
This was at least partly my fault: I didn't know what 'metavar' was
actually supposed to be, so assumed it was the name of the variable
the option set (hence them all being the same).
>
> After:
> --build_dir DIR
> --make_options X=Y
> --kunitconfig KUNITCONFIG
> --kconfig_add CONFIG_X=Y
> --arch ARCH
> --cross_compile PREFIX
> --qemu_config FILE
> --jobs N
> --timeout SECONDS
> --raw_output [{all,kunit}]
> --json [FILE]
>
> This patch tries to make the code more clear by specifying the _type_ of
> input we expect, e.g. --build_dir is a DIR, --qemu_config is a FILE.
> I also switched it to uppercase since it looked more clearly like
> placeholder text that way.
Looks good. I like all of these except possibly KUNITCONFIG, which I
think should probably be FILE, too.
>
> This patch also changes --raw_output to specify `choices` to make it
> more clear what the options are, and this way argparse can validate it
> for us, as shown by the added test case.
Excellent: that's much more discoverable (and the validation will no
doubt be useful).
>
> Signed-off-by: Daniel Latypov <dlatypov@google.com>
> ---
Reviewed-by: David Gow <davidgow@google.com>
Cheers,
-- David
> tools/testing/kunit/kunit.py | 26 ++++++++++++--------------
> tools/testing/kunit/kunit_tool_test.py | 5 +++++
> 2 files changed, 17 insertions(+), 14 deletions(-)
>
> diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py
> index 9274c6355809..566404f5e42a 100755
> --- a/tools/testing/kunit/kunit.py
> +++ b/tools/testing/kunit/kunit.py
> @@ -206,8 +206,6 @@ def parse_tests(request: KunitParseRequest, input_data: Iterable[str]) -> Tuple[
> pass
> elif request.raw_output == 'kunit':
> output = kunit_parser.extract_tap_lines(output)
> - else:
> - print(f'Unknown --raw_output option "{request.raw_output}"', file=sys.stderr)
> for line in output:
> print(line.rstrip())
>
> @@ -281,10 +279,10 @@ def add_common_opts(parser) -> None:
> parser.add_argument('--build_dir',
> help='As in the make command, it specifies the build '
> 'directory.',
> - type=str, default='.kunit', metavar='build_dir')
> + type=str, default='.kunit', metavar='DIR')
> parser.add_argument('--make_options',
> help='X=Y make option, can be repeated.',
> - action='append')
> + action='append', metavar='X=Y')
> parser.add_argument('--alltests',
> help='Run all KUnit tests through allyesconfig',
> action='store_true')
> @@ -292,11 +290,11 @@ def add_common_opts(parser) -> None:
> help='Path to Kconfig fragment that enables KUnit tests.'
> ' If given a directory, (e.g. lib/kunit), "/.kunitconfig" '
> 'will get automatically appended.',
> - metavar='kunitconfig')
> + metavar='KUNITCONFIG')
Is it worth making this something like FILE or PATH instead.
PATH_TO_KUNITCONFIG would be verbose, but this is a path being given,
so just KUNITCONFIG is still a bit useless.
> parser.add_argument('--kconfig_add',
> help='Additional Kconfig options to append to the '
> '.kunitconfig, e.g. CONFIG_KASAN=y. Can be repeated.',
> - action='append')
> + action='append', metavar='CONFIG_X=Y')
>
> parser.add_argument('--arch',
> help=('Specifies the architecture to run tests under. '
> @@ -304,7 +302,7 @@ def add_common_opts(parser) -> None:
> 'string passed to the ARCH make param, '
> 'e.g. i386, x86_64, arm, um, etc. Non-UML '
> 'architectures run on QEMU.'),
> - type=str, default='um', metavar='arch')
> + type=str, default='um', metavar='ARCH')
>
> parser.add_argument('--cross_compile',
> help=('Sets make\'s CROSS_COMPILE variable; it should '
> @@ -316,18 +314,18 @@ def add_common_opts(parser) -> None:
> 'if you have downloaded the microblaze toolchain '
> 'from the 0-day website to a directory in your '
> 'home directory called `toolchains`).'),
> - metavar='cross_compile')
> + metavar='PREFIX')
>
> parser.add_argument('--qemu_config',
> help=('Takes a path to a path to a file containing '
> 'a QemuArchParams object.'),
> - type=str, metavar='qemu_config')
> + type=str, metavar='FILE')
>
> def add_build_opts(parser) -> None:
> parser.add_argument('--jobs',
> help='As in the make command, "Specifies the number of '
> 'jobs (commands) to run simultaneously."',
> - type=int, default=get_default_jobs(), metavar='jobs')
> + type=int, default=get_default_jobs(), metavar='N')
>
> def add_exec_opts(parser) -> None:
> parser.add_argument('--timeout',
> @@ -336,7 +334,7 @@ def add_exec_opts(parser) -> None:
> 'tests.',
> type=int,
> default=300,
> - metavar='timeout')
> + metavar='SECONDS')
> parser.add_argument('filter_glob',
> help='Filter which KUnit test suites/tests run at '
> 'boot-time, e.g. list* or list*.*del_test',
> @@ -346,7 +344,7 @@ def add_exec_opts(parser) -> None:
> metavar='filter_glob')
> parser.add_argument('--kernel_args',
> help='Kernel command-line parameters. Maybe be repeated',
> - action='append')
> + action='append', metavar='')
> parser.add_argument('--run_isolated', help='If set, boot the kernel for each '
> 'individual suite/test. This is can be useful for debugging '
> 'a non-hermetic test, one that might pass/fail based on '
> @@ -357,13 +355,13 @@ def add_exec_opts(parser) -> None:
> def add_parse_opts(parser) -> None:
> parser.add_argument('--raw_output', help='If set don\'t format output from kernel. '
> 'If set to --raw_output=kunit, filters to just KUnit output.',
> - type=str, nargs='?', const='all', default=None)
> + type=str, nargs='?', const='all', default=None, choices=['all', 'kunit'])
> parser.add_argument('--json',
> nargs='?',
> help='Stores test results in a JSON, and either '
> 'prints to stdout or saves to file if a '
> 'filename is specified',
> - type=str, const='stdout', default=None)
> + type=str, const='stdout', default=None, metavar='FILE')
>
> def main(argv, linux=None):
> parser = argparse.ArgumentParser(
> diff --git a/tools/testing/kunit/kunit_tool_test.py b/tools/testing/kunit/kunit_tool_test.py
> index 352369dffbd9..eb2011d12c78 100755
> --- a/tools/testing/kunit/kunit_tool_test.py
> +++ b/tools/testing/kunit/kunit_tool_test.py
> @@ -595,6 +595,11 @@ class KUnitMainTest(unittest.TestCase):
> self.assertNotEqual(call, mock.call(StrContains('Testing complete.')))
> self.assertNotEqual(call, mock.call(StrContains(' 0 tests run')))
>
> + def test_run_raw_output_invalid(self):
> + self.linux_source_mock.run_kernel = mock.Mock(return_value=[])
> + with self.assertRaises(SystemExit) as e:
> + kunit.main(['run', '--raw_output=invalid'], self.linux_source_mock)
> +
> def test_run_raw_output_does_not_take_positional_args(self):
> # --raw_output is a string flag, but we don't want it to consume
> # any positional arguments, only ones after an '='
>
> base-commit: 5debe5bfa02c4c8922bd2d0f82c9c3a70bec8944
> --
> 2.35.1.574.g5d30c73bfb-goog
>
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] kunit: tool: more descriptive metavars/--help output
2022-02-26 4:29 ` David Gow
@ 2022-02-26 21:22 ` Daniel Latypov
0 siblings, 0 replies; 3+ messages in thread
From: Daniel Latypov @ 2022-02-26 21:22 UTC (permalink / raw)
To: David Gow
Cc: Brendan Higgins, Linux Kernel Mailing List, KUnit Development,
open list:KERNEL SELFTEST FRAMEWORK, Shuah Khan
On Fri, Feb 25, 2022 at 8:29 PM David Gow <davidgow@google.com> wrote:
> > @@ -292,11 +290,11 @@ def add_common_opts(parser) -> None:
> > help='Path to Kconfig fragment that enables KUnit tests.'
> > ' If given a directory, (e.g. lib/kunit), "/.kunitconfig" '
> > 'will get automatically appended.',
> > - metavar='kunitconfig')
> > + metavar='KUNITCONFIG')
>
> Is it worth making this something like FILE or PATH instead.
> PATH_TO_KUNITCONFIG would be verbose, but this is a path being given,
> so just KUNITCONFIG is still a bit useless.
Here's what the complete help output looks like right now:
--kunitconfig KUNITCONFIG
Path to Kconfig fragment that enables KUnit tests. If
given a directory, (e.g. lib/kunit), "/.kunitconfig"
will get automatically appended.
So I didn't think it mattered too much if we left it as-is.
But I like PATH (since users can use a dir, FILE felt potentially
misleading, even if technically correct).
Will send a v2 with the typo in the commit fixed and this changed to PATH.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-02-26 21:23 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-26 3:30 [PATCH] kunit: tool: more descriptive metavars/--help output Daniel Latypov
2022-02-26 4:29 ` David Gow
2022-02-26 21:22 ` Daniel Latypov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.