On Sat, Feb 26, 2022 at 11:31 AM Daniel Latypov <dlatypov@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
Reviewed-by: David Gow <davidgow@xxxxxxxxxx>
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:
smime.p7s
Description: S/MIME Cryptographic Signature
