This field is only used to pass along the parsed Test object from parse_tests(). Everywhere else the `result` field is ignored.
Instead make parse_tests() explicitly return a KunitResult and Test so we can retire the `result` field.
Signed-off-by: Daniel Latypov dlatypov@google.com --- 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
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 dlatypov@google.com --- 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):
On Wed, Jan 19, 2022 at 3:09 AM Daniel Latypov dlatypov@google.com 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 dlatypov@google.com
This looks good to me.
Reviewed-by: David Gow davidgow@google.com
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_resultdiff --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
On Tue, Jan 18, 2022 at 2:09 PM Daniel Latypov dlatypov@google.com 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 dlatypov@google.com
Reviewed-by: Brendan Higgins brendanhiggins@google.com
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 dlatypov@google.com --- 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')
On Wed, Jan 19, 2022 at 3:09 AM Daniel Latypov dlatypov@google.com 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 dlatypov@google.com
Reviewed-by: David Gow davidgow@google.com
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
On Tue, Jan 18, 2022 at 2:09 PM Daniel Latypov dlatypov@google.com 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 dlatypov@google.com
Reviewed-by: Brendan Higgins brendanhiggins@google.com
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 dlatypov@google.com --- 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 felixguoxiuping@gmail.com # Author: Brendan Higgins brendanhiggins@google.com
-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 brendanhiggins@google.com
-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]
On Wed, Jan 19, 2022 at 3:09 AM Daniel Latypov dlatypov@google.com 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 dlatypov@google.com
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 davidgow@google.com
-- 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 felixguoxiuping@gmail.com # Author: Brendan Higgins brendanhiggins@google.com
-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: strvalue: 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 brendanhiggins@google.com
-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
On Tue, Jan 18, 2022 at 2:09 PM Daniel Latypov dlatypov@google.com 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 dlatypov@google.com
Reviewed-by: Brendan Higgins brendanhiggins@google.com
--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 dlatypov@google.com --- 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):
On Wed, Jan 19, 2022 at 3:09 AM Daniel Latypov dlatypov@google.com 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 dlatypov@google.com
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 davidgow@google.com
-- 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 defaultclass 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
On Tue, Jan 18, 2022 at 2:09 PM Daniel Latypov dlatypov@google.com 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 dlatypov@google.com
Reviewed-by: Brendan Higgins brendanhiggins@google.com
On Wed, Jan 19, 2022 at 3:09 AM Daniel Latypov dlatypov@google.com wrote:
This field is only used to pass along the parsed Test object from parse_tests(). Everywhere else the `result` field is ignored.
Instead make parse_tests() explicitly return a KunitResult and Test so we can retire the `result` field.
Signed-off-by: Daniel Latypov dlatypov@google.com
I personally prefer having the Test as part of the result -- it gives a slightly rust-esque sense of needing to check the actual result before using anything that's parsed. (Also, I'm still not used to the whole multiple return value thing, which is not as clear as an explicit named struct member, IMHO). That being said, we're not actually checking the result before using the Test, and certainly the use of Any and mashing a textual error message in the same field is rather unpleasant.
My ideal solution would be to rename 'result' to something more sensible ('parsed_test', maybe?), and make it explicitly a Test rather than Any (and either add a separate field for the textual error message, or remove it as in this patch, having noticed that it's almost completely redundant to the enum).
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_resultdef 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
On Thu, Jan 20, 2022 at 12:29 AM David Gow davidgow@google.com wrote:
On Wed, Jan 19, 2022 at 3:09 AM Daniel Latypov dlatypov@google.com wrote:
This field is only used to pass along the parsed Test object from parse_tests(). Everywhere else the `result` field is ignored.
Instead make parse_tests() explicitly return a KunitResult and Test so we can retire the `result` field.
Signed-off-by: Daniel Latypov dlatypov@google.com
I personally prefer having the Test as part of the result -- it gives a slightly rust-esque sense of needing to check the actual result before using anything that's parsed. (Also, I'm still not used to the whole multiple return value thing, which is not as clear as an explicit named struct member, IMHO). That being said, we're not actually checking the result before using the Test, and certainly the use of Any and mashing a textual error message in the same field is rather unpleasant.
My ideal solution would be to rename 'result' to something more sensible ('parsed_test', maybe?), and make it explicitly a Test rather than Any (and either add a separate field for the textual error message, or remove it as in this patch, having noticed that it's almost completely redundant to the enum).
Yeah, I considered that for a bit, but I don't like having a field that is only sometimes set. I had thought we were passing back the test object from exec_tests(), but I was wrong. We were actually passing back a KunitResult with a KunitResult[Test] in it.
So when I saw only parse_tests() actually wanted to pass back a test object, I thought it was cleaner to just use a separate return value.
That being said, I can live with the current solution, but'd ideally like a comment or something to make the return value Tuple a bit more obvious.
A comment to explain that Tuple == multiple return values from a func? Or something else?
Also ah, I thought we had more instances of multiple return in kunit.py. Looks like the only other is get_source_tree_ops_from_qemu_config(). isolate_ktap_output() technically shows this off as well, but via yields.
Thoughts?
-- David
tools/testing/kunit/kunit.py | 24 ++++++++---------------- 1 file changed, 8 insertions(+), 16 deletions(-)
diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py index 7a706f96f68d..9274c6355809 100755 --- a/tools/testing/kunit/kunit.py +++ b/tools/testing/kunit/kunit.py @@ -17,7 +17,7 @@ assert sys.version_info >= (3, 7), "Python version is too old"
from dataclasses import dataclass from enum import Enum, auto -from typing import Any, Iterable, Sequence, List, Optional +from typing import Iterable, List, Optional, Sequence, Tuple
import kunit_json import kunit_kernel @@ -32,7 +32,6 @@ class KunitStatus(Enum): @dataclass class KunitResult: status: KunitStatus
result: Any elapsed_time: float@dataclass @@ -82,10 +81,8 @@ def config_tests(linux: kunit_kernel.LinuxSourceTree, config_end = time.time() if not success: return KunitResult(KunitStatus.CONFIG_FAILURE,
'could not configure kernel', config_end - config_start) return KunitResult(KunitStatus.SUCCESS,'configured kernel successfully', config_end - config_start)def build_tests(linux: kunit_kernel.LinuxSourceTree, @@ -100,14 +97,11 @@ def build_tests(linux: kunit_kernel.LinuxSourceTree, build_end = time.time() if not success: return KunitResult(KunitStatus.BUILD_FAILURE,
'could not build kernel', build_end - build_start) if not success: return KunitResult(KunitStatus.BUILD_FAILURE,'could not build kernel', build_end - build_start) return KunitResult(KunitStatus.SUCCESS,'built kernel successfully', build_end - build_start)def config_and_build_tests(linux: kunit_kernel.LinuxSourceTree, @@ -173,14 +167,14 @@ def exec_tests(linux: kunit_kernel.LinuxSourceTree, request: KunitExecRequest) - filter_glob=filter_glob, build_dir=request.build_dir)
result = parse_tests(request, run_result)
_, test_result = parse_tests(request, run_result) # run_kernel() doesn't block on the kernel exiting. # That only happens after we get the last line of output from `run_result`. # So exec_time here actually contains parsing + execution time, which is fine. test_end = time.time() exec_time += test_end - test_start
test_counts.add_subtest_counts(result.result.counts)
test_counts.add_subtest_counts(test_result.counts) if len(filter_globs) == 1 and test_counts.crashed > 0: bd = request.build_dir@@ -189,7 +183,7 @@ def exec_tests(linux: kunit_kernel.LinuxSourceTree, request: KunitExecRequest) - bd, bd, kunit_kernel.get_outfile_path(bd), bd, sys.argv[0]))
kunit_status = _map_to_overall_status(test_counts.get_status())
return KunitResult(status=kunit_status, result=result, elapsed_time=exec_time)
return KunitResult(status=kunit_status, elapsed_time=exec_time)def _map_to_overall_status(test_status: kunit_parser.TestStatus) -> KunitStatus: if test_status in (kunit_parser.TestStatus.SUCCESS, kunit_parser.TestStatus.SKIPPED): @@ -197,7 +191,7 @@ def _map_to_overall_status(test_status: kunit_parser.TestStatus) -> KunitStatus: else: return KunitStatus.TEST_FAILURE
-def parse_tests(request: KunitParseRequest, input_data: Iterable[str]) -> KunitResult: +def parse_tests(request: KunitParseRequest, input_data: Iterable[str]) -> Tuple[KunitResult, kunit_parser.Test]: parse_start = time.time()
test_result = kunit_parser.Test()@@ -231,11 +225,9 @@ def parse_tests(request: KunitParseRequest, input_data: Iterable[str]) -> KunitR print(json_obj)
if test_result.status != kunit_parser.TestStatus.SUCCESS:
return KunitResult(KunitStatus.TEST_FAILURE, test_result,parse_end - parse_start)
return KunitResult(KunitStatus.TEST_FAILURE, parse_end - parse_start), test_result
return KunitResult(KunitStatus.SUCCESS, test_result,parse_end - parse_start)
return KunitResult(KunitStatus.SUCCESS, parse_end - parse_start), test_resultdef 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
On Thu, Jan 20, 2022 at 9:19 AM Daniel Latypov dlatypov@google.com 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
On Wed, Jan 26, 2022 at 2:55 PM Daniel Latypov dlatypov@google.com wrote:
On Thu, Jan 20, 2022 at 9:19 AM Daniel Latypov dlatypov@google.com 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.
On Thu, Jan 27, 2022 at 3:55 AM 'Daniel Latypov' via KUnit Development kunit-dev@googlegroups.com wrote:
On Thu, Jan 20, 2022 at 9:19 AM Daniel Latypov dlatypov@google.com 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
On Wed, Jan 26, 2022 at 6:20 PM David Gow davidgow@google.com wrote:
On Thu, Jan 27, 2022 at 3:55 AM 'Daniel Latypov' via KUnit Development kunit-dev@googlegroups.com wrote:
On Thu, Jan 20, 2022 at 9:19 AM Daniel Latypov dlatypov@google.com 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
On Tue, Jan 18, 2022 at 2:09 PM Daniel Latypov dlatypov@google.com wrote:
This field is only used to pass along the parsed Test object from parse_tests(). Everywhere else the `result` field is ignored.
Instead make parse_tests() explicitly return a KunitResult and Test so we can retire the `result` field.
Signed-off-by: Daniel Latypov dlatypov@google.com
Reviewed-by: Brendan Higgins brendanhiggins@google.com
linux-kselftest-mirror@lists.linaro.org
-
Brendan Higgins -
Daniel Latypov -
David Gow