| Message ID | 20201203194127.1813731-2-dlatypov@google.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/3] kunit: tool: surface and address more typing issues | expand |
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
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)
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(-)