* [PATCH 1/3] kunit: tool: readability tweaks in KernelCI json generation logic
@ 2022-02-17 20:52 Daniel Latypov
2022-02-17 20:52 ` [PATCH 2/3] kunit: tool: refactor how we plumb metadata into JSON Daniel Latypov
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Daniel Latypov @ 2022-02-17 20:52 UTC (permalink / raw)
To: brendanhiggins, davidgow
Cc: linux-kernel, kunit-dev, linux-kselftest, skhan, Daniel Latypov
Use a more idiomatic check that a list is non-empty (`if mylist:`) and
sinmplify the function body by dedenting and using a dict to map between
the kunit TestStatus enum => KernelCI json status string.
The dict hopefully makes it less likely to have bugs like commit
9a6bb30a8830 ("kunit: tool: fix --json output for skipped tests").
Signed-off-by: Daniel Latypov <dlatypov@google.com>
---
Note: this series is based on my earlier set of kunit tool cleanups for
5.18, https://lore.kernel.org/linux-kselftest/20220118190922.1557074-1-dlatypov@google.com/
There's no interesting semantic dependency, just some boring merge
conflicts, specifically with patch #4 there, https://lore.kernel.org/linux-kselftest/20220118190922.1557074-5-dlatypov@google.com/
---
tools/testing/kunit/kunit_json.py | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/tools/testing/kunit/kunit_json.py b/tools/testing/kunit/kunit_json.py
index 24d103049bca..14a480d3308a 100644
--- a/tools/testing/kunit/kunit_json.py
+++ b/tools/testing/kunit/kunit_json.py
@@ -16,24 +16,24 @@ from typing import Any, Dict
JsonObj = Dict[str, Any]
+_status_map: Dict[TestStatus, str] = {
+ TestStatus.SUCCESS: "PASS",
+ TestStatus.SKIPPED: "SKIP",
+ TestStatus.TEST_CRASHED: "ERROR",
+}
+
def _get_group_json(test: Test, def_config: str, build_dir: str) -> JsonObj:
sub_groups = [] # List[JsonObj]
test_cases = [] # List[JsonObj]
for subtest in test.subtests:
- if len(subtest.subtests):
+ if subtest.subtests:
sub_group = _get_group_json(subtest, def_config,
build_dir)
sub_groups.append(sub_group)
- else:
- test_case = {"name": subtest.name, "status": "FAIL"}
- if subtest.status == TestStatus.SUCCESS:
- test_case["status"] = "PASS"
- elif subtest.status == TestStatus.SKIPPED:
- test_case["status"] = "SKIP"
- elif subtest.status == TestStatus.TEST_CRASHED:
- test_case["status"] = "ERROR"
- test_cases.append(test_case)
+ continue
+ status = _status_map.get(subtest.status, "FAIL")
+ test_cases.append({"name": subtest.name, "status": status})
test_group = {
"name": test.name,
--
2.35.1.473.g83b2b277ed-goog
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/3] kunit: tool: refactor how we plumb metadata into JSON
2022-02-17 20:52 [PATCH 1/3] kunit: tool: readability tweaks in KernelCI json generation logic Daniel Latypov
@ 2022-02-17 20:52 ` Daniel Latypov
2022-02-24 6:26 ` David Gow
2022-02-17 20:52 ` [PATCH 3/3] kunit: tool: properly report the used arch for --json, or '' if not known Daniel Latypov
2022-02-24 6:26 ` [PATCH 1/3] kunit: tool: readability tweaks in KernelCI json generation logic David Gow
2 siblings, 1 reply; 8+ messages in thread
From: Daniel Latypov @ 2022-02-17 20:52 UTC (permalink / raw)
To: brendanhiggins, davidgow
Cc: linux-kernel, kunit-dev, linux-kselftest, skhan, Daniel Latypov
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>
---
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 3/3] kunit: tool: properly report the used arch for --json, or '' if not known
2022-02-17 20:52 [PATCH 1/3] kunit: tool: readability tweaks in KernelCI json generation logic Daniel Latypov
2022-02-17 20:52 ` [PATCH 2/3] kunit: tool: refactor how we plumb metadata into JSON Daniel Latypov
@ 2022-02-17 20:52 ` Daniel Latypov
2022-02-24 6:26 ` David Gow
2022-02-24 6:26 ` [PATCH 1/3] kunit: tool: readability tweaks in KernelCI json generation logic David Gow
2 siblings, 1 reply; 8+ messages in thread
From: Daniel Latypov @ 2022-02-17 20:52 UTC (permalink / raw)
To: brendanhiggins, davidgow
Cc: linux-kernel, kunit-dev, linux-kselftest, skhan, Daniel Latypov
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".
This patch addresses all of those. E.g.
1.
$ ./tools/testing/kunit/kunit.py parse .kunit/test.log --json | grep -o '"arch.*' | sort -u
"arch": "",
2.
$ ./tools/testing/kunit/kunit.py run --json | ...
"arch": "um",
3.
$ ./tools/testing/kunit/kunit.py run --json --arch=x86_64 | ...
"arch": "x86_64",
Signed-off-by: Daniel Latypov <dlatypov@google.com>
---
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='')
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/3] kunit: tool: readability tweaks in KernelCI json generation logic
2022-02-17 20:52 [PATCH 1/3] kunit: tool: readability tweaks in KernelCI json generation logic Daniel Latypov
2022-02-17 20:52 ` [PATCH 2/3] kunit: tool: refactor how we plumb metadata into JSON Daniel Latypov
2022-02-17 20:52 ` [PATCH 3/3] kunit: tool: properly report the used arch for --json, or '' if not known Daniel Latypov
@ 2022-02-24 6:26 ` David Gow
2022-02-24 18:35 ` Daniel Latypov
2 siblings, 1 reply; 8+ messages in thread
From: David Gow @ 2022-02-24 6:26 UTC (permalink / raw)
To: Daniel Latypov
Cc: Brendan Higgins, Linux Kernel Mailing List, KUnit Development,
open list:KERNEL SELFTEST FRAMEWORK, Shuah Khan
[-- Attachment #1: Type: text/plain, Size: 3145 bytes --]
On Fri, Feb 18, 2022 at 4:52 AM Daniel Latypov <dlatypov@google.com> wrote:
>
> Use a more idiomatic check that a list is non-empty (`if mylist:`) and
> sinmplify the function body by dedenting and using a dict to map between
Nit: spelling of "simplify". (This is also the first time I've seen
"dedenting" as a word, which I thought was a typo for a while, too...)
> the kunit TestStatus enum => KernelCI json status string.
>
> The dict hopefully makes it less likely to have bugs like commit
> 9a6bb30a8830 ("kunit: tool: fix --json output for skipped tests").
>
> Signed-off-by: Daniel Latypov <dlatypov@google.com>
> ---
>
> Note: this series is based on my earlier set of kunit tool cleanups for
> 5.18, https://lore.kernel.org/linux-kselftest/20220118190922.1557074-1-dlatypov@google.com/
>
> There's no interesting semantic dependency, just some boring merge
> conflicts, specifically with patch #4 there, https://lore.kernel.org/linux-kselftest/20220118190922.1557074-5-dlatypov@google.com/
>
> ---
Looks good to me. While in general, I think I prefer an extra level of
indentation to using "continue", it doesn't worry me either way here.
The use of a Dict is definitely an improvement.
Reviewed-by: David Gow <davidgow@google.com>
-- David
> tools/testing/kunit/kunit_json.py | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/tools/testing/kunit/kunit_json.py b/tools/testing/kunit/kunit_json.py
> index 24d103049bca..14a480d3308a 100644
> --- a/tools/testing/kunit/kunit_json.py
> +++ b/tools/testing/kunit/kunit_json.py
> @@ -16,24 +16,24 @@ from typing import Any, Dict
>
> JsonObj = Dict[str, Any]
>
> +_status_map: Dict[TestStatus, str] = {
> + TestStatus.SUCCESS: "PASS",
> + TestStatus.SKIPPED: "SKIP",
> + TestStatus.TEST_CRASHED: "ERROR",
> +}
> +
> def _get_group_json(test: Test, def_config: str, build_dir: str) -> JsonObj:
> sub_groups = [] # List[JsonObj]
> test_cases = [] # List[JsonObj]
>
> for subtest in test.subtests:
> - if len(subtest.subtests):
> + if subtest.subtests:
> sub_group = _get_group_json(subtest, def_config,
> build_dir)
> sub_groups.append(sub_group)
> - else:
> - test_case = {"name": subtest.name, "status": "FAIL"}
> - if subtest.status == TestStatus.SUCCESS:
> - test_case["status"] = "PASS"
> - elif subtest.status == TestStatus.SKIPPED:
> - test_case["status"] = "SKIP"
> - elif subtest.status == TestStatus.TEST_CRASHED:
> - test_case["status"] = "ERROR"
> - test_cases.append(test_case)
> + continue
> + status = _status_map.get(subtest.status, "FAIL")
> + test_cases.append({"name": subtest.name, "status": status})
>
> test_group = {
> "name": test.name,
> --
> 2.35.1.473.g83b2b277ed-goog
>
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/3] kunit: tool: refactor how we plumb metadata into JSON
2022-02-17 20:52 ` [PATCH 2/3] kunit: tool: refactor how we plumb metadata into JSON Daniel Latypov
@ 2022-02-24 6:26 ` David Gow
0 siblings, 0 replies; 8+ messages in thread
From: David Gow @ 2022-02-24 6:26 UTC (permalink / raw)
To: Daniel Latypov
Cc: Brendan Higgins, Linux Kernel Mailing List, KUnit Development,
open list:KERNEL SELFTEST FRAMEWORK, Shuah Khan
[-- Attachment #1: Type: text/plain, Size: 10300 bytes --]
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
>
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 3/3] kunit: tool: properly report the used arch for --json, or '' if not known
2022-02-17 20:52 ` [PATCH 3/3] kunit: tool: properly report the used arch for --json, or '' if not known Daniel Latypov
@ 2022-02-24 6:26 ` David Gow
2022-02-24 19:19 ` Daniel Latypov
0 siblings, 1 reply; 8+ messages in thread
From: David Gow @ 2022-02-24 6:26 UTC (permalink / raw)
To: Daniel Latypov
Cc: Brendan Higgins, Linux Kernel Mailing List, KUnit Development,
open list:KERNEL SELFTEST FRAMEWORK, Shuah Khan
[-- Attachment #1: Type: text/plain, Size: 3234 bytes --]
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.
>
> 1.
> $ ./tools/testing/kunit/kunit.py parse .kunit/test.log --json | grep -o '"arch.*' | sort -u
> "arch": "",
>
> 2.
> $ ./tools/testing/kunit/kunit.py run --json | ...
> "arch": "um",
>
> 3.
> $ ./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
>
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/3] kunit: tool: readability tweaks in KernelCI json generation logic
2022-02-24 6:26 ` [PATCH 1/3] kunit: tool: readability tweaks in KernelCI json generation logic David Gow
@ 2022-02-24 18:35 ` Daniel Latypov
0 siblings, 0 replies; 8+ messages in thread
From: Daniel Latypov @ 2022-02-24 18:35 UTC (permalink / raw)
To: David Gow
Cc: Brendan Higgins, Linux Kernel Mailing List, KUnit Development,
open list:KERNEL SELFTEST FRAMEWORK, Shuah Khan
On Wed, Feb 23, 2022 at 10:26 PM David Gow <davidgow@google.com> wrote:
>
> On Fri, Feb 18, 2022 at 4:52 AM Daniel Latypov <dlatypov@google.com> wrote:
> >
> > Use a more idiomatic check that a list is non-empty (`if mylist:`) and
> > sinmplify the function body by dedenting and using a dict to map between
>
> Nit: spelling of "simplify". (This is also the first time I've seen
Good catch, fixed.
> "dedenting" as a word, which I thought was a typo for a while, too...)
"outdent" is the more proper word here.
Afaik programmers invented "dedent", e.g.
https://docs.python.org/3/library/textwrap.html#textwrap.dedent
I found "dedent" more obvious and used it enough since that I've
forgotten it's not really a word.
>
> > the kunit TestStatus enum => KernelCI json status string.
> >
> > The dict hopefully makes it less likely to have bugs like commit
> > 9a6bb30a8830 ("kunit: tool: fix --json output for skipped tests").
> >
> > Signed-off-by: Daniel Latypov <dlatypov@google.com>
> > ---
> >
> > Note: this series is based on my earlier set of kunit tool cleanups for
> > 5.18, https://lore.kernel.org/linux-kselftest/20220118190922.1557074-1-dlatypov@google.com/
> >
> > There's no interesting semantic dependency, just some boring merge
> > conflicts, specifically with patch #4 there, https://lore.kernel.org/linux-kselftest/20220118190922.1557074-5-dlatypov@google.com/
> >
> > ---
>
> Looks good to me. While in general, I think I prefer an extra level of
> indentation to using "continue", it doesn't worry me either way here.
It's not really a concern here, but I'm always wary given python has
significant whitespace.
I've seen enough instances of moved code where the indentation wasn't
properly updated.
> The use of a Dict is definitely an improvement.
>
> Reviewed-by: David Gow <davidgow@google.com>
>
>
> -- David
>
> > tools/testing/kunit/kunit_json.py | 20 ++++++++++----------
> > 1 file changed, 10 insertions(+), 10 deletions(-)
> >
> > diff --git a/tools/testing/kunit/kunit_json.py b/tools/testing/kunit/kunit_json.py
> > index 24d103049bca..14a480d3308a 100644
> > --- a/tools/testing/kunit/kunit_json.py
> > +++ b/tools/testing/kunit/kunit_json.py
> > @@ -16,24 +16,24 @@ from typing import Any, Dict
> >
> > JsonObj = Dict[str, Any]
> >
> > +_status_map: Dict[TestStatus, str] = {
> > + TestStatus.SUCCESS: "PASS",
> > + TestStatus.SKIPPED: "SKIP",
> > + TestStatus.TEST_CRASHED: "ERROR",
> > +}
> > +
> > def _get_group_json(test: Test, def_config: str, build_dir: str) -> JsonObj:
> > sub_groups = [] # List[JsonObj]
> > test_cases = [] # List[JsonObj]
> >
> > for subtest in test.subtests:
> > - if len(subtest.subtests):
> > + if subtest.subtests:
> > sub_group = _get_group_json(subtest, def_config,
> > build_dir)
> > sub_groups.append(sub_group)
> > - else:
> > - test_case = {"name": subtest.name, "status": "FAIL"}
> > - if subtest.status == TestStatus.SUCCESS:
> > - test_case["status"] = "PASS"
> > - elif subtest.status == TestStatus.SKIPPED:
> > - test_case["status"] = "SKIP"
> > - elif subtest.status == TestStatus.TEST_CRASHED:
> > - test_case["status"] = "ERROR"
> > - test_cases.append(test_case)
> > + continue
> > + status = _status_map.get(subtest.status, "FAIL")
> > + test_cases.append({"name": subtest.name, "status": status})
> >
> > test_group = {
> > "name": test.name,
> > --
> > 2.35.1.473.g83b2b277ed-goog
> >
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 3/3] kunit: tool: properly report the used arch for --json, or '' if not known
2022-02-24 6:26 ` David Gow
@ 2022-02-24 19:19 ` Daniel Latypov
0 siblings, 0 replies; 8+ messages in thread
From: Daniel Latypov @ 2022-02-24 19:19 UTC (permalink / raw)
To: David Gow
Cc: Brendan Higgins, Linux Kernel Mailing List, KUnit Development,
open list:KERNEL SELFTEST FRAMEWORK, Shuah Khan
On Wed, Feb 23, 2022 at 10:26 PM David Gow <davidgow@google.com> wrote:
>
> 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.
> >
> > 1.
> > $ ./tools/testing/kunit/kunit.py parse .kunit/test.log --json | grep -o '"arch.*' | sort -u
> > "arch": "",
> >
> > 2.
> > $ ./tools/testing/kunit/kunit.py run --json | ...
> > "arch": "um",
> >
> > 3.
> > $ ./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?
Good point, we should just make '' the defaults now.
I'll move the hard-coding of "kunit_defconfig" out of the defaults and
into exec_tests() then.
>
>
> > 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
> >
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-02-24 19:19 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-17 20:52 [PATCH 1/3] kunit: tool: readability tweaks in KernelCI json generation logic Daniel Latypov
2022-02-17 20:52 ` [PATCH 2/3] kunit: tool: refactor how we plumb metadata into JSON Daniel Latypov
2022-02-24 6:26 ` David Gow
2022-02-17 20:52 ` [PATCH 3/3] kunit: tool: properly report the used arch for --json, or '' if not known Daniel Latypov
2022-02-24 6:26 ` David Gow
2022-02-24 19:19 ` Daniel Latypov
2022-02-24 6:26 ` [PATCH 1/3] kunit: tool: readability tweaks in KernelCI json generation logic David Gow
2022-02-24 18:35 ` Daniel Latypov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.