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

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

[Daniel Latypov]

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
-- 
2.34.1.703.g22d0c6ccf7-goog

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

[Daniel Latypov]

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):
-- 
2.34.1.703.g22d0c6ccf7-goog

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

[Daniel Latypov]

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')
-- 
2.34.1.703.g22d0c6ccf7-goog

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

[Daniel Latypov]

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]
-- 
2.34.1.703.g22d0c6ccf7-goog

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

[Daniel Latypov]

--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):
-- 
2.34.1.703.g22d0c6ccf7-goog

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

[David Gow]

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

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

[David Gow]

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

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

[David Gow]

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
>

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

[David Gow]

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: 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]
> --
> 2.34.1.703.g22d0c6ccf7-goog
>

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

[David Gow]

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

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

[Daniel Latypov]

On Thu, Jan 20, 2022 at 12:29 AM David Gow <davidgow@google.com> wrote:
>
> On Wed, Jan 19, 2022 at 3:09 AM Daniel Latypov <dlatypov@google.com> wrote:
> >
> > This field is only used to pass along the parsed Test object from
> > parse_tests().
> > Everywhere else the `result` field is ignored.
> >
> > Instead make parse_tests() explicitly return a KunitResult and Test so
> > we can retire the `result` field.
> >
> > Signed-off-by: Daniel Latypov <dlatypov@google.com>
> > ---
>
> I personally prefer having the Test as part of the result -- it gives
> a slightly rust-esque sense of needing to check the actual result
> before using anything that's parsed. (Also, I'm still not used to the
> whole multiple return value thing, which is not as clear as an
> explicit named struct member, IMHO).
> That being said, we're not actually checking the result before using
> the Test, and certainly the use of Any and mashing a textual error
> message in the same field is rather unpleasant.
>
> My ideal solution would be to rename 'result' to something more
> sensible ('parsed_test', maybe?), and make it explicitly a Test rather
> than Any (and either add a separate field for the textual error
> message, or remove it as in this patch, having noticed that it's
> almost completely redundant to the enum).

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

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

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

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

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

>
> Thoughts?
>
>
> -- David
>
> >  tools/testing/kunit/kunit.py | 24 ++++++++----------------
> >  1 file changed, 8 insertions(+), 16 deletions(-)
> >
> > diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py
> > index 7a706f96f68d..9274c6355809 100755
> > --- a/tools/testing/kunit/kunit.py
> > +++ b/tools/testing/kunit/kunit.py
> > @@ -17,7 +17,7 @@ assert sys.version_info >= (3, 7), "Python version is too old"
> >
> >  from dataclasses import dataclass
> >  from enum import Enum, auto
> > -from typing import Any, Iterable, Sequence, List, Optional
> > +from typing import Iterable, List, Optional, Sequence, Tuple
> >
> >  import kunit_json
> >  import kunit_kernel
> > @@ -32,7 +32,6 @@ class KunitStatus(Enum):
> >  @dataclass
> >  class KunitResult:
> >         status: KunitStatus
> > -       result: Any
> >         elapsed_time: float
> >
> >  @dataclass
> > @@ -82,10 +81,8 @@ def config_tests(linux: kunit_kernel.LinuxSourceTree,
> >         config_end = time.time()
> >         if not success:
> >                 return KunitResult(KunitStatus.CONFIG_FAILURE,
> > -                                  'could not configure kernel',
> >                                    config_end - config_start)
> >         return KunitResult(KunitStatus.SUCCESS,
> > -                          'configured kernel successfully',
> >                            config_end - config_start)
> >
> >  def build_tests(linux: kunit_kernel.LinuxSourceTree,
> > @@ -100,14 +97,11 @@ def build_tests(linux: kunit_kernel.LinuxSourceTree,
> >         build_end = time.time()
> >         if not success:
> >                 return KunitResult(KunitStatus.BUILD_FAILURE,
> > -                                  'could not build kernel',
> >                                    build_end - build_start)
> >         if not success:
> >                 return KunitResult(KunitStatus.BUILD_FAILURE,
> > -                                  'could not build kernel',
> >                                    build_end - build_start)
> >         return KunitResult(KunitStatus.SUCCESS,
> > -                          'built kernel successfully',
> >                            build_end - build_start)
> >
> >  def config_and_build_tests(linux: kunit_kernel.LinuxSourceTree,
> > @@ -173,14 +167,14 @@ def exec_tests(linux: kunit_kernel.LinuxSourceTree, request: KunitExecRequest) -
> >                         filter_glob=filter_glob,
> >                         build_dir=request.build_dir)
> >
> > -               result = parse_tests(request, run_result)
> > +               _, test_result = parse_tests(request, run_result)
> >                 # run_kernel() doesn't block on the kernel exiting.
> >                 # That only happens after we get the last line of output from `run_result`.
> >                 # So exec_time here actually contains parsing + execution time, which is fine.
> >                 test_end = time.time()
> >                 exec_time += test_end - test_start
> >
> > -               test_counts.add_subtest_counts(result.result.counts)
> > +               test_counts.add_subtest_counts(test_result.counts)
> >
> >         if len(filter_globs) == 1 and test_counts.crashed > 0:
> >                 bd = request.build_dir
> > @@ -189,7 +183,7 @@ def exec_tests(linux: kunit_kernel.LinuxSourceTree, request: KunitExecRequest) -
> >                                 bd, bd, kunit_kernel.get_outfile_path(bd), bd, sys.argv[0]))
> >
> >         kunit_status = _map_to_overall_status(test_counts.get_status())
> > -       return KunitResult(status=kunit_status, result=result, elapsed_time=exec_time)
> > +       return KunitResult(status=kunit_status, elapsed_time=exec_time)
> >
> >  def _map_to_overall_status(test_status: kunit_parser.TestStatus) -> KunitStatus:
> >         if test_status in (kunit_parser.TestStatus.SUCCESS, kunit_parser.TestStatus.SKIPPED):
> > @@ -197,7 +191,7 @@ def _map_to_overall_status(test_status: kunit_parser.TestStatus) -> KunitStatus:
> >         else:
> >                 return KunitStatus.TEST_FAILURE
> >
> > -def parse_tests(request: KunitParseRequest, input_data: Iterable[str]) -> KunitResult:
> > +def parse_tests(request: KunitParseRequest, input_data: Iterable[str]) -> Tuple[KunitResult, kunit_parser.Test]:
> >         parse_start = time.time()
> >
> >         test_result = kunit_parser.Test()
> > @@ -231,11 +225,9 @@ def parse_tests(request: KunitParseRequest, input_data: Iterable[str]) -> KunitR
> >                         print(json_obj)
> >
> >         if test_result.status != kunit_parser.TestStatus.SUCCESS:
> > -               return KunitResult(KunitStatus.TEST_FAILURE, test_result,
> > -                                  parse_end - parse_start)
> > +               return KunitResult(KunitStatus.TEST_FAILURE, parse_end - parse_start), test_result
> >
> > -       return KunitResult(KunitStatus.SUCCESS, test_result,
> > -                               parse_end - parse_start)
> > +       return KunitResult(KunitStatus.SUCCESS, parse_end - parse_start), test_result
> >
> >  def run_tests(linux: kunit_kernel.LinuxSourceTree,
> >               request: KunitRequest) -> KunitResult:
> > @@ -513,7 +505,7 @@ def main(argv, linux=None):
> >                 request = KunitParseRequest(raw_output=cli_args.raw_output,
> >                                             build_dir='',
> >                                             json=cli_args.json)
> > -               result = parse_tests(request, kunit_output)
> > +               result, _ = parse_tests(request, kunit_output)
> >                 if result.status != KunitStatus.SUCCESS:
> >                         sys.exit(1)
> >         else:
> >
> > base-commit: f079ab01b5609fb0c9acc52c88168bf1eed82373
> > --
> > 2.34.1.703.g22d0c6ccf7-goog
> >

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

[Daniel Latypov]

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

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

[Brendan Higgins]

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.

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

[Brendan Higgins]

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>

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

[David Gow]

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

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

[Daniel Latypov]

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

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

[Brendan Higgins]

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>

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

[Brendan Higgins]

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>

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

[Brendan Higgins]

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>

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

[Brendan Higgins]

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>