Re: [PATCH 1/5] kunit: tool: drop mostly unused KunitResult.result field

On Thu, Jan 20, 2022 at 12:29 AM David Gow davidgow@google.com wrote:

On Wed, Jan 19, 2022 at 3:09 AM Daniel Latypov dlatypov@google.com wrote:

...

This field is only used to pass along the parsed Test object from parse_tests(). Everywhere else the `result` field is ignored.

Instead make parse_tests() explicitly return a KunitResult and Test so we can retire the `result` field.

Signed-off-by: Daniel Latypov dlatypov@google.com

I personally prefer having the Test as part of the result -- it gives a slightly rust-esque sense of needing to check the actual result before using anything that's parsed. (Also, I'm still not used to the whole multiple return value thing, which is not as clear as an explicit named struct member, IMHO). That being said, we're not actually checking the result before using the Test, and certainly the use of Any and mashing a textual error message in the same field is rather unpleasant.

My ideal solution would be to rename 'result' to something more sensible ('parsed_test', maybe?), and make it explicitly a Test rather than Any (and either add a separate field for the textual error message, or remove it as in this patch, having noticed that it's almost completely redundant to the enum).

Yeah, I considered that for a bit, but I don't like having a field that is only sometimes set. I had thought we were passing back the test object from exec_tests(), but I was wrong. We were actually passing back a KunitResult with a KunitResult[Test] in it.

So when I saw only parse_tests() actually wanted to pass back a test object, I thought it was cleaner to just use a separate return value.

That being said, I can live with the current solution, but'd ideally like a comment or something to make the return value Tuple a bit more obvious.

A comment to explain that Tuple == multiple return values from a func? Or something else?

Also ah, I thought we had more instances of multiple return in kunit.py. Looks like the only other is get_source_tree_ops_from_qemu_config(). isolate_ktap_output() technically shows this off as well, but via yields.

Thoughts?

-- David

...

tools/testing/kunit/kunit.py | 24 ++++++++---------------- 1 file changed, 8 insertions(+), 16 deletions(-)

diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py index 7a706f96f68d..9274c6355809 100755 --- a/tools/testing/kunit/kunit.py +++ b/tools/testing/kunit/kunit.py @@ -17,7 +17,7 @@ assert sys.version_info >= (3, 7), "Python version is too old"

from dataclasses import dataclass from enum import Enum, auto -from typing import Any, Iterable, Sequence, List, Optional +from typing import Iterable, List, Optional, Sequence, Tuple

import kunit_json import kunit_kernel @@ -32,7 +32,6 @@ class KunitStatus(Enum): @dataclass class KunitResult: status: KunitStatus

•   result: Any
    elapsed_time: float
  
  

@dataclass @@ -82,10 +81,8 @@ def config_tests(linux: kunit_kernel.LinuxSourceTree, config_end = time.time() if not success: return KunitResult(KunitStatus.CONFIG_FAILURE,

•                              'could not configure kernel',
                               config_end - config_start)
    return KunitResult(KunitStatus.SUCCESS,
  

•                      'configured kernel successfully',
                       config_end - config_start)
  
  

def build_tests(linux: kunit_kernel.LinuxSourceTree, @@ -100,14 +97,11 @@ def build_tests(linux: kunit_kernel.LinuxSourceTree, build_end = time.time() if not success: return KunitResult(KunitStatus.BUILD_FAILURE,

•                              'could not build kernel',
                               build_end - build_start)
    if not success:
            return KunitResult(KunitStatus.BUILD_FAILURE,
  

•                              'could not build kernel',
                               build_end - build_start)
    return KunitResult(KunitStatus.SUCCESS,
  

•                      'built kernel successfully',
                       build_end - build_start)
  
  

def config_and_build_tests(linux: kunit_kernel.LinuxSourceTree, @@ -173,14 +167,14 @@ def exec_tests(linux: kunit_kernel.LinuxSourceTree, request: KunitExecRequest) - filter_glob=filter_glob, build_dir=request.build_dir)

•           result = parse_tests(request, run_result)
  

•           _, test_result = parse_tests(request, run_result)
            # run_kernel() doesn't block on the kernel exiting.
            # That only happens after we get the last line of output from `run_result`.
            # So exec_time here actually contains parsing + execution time, which is fine.
            test_end = time.time()
            exec_time += test_end - test_start
  
  

•           test_counts.add_subtest_counts(result.result.counts)
  

•           test_counts.add_subtest_counts(test_result.counts)
  
    if len(filter_globs) == 1 and test_counts.crashed > 0:
            bd = request.build_dir
  

@@ -189,7 +183,7 @@ def exec_tests(linux: kunit_kernel.LinuxSourceTree, request: KunitExecRequest) - bd, bd, kunit_kernel.get_outfile_path(bd), bd, sys.argv[0]))

    kunit_status = _map_to_overall_status(test_counts.get_status())

•   return KunitResult(status=kunit_status, result=result, elapsed_time=exec_time)
  

•   return KunitResult(status=kunit_status, elapsed_time=exec_time)
  
  

def _map_to_overall_status(test_status: kunit_parser.TestStatus) -> KunitStatus: if test_status in (kunit_parser.TestStatus.SUCCESS, kunit_parser.TestStatus.SKIPPED): @@ -197,7 +191,7 @@ def _map_to_overall_status(test_status: kunit_parser.TestStatus) -> KunitStatus: else: return KunitStatus.TEST_FAILURE

-def parse_tests(request: KunitParseRequest, input_data: Iterable[str]) -> KunitResult: +def parse_tests(request: KunitParseRequest, input_data: Iterable[str]) -> Tuple[KunitResult, kunit_parser.Test]: parse_start = time.time()

    test_result = kunit_parser.Test()

@@ -231,11 +225,9 @@ def parse_tests(request: KunitParseRequest, input_data: Iterable[str]) -> KunitR print(json_obj)

    if test_result.status != kunit_parser.TestStatus.SUCCESS:

•           return KunitResult(KunitStatus.TEST_FAILURE, test_result,
  

•                              parse_end - parse_start)
  

•           return KunitResult(KunitStatus.TEST_FAILURE, parse_end - parse_start), test_result
  
  

•   return KunitResult(KunitStatus.SUCCESS, test_result,
  

•                           parse_end - parse_start)
  

•   return KunitResult(KunitStatus.SUCCESS, parse_end - parse_start), test_result
  
  

def run_tests(linux: kunit_kernel.LinuxSourceTree, request: KunitRequest) -> KunitResult: @@ -513,7 +505,7 @@ def main(argv, linux=None): request = KunitParseRequest(raw_output=cli_args.raw_output, build_dir='', json=cli_args.json)

•           result = parse_tests(request, kunit_output)
  

•           result, _ = parse_tests(request, kunit_output)
            if result.status != KunitStatus.SUCCESS:
                    sys.exit(1)
    else:
  
  

base-commit: f079ab01b5609fb0c9acc52c88168bf1eed82373

2.34.1.703.g22d0c6ccf7-goog