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

[permalink] [raw]

Subject: [PATCH 2/5] kunit: tool: make --json handling a bit clearer

Currently kunit_json.get_json_result() will output the JSON-ified test
output to json_path, but iff it's not "stdout".

Instead, move the responsibility entirely over to the one caller.

Signed-off-by: Daniel Latypov <[email protected]>
---
tools/testing/kunit/kunit.py | 12 ++++++++----
tools/testing/kunit/kunit_json.py | 12 ++----------
tools/testing/kunit/kunit_tool_test.py | 3 +--
3 files changed, 11 insertions(+), 16 deletions(-)

diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py
index 9274c6355809..bd2f7f088c72 100755
--- a/tools/testing/kunit/kunit.py
+++ b/tools/testing/kunit/kunit.py
@@ -216,13 +216,17 @@ def parse_tests(request: KunitParseRequest, input_data: Iterable[str]) -> Tuple[
parse_end = time.time()

if request.json:
- json_obj = kunit_json.get_json_result(
+ json_str = kunit_json.get_json_result(
test=test_result,
def_config='kunit_defconfig',
- build_dir=request.build_dir,
- json_path=request.json)
+ build_dir=request.build_dir)
if request.json == 'stdout':
- print(json_obj)
+ print(json_str)
+ else:
+ with open(request.json, 'w') as f:
+ f.write(json_str)
+ kunit_parser.print_with_timestamp("Test results stored in %s" %
+ os.path.abspath(request.json))

if test_result.status != kunit_parser.TestStatus.SUCCESS:
return KunitResult(KunitStatus.TEST_FAILURE, parse_end - parse_start), test_result
diff --git a/tools/testing/kunit/kunit_json.py b/tools/testing/kunit/kunit_json.py
index 6862671709bc..61091878f51e 100644
--- a/tools/testing/kunit/kunit_json.py
+++ b/tools/testing/kunit/kunit_json.py
@@ -51,15 +51,7 @@ def _get_group_json(test: Test, def_config: str,
return test_group

def get_json_result(test: Test, def_config: str,
- build_dir: Optional[str], json_path: str) -> str:
+ build_dir: Optional[str]) -> str:
test_group = _get_group_json(test, def_config, build_dir)
test_group["name"] = "KUnit Test Group"
- json_obj = json.dumps(test_group, indent=4)
- if json_path != 'stdout':
- with open(json_path, 'w') as result_path:
- result_path.write(json_obj)
- root = __file__.split('tools/testing/kunit/')[0]
- kunit_parser.print_with_timestamp(
- "Test results stored in %s" %
- os.path.join(root, result_path.name))
- return json_obj
+ 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 352369dffbd9..f7cbc248a405 100755
--- a/tools/testing/kunit/kunit_tool_test.py
+++ b/tools/testing/kunit/kunit_tool_test.py
@@ -469,8 +469,7 @@ class KUnitJsonTest(unittest.TestCase):
json_obj = kunit_json.get_json_result(
test=test_result,
def_config='kunit_defconfig',
- build_dir=None,
- json_path='stdout')
+ build_dir=None)
return json.loads(json_obj)

def test_failed_test_json(self):
--
2.34.1.703.g22d0c6ccf7-goog

2022-01-21 04:02:04

[permalink] [raw]

Subject: [PATCH 3/5] kunit: tool: drop unused KernelDirectoryPath var

Commit be886ba90cce ("kunit: run kunit_tool from any directory")
introduced this variable, but it was unused even in that commit.

Since it's still unused now and callers can instead use
get_kernel_root_path(), delete this var.

Signed-off-by: Daniel Latypov <[email protected]>
---
tools/testing/kunit/kunit.py | 2 --
1 file changed, 2 deletions(-)

diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py
index bd2f7f088c72..4cb91d191f1d 100755
--- a/tools/testing/kunit/kunit.py
+++ b/tools/testing/kunit/kunit.py
@@ -63,8 +63,6 @@ class KunitRequest(KunitExecRequest, KunitBuildRequest):
pass

-KernelDirectoryPath = sys.argv[0].split('tools/testing/kunit/')[0]
-
def get_kernel_root_path() -> str:
path = sys.argv[0] if not __file__ else __file__
parts = os.path.realpath(path).split('tools/testing/kunit')
--
2.34.1.703.g22d0c6ccf7-goog

2022-01-21 04:02:19

[permalink] [raw]

Subject: [PATCH 4/5] kunit: tool: drop last uses of collections.namedtuple

Since we formally require python3.7+ since commit df4b0807ca1a
("kunit: tool: Assert the version requirement"), we can just use
@dataclasses.dataclass instead.

In kunit_config.py, we used namedtuple to create a hashable type that
had `name` and `value` fields and had to subclass it to define a custom
`__str__()`.
@datalcass lets us just define one type instead.

In qemu_config.py, we use namedtuple to allow modules to define various
parameters. Using @dataclass, we can add type-annotations for all these
fields, making our code more typesafe and making it easier for users to
figure out how to define new configs.

Signed-off-by: Daniel Latypov <[email protected]>
---
tools/testing/kunit/kunit_config.py | 9 +++++----
tools/testing/kunit/qemu_config.py | 17 ++++++++++-------
2 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/tools/testing/kunit/kunit_config.py b/tools/testing/kunit/kunit_config.py
index 677354546156..ca33e4b7bcc5 100644
--- a/tools/testing/kunit/kunit_config.py
+++ b/tools/testing/kunit/kunit_config.py
@@ -6,16 +6,17 @@
# Author: Felix Guo <[email protected]>
# Author: Brendan Higgins <[email protected]>

-import collections
+from dataclasses import dataclass
import re
from typing import List, Set

CONFIG_IS_NOT_SET_PATTERN = r'^# CONFIG_(\w+) is not set$'
CONFIG_PATTERN = r'^CONFIG_(\w+)=(\S+|".*")$'

-KconfigEntryBase = collections.namedtuple('KconfigEntryBase', ['name', 'value'])
-
-class KconfigEntry(KconfigEntryBase):
+@dataclass(frozen=True)
+class KconfigEntry:
+ name: str
+ value: str

def __str__(self) -> str:
if self.value == 'n':
diff --git a/tools/testing/kunit/qemu_config.py b/tools/testing/kunit/qemu_config.py
index 1672f6184e95..0b6a80398ccc 100644
--- a/tools/testing/kunit/qemu_config.py
+++ b/tools/testing/kunit/qemu_config.py
@@ -5,12 +5,15 @@
# Copyright (C) 2021, Google LLC.
# Author: Brendan Higgins <[email protected]>

-from collections import namedtuple
+from dataclasses import dataclass
+from typing import List

-QemuArchParams = namedtuple('QemuArchParams', ['linux_arch',
- 'kconfig',
- 'qemu_arch',
- 'kernel_path',
- 'kernel_command_line',
- 'extra_qemu_params'])
+@dataclass(frozen=True)
+class QemuArchParams:
+ linux_arch: str
+ kconfig: str
+ qemu_arch: str
+ kernel_path: str
+ kernel_command_line: str
+ extra_qemu_params: List[str]
--
2.34.1.703.g22d0c6ccf7-goog

2022-01-21 04:08:54

[permalink] [raw]

Subject: [PATCH 5/5] kunit: tool: simplify code since build_dir can't be None

--build_dir is set to a default of '.kunit' since commit ddbd60c779b4
("kunit: use --build_dir=.kunit as default"), but even before then it
was explicitly set to ''.

So outside of one unit test, there was no way for the build_dir to be
ever be None, and we can simplify code by fixing the unit test and
enforcing that via updated type annotations.

E.g. this lets us drop `get_file_path()` since it's now exactly
equivalent to os.path.join().

Note: there's some `if build_dir` checks that also fail if build_dir is
explicitly set to '' that just guard against passing "O=" to make.
But running `make O=` works just fine, so drop these checks.

Signed-off-by: Daniel Latypov <[email protected]>
---
tools/testing/kunit/kunit_json.py | 8 ++--
tools/testing/kunit/kunit_kernel.py | 51 ++++++++++----------------
tools/testing/kunit/kunit_tool_test.py | 2 +-
3 files changed, 24 insertions(+), 37 deletions(-)

diff --git a/tools/testing/kunit/kunit_json.py b/tools/testing/kunit/kunit_json.py
index 61091878f51e..24d103049bca 100644
--- a/tools/testing/kunit/kunit_json.py
+++ b/tools/testing/kunit/kunit_json.py
@@ -12,12 +12,11 @@ import os
import kunit_parser

from kunit_parser import Test, TestStatus
-from typing import Any, Dict, Optional
+from typing import Any, Dict

JsonObj = Dict[str, Any]

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

@@ -50,8 +49,7 @@ def _get_group_json(test: Test, def_config: str,
}
return test_group

-def get_json_result(test: Test, def_config: str,
- build_dir: Optional[str]) -> str:
+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["name"] = "KUnit Test Group"
return json.dumps(test_group, indent=4)
diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py
index 44bbe54f25f1..fe159e7ff697 100644
--- a/tools/testing/kunit/kunit_kernel.py
+++ b/tools/testing/kunit/kunit_kernel.py
@@ -28,11 +28,6 @@ OUTFILE_PATH = 'test.log'
ABS_TOOL_PATH = os.path.abspath(os.path.dirname(__file__))
QEMU_CONFIGS_DIR = os.path.join(ABS_TOOL_PATH, 'qemu_configs')

-def get_file_path(build_dir, default):
- if build_dir:
- default = os.path.join(build_dir, default)
- return default
-
class ConfigError(Exception):
"""Represents an error trying to configure the Linux kernel."""

@@ -59,17 +54,15 @@ class LinuxSourceTreeOperations(object):
def make_arch_qemuconfig(self, kconfig: kunit_config.Kconfig) -> None:
pass

- def make_allyesconfig(self, build_dir, make_options) -> None:
+ def make_allyesconfig(self, build_dir: str, make_options) -> None:
raise ConfigError('Only the "um" arch is supported for alltests')

- def make_olddefconfig(self, build_dir, make_options) -> None:
- command = ['make', 'ARCH=' + self._linux_arch, 'olddefconfig']
+ def make_olddefconfig(self, build_dir: str, make_options) -> None:
+ command = ['make', 'ARCH=' + self._linux_arch, 'O=' + build_dir, 'olddefconfig']
if self._cross_compile:
command += ['CROSS_COMPILE=' + self._cross_compile]
if make_options:
command.extend(make_options)
- if build_dir:
- command += ['O=' + build_dir]
print('Populating config with:\n$', ' '.join(command))
try:
subprocess.check_output(command, stderr=subprocess.STDOUT)
@@ -78,14 +71,12 @@ class LinuxSourceTreeOperations(object):
except subprocess.CalledProcessError as e:
raise ConfigError(e.output.decode())

- def make(self, jobs, build_dir, make_options) -> None:
- command = ['make', 'ARCH=' + self._linux_arch, '--jobs=' + str(jobs)]
+ def make(self, jobs, build_dir: str, make_options) -> None:
+ command = ['make', 'ARCH=' + self._linux_arch, 'O=' + build_dir, '--jobs=' + str(jobs)]
if make_options:
command.extend(make_options)
if self._cross_compile:
command += ['CROSS_COMPILE=' + self._cross_compile]
- if build_dir:
- command += ['O=' + build_dir]
print('Building with:\n$', ' '.join(command))
try:
proc = subprocess.Popen(command,
@@ -143,14 +134,12 @@ class LinuxSourceTreeOperationsUml(LinuxSourceTreeOperations):
def __init__(self, cross_compile=None):
super().__init__(linux_arch='um', cross_compile=cross_compile)

- def make_allyesconfig(self, build_dir, make_options) -> None:
+ def make_allyesconfig(self, build_dir: str, make_options) -> None:
kunit_parser.print_with_timestamp(
'Enabling all CONFIGs for UML...')
- command = ['make', 'ARCH=um', 'allyesconfig']
+ command = ['make', 'ARCH=um', 'O=' + build_dir, 'allyesconfig']
if make_options:
command.extend(make_options)
- if build_dir:
- command += ['O=' + build_dir]
process = subprocess.Popen(
command,
stdout=subprocess.DEVNULL,
@@ -167,24 +156,24 @@ class LinuxSourceTreeOperationsUml(LinuxSourceTreeOperations):

def start(self, params: List[str], build_dir: str) -> subprocess.Popen:
"""Runs the Linux UML binary. Must be named 'linux'."""
- linux_bin = get_file_path(build_dir, 'linux')
+ linux_bin = os.path.join(build_dir, 'linux')
return subprocess.Popen([linux_bin] + params,
stdin=subprocess.PIPE,
stdout=subprocess.PIPE,
stderr=subprocess.STDOUT,
text=True, errors='backslashreplace')

-def get_kconfig_path(build_dir) -> str:
- return get_file_path(build_dir, KCONFIG_PATH)
+def get_kconfig_path(build_dir: str) -> str:
+ return os.path.join(build_dir, KCONFIG_PATH)

-def get_kunitconfig_path(build_dir) -> str:
- return get_file_path(build_dir, KUNITCONFIG_PATH)
+def get_kunitconfig_path(build_dir: str) -> str:
+ return os.path.join(build_dir, KUNITCONFIG_PATH)

-def get_old_kunitconfig_path(build_dir) -> str:
- return get_file_path(build_dir, OLD_KUNITCONFIG_PATH)
+def get_old_kunitconfig_path(build_dir: str) -> str:
+ return os.path.join(build_dir, OLD_KUNITCONFIG_PATH)

-def get_outfile_path(build_dir) -> str:
- return get_file_path(build_dir, OUTFILE_PATH)
+def get_outfile_path(build_dir: str) -> str:
+ return os.path.join(build_dir, OUTFILE_PATH)

def get_source_tree_ops(arch: str, cross_compile: Optional[str]) -> LinuxSourceTreeOperations:
config_path = os.path.join(QEMU_CONFIGS_DIR, arch + '.py')
@@ -268,7 +257,7 @@ class LinuxSourceTree(object):
return False
return True

- def validate_config(self, build_dir) -> bool:
+ def validate_config(self, build_dir: str) -> bool:
kconfig_path = get_kconfig_path(build_dir)
validated_kconfig = kunit_config.parse_file(kconfig_path)
if self._kconfig.is_subset_of(validated_kconfig):
@@ -283,7 +272,7 @@ class LinuxSourceTree(object):
logging.error(message)
return False

- def build_config(self, build_dir, make_options) -> bool:
+ def build_config(self, build_dir: str, make_options) -> bool:
kconfig_path = get_kconfig_path(build_dir)
if build_dir and not os.path.exists(build_dir):
os.mkdir(build_dir)
@@ -311,7 +300,7 @@ class LinuxSourceTree(object):
old_kconfig = kunit_config.parse_file(old_path)
return old_kconfig.entries() != self._kconfig.entries()

- def build_reconfig(self, build_dir, make_options) -> bool:
+ def build_reconfig(self, build_dir: str, make_options) -> bool:
"""Creates a new .config if it is not a subset of the .kunitconfig."""
kconfig_path = get_kconfig_path(build_dir)
if not os.path.exists(kconfig_path):
@@ -326,7 +315,7 @@ class LinuxSourceTree(object):
os.remove(kconfig_path)
return self.build_config(build_dir, make_options)

- def build_kernel(self, alltests, jobs, build_dir, make_options) -> bool:
+ def build_kernel(self, alltests, jobs, build_dir: str, make_options) -> bool:
try:
if alltests:
self._ops.make_allyesconfig(build_dir, make_options)
diff --git a/tools/testing/kunit/kunit_tool_test.py b/tools/testing/kunit/kunit_tool_test.py
index f7cbc248a405..a3c036a620b2 100755
--- a/tools/testing/kunit/kunit_tool_test.py
+++ b/tools/testing/kunit/kunit_tool_test.py
@@ -469,7 +469,7 @@ class KUnitJsonTest(unittest.TestCase):
json_obj = kunit_json.get_json_result(
test=test_result,
def_config='kunit_defconfig',
- build_dir=None)
+ build_dir='.kunit')
return json.loads(json_obj)

def test_failed_test_json(self):
--
2.34.1.703.g22d0c6ccf7-goog

2022-01-21 21:11:17

[permalink] [raw]

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

On Wed, Jan 19, 2022 at 3:09 AM Daniel Latypov <[email protected]> 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 <[email protected]>
> ---

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).

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.

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
>

2022-01-21 21:11:19

[permalink] [raw]

Subject: Re: [PATCH 2/5] kunit: tool: make --json handling a bit clearer

On Wed, Jan 19, 2022 at 3:09 AM Daniel Latypov <[email protected]> wrote:
>
> Currently kunit_json.get_json_result() will output the JSON-ified test
> output to json_path, but iff it's not "stdout".
>
> Instead, move the responsibility entirely over to the one caller.
>
> Signed-off-by: Daniel Latypov <[email protected]>
> ---

This looks good to me.

Reviewed-by: David Gow <[email protected]>

Cheers,
-- David

> tools/testing/kunit/kunit.py | 12 ++++++++----
> tools/testing/kunit/kunit_json.py | 12 ++----------
> tools/testing/kunit/kunit_tool_test.py | 3 +--
> 3 files changed, 11 insertions(+), 16 deletions(-)
>
> diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py
> index 9274c6355809..bd2f7f088c72 100755
> --- a/tools/testing/kunit/kunit.py
> +++ b/tools/testing/kunit/kunit.py
> @@ -216,13 +216,17 @@ def parse_tests(request: KunitParseRequest, input_data: Iterable[str]) -> Tuple[
> parse_end = time.time()
>
> if request.json:
> - json_obj = kunit_json.get_json_result(
> + json_str = kunit_json.get_json_result(
> test=test_result,
> def_config='kunit_defconfig',
> - build_dir=request.build_dir,
> - json_path=request.json)
> + build_dir=request.build_dir)
> if request.json == 'stdout':
> - print(json_obj)
> + print(json_str)
> + else:
> + with open(request.json, 'w') as f:
> + f.write(json_str)
> + kunit_parser.print_with_timestamp("Test results stored in %s" %
> + os.path.abspath(request.json))
>
> if test_result.status != kunit_parser.TestStatus.SUCCESS:
> return KunitResult(KunitStatus.TEST_FAILURE, parse_end - parse_start), test_result
> diff --git a/tools/testing/kunit/kunit_json.py b/tools/testing/kunit/kunit_json.py
> index 6862671709bc..61091878f51e 100644
> --- a/tools/testing/kunit/kunit_json.py
> +++ b/tools/testing/kunit/kunit_json.py
> @@ -51,15 +51,7 @@ def _get_group_json(test: Test, def_config: str,
> return test_group
>
> def get_json_result(test: Test, def_config: str,
> - build_dir: Optional[str], json_path: str) -> str:
> + build_dir: Optional[str]) -> str:
> test_group = _get_group_json(test, def_config, build_dir)
> test_group["name"] = "KUnit Test Group"
> - json_obj = json.dumps(test_group, indent=4)
> - if json_path != 'stdout':
> - with open(json_path, 'w') as result_path:
> - result_path.write(json_obj)
> - root = __file__.split('tools/testing/kunit/')[0]
> - kunit_parser.print_with_timestamp(
> - "Test results stored in %s" %
> - os.path.join(root, result_path.name))
> - return json_obj
> + 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 352369dffbd9..f7cbc248a405 100755
> --- a/tools/testing/kunit/kunit_tool_test.py
> +++ b/tools/testing/kunit/kunit_tool_test.py
> @@ -469,8 +469,7 @@ class KUnitJsonTest(unittest.TestCase):
> json_obj = kunit_json.get_json_result(
> test=test_result,
> def_config='kunit_defconfig',
> - build_dir=None,
> - json_path='stdout')
> + build_dir=None)
> return json.loads(json_obj)
>
> def test_failed_test_json(self):
> --
> 2.34.1.703.g22d0c6ccf7-goog
>

2022-01-21 21:11:24

[permalink] [raw]

Subject: Re: [PATCH 3/5] kunit: tool: drop unused KernelDirectoryPath var

On Wed, Jan 19, 2022 at 3:09 AM Daniel Latypov <[email protected]> wrote:
>
> Commit be886ba90cce ("kunit: run kunit_tool from any directory")
> introduced this variable, but it was unused even in that commit.
>
> Since it's still unused now and callers can instead use
> get_kernel_root_path(), delete this var.
>
> Signed-off-by: Daniel Latypov <[email protected]>
> ---

Reviewed-by: David Gow <[email protected]>

Cheers,
-- David

> tools/testing/kunit/kunit.py | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py
> index bd2f7f088c72..4cb91d191f1d 100755
> --- a/tools/testing/kunit/kunit.py
> +++ b/tools/testing/kunit/kunit.py
> @@ -63,8 +63,6 @@ class KunitRequest(KunitExecRequest, KunitBuildRequest):
> pass
>
>
> -KernelDirectoryPath = sys.argv[0].split('tools/testing/kunit/')[0]
> -
> def get_kernel_root_path() -> str:
> path = sys.argv[0] if not __file__ else __file__
> parts = os.path.realpath(path).split('tools/testing/kunit')
> --
> 2.34.1.703.g22d0c6ccf7-goog
>

2022-01-21 21:11:24

[permalink] [raw]

Subject: Re: [PATCH 4/5] kunit: tool: drop last uses of collections.namedtuple

On Wed, Jan 19, 2022 at 3:09 AM Daniel Latypov <[email protected]> wrote:
>
> Since we formally require python3.7+ since commit df4b0807ca1a
> ("kunit: tool: Assert the version requirement"), we can just use
> @dataclasses.dataclass instead.
>
> In kunit_config.py, we used namedtuple to create a hashable type that
> had `name` and `value` fields and had to subclass it to define a custom
> `__str__()`.
> @datalcass lets us just define one type instead.
>
> In qemu_config.py, we use namedtuple to allow modules to define various
> parameters. Using @dataclass, we can add type-annotations for all these
> fields, making our code more typesafe and making it easier for users to
> figure out how to define new configs.
>
> Signed-off-by: Daniel Latypov <[email protected]>
> ---

This seems sensible, and the type-annotations are definitely a good thing.

I guess I'm going to have to learn how to use @dataclass, though...

Reviewed-by: David Gow <[email protected]>

-- David

> tools/testing/kunit/kunit_config.py | 9 +++++----
> tools/testing/kunit/qemu_config.py | 17 ++++++++++-------
> 2 files changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/tools/testing/kunit/kunit_config.py b/tools/testing/kunit/kunit_config.py
> index 677354546156..ca33e4b7bcc5 100644
> --- a/tools/testing/kunit/kunit_config.py
> +++ b/tools/testing/kunit/kunit_config.py
> @@ -6,16 +6,17 @@
> # Author: Felix Guo <[email protected]>
> # Author: Brendan Higgins <[email protected]>
>
> -import collections
> +from dataclasses import dataclass
> import re
> from typing import List, Set
>
> CONFIG_IS_NOT_SET_PATTERN = r'^# CONFIG_(\w+) is not set$'
> CONFIG_PATTERN = r'^CONFIG_(\w+)=(\S+|".*")$'
>
> -KconfigEntryBase = collections.namedtuple('KconfigEntryBase', ['name', 'value'])
> -
> -class KconfigEntry(KconfigEntryBase):
> +@dataclass(frozen=True)
> +class KconfigEntry:
> + name: str
> + value: str
>
> def __str__(self) -> str:
> if self.value == 'n':
> diff --git a/tools/testing/kunit/qemu_config.py b/tools/testing/kunit/qemu_config.py
> index 1672f6184e95..0b6a80398ccc 100644
> --- a/tools/testing/kunit/qemu_config.py
> +++ b/tools/testing/kunit/qemu_config.py
> @@ -5,12 +5,15 @@
> # Copyright (C) 2021, Google LLC.
> # Author: Brendan Higgins <[email protected]>
>
> -from collections import namedtuple
> +from dataclasses import dataclass
> +from typing import List
>
>
> -QemuArchParams = namedtuple('QemuArchParams', ['linux_arch',
> - 'kconfig',
> - 'qemu_arch',
> - 'kernel_path',
> - 'kernel_command_line',
> - 'extra_qemu_params'])
> +@dataclass(frozen=True)
> +class QemuArchParams:
> + linux_arch: str
> + kconfig: str
> + qemu_arch: str
> + kernel_path: str
> + kernel_command_line: str
> + extra_qemu_params: List[str]
> --
> 2.34.1.703.g22d0c6ccf7-goog
>

2022-01-21 21:11:29

[permalink] [raw]

Subject: Re: [PATCH 5/5] kunit: tool: simplify code since build_dir can't be None

On Wed, Jan 19, 2022 at 3:09 AM Daniel Latypov <[email protected]> wrote:
>
> --build_dir is set to a default of '.kunit' since commit ddbd60c779b4
> ("kunit: use --build_dir=.kunit as default"), but even before then it
> was explicitly set to ''.
>
> So outside of one unit test, there was no way for the build_dir to be
> ever be None, and we can simplify code by fixing the unit test and
> enforcing that via updated type annotations.
>
> E.g. this lets us drop `get_file_path()` since it's now exactly
> equivalent to os.path.join().
>
> Note: there's some `if build_dir` checks that also fail if build_dir is
> explicitly set to '' that just guard against passing "O=" to make.
> But running `make O=` works just fine, so drop these checks.
>
> Signed-off-by: Daniel Latypov <[email protected]>
> ---

Looks good to me.
Passing "--build_dir=" to KUnit didn't work before, as well, as "" is
not considered a valid path. You've got to specify --build-dir=. to
get anything useful to happen.

Reviewed-by: David Gow <[email protected]>

-- David

> tools/testing/kunit/kunit_json.py | 8 ++--
> tools/testing/kunit/kunit_kernel.py | 51 ++++++++++----------------
> tools/testing/kunit/kunit_tool_test.py | 2 +-
> 3 files changed, 24 insertions(+), 37 deletions(-)
>
> diff --git a/tools/testing/kunit/kunit_json.py b/tools/testing/kunit/kunit_json.py
> index 61091878f51e..24d103049bca 100644
> --- a/tools/testing/kunit/kunit_json.py
> +++ b/tools/testing/kunit/kunit_json.py
> @@ -12,12 +12,11 @@ import os
> import kunit_parser
>
> from kunit_parser import Test, TestStatus
> -from typing import Any, Dict, Optional
> +from typing import Any, Dict
>
> JsonObj = Dict[str, Any]
>
> -def _get_group_json(test: Test, def_config: str,
> - build_dir: Optional[str]) -> JsonObj:
> +def _get_group_json(test: Test, def_config: str, build_dir: str) -> JsonObj:
> sub_groups = [] # List[JsonObj]
> test_cases = [] # List[JsonObj]
>
> @@ -50,8 +49,7 @@ def _get_group_json(test: Test, def_config: str,
> }
> return test_group
>
> -def get_json_result(test: Test, def_config: str,
> - build_dir: Optional[str]) -> str:
> +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["name"] = "KUnit Test Group"
> return json.dumps(test_group, indent=4)
> diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py
> index 44bbe54f25f1..fe159e7ff697 100644
> --- a/tools/testing/kunit/kunit_kernel.py
> +++ b/tools/testing/kunit/kunit_kernel.py
> @@ -28,11 +28,6 @@ OUTFILE_PATH = 'test.log'
> ABS_TOOL_PATH = os.path.abspath(os.path.dirname(__file__))
> QEMU_CONFIGS_DIR = os.path.join(ABS_TOOL_PATH, 'qemu_configs')
>
> -def get_file_path(build_dir, default):
> - if build_dir:
> - default = os.path.join(build_dir, default)
> - return default
> -
> class ConfigError(Exception):
> """Represents an error trying to configure the Linux kernel."""
>
> @@ -59,17 +54,15 @@ class LinuxSourceTreeOperations(object):
> def make_arch_qemuconfig(self, kconfig: kunit_config.Kconfig) -> None:
> pass
>
> - def make_allyesconfig(self, build_dir, make_options) -> None:
> + def make_allyesconfig(self, build_dir: str, make_options) -> None:
> raise ConfigError('Only the "um" arch is supported for alltests')
>
> - def make_olddefconfig(self, build_dir, make_options) -> None:
> - command = ['make', 'ARCH=' + self._linux_arch, 'olddefconfig']
> + def make_olddefconfig(self, build_dir: str, make_options) -> None:
> + command = ['make', 'ARCH=' + self._linux_arch, 'O=' + build_dir, 'olddefconfig']
> if self._cross_compile:
> command += ['CROSS_COMPILE=' + self._cross_compile]
> if make_options:
> command.extend(make_options)
> - if build_dir:
> - command += ['O=' + build_dir]
> print('Populating config with:\n$', ' '.join(command))
> try:
> subprocess.check_output(command, stderr=subprocess.STDOUT)
> @@ -78,14 +71,12 @@ class LinuxSourceTreeOperations(object):
> except subprocess.CalledProcessError as e:
> raise ConfigError(e.output.decode())
>
> - def make(self, jobs, build_dir, make_options) -> None:
> - command = ['make', 'ARCH=' + self._linux_arch, '--jobs=' + str(jobs)]
> + def make(self, jobs, build_dir: str, make_options) -> None:
> + command = ['make', 'ARCH=' + self._linux_arch, 'O=' + build_dir, '--jobs=' + str(jobs)]
> if make_options:
> command.extend(make_options)
> if self._cross_compile:
> command += ['CROSS_COMPILE=' + self._cross_compile]
> - if build_dir:
> - command += ['O=' + build_dir]
> print('Building with:\n$', ' '.join(command))
> try:
> proc = subprocess.Popen(command,
> @@ -143,14 +134,12 @@ class LinuxSourceTreeOperationsUml(LinuxSourceTreeOperations):
> def __init__(self, cross_compile=None):
> super().__init__(linux_arch='um', cross_compile=cross_compile)
>
> - def make_allyesconfig(self, build_dir, make_options) -> None:
> + def make_allyesconfig(self, build_dir: str, make_options) -> None:
> kunit_parser.print_with_timestamp(
> 'Enabling all CONFIGs for UML...')
> - command = ['make', 'ARCH=um', 'allyesconfig']
> + command = ['make', 'ARCH=um', 'O=' + build_dir, 'allyesconfig']
> if make_options:
> command.extend(make_options)
> - if build_dir:
> - command += ['O=' + build_dir]
> process = subprocess.Popen(
> command,
> stdout=subprocess.DEVNULL,
> @@ -167,24 +156,24 @@ class LinuxSourceTreeOperationsUml(LinuxSourceTreeOperations):
>
> def start(self, params: List[str], build_dir: str) -> subprocess.Popen:
> """Runs the Linux UML binary. Must be named 'linux'."""
> - linux_bin = get_file_path(build_dir, 'linux')
> + linux_bin = os.path.join(build_dir, 'linux')
> return subprocess.Popen([linux_bin] + params,
> stdin=subprocess.PIPE,
> stdout=subprocess.PIPE,
> stderr=subprocess.STDOUT,
> text=True, errors='backslashreplace')
>
> -def get_kconfig_path(build_dir) -> str:
> - return get_file_path(build_dir, KCONFIG_PATH)
> +def get_kconfig_path(build_dir: str) -> str:
> + return os.path.join(build_dir, KCONFIG_PATH)
>
> -def get_kunitconfig_path(build_dir) -> str:
> - return get_file_path(build_dir, KUNITCONFIG_PATH)
> +def get_kunitconfig_path(build_dir: str) -> str:
> + return os.path.join(build_dir, KUNITCONFIG_PATH)
>
> -def get_old_kunitconfig_path(build_dir) -> str:
> - return get_file_path(build_dir, OLD_KUNITCONFIG_PATH)
> +def get_old_kunitconfig_path(build_dir: str) -> str:
> + return os.path.join(build_dir, OLD_KUNITCONFIG_PATH)
>
> -def get_outfile_path(build_dir) -> str:
> - return get_file_path(build_dir, OUTFILE_PATH)
> +def get_outfile_path(build_dir: str) -> str:
> + return os.path.join(build_dir, OUTFILE_PATH)
>
> def get_source_tree_ops(arch: str, cross_compile: Optional[str]) -> LinuxSourceTreeOperations:
> config_path = os.path.join(QEMU_CONFIGS_DIR, arch + '.py')
> @@ -268,7 +257,7 @@ class LinuxSourceTree(object):
> return False
> return True
>
> - def validate_config(self, build_dir) -> bool:
> + def validate_config(self, build_dir: str) -> bool:
> kconfig_path = get_kconfig_path(build_dir)
> validated_kconfig = kunit_config.parse_file(kconfig_path)
> if self._kconfig.is_subset_of(validated_kconfig):
> @@ -283,7 +272,7 @@ class LinuxSourceTree(object):
> logging.error(message)
> return False
>
> - def build_config(self, build_dir, make_options) -> bool:
> + def build_config(self, build_dir: str, make_options) -> bool:
> kconfig_path = get_kconfig_path(build_dir)
> if build_dir and not os.path.exists(build_dir):
> os.mkdir(build_dir)
> @@ -311,7 +300,7 @@ class LinuxSourceTree(object):
> old_kconfig = kunit_config.parse_file(old_path)
> return old_kconfig.entries() != self._kconfig.entries()
>
> - def build_reconfig(self, build_dir, make_options) -> bool:
> + def build_reconfig(self, build_dir: str, make_options) -> bool:
> """Creates a new .config if it is not a subset of the .kunitconfig."""
> kconfig_path = get_kconfig_path(build_dir)
> if not os.path.exists(kconfig_path):
> @@ -326,7 +315,7 @@ class LinuxSourceTree(object):
> os.remove(kconfig_path)
> return self.build_config(build_dir, make_options)
>
> - def build_kernel(self, alltests, jobs, build_dir, make_options) -> bool:
> + def build_kernel(self, alltests, jobs, build_dir: str, make_options) -> bool:
> try:
> if alltests:
> self._ops.make_allyesconfig(build_dir, make_options)
> diff --git a/tools/testing/kunit/kunit_tool_test.py b/tools/testing/kunit/kunit_tool_test.py
> index f7cbc248a405..a3c036a620b2 100755
> --- a/tools/testing/kunit/kunit_tool_test.py
> +++ b/tools/testing/kunit/kunit_tool_test.py
> @@ -469,7 +469,7 @@ class KUnitJsonTest(unittest.TestCase):
> json_obj = kunit_json.get_json_result(
> test=test_result,
> def_config='kunit_defconfig',
> - build_dir=None)
> + build_dir='.kunit')
> return json.loads(json_obj)
>
> def test_failed_test_json(self):
> --
> 2.34.1.703.g22d0c6ccf7-goog
>

2022-01-21 22:27:07

[permalink] [raw]

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

On Thu, Jan 20, 2022 at 12:29 AM David Gow <[email protected]> wrote:
>
> On Wed, Jan 19, 2022 at 3:09 AM Daniel Latypov <[email protected]> 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 <[email protected]>
> > ---
>
> 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
> >

2022-01-26 22:35:06

[permalink] [raw]

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

On Thu, Jan 20, 2022 at 9:19 AM Daniel Latypov <[email protected]> wrote:
> > 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?

Friendly ping.
Do we want a comment like this?

# Note: Python uses tuples internally for multiple return values
def foo() -> Tuple[int, int]
return 0, 1

I can go ahead and add that and send a v2 out.

FYI, if you do this in a REPL
>>> a = foo()
>>> type(a)
<class 'tuple'>

The syntax for `a, b = foo()` is just using Python's unpacking feature, i.e.
b, c = (1, 2)

So it's all just syntactic sugar around tuples.

>
> 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

2022-01-27 02:01:55

[permalink] [raw]

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

On Wed, Jan 26, 2022 at 2:55 PM Daniel Latypov <[email protected]> wrote:
>
> On Thu, Jan 20, 2022 at 9:19 AM Daniel Latypov <[email protected]> wrote:
> > > 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?
>
> Friendly ping.
> Do we want a comment like this?
>
> # Note: Python uses tuples internally for multiple return values
> def foo() -> Tuple[int, int]
> return 0, 1

I don't feel that's necessary. I think the use of tuple return types
in Python is fairly common and don't require a comment, but I don't
feel strongly about it either way.

> I can go ahead and add that and send a v2 out.
>
> FYI, if you do this in a REPL
> >>> a = foo()
> >>> type(a)
> <class 'tuple'>
>
> The syntax for `a, b = foo()` is just using Python's unpacking feature, i.e.
> b, c = (1, 2)
>
> So it's all just syntactic sugar around tuples.
>
> >
> > 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?

Personally, I think the change as is.

2022-01-27 02:02:13

[permalink] [raw]

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

On Tue, Jan 18, 2022 at 2:09 PM Daniel Latypov <[email protected]> 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 <[email protected]>

Reviewed-by: Brendan Higgins <[email protected]>

2022-01-27 09:14:49

[permalink] [raw]

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

On Thu, Jan 27, 2022 at 3:55 AM 'Daniel Latypov' via KUnit Development
<[email protected]> wrote:
>
> On Thu, Jan 20, 2022 at 9:19 AM Daniel Latypov <[email protected]> wrote:
> > > 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?
>
> Friendly ping.
> Do we want a comment like this?
>
> # Note: Python uses tuples internally for multiple return values
> def foo() -> Tuple[int, int]
> return 0, 1
>

Whoops -- forgot to send my response to this.

I was less worried about explaining the concept of multiple return
values, and more about naming what the return values were: that the
first one is the result information, and the second is the parsed
test.

That being said, it's reasonably obvious from the types in this case,
so I'm okay leaving this as-is, though in general I'm wary of tuples
when the order doesn't matter, and a struct-style thing (with named
members) fits that better.

I'm no Python expert though, so don't let my whinging get too much in the way.

-- David

2022-01-28 13:20:06

[permalink] [raw]

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

On Wed, Jan 26, 2022 at 6:20 PM David Gow <[email protected]> wrote:
>
> On Thu, Jan 27, 2022 at 3:55 AM 'Daniel Latypov' via KUnit Development
> <[email protected]> wrote:
> >
> > On Thu, Jan 20, 2022 at 9:19 AM Daniel Latypov <[email protected]> wrote:
> > > > 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?
> >
> > Friendly ping.
> > Do we want a comment like this?
> >
> > # Note: Python uses tuples internally for multiple return values
> > def foo() -> Tuple[int, int]
> > return 0, 1
> >
>
> Whoops -- forgot to send my response to this.
>
> I was less worried about explaining the concept of multiple return
> values, and more about naming what the return values were: that the
> first one is the result information, and the second is the parsed
> test.
>
> That being said, it's reasonably obvious from the types in this case,
> so I'm okay leaving this as-is, though in general I'm wary of tuples
> when the order doesn't matter, and a struct-style thing (with named
> members) fits that better.

Ack.
Yeah, in this case I don't think creating a new type to name each
value is worth it.
From what I've seen of python codebases, this info is usually captured
in docstrings, but yeah, this particular case seems straightforward
enough that it doesn't need it.

>
> I'm no Python expert though, so don't let my whinging get too much in the way.
>
> -- David

2022-03-24 16:55:02

[permalink] [raw]

Subject: Re: [PATCH 3/5] kunit: tool: drop unused KernelDirectoryPath var

On Tue, Jan 18, 2022 at 2:09 PM Daniel Latypov <[email protected]> wrote:
>
> Commit be886ba90cce ("kunit: run kunit_tool from any directory")
> introduced this variable, but it was unused even in that commit.
>
> Since it's still unused now and callers can instead use
> get_kernel_root_path(), delete this var.
>
> Signed-off-by: Daniel Latypov <[email protected]>

Reviewed-by: Brendan Higgins <[email protected]>

2022-03-24 21:34:37

[permalink] [raw]

Subject: Re: [PATCH 4/5] kunit: tool: drop last uses of collections.namedtuple

On Tue, Jan 18, 2022 at 2:09 PM Daniel Latypov <[email protected]> wrote:
>
> Since we formally require python3.7+ since commit df4b0807ca1a
> ("kunit: tool: Assert the version requirement"), we can just use
> @dataclasses.dataclass instead.
>
> In kunit_config.py, we used namedtuple to create a hashable type that
> had `name` and `value` fields and had to subclass it to define a custom
> `__str__()`.
> @datalcass lets us just define one type instead.
>
> In qemu_config.py, we use namedtuple to allow modules to define various
> parameters. Using @dataclass, we can add type-annotations for all these
> fields, making our code more typesafe and making it easier for users to
> figure out how to define new configs.
>
> Signed-off-by: Daniel Latypov <[email protected]>

Reviewed-by: Brendan Higgins <[email protected]>

2022-03-25 15:14:08

[permalink] [raw]

Subject: Re: [PATCH 2/5] kunit: tool: make --json handling a bit clearer

On Tue, Jan 18, 2022 at 2:09 PM Daniel Latypov <[email protected]> wrote:
>
> Currently kunit_json.get_json_result() will output the JSON-ified test
> output to json_path, but iff it's not "stdout".
>
> Instead, move the responsibility entirely over to the one caller.
>
> Signed-off-by: Daniel Latypov <[email protected]>

Reviewed-by: Brendan Higgins <[email protected]>

2022-03-25 15:35:16