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