Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20200311222157.259707-1-davidgow@google.com>
In-Reply-To: <20200311222157.259707-1-davidgow@google.com>
From:   Brendan Higgins <brendanhiggins@google.com>
Date:   Wed, 25 Mar 2020 11:09:57 -0700
Message-ID: <CAFd5g47c+g_ufcOKEkKtCtj71nQtFwp40JuWajh_jPhQt50B_w@mail.gmail.com>
Subject: Re: [RFC PATCH kunit-next] kunit: kunit_tool: Separate out config/build/exec/parse
To:     David Gow <davidgow@google.com>
Cc:     "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@vger.kernel.org>,
        KUnit Development <kunit-dev@googlegroups.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Mar 11, 2020 at 3:23 PM David Gow <davidgow@google.com> wrote:
>
> Add new subcommands to kunit.py to allow stages of the existing 'run'
> subcommand to be run independently:
> - 'config': Verifies that .config is a subset of .kunitconfig
> - 'build': Compiles a UML kernel for KUnit
> - 'exec': Runs the kernel, and outputs the test results.
> - 'parse': Parses test results from a file or stdin

I think all the names are fine. I like the verb-noun pattern.

>
> Most of these are not hugely useful by themselves yet, and there's room
> for plenty of bikeshedding on the names, but this hopefully can form a
> foundation for future improvements.
>
> Signed-off-by: David Gow <davidgow@google.com>

This looks really good! I really only have one minor comment right
now, see below.

Reviewed-by: Brendan Higgins <brendanhiggins@google.com>

> ---
> [Whoops: typo-ed Brendan's email. Sorry about that!]
>
> As was briefly disccussed in [1], this change is part of a "separation
> of concerns" in kunit_tool. This should make it easier to integrate
> kunit_tool into other setups.
>
> Of particular intrest is probably 'kunit.py parse', which should allow
> KUnit results to be parsed from other sources, such as after loading a
> module, or from a non-UML kernel, or from debugfs when that's
> supported[2].
>
> [1]: https://lkml.org/lkml/2020/2/5/552
> [2]: https://patchwork.kernel.org/patch/11419901/
>
>  tools/testing/kunit/kunit.py           | 236 ++++++++++++++++++++-----
>  tools/testing/kunit/kunit_tool_test.py |  55 ++++++
>  2 files changed, 242 insertions(+), 49 deletions(-)
>
> diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py
> index 180ad1e1b04f..92a634594cf6 100755
> --- a/tools/testing/kunit/kunit.py
> +++ b/tools/testing/kunit/kunit.py

[...]

> @@ -147,6 +244,47 @@ def main(argv, linux=None):
>                 result = run_tests(linux, request)
>                 if result.status != KunitStatus.SUCCESS:
>                         sys.exit(1)
> +       elif cli_args.subcommand == 'config':
> +               request = KunitConfigRequest(cli_args.build_dir,
> +                                            cli_args.defconfig)
> +               result = config_tests(linux, request)
> +               kunit_parser.print_with_timestamp((
> +                       'Elapsed time: %.3fs\n') % (
> +                               result.elapsed_time))
> +               if result.status != KunitStatus.SUCCESS:
> +                       sys.exit(1)
> +       elif cli_args.subcommand == 'build':
> +               request = KunitBuildRequest(cli_args.jobs,
> +                                           cli_args.build_dir)
> +               result = build_tests(linux, request)
> +               kunit_parser.print_with_timestamp((
> +                       'Elapsed time: %.3fs\n') % (
> +                               result.elapsed_time))
> +               if result.status != KunitStatus.SUCCESS:
> +                       sys.exit(1)
> +       elif cli_args.subcommand == 'exec':
> +               exec_request = KunitExecRequest(cli_args.timeout,
> +                                               cli_args.build_dir)
> +               exec_result = exec_tests(linux, exec_request)
> +               parse_request = KunitParseRequest(cli_args.raw_output,
> +                                                 exec_result.result)
> +               result = parse_tests(parse_request)
> +               kunit_parser.print_with_timestamp((
> +                       'Elapsed time: %.3fs\n') % (
> +                               exec_result.elapsed_time))
> +               if result.status != KunitStatus.SUCCESS:
> +                       sys.exit(1)
> +       elif cli_args.subcommand == 'parse':
> +               if cli_args.file == '-':
> +                       kunit_output = sys.stdin

Could you make it so parse accepts the dmesg log from stdin if no file
is specified instead of a '-'?

> +               else:
> +                       with open(cli_args.file, 'r') as f:
> +                               kunit_output = f.read().splitlines()
> +               request = KunitParseRequest(cli_args.raw_output,
> +                                           kunit_output)
> +               result = parse_tests(request)
> +               if result.status != KunitStatus.SUCCESS:
> +                       sys.exit(1)
>         else:
>                 parser.print_help()
>

[...]