The authors of this tool were more familiar with a different type-checker, https://github.com/google/pytype.
That's open source, but mypy seems more prevalent (and runs faster). And unlike pytype, mypy doesn't try to infer types so it doesn't check unanotated functions.
So annotate ~all functions in kunit tool to increase type-checking coverage. Note: per https://www.python.org/dev/peps/pep-0484/, `__init__()` should be annotated as `-> None`.
Doing so makes mypy discover a number of new violations. Exclude main() since we reuse `request` for the different types of requests, which mypy isn't happy about.
This commit fixes all but one error, where `TestSuite.status` might be None.
Signed-off-by: Daniel Latypov dlatypov@google.com --- tools/testing/kunit/kunit.py | 14 ++++----- tools/testing/kunit/kunit_config.py | 7 +++-- tools/testing/kunit/kunit_json.py | 2 +- tools/testing/kunit/kunit_kernel.py | 37 ++++++++++++----------- tools/testing/kunit/kunit_parser.py | 46 ++++++++++++++--------------- 5 files changed, 54 insertions(+), 52 deletions(-)
diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py index d4f7846d0745..08951a114654 100755 --- a/tools/testing/kunit/kunit.py +++ b/tools/testing/kunit/kunit.py @@ -43,9 +43,9 @@ class KunitStatus(Enum): BUILD_FAILURE = auto() TEST_FAILURE = auto()
-def get_kernel_root_path(): - parts = sys.argv[0] if not __file__ else __file__ - parts = os.path.realpath(parts).split('tools/testing/kunit') +def get_kernel_root_path() -> str: + path = sys.argv[0] if not __file__ else __file__ + parts = os.path.realpath(path).split('tools/testing/kunit') if len(parts) != 2: sys.exit(1) return parts[0] @@ -171,7 +171,7 @@ def run_tests(linux: kunit_kernel.LinuxSourceTree, exec_result.elapsed_time)) return parse_result
-def add_common_opts(parser): +def add_common_opts(parser) -> None: parser.add_argument('--build_dir', help='As in the make command, it specifies the build ' 'directory.', @@ -183,13 +183,13 @@ def add_common_opts(parser): help='Run all KUnit tests through allyesconfig', action='store_true')
-def add_build_opts(parser): +def add_build_opts(parser) -> None: parser.add_argument('--jobs', help='As in the make command, "Specifies the number of ' 'jobs (commands) to run simultaneously."', type=int, default=8, metavar='jobs')
-def add_exec_opts(parser): +def add_exec_opts(parser) -> None: parser.add_argument('--timeout', help='maximum number of seconds to allow for all tests ' 'to run. This does not include time taken to build the ' @@ -198,7 +198,7 @@ def add_exec_opts(parser): default=300, metavar='timeout')
-def add_parse_opts(parser): +def add_parse_opts(parser) -> None: parser.add_argument('--raw_output', help='don't format output from kernel', action='store_true') parser.add_argument('--json', diff --git a/tools/testing/kunit/kunit_config.py b/tools/testing/kunit/kunit_config.py index 02ffc3a3e5dc..bdd60230764b 100644 --- a/tools/testing/kunit/kunit_config.py +++ b/tools/testing/kunit/kunit_config.py @@ -8,6 +8,7 @@
import collections import re +from typing import List, Set
CONFIG_IS_NOT_SET_PATTERN = r'^# CONFIG_(\w+) is not set$' CONFIG_PATTERN = r'^CONFIG_(\w+)=(\S+|".*")$' @@ -30,10 +31,10 @@ class KconfigParseError(Exception): class Kconfig(object): """Represents defconfig or .config specified using the Kconfig language."""
- def __init__(self): - self._entries = [] + def __init__(self) -> None: + self._entries = [] # type: List[KconfigEntry]
- def entries(self): + def entries(self) -> Set[KconfigEntry]: return set(self._entries)
def add_entry(self, entry: KconfigEntry) -> None: diff --git a/tools/testing/kunit/kunit_json.py b/tools/testing/kunit/kunit_json.py index 624b31b2dbd6..f5cca5c38cac 100644 --- a/tools/testing/kunit/kunit_json.py +++ b/tools/testing/kunit/kunit_json.py @@ -13,7 +13,7 @@ import kunit_parser
from kunit_parser import TestStatus
-def get_json_result(test_result, def_config, build_dir, json_path): +def get_json_result(test_result, def_config, build_dir, json_path) -> str: sub_groups = []
# Each test suite is mapped to a KernelCI sub_group diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py index 2e3cc0fac726..bda7c4fd4d3e 100644 --- a/tools/testing/kunit/kunit_kernel.py +++ b/tools/testing/kunit/kunit_kernel.py @@ -11,6 +11,7 @@ import subprocess import os import shutil import signal +from typing import Iterator
from contextlib import ExitStack
@@ -34,7 +35,7 @@ class BuildError(Exception): class LinuxSourceTreeOperations(object): """An abstraction over command line operations performed on a source tree."""
- def make_mrproper(self): + def make_mrproper(self) -> None: try: subprocess.check_output(['make', 'mrproper'], stderr=subprocess.STDOUT) except OSError as e: @@ -42,7 +43,7 @@ class LinuxSourceTreeOperations(object): except subprocess.CalledProcessError as e: raise ConfigError(e.output.decode())
- def make_olddefconfig(self, build_dir, make_options): + def make_olddefconfig(self, build_dir, make_options) -> None: command = ['make', 'ARCH=um', 'olddefconfig'] if make_options: command.extend(make_options) @@ -55,7 +56,7 @@ class LinuxSourceTreeOperations(object): except subprocess.CalledProcessError as e: raise ConfigError(e.output.decode())
- def make_allyesconfig(self, build_dir, make_options): + def make_allyesconfig(self, build_dir, make_options) -> None: kunit_parser.print_with_timestamp( 'Enabling all CONFIGs for UML...') command = ['make', 'ARCH=um', 'allyesconfig'] @@ -77,7 +78,7 @@ class LinuxSourceTreeOperations(object): kunit_parser.print_with_timestamp( 'Starting Kernel with all configs takes a few minutes...')
- def make(self, jobs, build_dir, make_options): + def make(self, jobs, build_dir, make_options) -> None: command = ['make', 'ARCH=um', '--jobs=' + str(jobs)] if make_options: command.extend(make_options) @@ -95,7 +96,7 @@ class LinuxSourceTreeOperations(object): if stderr: # likely only due to build warnings print(stderr.decode())
- def linux_bin(self, params, timeout, build_dir): + def linux_bin(self, params, timeout, build_dir) -> None: """Runs the Linux UML binary. Must be named 'linux'.""" linux_bin = './linux' if build_dir: @@ -107,19 +108,19 @@ class LinuxSourceTreeOperations(object): stderr=subprocess.STDOUT) process.wait(timeout)
-def get_kconfig_path(build_dir): +def get_kconfig_path(build_dir) -> str: kconfig_path = KCONFIG_PATH if build_dir: kconfig_path = os.path.join(build_dir, KCONFIG_PATH) return kconfig_path
-def get_kunitconfig_path(build_dir): +def get_kunitconfig_path(build_dir) -> str: kunitconfig_path = KUNITCONFIG_PATH if build_dir: kunitconfig_path = os.path.join(build_dir, KUNITCONFIG_PATH) return kunitconfig_path
-def get_outfile_path(build_dir): +def get_outfile_path(build_dir) -> str: outfile_path = OUTFILE_PATH if build_dir: outfile_path = os.path.join(build_dir, OUTFILE_PATH) @@ -128,11 +129,11 @@ def get_outfile_path(build_dir): class LinuxSourceTree(object): """Represents a Linux kernel source tree with KUnit tests."""
- def __init__(self): + def __init__(self) -> None: self._ops = LinuxSourceTreeOperations() signal.signal(signal.SIGINT, self.signal_handler)
- def clean(self): + def clean(self) -> bool: try: self._ops.make_mrproper() except ConfigError as e: @@ -140,17 +141,17 @@ class LinuxSourceTree(object): return False return True
- def create_kunitconfig(self, build_dir, defconfig=DEFAULT_KUNITCONFIG_PATH): + def create_kunitconfig(self, build_dir, defconfig=DEFAULT_KUNITCONFIG_PATH) -> None: kunitconfig_path = get_kunitconfig_path(build_dir) if not os.path.exists(kunitconfig_path): shutil.copyfile(defconfig, kunitconfig_path)
- def read_kunitconfig(self, build_dir): + def read_kunitconfig(self, build_dir) -> None: kunitconfig_path = get_kunitconfig_path(build_dir) self._kconfig = kunit_config.Kconfig() self._kconfig.read_from_file(kunitconfig_path)
- def validate_config(self, build_dir): + def validate_config(self, build_dir) -> bool: kconfig_path = get_kconfig_path(build_dir) validated_kconfig = kunit_config.Kconfig() validated_kconfig.read_from_file(kconfig_path) @@ -164,7 +165,7 @@ class LinuxSourceTree(object): return False return True
- def build_config(self, build_dir, make_options): + def build_config(self, build_dir, make_options) -> bool: kconfig_path = get_kconfig_path(build_dir) if build_dir and not os.path.exists(build_dir): os.mkdir(build_dir) @@ -176,7 +177,7 @@ class LinuxSourceTree(object): return False return self.validate_config(build_dir)
- def build_reconfig(self, build_dir, make_options): + def build_reconfig(self, build_dir, make_options) -> bool: """Creates a new .config if it is not a subset of the .kunitconfig.""" kconfig_path = get_kconfig_path(build_dir) if os.path.exists(kconfig_path): @@ -192,7 +193,7 @@ class LinuxSourceTree(object): print('Generating .config ...') return self.build_config(build_dir, make_options)
- def build_um_kernel(self, alltests, jobs, build_dir, make_options): + def build_um_kernel(self, alltests, jobs, build_dir, make_options) -> bool: try: if alltests: self._ops.make_allyesconfig(build_dir, make_options) @@ -203,7 +204,7 @@ class LinuxSourceTree(object): return False return self.validate_config(build_dir)
- def run_kernel(self, args=[], build_dir='', timeout=None): + def run_kernel(self, args=[], build_dir='', timeout=None) -> Iterator[str]: args.extend(['mem=1G']) self._ops.linux_bin(args, timeout, build_dir) outfile = get_outfile_path(build_dir) @@ -212,6 +213,6 @@ class LinuxSourceTree(object): for line in file: yield line
- def signal_handler(self, sig, frame): + def signal_handler(self, sig, frame) -> None: logging.error('Build interruption occurred. Cleaning console.') subprocess.call(['stty', 'sane']) diff --git a/tools/testing/kunit/kunit_parser.py b/tools/testing/kunit/kunit_parser.py index bbfe1b4e4c1c..24954bbc9baf 100644 --- a/tools/testing/kunit/kunit_parser.py +++ b/tools/testing/kunit/kunit_parser.py @@ -12,32 +12,32 @@ from collections import namedtuple from datetime import datetime from enum import Enum, auto from functools import reduce -from typing import List, Optional, Tuple +from typing import Iterator, List, Optional, Tuple
TestResult = namedtuple('TestResult', ['status','suites','log'])
class TestSuite(object): - def __init__(self): - self.status = None - self.name = None - self.cases = [] + def __init__(self) -> None: + self.status = None # type: Optional[TestStatus] + self.name = '' + self.cases = [] # type: List[TestCase]
- def __str__(self): - return 'TestSuite(' + self.status + ',' + self.name + ',' + str(self.cases) + ')' + def __str__(self) -> str: + return 'TestSuite(' + str(self.status) + ',' + self.name + ',' + str(self.cases) + ')'
- def __repr__(self): + def __repr__(self) -> str: return str(self)
class TestCase(object): - def __init__(self): - self.status = None + def __init__(self) -> None: + self.status = None # type: Optional[TestStatus] self.name = '' - self.log = [] + self.log = [] # type: List[str]
- def __str__(self): - return 'TestCase(' + self.status + ',' + self.name + ',' + str(self.log) + ')' + def __str__(self) -> str: + return 'TestCase(' + str(self.status) + ',' + self.name + ',' + str(self.log) + ')'
- def __repr__(self): + def __repr__(self) -> str: return str(self)
class TestStatus(Enum): @@ -51,7 +51,7 @@ kunit_start_re = re.compile(r'TAP version [0-9]+$') kunit_end_re = re.compile('(List of all partitions:|' 'Kernel panic - not syncing: VFS:)')
-def isolate_kunit_output(kernel_output): +def isolate_kunit_output(kernel_output) -> Iterator[str]: started = False for line in kernel_output: line = line.rstrip() # line always has a trailing \n @@ -64,7 +64,7 @@ def isolate_kunit_output(kernel_output): elif started: yield line[prefix_len:] if prefix_len > 0 else line
-def raw_output(kernel_output): +def raw_output(kernel_output) -> None: for line in kernel_output: print(line.rstrip())
@@ -72,26 +72,26 @@ DIVIDER = '=' * 60
RESET = '\033[0;0m'
-def red(text): +def red(text) -> str: return '\033[1;31m' + text + RESET
-def yellow(text): +def yellow(text) -> str: return '\033[1;33m' + text + RESET
-def green(text): +def green(text) -> str: return '\033[1;32m' + text + RESET
-def print_with_timestamp(message): +def print_with_timestamp(message) -> None: print('[%s] %s' % (datetime.now().strftime('%H:%M:%S'), message))
-def format_suite_divider(message): +def format_suite_divider(message) -> str: return '======== ' + message + ' ========'
-def print_suite_divider(message): +def print_suite_divider(message) -> None: print_with_timestamp(DIVIDER) print_with_timestamp(format_suite_divider(message))
-def print_log(log): +def print_log(log) -> None: for m in log: print_with_timestamp(m)
base-commit: 34816d20f173a90389c8a7e641166d8ea9dce70a
The code to handle aggregating statuses didn't check that the status actually got set. Default the value to SUCCESS.
This sorta follows the precedent in commit 3fc48259d525 ("kunit: Don't fail test suites if one of them is empty").
Also slightly simplify the code and add type annotations.
Signed-off-by: Daniel Latypov dlatypov@google.com --- tools/testing/kunit/kunit_parser.py | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/tools/testing/kunit/kunit_parser.py b/tools/testing/kunit/kunit_parser.py index 24954bbc9baf..97e070506c31 100644 --- a/tools/testing/kunit/kunit_parser.py +++ b/tools/testing/kunit/kunit_parser.py @@ -12,13 +12,13 @@ from collections import namedtuple from datetime import datetime from enum import Enum, auto from functools import reduce -from typing import Iterator, List, Optional, Tuple +from typing import Iterable, Iterator, List, Optional, Tuple
TestResult = namedtuple('TestResult', ['status','suites','log'])
class TestSuite(object): def __init__(self) -> None: - self.status = None # type: Optional[TestStatus] + self.status = TestStatus.SUCCESS self.name = '' self.cases = [] # type: List[TestCase]
@@ -30,7 +30,7 @@ class TestSuite(object):
class TestCase(object): def __init__(self) -> None: - self.status = None # type: Optional[TestStatus] + self.status = TestStatus.SUCCESS self.name = '' self.log = [] # type: List[str]
@@ -223,12 +223,11 @@ def parse_ok_not_ok_test_suite(lines: List[str], else: return False
-def bubble_up_errors(to_status, status_container_list) -> TestStatus: - status_list = map(to_status, status_container_list) - return reduce(max_status, status_list, TestStatus.SUCCESS) +def bubble_up_errors(statuses: Iterable[TestStatus]) -> TestStatus: + return reduce(max_status, statuses, TestStatus.SUCCESS)
def bubble_up_test_case_errors(test_suite: TestSuite) -> TestStatus: - max_test_case_status = bubble_up_errors(lambda x: x.status, test_suite.cases) + max_test_case_status = bubble_up_errors(x.status for x in test_suite.cases) return max_status(max_test_case_status, test_suite.status)
def parse_test_suite(lines: List[str], expected_suite_index: int) -> Optional[TestSuite]: @@ -281,8 +280,8 @@ def parse_test_plan(lines: List[str]) -> Optional[int]: else: return None
-def bubble_up_suite_errors(test_suite_list: List[TestSuite]) -> TestStatus: - return bubble_up_errors(lambda x: x.status, test_suite_list) +def bubble_up_suite_errors(test_suites: Iterable[TestSuite]) -> TestStatus: + return bubble_up_errors(x.status for x in test_suites)
def parse_test_result(lines: List[str]) -> TestResult: consume_non_diagnositic(lines)
On Fri, Dec 4, 2020 at 3:41 AM Daniel Latypov dlatypov@google.com wrote:
This seems good to me, but I have a few questions, particularly around the description.
The code to handle aggregating statuses didn't check that the status actually got set.
I don't understand what you mean here. Does this refer to Test{Case,Suite}::status potentially being None, and that not being supported properly in bubble_up_{suite_,test_case_,}errors(), or something else? Either way, I can't see any additional code to "check" that the status has been set. As far as I can tell everything except the default to SUCCESS is a no-op, or am I missing something?
Default the value to SUCCESS.
I'm a little iffy about defaulting this to success, but I think it's okay for now: the skip test support will eventually change this to a SKIPPED value.
This sorta follows the precedent in commit 3fc48259d525 ("kunit: Don't fail test suites if one of them is empty").
Also slightly simplify the code and add type annotations.
Signed-off-by: Daniel Latypov dlatypov@google.com
Otherwise, the actual code changes all seem sensible, and it worked fine when I tested it, so:
Reviewed-by: David Gow davidgow@google.com
-- David
On Thu, Dec 3, 2020 at 8:17 PM David Gow davidgow@google.com wrote:
On Fri, Dec 4, 2020 at 3:41 AM Daniel Latypov dlatypov@google.com wrote:
This seems good to me, but I have a few questions, particularly around the description.
The code to handle aggregating statuses didn't check that the status actually got set.
I don't understand what you mean here. Does this refer to Test{Case,Suite}::status potentially being None, and that not being supported properly in bubble_up_{suite_,test_case_,}errors(), or something else? Either way, I can't see any additional code to "check" that the status has been set. As far as I can tell everything except the default to SUCCESS is a no-op, or am I missing something?
mypy (rightly) sees the type is TestStatus or None and complains we don't bother handling None, so we risk a crash in the tool. The status will be none until we explicitly assign a value to it later, which we always do currently, afaict.
This change just avoids that by giving it a default value to make mypy happy, which shouldn't change behaviour at all right now.
There is no other (potential) behavioural change.
Default the value to SUCCESS.
I'm a little iffy about defaulting this to success, but I think it's okay for now: the skip test support will eventually change this to a SKIPPED value.
Sounds good!
This sorta follows the precedent in commit 3fc48259d525 ("kunit: Don't fail test suites if one of them is empty").
Also slightly simplify the code and add type annotations.
Signed-off-by: Daniel Latypov dlatypov@google.com
Otherwise, the actual code changes all seem sensible, and it worked fine when I tested it, so:
Reviewed-by: David Gow davidgow@google.com
-- David
LinuxSourceTree will unceremoniously crash if the user doesn't call read_kunitconfig() first in a number of functions.
Adn currently every place we create an instance, the caller also calls create_kunitconfig() and read_kunitconfig().
Move these instead into __init__() so they can't be forgotten and to reduce copy-paste.
The https://github.com/google/pytype type-checker complained that _config wasn't initialized. With this, kunit_tool now type checks under both pytype and mypy.
Signed-off-by: Daniel Latypov dlatypov@google.com --- tools/testing/kunit/kunit.py | 20 ++++---------------- tools/testing/kunit/kunit_kernel.py | 19 +++++++------------ 2 files changed, 11 insertions(+), 28 deletions(-)
diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py index 08951a114654..b58fb3733cfa 100755 --- a/tools/testing/kunit/kunit.py +++ b/tools/testing/kunit/kunit.py @@ -256,10 +256,7 @@ def main(argv, linux=None): os.mkdir(cli_args.build_dir)
if not linux: - linux = kunit_kernel.LinuxSourceTree() - - linux.create_kunitconfig(cli_args.build_dir) - linux.read_kunitconfig(cli_args.build_dir) + linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir)
request = KunitRequest(cli_args.raw_output, cli_args.timeout, @@ -277,10 +274,7 @@ def main(argv, linux=None): os.mkdir(cli_args.build_dir)
if not linux: - linux = kunit_kernel.LinuxSourceTree() - - linux.create_kunitconfig(cli_args.build_dir) - linux.read_kunitconfig(cli_args.build_dir) + linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir)
request = KunitConfigRequest(cli_args.build_dir, cli_args.make_options) @@ -292,10 +286,7 @@ def main(argv, linux=None): sys.exit(1) elif cli_args.subcommand == 'build': if not linux: - linux = kunit_kernel.LinuxSourceTree() - - linux.create_kunitconfig(cli_args.build_dir) - linux.read_kunitconfig(cli_args.build_dir) + linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir)
request = KunitBuildRequest(cli_args.jobs, cli_args.build_dir, @@ -309,10 +300,7 @@ def main(argv, linux=None): sys.exit(1) elif cli_args.subcommand == 'exec': if not linux: - linux = kunit_kernel.LinuxSourceTree() - - linux.create_kunitconfig(cli_args.build_dir) - linux.read_kunitconfig(cli_args.build_dir) + linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir)
exec_request = KunitExecRequest(cli_args.timeout, cli_args.build_dir, diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py index bda7c4fd4d3e..79793031d2c4 100644 --- a/tools/testing/kunit/kunit_kernel.py +++ b/tools/testing/kunit/kunit_kernel.py @@ -129,10 +129,15 @@ def get_outfile_path(build_dir) -> str: class LinuxSourceTree(object): """Represents a Linux kernel source tree with KUnit tests."""
- def __init__(self) -> None: - self._ops = LinuxSourceTreeOperations() + def __init__(self, build_dir: str, defconfig=DEFAULT_KUNITCONFIG_PATH) -> None: signal.signal(signal.SIGINT, self.signal_handler)
+ self._ops = LinuxSourceTreeOperations() + + kunitconfig_path = get_kunitconfig_path(build_dir) + self._kconfig = kunit_config.Kconfig() + self._kconfig.read_from_file(kunitconfig_path) + def clean(self) -> bool: try: self._ops.make_mrproper() @@ -141,16 +146,6 @@ class LinuxSourceTree(object): return False return True
- def create_kunitconfig(self, build_dir, defconfig=DEFAULT_KUNITCONFIG_PATH) -> None: - kunitconfig_path = get_kunitconfig_path(build_dir) - if not os.path.exists(kunitconfig_path): - shutil.copyfile(defconfig, kunitconfig_path) - - def read_kunitconfig(self, build_dir) -> None: - kunitconfig_path = get_kunitconfig_path(build_dir) - self._kconfig = kunit_config.Kconfig() - self._kconfig.read_from_file(kunitconfig_path) - def validate_config(self, build_dir) -> bool: kconfig_path = get_kconfig_path(build_dir) validated_kconfig = kunit_config.Kconfig()
On Fri, Dec 4, 2020 at 3:41 AM Daniel Latypov dlatypov@google.com wrote:
LinuxSourceTree will unceremoniously crash if the user doesn't call read_kunitconfig() first in a number of functions.
This patch seems to partly be reverting the changes here, right: https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/co... (That patch moved the reading of kunitconfig out of __init__)
My overall concern is that, really, there are some operations that shouldn't need a kunitconfig (even if they do at the moment), so we'd ideally want at least some of the operations currently under LinuxSourceTree to be able to be run without first reading a kunitconfig. Most notably, it'd be nice if kunit.py exec (and hence LinuxSourceTree::run_kernel()) didn't need a kunitconfig, as the kernel ought to already be built at this point.
Now, this is all a little bit hypothetical, as we haven't bothered to make kunit.py exec work without a kunitconfig thus far, but I'm a touch hesitant to make it harder to bypass the kunitconfig reading anyway.
Adn currently every place we create an instance, the caller also calls create_kunitconfig() and read_kunitconfig().
Move these instead into __init__() so they can't be forgotten and to reduce copy-paste.
This seems to now be missing the create_kunitconfig() stuff (see below).
The https://github.com/google/pytype type-checker complained that _config wasn't initialized. With this, kunit_tool now type checks under both pytype and mypy.
Signed-off-by: Daniel Latypov dlatypov@google.com
tools/testing/kunit/kunit.py | 20 ++++---------------- tools/testing/kunit/kunit_kernel.py | 19 +++++++------------ 2 files changed, 11 insertions(+), 28 deletions(-)
diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py index 08951a114654..b58fb3733cfa 100755 --- a/tools/testing/kunit/kunit.py +++ b/tools/testing/kunit/kunit.py @@ -256,10 +256,7 @@ def main(argv, linux=None): os.mkdir(cli_args.build_dir)
if not linux:
linux = kunit_kernel.LinuxSourceTree()linux.create_kunitconfig(cli_args.build_dir)linux.read_kunitconfig(cli_args.build_dir)
linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir) request = KunitRequest(cli_args.raw_output, cli_args.timeout,@@ -277,10 +274,7 @@ def main(argv, linux=None): os.mkdir(cli_args.build_dir)
if not linux:
linux = kunit_kernel.LinuxSourceTree()linux.create_kunitconfig(cli_args.build_dir)linux.read_kunitconfig(cli_args.build_dir)
linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir) request = KunitConfigRequest(cli_args.build_dir, cli_args.make_options)@@ -292,10 +286,7 @@ def main(argv, linux=None): sys.exit(1) elif cli_args.subcommand == 'build': if not linux:
linux = kunit_kernel.LinuxSourceTree()linux.create_kunitconfig(cli_args.build_dir)linux.read_kunitconfig(cli_args.build_dir)
linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir) request = KunitBuildRequest(cli_args.jobs, cli_args.build_dir,@@ -309,10 +300,7 @@ def main(argv, linux=None): sys.exit(1) elif cli_args.subcommand == 'exec': if not linux:
linux = kunit_kernel.LinuxSourceTree()linux.create_kunitconfig(cli_args.build_dir)linux.read_kunitconfig(cli_args.build_dir)
linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir) exec_request = KunitExecRequest(cli_args.timeout, cli_args.build_dir,diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py index bda7c4fd4d3e..79793031d2c4 100644 --- a/tools/testing/kunit/kunit_kernel.py +++ b/tools/testing/kunit/kunit_kernel.py @@ -129,10 +129,15 @@ def get_outfile_path(build_dir) -> str: class LinuxSourceTree(object): """Represents a Linux kernel source tree with KUnit tests."""
def __init__(self) -> None:self._ops = LinuxSourceTreeOperations()
def __init__(self, build_dir: str, defconfig=DEFAULT_KUNITCONFIG_PATH) -> None: signal.signal(signal.SIGINT, self.signal_handler)self._ops = LinuxSourceTreeOperations()kunitconfig_path = get_kunitconfig_path(build_dir)self._kconfig = kunit_config.Kconfig()self._kconfig.read_from_file(kunitconfig_path)def clean(self) -> bool: try: self._ops.make_mrproper()@@ -141,16 +146,6 @@ class LinuxSourceTree(object): return False return True
def create_kunitconfig(self, build_dir, defconfig=DEFAULT_KUNITCONFIG_PATH) -> None:kunitconfig_path = get_kunitconfig_path(build_dir)if not os.path.exists(kunitconfig_path):shutil.copyfile(defconfig, kunitconfig_path)
What happened to create_kunitconfig() here? With this patch, I can no longer run .../kunit.py run with an empty build_dir and get results, instead getting: --- Traceback (most recent call last): File "./tools/testing/kunit/kunit.py", line 336, in <module> main(sys.argv[1:]) File "./tools/testing/kunit/kunit.py", line 259, in main linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir) File "./tools/testing/kunit/kunit_kernel.py", line 139, in __init__ self._kconfig.read_from_file(kunitconfig_path) File "./tools/testing/kunit/kunit_config.py", line 89, in read_from_file with open(path, 'r') as f: FileNotFoundError: [Errno 2] No such file or directory: 'asdf/.kunitconfig' ---
Prior to this change, the defconfig is copied over, and the kernel is built, tests run succesfully.
def read_kunitconfig(self, build_dir) -> None:kunitconfig_path = get_kunitconfig_path(build_dir)self._kconfig = kunit_config.Kconfig()self._kconfig.read_from_file(kunitconfig_path)def validate_config(self, build_dir) -> bool: kconfig_path = get_kconfig_path(build_dir) validated_kconfig = kunit_config.Kconfig()-- 2.29.2.576.ga3fc446d84-goog
On Thu, Dec 3, 2020 at 7:57 PM David Gow davidgow@google.com wrote:
On Fri, Dec 4, 2020 at 3:41 AM Daniel Latypov dlatypov@google.com wrote:
LinuxSourceTree will unceremoniously crash if the user doesn't call read_kunitconfig() first in a number of functions.
This patch seems to partly be reverting the changes here, right: https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/co... (That patch moved the reading of kunitconfig out of __init__)
Yes.
My overall concern is that, really, there are some operations that shouldn't need a kunitconfig (even if they do at the moment), so we'd ideally want at least some of the operations currently under LinuxSourceTree to be able to be run without first reading a kunitconfig. Most notably, it'd be nice if kunit.py exec (and hence LinuxSourceTree::run_kernel()) didn't need a kunitconfig, as the kernel ought to already be built at this point.
Now, this is all a little bit hypothetical, as we haven't bothered to make kunit.py exec work without a kunitconfig thus far, but I'm a touch hesitant to make it harder to bypass the kunitconfig reading anyway.
Fair point.
So one alternative to this to make type-checkers happy is to declare _config instead of sneakily setting it in some random later method. Then in all the places that rely on _config, we'd need to add in checks that it's in fact set to give a better error message (so it's clear to the user that it's an internal tool bug and has nothing to do with them).
The copy-paste of create+read_kunitconfig() is annoying, which is why I went with this. How about __init__ takes an optional argument that can disable this parsing?
E.g.
def __init__(kconfig = None): if kconfig is not None: self._config = kconfig else: // create and read
Or if we don't like the idea of requiring users who don't want a kconfig to pass in a dummy,
def __init__(load_kconfig=True): if not load_kconfig: self._config = None ...
Adn currently every place we create an instance, the caller also calls create_kunitconfig() and read_kunitconfig().
Move these instead into __init__() so they can't be forgotten and to reduce copy-paste.
This seems to now be missing the create_kunitconfig() stuff (see below).
Ah good catch. Completely unintentional. I'm sure I had the create_kunitconfig() stuff in __init__() at some point but must have inadvertently removed it somehow later on.
The https://github.com/google/pytype type-checker complained that _config wasn't initialized. With this, kunit_tool now type checks under both pytype and mypy.
Signed-off-by: Daniel Latypov dlatypov@google.com
tools/testing/kunit/kunit.py | 20 ++++---------------- tools/testing/kunit/kunit_kernel.py | 19 +++++++------------ 2 files changed, 11 insertions(+), 28 deletions(-)
diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py index 08951a114654..b58fb3733cfa 100755 --- a/tools/testing/kunit/kunit.py +++ b/tools/testing/kunit/kunit.py @@ -256,10 +256,7 @@ def main(argv, linux=None): os.mkdir(cli_args.build_dir)
if not linux:
linux = kunit_kernel.LinuxSourceTree()linux.create_kunitconfig(cli_args.build_dir)linux.read_kunitconfig(cli_args.build_dir)
linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir) request = KunitRequest(cli_args.raw_output, cli_args.timeout,@@ -277,10 +274,7 @@ def main(argv, linux=None): os.mkdir(cli_args.build_dir)
if not linux:
linux = kunit_kernel.LinuxSourceTree()linux.create_kunitconfig(cli_args.build_dir)linux.read_kunitconfig(cli_args.build_dir)
linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir) request = KunitConfigRequest(cli_args.build_dir, cli_args.make_options)@@ -292,10 +286,7 @@ def main(argv, linux=None): sys.exit(1) elif cli_args.subcommand == 'build': if not linux:
linux = kunit_kernel.LinuxSourceTree()linux.create_kunitconfig(cli_args.build_dir)linux.read_kunitconfig(cli_args.build_dir)
linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir) request = KunitBuildRequest(cli_args.jobs, cli_args.build_dir,@@ -309,10 +300,7 @@ def main(argv, linux=None): sys.exit(1) elif cli_args.subcommand == 'exec': if not linux:
linux = kunit_kernel.LinuxSourceTree()linux.create_kunitconfig(cli_args.build_dir)linux.read_kunitconfig(cli_args.build_dir)
linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir) exec_request = KunitExecRequest(cli_args.timeout, cli_args.build_dir,diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py index bda7c4fd4d3e..79793031d2c4 100644 --- a/tools/testing/kunit/kunit_kernel.py +++ b/tools/testing/kunit/kunit_kernel.py @@ -129,10 +129,15 @@ def get_outfile_path(build_dir) -> str: class LinuxSourceTree(object): """Represents a Linux kernel source tree with KUnit tests."""
def __init__(self) -> None:self._ops = LinuxSourceTreeOperations()
def __init__(self, build_dir: str, defconfig=DEFAULT_KUNITCONFIG_PATH) -> None: signal.signal(signal.SIGINT, self.signal_handler)self._ops = LinuxSourceTreeOperations()kunitconfig_path = get_kunitconfig_path(build_dir)self._kconfig = kunit_config.Kconfig()self._kconfig.read_from_file(kunitconfig_path)def clean(self) -> bool: try: self._ops.make_mrproper()@@ -141,16 +146,6 @@ class LinuxSourceTree(object): return False return True
def create_kunitconfig(self, build_dir, defconfig=DEFAULT_KUNITCONFIG_PATH) -> None:kunitconfig_path = get_kunitconfig_path(build_dir)if not os.path.exists(kunitconfig_path):shutil.copyfile(defconfig, kunitconfig_path)What happened to create_kunitconfig() here? With this patch, I can no longer run .../kunit.py run with an empty build_dir and get results, instead getting:
Traceback (most recent call last): File "./tools/testing/kunit/kunit.py", line 336, in <module> main(sys.argv[1:]) File "./tools/testing/kunit/kunit.py", line 259, in main linux = kunit_kernel.LinuxSourceTree(cli_args.build_dir) File "./tools/testing/kunit/kunit_kernel.py", line 139, in __init__ self._kconfig.read_from_file(kunitconfig_path) File "./tools/testing/kunit/kunit_config.py", line 89, in read_from_file with open(path, 'r') as f: FileNotFoundError: [Errno 2] No such file or directory: 'asdf/.kunitconfig'
Prior to this change, the defconfig is copied over, and the kernel is built, tests run succesfully.
def read_kunitconfig(self, build_dir) -> None:kunitconfig_path = get_kunitconfig_path(build_dir)self._kconfig = kunit_config.Kconfig()self._kconfig.read_from_file(kunitconfig_path)def validate_config(self, build_dir) -> bool: kconfig_path = get_kconfig_path(build_dir) validated_kconfig = kunit_config.Kconfig()-- 2.29.2.576.ga3fc446d84-goog
On Sat, Dec 5, 2020 at 2:18 AM Daniel Latypov dlatypov@google.com wrote:
On Thu, Dec 3, 2020 at 7:57 PM David Gow davidgow@google.com wrote:
On Fri, Dec 4, 2020 at 3:41 AM Daniel Latypov dlatypov@google.com wrote:
LinuxSourceTree will unceremoniously crash if the user doesn't call read_kunitconfig() first in a number of functions.
This patch seems to partly be reverting the changes here, right: https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/co... (That patch moved the reading of kunitconfig out of __init__)
Yes.
My overall concern is that, really, there are some operations that shouldn't need a kunitconfig (even if they do at the moment), so we'd ideally want at least some of the operations currently under LinuxSourceTree to be able to be run without first reading a kunitconfig. Most notably, it'd be nice if kunit.py exec (and hence LinuxSourceTree::run_kernel()) didn't need a kunitconfig, as the kernel ought to already be built at this point.
Now, this is all a little bit hypothetical, as we haven't bothered to make kunit.py exec work without a kunitconfig thus far, but I'm a touch hesitant to make it harder to bypass the kunitconfig reading anyway.
Fair point.
So one alternative to this to make type-checkers happy is to declare _config instead of sneakily setting it in some random later method. Then in all the places that rely on _config, we'd need to add in checks that it's in fact set to give a better error message (so it's clear to the user that it's an internal tool bug and has nothing to do with them).
This seems plausible, if a bit verbose.
The copy-paste of create+read_kunitconfig() is annoying, which is why I went with this.
Personally, the duplication of calls to {create,read}_kunitconfig() doesn't bother me, but I definitely can see the advantage of having the type system pick up when we've missed one.
How about __init__ takes an optional argument that can disable this parsing?
This would be okay: I'm starting to feel that really, the ultimate solution is either to split LinuxSourceTree up (and have separate things for configuring, building, and running the kernel), or to pass the kconfig stuff into just the functions that require it. But that is a much more serious refactor, which I haven't fully thought through, and I don't want to let the perfect be the enemy of the good here.
E.g.
def __init__(kconfig = None): if kconfig is not None: self._config = kconfig else: // create and read
What would the kconfig argument here be? Just an empty Kconfig()? I'm not a huge fan of passing a "None" kconfig object when we want to load a config, and a non-None one when we want an empty one: that seems confusingly backwards. Maybe it'd be possible to move the loading of the kunitconfig outside LinuxSourceTree, and pass that (or an empty one) as needed?
Or if we don't like the idea of requiring users who don't want a kconfig to pass in a dummy,
def __init__(load_kconfig=True): if not load_kconfig: self._config = None ...
I slightly prefer this, for the reasons above: True/False makes more sense than None/Kconfig().
Adn currently every place we create an instance, the caller also calls create_kunitconfig() and read_kunitconfig().
Move these instead into __init__() so they can't be forgotten and to reduce copy-paste.
This seems to now be missing the create_kunitconfig() stuff (see below).
Ah good catch. Completely unintentional. I'm sure I had the create_kunitconfig() stuff in __init__() at some point but must have inadvertently removed it somehow later on.
The https://github.com/google/pytype type-checker complained that _config wasn't initialized. With this, kunit_tool now type checks under both pytype and mypy.
Signed-off-by: Daniel Latypov dlatypov@google.com
Okay, so it looks like there are a few options with _kconfig: 1. Check for None everywhere (after explicitly setting it in the constructor). Pros: Nicer error messages, doesn't require other changes, Cons: verbose, still somewhat prone to error (could forget {create,read}_kunitconfig())
2. Pass a Kconfig object into the constructor. Pros: a kconfig must exist, so less error prone, Cons: if we allow passing None to load it, that's confusing.
3. Pass a bool into the constructor. Pros: similarly less error prone. Cons: True/False is not very descriptive.
4. Refactor and (e.g.) pass the kconfig only into functions which require it. Pros: removes a "false dependency" on a kconfig for things which don't need it. Cons: more work, more verbose.
Of these, I probably like them in this order 4, 2 (without None), 3, 2 (with None), 1. But, to be honest, I'd be happy enough with any of them.
So, if we can get a v2 that fixes the missing create_kunitconfig(), and does any of the above things, that sounds good to me.
Cheers, -- David
On Fri, Dec 4, 2020 at 3:41 AM Daniel Latypov dlatypov@google.com wrote:
The authors of this tool were more familiar with a different type-checker, https://github.com/google/pytype.
That's open source, but mypy seems more prevalent (and runs faster). And unlike pytype, mypy doesn't try to infer types so it doesn't check unanotated functions.
So annotate ~all functions in kunit tool to increase type-checking coverage. Note: per https://www.python.org/dev/peps/pep-0484/, `__init__()` should be annotated as `-> None`.
Doing so makes mypy discover a number of new violations. Exclude main() since we reuse `request` for the different types of requests, which mypy isn't happy about.
This commit fixes all but one error, where `TestSuite.status` might be None.
Signed-off-by: Daniel Latypov dlatypov@google.com
This looks good to me: I gave it some quick testing, and reading through it, all of the changes seem sensible.
I wasn't able to get pytype running here, but mypy worked fine.
Reviewed-by: David Gow davidgow@google.com
-- David
linux-kselftest-mirror@lists.linaro.org
-
Daniel Latypov -
David Gow