Re: [PATCH] kunit: tool: more descriptive metavars/--help output

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Share Photos]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux