* [PATCH] kunit: tool: misc cleanups
@ 2022-05-09 20:49 Daniel Latypov
2022-05-10 6:22 ` David Gow
2022-05-12 18:37 ` Brendan Higgins
0 siblings, 2 replies; 4+ messages in thread
From: Daniel Latypov @ 2022-05-09 20:49 UTC (permalink / raw)
To: brendanhiggins, davidgow
Cc: linux-kernel, kunit-dev, linux-kselftest, skhan, Daniel Latypov
This primarily comes from running pylint over kunit tool code and
ignoring some warnings we don't care about.
If we ever got a fully clean setup, we could add this to run_checks.py,
but we're not there yet.
Fix things like
* Drop unused imports
* check `is None`, not `== None` (see PEP 8)
* remove redundant parens around returns
* remove redundant `else` / convert `elif` to `if` where appropriate
* rename make_arch_qemuconfig() param to base_kunitconfig (this is the
name used in the subclass, and it's a better one)
* kunit_tool_test: check the exit code for SystemExit (could be 0)
Signed-off-by: Daniel Latypov <dlatypov@google.com>
---
Note: this patch only applies cleanly on top of https://lore.kernel.org/linux-kselftest/20220426173334.3871399-3-dlatypov@google.com
---
tools/testing/kunit/kunit.py | 9 +++----
tools/testing/kunit/kunit_config.py | 12 ++++-----
tools/testing/kunit/kunit_json.py | 5 +---
tools/testing/kunit/kunit_kernel.py | 10 +++----
tools/testing/kunit/kunit_parser.py | 37 ++++++++++++--------------
tools/testing/kunit/kunit_tool_test.py | 10 ++++---
tools/testing/kunit/run_checks.py | 2 +-
7 files changed, 39 insertions(+), 46 deletions(-)
diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py
index 6dc710d3996b..13bd72e47da8 100755
--- a/tools/testing/kunit/kunit.py
+++ b/tools/testing/kunit/kunit.py
@@ -124,7 +124,7 @@ def _list_tests(linux: kunit_kernel.LinuxSourceTree, request: KunitExecRequest)
lines.pop()
# Filter out any extraneous non-test output that might have gotten mixed in.
- return [l for l in lines if re.match('^[^\s.]+\.[^\s.]+$', l)]
+ return [l for l in lines if re.match(r'^[^\s.]+\.[^\s.]+$', l)]
def _suites_from_test_list(tests: List[str]) -> List[str]:
"""Extracts all the suites from an ordered list of tests."""
@@ -188,8 +188,7 @@ def exec_tests(linux: kunit_kernel.LinuxSourceTree, request: KunitExecRequest) -
def _map_to_overall_status(test_status: kunit_parser.TestStatus) -> KunitStatus:
if test_status in (kunit_parser.TestStatus.SUCCESS, kunit_parser.TestStatus.SKIPPED):
return KunitStatus.SUCCESS
- else:
- return KunitStatus.TEST_FAILURE
+ return KunitStatus.TEST_FAILURE
def parse_tests(request: KunitParseRequest, metadata: kunit_json.Metadata, input_data: Iterable[str]) -> Tuple[KunitResult, kunit_parser.Test]:
parse_start = time.time()
@@ -353,7 +352,7 @@ def add_exec_opts(parser) -> None:
'a non-hermetic test, one that might pass/fail based on '
'what ran before it.',
type=str,
- choices=['suite', 'test']),
+ choices=['suite', 'test'])
def add_parse_opts(parser) -> None:
parser.add_argument('--raw_output', help='If set don\'t format output from kernel. '
@@ -497,7 +496,7 @@ def main(argv, linux=None):
if result.status != KunitStatus.SUCCESS:
sys.exit(1)
elif cli_args.subcommand == 'parse':
- if cli_args.file == None:
+ if cli_args.file is None:
sys.stdin.reconfigure(errors='backslashreplace') # pytype: disable=attribute-error
kunit_output = sys.stdin
else:
diff --git a/tools/testing/kunit/kunit_config.py b/tools/testing/kunit/kunit_config.py
index ca33e4b7bcc5..75a8dc1683d4 100644
--- a/tools/testing/kunit/kunit_config.py
+++ b/tools/testing/kunit/kunit_config.py
@@ -20,16 +20,15 @@ class KconfigEntry:
def __str__(self) -> str:
if self.value == 'n':
- return r'# CONFIG_%s is not set' % (self.name)
- else:
- return r'CONFIG_%s=%s' % (self.name, self.value)
+ return f'# CONFIG_{self.name} is not set'
+ return f'CONFIG_{self.name}={self.value}'
class KconfigParseError(Exception):
"""Error parsing Kconfig defconfig or .config."""
-class Kconfig(object):
+class Kconfig:
"""Represents defconfig or .config specified using the Kconfig language."""
def __init__(self) -> None:
@@ -49,7 +48,7 @@ class Kconfig(object):
if a.value == 'n':
continue
return False
- elif a.value != b:
+ if a.value != b:
return False
return True
@@ -91,6 +90,5 @@ def parse_from_string(blob: str) -> Kconfig:
if line[0] == '#':
continue
- else:
- raise KconfigParseError('Failed to parse: ' + line)
+ raise KconfigParseError('Failed to parse: ' + line)
return kconfig
diff --git a/tools/testing/kunit/kunit_json.py b/tools/testing/kunit/kunit_json.py
index 1212423fe6bc..10ff65689dd8 100644
--- a/tools/testing/kunit/kunit_json.py
+++ b/tools/testing/kunit/kunit_json.py
@@ -8,12 +8,9 @@
from dataclasses import dataclass
import json
-import os
-
-import kunit_parser
+from typing import Any, Dict
from kunit_parser import Test, TestStatus
-from typing import Any, Dict
@dataclass
class Metadata:
diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py
index 483f78e15ce9..93446a2b6414 100644
--- a/tools/testing/kunit/kunit_kernel.py
+++ b/tools/testing/kunit/kunit_kernel.py
@@ -37,7 +37,7 @@ class BuildError(Exception):
"""Represents an error trying to build the Linux kernel."""
-class LinuxSourceTreeOperations(object):
+class LinuxSourceTreeOperations:
"""An abstraction over command line operations performed on a source tree."""
def __init__(self, linux_arch: str, cross_compile: Optional[str]):
@@ -52,7 +52,7 @@ class LinuxSourceTreeOperations(object):
except subprocess.CalledProcessError as e:
raise ConfigError(e.output.decode())
- def make_arch_qemuconfig(self, kconfig: kunit_config.Kconfig) -> None:
+ def make_arch_qemuconfig(self, base_kunitconfig: kunit_config.Kconfig) -> None:
pass
def make_allyesconfig(self, build_dir: str, make_options) -> None:
@@ -180,7 +180,7 @@ def get_source_tree_ops(arch: str, cross_compile: Optional[str]) -> LinuxSourceT
config_path = os.path.join(QEMU_CONFIGS_DIR, arch + '.py')
if arch == 'um':
return LinuxSourceTreeOperationsUml(cross_compile=cross_compile)
- elif os.path.isfile(config_path):
+ if os.path.isfile(config_path):
return get_source_tree_ops_from_qemu_config(config_path, cross_compile)[1]
options = [f[:-3] for f in os.listdir(QEMU_CONFIGS_DIR) if f.endswith('.py')]
@@ -211,7 +211,7 @@ def get_source_tree_ops_from_qemu_config(config_path: str,
return params.linux_arch, LinuxSourceTreeOperationsQemu(
params, cross_compile=cross_compile)
-class LinuxSourceTree(object):
+class LinuxSourceTree:
"""Represents a Linux kernel source tree with KUnit tests."""
def __init__(
@@ -366,6 +366,6 @@ class LinuxSourceTree(object):
waiter.join()
subprocess.call(['stty', 'sane'])
- def signal_handler(self, sig, frame) -> None:
+ def signal_handler(self, unused_sig, unused_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 d56d530fab24..5c90842d7017 100644
--- a/tools/testing/kunit/kunit_parser.py
+++ b/tools/testing/kunit/kunit_parser.py
@@ -15,10 +15,9 @@ import sys
import datetime
from enum import Enum, auto
-from functools import reduce
from typing import Iterable, Iterator, List, Optional, Tuple
-class Test(object):
+class Test:
"""
A class to represent a test parsed from KTAP results. All KTAP
results within a test log are stored in a main Test object as
@@ -126,17 +125,16 @@ class TestCounts:
"""
if self.total() == 0:
return TestStatus.NO_TESTS
- elif self.crashed:
+ if self.crashed:
# Crashes should take priority.
return TestStatus.TEST_CRASHED
- elif self.failed:
+ if self.failed:
return TestStatus.FAILURE
- elif self.passed:
+ if self.passed:
# No failures or crashes, looks good!
return TestStatus.SUCCESS
- else:
- # We have only skipped tests.
- return TestStatus.SKIPPED
+ # We have only skipped tests.
+ return TestStatus.SKIPPED
def add_status(self, status: TestStatus) -> None:
"""Increments the count for `status`."""
@@ -381,7 +379,7 @@ def peek_test_name_match(lines: LineStream, test: Test) -> bool:
if not match:
return False
name = match.group(4)
- return (name == test.name)
+ return name == test.name
def parse_test_result(lines: LineStream, test: Test,
expected_num: int) -> bool:
@@ -553,17 +551,16 @@ def format_test_result(test: Test) -> str:
String containing formatted test result
"""
if test.status == TestStatus.SUCCESS:
- return (green('[PASSED] ') + test.name)
- elif test.status == TestStatus.SKIPPED:
- return (yellow('[SKIPPED] ') + test.name)
- elif test.status == TestStatus.NO_TESTS:
- return (yellow('[NO TESTS RUN] ') + test.name)
- elif test.status == TestStatus.TEST_CRASHED:
- print_log(test.log)
- return (red('[CRASHED] ') + test.name)
- else:
+ return green('[PASSED] ') + test.name
+ if test.status == TestStatus.SKIPPED:
+ return yellow('[SKIPPED] ') + test.name
+ if test.status == TestStatus.NO_TESTS:
+ return yellow('[NO TESTS RUN] ') + test.name
+ if test.status == TestStatus.TEST_CRASHED:
print_log(test.log)
- return (red('[FAILED] ') + test.name)
+ return red('[CRASHED] ') + test.name
+ print_log(test.log)
+ return red('[FAILED] ') + test.name
def print_test_result(test: Test) -> None:
"""
@@ -607,7 +604,7 @@ def print_summary_line(test: Test) -> None:
"""
if test.status == TestStatus.SUCCESS:
color = green
- elif test.status == TestStatus.SKIPPED or test.status == TestStatus.NO_TESTS:
+ elif test.status in (TestStatus.SKIPPED, TestStatus.NO_TESTS):
color = yellow
else:
color = red
diff --git a/tools/testing/kunit/kunit_tool_test.py b/tools/testing/kunit/kunit_tool_test.py
index 1200e451c418..d2ab24f736f8 100755
--- a/tools/testing/kunit/kunit_tool_test.py
+++ b/tools/testing/kunit/kunit_tool_test.py
@@ -251,8 +251,8 @@ class KUnitParserTest(unittest.TestCase):
def test_ignores_hyphen(self):
hyphen_log = test_data_path('test_strip_hyphen.log')
- file = open(hyphen_log)
- result = kunit_parser.parse_run_tests(file.readlines())
+ with open(hyphen_log) as file:
+ result = kunit_parser.parse_run_tests(file.readlines())
# A skipped test does not fail the whole suite.
self.assertEqual(
@@ -347,7 +347,7 @@ class LineStreamTest(unittest.TestCase):
called_times = 0
def generator():
nonlocal called_times
- for i in range(1,5):
+ for _ in range(1,5):
called_times += 1
yield called_times, str(called_times)
@@ -553,7 +553,8 @@ class KUnitMainTest(unittest.TestCase):
def test_exec_no_tests(self):
self.linux_source_mock.run_kernel = mock.Mock(return_value=['TAP version 14', '1..0'])
with self.assertRaises(SystemExit) as e:
- kunit.main(['run'], self.linux_source_mock)
+ kunit.main(['run'], self.linux_source_mock)
+ self.assertEqual(e.exception.code, 1)
self.linux_source_mock.run_kernel.assert_called_once_with(
args=None, build_dir='.kunit', filter_glob='', timeout=300)
self.print_mock.assert_any_call(StrContains(' 0 tests run!'))
@@ -588,6 +589,7 @@ class KUnitMainTest(unittest.TestCase):
self.linux_source_mock.run_kernel = mock.Mock(return_value=[])
with self.assertRaises(SystemExit) as e:
kunit.main(['run', '--raw_output=invalid'], self.linux_source_mock)
+ self.assertNotEqual(e.exception.code, 0)
def test_run_raw_output_does_not_take_positional_args(self):
# --raw_output is a string flag, but we don't want it to consume
diff --git a/tools/testing/kunit/run_checks.py b/tools/testing/kunit/run_checks.py
index 13d854afca9d..066e6f938f6d 100755
--- a/tools/testing/kunit/run_checks.py
+++ b/tools/testing/kunit/run_checks.py
@@ -14,7 +14,7 @@ import shutil
import subprocess
import sys
import textwrap
-from typing import Dict, List, Sequence, Tuple
+from typing import Dict, List, Sequence
ABS_TOOL_PATH = os.path.abspath(os.path.dirname(__file__))
TIMEOUT = datetime.timedelta(minutes=5).total_seconds()
--
2.36.0.512.ge40c2bad7a-goog
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] kunit: tool: misc cleanups
2022-05-09 20:49 [PATCH] kunit: tool: misc cleanups Daniel Latypov
@ 2022-05-10 6:22 ` David Gow
2022-05-10 17:27 ` Daniel Latypov
2022-05-12 18:37 ` Brendan Higgins
1 sibling, 1 reply; 4+ messages in thread
From: David Gow @ 2022-05-10 6:22 UTC (permalink / raw)
To: Daniel Latypov
Cc: Brendan Higgins, Linux Kernel Mailing List, KUnit Development,
open list:KERNEL SELFTEST FRAMEWORK, Shuah Khan
On Tue, May 10, 2022 at 4:49 AM Daniel Latypov <dlatypov@google.com> wrote:
>
> This primarily comes from running pylint over kunit tool code and
> ignoring some warnings we don't care about.
> If we ever got a fully clean setup, we could add this to run_checks.py,
> but we're not there yet.
>
> Fix things like
> * Drop unused imports
> * check `is None`, not `== None` (see PEP 8)
> * remove redundant parens around returns
> * remove redundant `else` / convert `elif` to `if` where appropriate
Personally, I find the explicit 'elif' much more readable in most of
these cases, but if we're annoying a linter, I guess we should change
them...
> * rename make_arch_qemuconfig() param to base_kunitconfig (this is the
> name used in the subclass, and it's a better one)
> * kunit_tool_test: check the exit code for SystemExit (could be 0)
>
> Signed-off-by: Daniel Latypov <dlatypov@google.com>
> ---
All of these changes seem correct to me, even if I'm not sure I'd
bother with most of them if they weren't causing pylint to show
errors.
Given that apparently it does, though, I'm okay with it going through.
(I'll just grumble quietly in my corner. :-))
Reviewed-by: David Gow <davidgow@google.com>
-- David
> Note: this patch only applies cleanly on top of https://lore.kernel.org/linux-kselftest/20220426173334.3871399-3-dlatypov@google.com
> ---
> tools/testing/kunit/kunit.py | 9 +++----
> tools/testing/kunit/kunit_config.py | 12 ++++-----
> tools/testing/kunit/kunit_json.py | 5 +---
> tools/testing/kunit/kunit_kernel.py | 10 +++----
> tools/testing/kunit/kunit_parser.py | 37 ++++++++++++--------------
> tools/testing/kunit/kunit_tool_test.py | 10 ++++---
> tools/testing/kunit/run_checks.py | 2 +-
> 7 files changed, 39 insertions(+), 46 deletions(-)
>
> diff --git a/tools/testing/kunit/kunit.py b/tools/testing/kunit/kunit.py
> index 6dc710d3996b..13bd72e47da8 100755
> --- a/tools/testing/kunit/kunit.py
> +++ b/tools/testing/kunit/kunit.py
> @@ -124,7 +124,7 @@ def _list_tests(linux: kunit_kernel.LinuxSourceTree, request: KunitExecRequest)
> lines.pop()
>
> # Filter out any extraneous non-test output that might have gotten mixed in.
> - return [l for l in lines if re.match('^[^\s.]+\.[^\s.]+$', l)]
> + return [l for l in lines if re.match(r'^[^\s.]+\.[^\s.]+$', l)]
>
> def _suites_from_test_list(tests: List[str]) -> List[str]:
> """Extracts all the suites from an ordered list of tests."""
> @@ -188,8 +188,7 @@ def exec_tests(linux: kunit_kernel.LinuxSourceTree, request: KunitExecRequest) -
> def _map_to_overall_status(test_status: kunit_parser.TestStatus) -> KunitStatus:
> if test_status in (kunit_parser.TestStatus.SUCCESS, kunit_parser.TestStatus.SKIPPED):
> return KunitStatus.SUCCESS
> - else:
> - return KunitStatus.TEST_FAILURE
> + return KunitStatus.TEST_FAILURE
>
> def parse_tests(request: KunitParseRequest, metadata: kunit_json.Metadata, input_data: Iterable[str]) -> Tuple[KunitResult, kunit_parser.Test]:
> parse_start = time.time()
> @@ -353,7 +352,7 @@ def add_exec_opts(parser) -> None:
> 'a non-hermetic test, one that might pass/fail based on '
> 'what ran before it.',
> type=str,
> - choices=['suite', 'test']),
> + choices=['suite', 'test'])
>
> def add_parse_opts(parser) -> None:
> parser.add_argument('--raw_output', help='If set don\'t format output from kernel. '
> @@ -497,7 +496,7 @@ def main(argv, linux=None):
> if result.status != KunitStatus.SUCCESS:
> sys.exit(1)
> elif cli_args.subcommand == 'parse':
> - if cli_args.file == None:
> + if cli_args.file is None:
> sys.stdin.reconfigure(errors='backslashreplace') # pytype: disable=attribute-error
> kunit_output = sys.stdin
> else:
> diff --git a/tools/testing/kunit/kunit_config.py b/tools/testing/kunit/kunit_config.py
> index ca33e4b7bcc5..75a8dc1683d4 100644
> --- a/tools/testing/kunit/kunit_config.py
> +++ b/tools/testing/kunit/kunit_config.py
> @@ -20,16 +20,15 @@ class KconfigEntry:
>
> def __str__(self) -> str:
> if self.value == 'n':
> - return r'# CONFIG_%s is not set' % (self.name)
> - else:
> - return r'CONFIG_%s=%s' % (self.name, self.value)
> + return f'# CONFIG_{self.name} is not set'
> + return f'CONFIG_{self.name}={self.value}'
>
>
> class KconfigParseError(Exception):
> """Error parsing Kconfig defconfig or .config."""
>
>
> -class Kconfig(object):
> +class Kconfig:
> """Represents defconfig or .config specified using the Kconfig language."""
>
> def __init__(self) -> None:
> @@ -49,7 +48,7 @@ class Kconfig(object):
> if a.value == 'n':
> continue
> return False
> - elif a.value != b:
> + if a.value != b:
> return False
> return True
>
> @@ -91,6 +90,5 @@ def parse_from_string(blob: str) -> Kconfig:
>
> if line[0] == '#':
> continue
> - else:
> - raise KconfigParseError('Failed to parse: ' + line)
> + raise KconfigParseError('Failed to parse: ' + line)
> return kconfig
> diff --git a/tools/testing/kunit/kunit_json.py b/tools/testing/kunit/kunit_json.py
> index 1212423fe6bc..10ff65689dd8 100644
> --- a/tools/testing/kunit/kunit_json.py
> +++ b/tools/testing/kunit/kunit_json.py
> @@ -8,12 +8,9 @@
>
> from dataclasses import dataclass
> import json
> -import os
> -
> -import kunit_parser
> +from typing import Any, Dict
>
> from kunit_parser import Test, TestStatus
> -from typing import Any, Dict
>
> @dataclass
> class Metadata:
> diff --git a/tools/testing/kunit/kunit_kernel.py b/tools/testing/kunit/kunit_kernel.py
> index 483f78e15ce9..93446a2b6414 100644
> --- a/tools/testing/kunit/kunit_kernel.py
> +++ b/tools/testing/kunit/kunit_kernel.py
> @@ -37,7 +37,7 @@ class BuildError(Exception):
> """Represents an error trying to build the Linux kernel."""
>
>
> -class LinuxSourceTreeOperations(object):
> +class LinuxSourceTreeOperations:
> """An abstraction over command line operations performed on a source tree."""
>
> def __init__(self, linux_arch: str, cross_compile: Optional[str]):
> @@ -52,7 +52,7 @@ class LinuxSourceTreeOperations(object):
> except subprocess.CalledProcessError as e:
> raise ConfigError(e.output.decode())
>
> - def make_arch_qemuconfig(self, kconfig: kunit_config.Kconfig) -> None:
> + def make_arch_qemuconfig(self, base_kunitconfig: kunit_config.Kconfig) -> None:
> pass
>
> def make_allyesconfig(self, build_dir: str, make_options) -> None:
> @@ -180,7 +180,7 @@ def get_source_tree_ops(arch: str, cross_compile: Optional[str]) -> LinuxSourceT
> config_path = os.path.join(QEMU_CONFIGS_DIR, arch + '.py')
> if arch == 'um':
> return LinuxSourceTreeOperationsUml(cross_compile=cross_compile)
> - elif os.path.isfile(config_path):
> + if os.path.isfile(config_path):
> return get_source_tree_ops_from_qemu_config(config_path, cross_compile)[1]
>
> options = [f[:-3] for f in os.listdir(QEMU_CONFIGS_DIR) if f.endswith('.py')]
> @@ -211,7 +211,7 @@ def get_source_tree_ops_from_qemu_config(config_path: str,
> return params.linux_arch, LinuxSourceTreeOperationsQemu(
> params, cross_compile=cross_compile)
>
> -class LinuxSourceTree(object):
> +class LinuxSourceTree:
> """Represents a Linux kernel source tree with KUnit tests."""
>
> def __init__(
> @@ -366,6 +366,6 @@ class LinuxSourceTree(object):
> waiter.join()
> subprocess.call(['stty', 'sane'])
>
> - def signal_handler(self, sig, frame) -> None:
> + def signal_handler(self, unused_sig, unused_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 d56d530fab24..5c90842d7017 100644
> --- a/tools/testing/kunit/kunit_parser.py
> +++ b/tools/testing/kunit/kunit_parser.py
> @@ -15,10 +15,9 @@ import sys
>
> import datetime
> from enum import Enum, auto
> -from functools import reduce
> from typing import Iterable, Iterator, List, Optional, Tuple
>
> -class Test(object):
> +class Test:
> """
> A class to represent a test parsed from KTAP results. All KTAP
> results within a test log are stored in a main Test object as
> @@ -126,17 +125,16 @@ class TestCounts:
> """
> if self.total() == 0:
> return TestStatus.NO_TESTS
> - elif self.crashed:
> + if self.crashed:
> # Crashes should take priority.
> return TestStatus.TEST_CRASHED
> - elif self.failed:
> + if self.failed:
> return TestStatus.FAILURE
> - elif self.passed:
> + if self.passed:
> # No failures or crashes, looks good!
> return TestStatus.SUCCESS
> - else:
> - # We have only skipped tests.
> - return TestStatus.SKIPPED
> + # We have only skipped tests.
> + return TestStatus.SKIPPED
>
> def add_status(self, status: TestStatus) -> None:
> """Increments the count for `status`."""
> @@ -381,7 +379,7 @@ def peek_test_name_match(lines: LineStream, test: Test) -> bool:
> if not match:
> return False
> name = match.group(4)
> - return (name == test.name)
> + return name == test.name
>
> def parse_test_result(lines: LineStream, test: Test,
> expected_num: int) -> bool:
> @@ -553,17 +551,16 @@ def format_test_result(test: Test) -> str:
> String containing formatted test result
> """
> if test.status == TestStatus.SUCCESS:
> - return (green('[PASSED] ') + test.name)
> - elif test.status == TestStatus.SKIPPED:
> - return (yellow('[SKIPPED] ') + test.name)
> - elif test.status == TestStatus.NO_TESTS:
> - return (yellow('[NO TESTS RUN] ') + test.name)
> - elif test.status == TestStatus.TEST_CRASHED:
> - print_log(test.log)
> - return (red('[CRASHED] ') + test.name)
> - else:
> + return green('[PASSED] ') + test.name
> + if test.status == TestStatus.SKIPPED:
> + return yellow('[SKIPPED] ') + test.name
> + if test.status == TestStatus.NO_TESTS:
> + return yellow('[NO TESTS RUN] ') + test.name
> + if test.status == TestStatus.TEST_CRASHED:
> print_log(test.log)
> - return (red('[FAILED] ') + test.name)
> + return red('[CRASHED] ') + test.name
> + print_log(test.log)
> + return red('[FAILED] ') + test.name
>
> def print_test_result(test: Test) -> None:
> """
> @@ -607,7 +604,7 @@ def print_summary_line(test: Test) -> None:
> """
> if test.status == TestStatus.SUCCESS:
> color = green
> - elif test.status == TestStatus.SKIPPED or test.status == TestStatus.NO_TESTS:
> + elif test.status in (TestStatus.SKIPPED, TestStatus.NO_TESTS):
> color = yellow
> else:
> color = red
> diff --git a/tools/testing/kunit/kunit_tool_test.py b/tools/testing/kunit/kunit_tool_test.py
> index 1200e451c418..d2ab24f736f8 100755
> --- a/tools/testing/kunit/kunit_tool_test.py
> +++ b/tools/testing/kunit/kunit_tool_test.py
> @@ -251,8 +251,8 @@ class KUnitParserTest(unittest.TestCase):
>
> def test_ignores_hyphen(self):
> hyphen_log = test_data_path('test_strip_hyphen.log')
> - file = open(hyphen_log)
> - result = kunit_parser.parse_run_tests(file.readlines())
> + with open(hyphen_log) as file:
> + result = kunit_parser.parse_run_tests(file.readlines())
>
> # A skipped test does not fail the whole suite.
> self.assertEqual(
> @@ -347,7 +347,7 @@ class LineStreamTest(unittest.TestCase):
> called_times = 0
> def generator():
> nonlocal called_times
> - for i in range(1,5):
> + for _ in range(1,5):
> called_times += 1
> yield called_times, str(called_times)
>
> @@ -553,7 +553,8 @@ class KUnitMainTest(unittest.TestCase):
> def test_exec_no_tests(self):
> self.linux_source_mock.run_kernel = mock.Mock(return_value=['TAP version 14', '1..0'])
> with self.assertRaises(SystemExit) as e:
> - kunit.main(['run'], self.linux_source_mock)
> + kunit.main(['run'], self.linux_source_mock)
> + self.assertEqual(e.exception.code, 1)
> self.linux_source_mock.run_kernel.assert_called_once_with(
> args=None, build_dir='.kunit', filter_glob='', timeout=300)
> self.print_mock.assert_any_call(StrContains(' 0 tests run!'))
> @@ -588,6 +589,7 @@ class KUnitMainTest(unittest.TestCase):
> self.linux_source_mock.run_kernel = mock.Mock(return_value=[])
> with self.assertRaises(SystemExit) as e:
> kunit.main(['run', '--raw_output=invalid'], self.linux_source_mock)
> + self.assertNotEqual(e.exception.code, 0)
>
> def test_run_raw_output_does_not_take_positional_args(self):
> # --raw_output is a string flag, but we don't want it to consume
> diff --git a/tools/testing/kunit/run_checks.py b/tools/testing/kunit/run_checks.py
> index 13d854afca9d..066e6f938f6d 100755
> --- a/tools/testing/kunit/run_checks.py
> +++ b/tools/testing/kunit/run_checks.py
> @@ -14,7 +14,7 @@ import shutil
> import subprocess
> import sys
> import textwrap
> -from typing import Dict, List, Sequence, Tuple
> +from typing import Dict, List, Sequence
>
> ABS_TOOL_PATH = os.path.abspath(os.path.dirname(__file__))
> TIMEOUT = datetime.timedelta(minutes=5).total_seconds()
> --
> 2.36.0.512.ge40c2bad7a-goog
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] kunit: tool: misc cleanups
2022-05-10 6:22 ` David Gow
@ 2022-05-10 17:27 ` Daniel Latypov
0 siblings, 0 replies; 4+ messages in thread
From: Daniel Latypov @ 2022-05-10 17:27 UTC (permalink / raw)
To: David Gow
Cc: Brendan Higgins, Linux Kernel Mailing List, KUnit Development,
open list:KERNEL SELFTEST FRAMEWORK, Shuah Khan
On Mon, May 9, 2022 at 11:22 PM David Gow <davidgow@google.com> wrote:
>
> On Tue, May 10, 2022 at 4:49 AM Daniel Latypov <dlatypov@google.com> wrote:
> >
> > This primarily comes from running pylint over kunit tool code and
> > ignoring some warnings we don't care about.
> > If we ever got a fully clean setup, we could add this to run_checks.py,
> > but we're not there yet.
> >
> > Fix things like
> > * Drop unused imports
> > * check `is None`, not `== None` (see PEP 8)
> > * remove redundant parens around returns
> > * remove redundant `else` / convert `elif` to `if` where appropriate
>
> Personally, I find the explicit 'elif' much more readable in most of
> these cases, but if we're annoying a linter, I guess we should change
> them...
Same, some of them felt a tad more readable, but using `if` is a bit
more explicit about the actual control flow.
For short branches, like most of the ones here, they don't make too
much of a difference, but for longer blocks of code, this can help.
E.g. if one sees
elif check2():
do_thing2()
one might think they can tack on a
do_cleanup()
and have it run for all branches, if they're not careful.
>
> > * rename make_arch_qemuconfig() param to base_kunitconfig (this is the
> > name used in the subclass, and it's a better one)
> > * kunit_tool_test: check the exit code for SystemExit (could be 0)
> >
> > Signed-off-by: Daniel Latypov <dlatypov@google.com>
> > ---
>
> All of these changes seem correct to me, even if I'm not sure I'd
> bother with most of them if they weren't causing pylint to show
> errors.
>
> Given that apparently it does, though, I'm okay with it going through.
> (I'll just grumble quietly in my corner. :-))
To be fair, we could ignore more of these warnings.
If we ever try to get the code clean wrt linter warnings, we'd already
need to tweak the settings.
But I think I've taken care of most of the reasonable warnings in this patch.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] kunit: tool: misc cleanups
2022-05-09 20:49 [PATCH] kunit: tool: misc cleanups Daniel Latypov
2022-05-10 6:22 ` David Gow
@ 2022-05-12 18:37 ` Brendan Higgins
1 sibling, 0 replies; 4+ messages in thread
From: Brendan Higgins @ 2022-05-12 18:37 UTC (permalink / raw)
To: Daniel Latypov; +Cc: davidgow, linux-kernel, kunit-dev, linux-kselftest, skhan
On Mon, May 9, 2022 at 4:49 PM Daniel Latypov <dlatypov@google.com> wrote:
>
> This primarily comes from running pylint over kunit tool code and
> ignoring some warnings we don't care about.
> If we ever got a fully clean setup, we could add this to run_checks.py,
> but we're not there yet.
>
> Fix things like
> * Drop unused imports
> * check `is None`, not `== None` (see PEP 8)
> * remove redundant parens around returns
> * remove redundant `else` / convert `elif` to `if` where appropriate
> * rename make_arch_qemuconfig() param to base_kunitconfig (this is the
> name used in the subclass, and it's a better one)
> * kunit_tool_test: check the exit code for SystemExit (could be 0)
>
> Signed-off-by: Daniel Latypov <dlatypov@google.com>
Reviewed-by: Brendan Higgins <brendanhiggins@google.com>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-05-12 18:37 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-09 20:49 [PATCH] kunit: tool: misc cleanups Daniel Latypov
2022-05-10 6:22 ` David Gow
2022-05-10 17:27 ` Daniel Latypov
2022-05-12 18:37 ` Brendan Higgins
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).