On Fri, Feb 18, 2022 at 4:52 AM Daniel Latypov dlatypov@google.com wrote:

When using --json, kunit.py run/exec/parse will produce results in KernelCI json format. As part of that, we include the build_dir that was used, and we (incorrectly) hardcode in the arch, etc.

We'll want a way to plumb more values (as well as the correct `arch`), so this patch groups those fields into kunit_json.Metadata type. This patch should have no user visible changes.

And since we only used build_dir in KunitParseRequest for json, we can now move it out of that struct and add it into KunitExecRequest, which needs it and used to get it via inheritance.

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

This is much nicer, thanks.

Reviewed-by: David Gow davidgow@google.com

-- David

tools/testing/kunit/kunit.py | 16 +++++++------- tools/testing/kunit/kunit_json.py | 29 ++++++++++++++++++-------- tools/testing/kunit/kunit_tool_test.py | 9 ++++---- 3 files changed, 33 insertions(+), 21 deletions(-)

diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py index 4cb91d191f1d..7dd6ed42141f 100755 --- a/tools/testing/kunit/kunit.py +++ b/tools/testing/kunit/kunit.py @@ -47,11 +47,11 @@ class KunitBuildRequest(KunitConfigRequest): @dataclass class KunitParseRequest: raw_output: Optional[str]

•   build_dir: str
    json: Optional[str]
  
  

@dataclass class KunitExecRequest(KunitParseRequest):

•   build_dir: str
    timeout: int
    alltests: bool
    filter_glob: str
  

@@ -153,6 +153,8 @@ def exec_tests(linux: kunit_kernel.LinuxSourceTree, request: KunitExecRequest) - test_glob = request.filter_glob.split('.', maxsplit=2)[1] filter_globs = [g + '.'+ test_glob for g in filter_globs]

•   metadata = kunit_json.Metadata(build_dir=request.build_dir)
  

•   test_counts = kunit_parser.TestCounts()
    exec_time = 0.0
    for i, filter_glob in enumerate(filter_globs):
  

@@ -165,7 +167,7 @@ def exec_tests(linux: kunit_kernel.LinuxSourceTree, request: KunitExecRequest) - filter_glob=filter_glob, build_dir=request.build_dir)

•           _, test_result = parse_tests(request, run_result)
  

•           _, test_result = parse_tests(request, metadata, 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.
  

@@ -189,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]) -> Tuple[KunitResult, kunit_parser.Test]: +def parse_tests(request: KunitParseRequest, metadata: kunit_json.Metadata, input_data: Iterable[str]) -> Tuple[KunitResult, kunit_parser.Test]: parse_start = time.time()

    test_result = kunit_parser.Test()

@@ -216,8 +218,7 @@ def parse_tests(request: KunitParseRequest, input_data: Iterable[str]) -> Tuple[ if request.json: json_str = kunit_json.get_json_result( test=test_result,

•                                   def_config='kunit_defconfig',
  

•                                   build_dir=request.build_dir)
  

•                                   metadata=metadata)
            if request.json == 'stdout':
                    print(json_str)
            else:
  

@@ -504,10 +505,11 @@ def main(argv, linux=None): else: with open(cli_args.file, 'r', errors='backslashreplace') as f: kunit_output = f.read().splitlines()

•           # We know nothing about how the result was created!
  

•           metadata = kunit_json.Metadata()
            request = KunitParseRequest(raw_output=cli_args.raw_output,
  

•                                       build_dir='',
                                        json=cli_args.json)
  

•           result, _ = parse_tests(request, kunit_output)
  

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

diff --git a/tools/testing/kunit/kunit_json.py b/tools/testing/kunit/kunit_json.py index 14a480d3308a..0a7e29a315ed 100644 --- a/tools/testing/kunit/kunit_json.py +++ b/tools/testing/kunit/kunit_json.py @@ -6,6 +6,7 @@ # Copyright (C) 2020, Google LLC. # Author: Heidi Fahim heidifahim@google.com

+from dataclasses import dataclass import json import os

@@ -14,6 +15,13 @@ import kunit_parser from kunit_parser import Test, TestStatus from typing import Any, Dict

+@dataclass +class Metadata:

•   """Stores metadata about this run to include in get_json_result()."""
  

•   arch: str = 'UM'
  

•   def_config: str = 'kunit_defconfig'
  

•   build_dir: str = ''
  

JsonObj = Dict[str, Any]

_status_map: Dict[TestStatus, str] = { @@ -22,14 +30,13 @@ _status_map: Dict[TestStatus, str] = { TestStatus.TEST_CRASHED: "ERROR", }

-def _get_group_json(test: Test, def_config: str, build_dir: str) -> JsonObj: +def _get_group_json(test: Test, common_fields: JsonObj) -> JsonObj: sub_groups = [] # List[JsonObj] test_cases = [] # List[JsonObj]

    for subtest in test.subtests:
            if subtest.subtests:

•                   sub_group = _get_group_json(subtest, def_config,
  

•                           build_dir)
  

•                   sub_group = _get_group_json(subtest, common_fields)
                    sub_groups.append(sub_group)
                    continue
            status = _status_map.get(subtest.status, "FAIL")
  

@@ -37,19 +44,23 @@ def _get_group_json(test: Test, def_config: str, build_dir: str) -> JsonObj:

    test_group = {
            "name": test.name,

•           "arch": "UM",
  

•           "defconfig": def_config,
  

•           "build_environment": build_dir,
            "sub_groups": sub_groups,
            "test_cases": test_cases,
  

•   }
  

•   test_group.update(common_fields)
  

•   return test_group
  

+def get_json_result(test: Test, metadata: Metadata) -> str:

•   common_fields = {
  

•           "arch": metadata.arch,
  

•           "defconfig": metadata.def_config,
  

•           "build_environment": metadata.build_dir,
            "lab_name": None,
            "kernel": None,
            "job": None,
            "git_branch": "kselftest",
    }
  

•   return test_group
  
  

-def get_json_result(test: Test, def_config: str, build_dir: str) -> str:

•   test_group = _get_group_json(test, def_config, build_dir)
  

•   test_group = _get_group_json(test, common_fields)
    test_group["name"] = "KUnit Test Group"
    return json.dumps(test_group, indent=4)
  

diff --git a/tools/testing/kunit/kunit_tool_test.py b/tools/testing/kunit/kunit_tool_test.py index a3c036a620b2..60806994683c 100755 --- a/tools/testing/kunit/kunit_tool_test.py +++ b/tools/testing/kunit/kunit_tool_test.py @@ -468,8 +468,7 @@ class KUnitJsonTest(unittest.TestCase): test_result = kunit_parser.parse_run_tests(file) json_obj = kunit_json.get_json_result( test=test_result,

•                           def_config='kunit_defconfig',
  

•                           build_dir='.kunit')
  

•                           metadata=kunit_json.Metadata())
            return json.loads(json_obj)
  
    def test_failed_test_json(self):
  

@@ -691,7 +690,7 @@ class KUnitMainTest(unittest.TestCase): self.linux_source_mock.run_kernel.return_value = ['TAP version 14', 'init: random output'] + want

            got = kunit._list_tests(self.linux_source_mock,

•                                kunit.KunitExecRequest(None, '.kunit', None, 300, False, 'suite*', None, 'suite'))
  

•                                kunit.KunitExecRequest(None, None, '.kunit', 300, False, 'suite*', None, 'suite'))
  
            self.assertEqual(got, want)
            # Should respect the user's filter glob when listing tests.
  

@@ -706,7 +705,7 @@ class KUnitMainTest(unittest.TestCase):

            # Should respect the user's filter glob when listing tests.
            mock_tests.assert_called_once_with(mock.ANY,

•                                kunit.KunitExecRequest(None, '.kunit', None, 300, False, 'suite*.test*', None, 'suite'))
  

•                                kunit.KunitExecRequest(None, None, '.kunit', 300, False, 'suite*.test*', None, 'suite'))
            self.linux_source_mock.run_kernel.assert_has_calls([
                    mock.call(args=None, build_dir='.kunit', filter_glob='suite.test*', timeout=300),
                    mock.call(args=None, build_dir='.kunit', filter_glob='suite2.test*', timeout=300),
  

@@ -719,7 +718,7 @@ class KUnitMainTest(unittest.TestCase):

            # Should respect the user's filter glob when listing tests.
            mock_tests.assert_called_once_with(mock.ANY,

•                                kunit.KunitExecRequest(None, '.kunit', None, 300, False, 'suite*', None, 'test'))
  

•                                kunit.KunitExecRequest(None, None, '.kunit', 300, False, 'suite*', None, 'test'))
            self.linux_source_mock.run_kernel.assert_has_calls([
                    mock.call(args=None, build_dir='.kunit', filter_glob='suite.test1', timeout=300),
                    mock.call(args=None, build_dir='.kunit', filter_glob='suite.test2', timeout=300),
  

-- 2.35.1.473.g83b2b277ed-goog

On Fri, Feb 18, 2022 at 4:52 AM Daniel Latypov dlatypov@google.com wrote:

Before, kunit.py always printed "arch": "UM" in its json output, but...

1. With `kunit.py parse`, we could be parsing output from anywhere, so we can't say that.
2. Capitalizing it is probably wrong, as it's `ARCH=um`
3. Commit 87c9c1631788 ("kunit: tool: add support for QEMU") made it so kunit.py could knowingly run a different arch, yet we'd still always claim "UM".

Agreed on all counts!

This patch addresses all of those. E.g.

$ ./tools/testing/kunit/kunit.py parse .kunit/test.log --json | grep -o '"arch.*' | sort -u "arch": "",

$ ./tools/testing/kunit/kunit.py run --json | ... "arch": "um",

$ ./tools/testing/kunit/kunit.py run --json --arch=x86_64 | ... "arch": "x86_64",

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

Looks good, and works well here. One question/comment below, but in general:

Reviewed-by: David Gow davidgow@google.com

Cheers, -- David

tools/testing/kunit/kunit.py | 4 ++-- tools/testing/kunit/kunit_kernel.py | 2 ++ 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py index 7dd6ed42141f..5ccdafd4d5aa 100755 --- a/tools/testing/kunit/kunit.py +++ b/tools/testing/kunit/kunit.py @@ -153,7 +153,7 @@ def exec_tests(linux: kunit_kernel.LinuxSourceTree, request: KunitExecRequest) - test_glob = request.filter_glob.split('.', maxsplit=2)[1] filter_globs = [g + '.'+ test_glob for g in filter_globs]

•   metadata = kunit_json.Metadata(build_dir=request.build_dir)
  

•   metadata = kunit_json.Metadata(arch=linux.arch(), build_dir=request.build_dir)
  
    test_counts = kunit_parser.TestCounts()
    exec_time = 0.0
  

@@ -506,7 +506,7 @@ def main(argv, linux=None): with open(cli_args.file, 'r', errors='backslashreplace') as f: kunit_output = f.read().splitlines() # We know nothing about how the result was created!

•           metadata = kunit_json.Metadata()
  

•           metadata = kunit_json.Metadata(arch='', build_dir='', def_config='')
  

Why do we explicitly pass empty strings in here, rather than making the defaults correct for this case?

            request = KunitParseRequest(raw_output=cli_args.raw_output,
                                        json=cli_args.json)
            result, _ = parse_tests(request, metadata, kunit_output)

diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py index fe159e7ff697..bbbe2ffe30b7 100644 --- a/tools/testing/kunit/kunit_kernel.py +++ b/tools/testing/kunit/kunit_kernel.py @@ -248,6 +248,8 @@ class LinuxSourceTree(object): kconfig = kunit_config.parse_from_string('\n'.join(kconfig_add)) self._kconfig.merge_in_entries(kconfig)

•   def arch(self) -> str:
  

•           return self._arch
  
    def clean(self) -> bool:
            try:
  

-- 2.35.1.473.g83b2b277ed-goog