| Message ID | 20220118190922.1557074-1-dlatypov@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 95dcbc55fe4ffe262795bea02a42695c85a22dc4 |
| Delegated to: | Brendan Higgins |
| Headers | show |
| Series | [1/5] kunit: tool: drop mostly unused KunitResult.result field | expand |
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 >
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 > >
On Thu, Jan 20, 2022 at 9:19 AM Daniel Latypov <dlatypov@google.com> wrote: > > That being said, I can live with the current solution, but'd ideally > > like a comment or something to make the return value Tuple a bit more > > obvious. > > A comment to explain that Tuple == multiple return values from a func? > Or something else? Friendly ping. Do we want a comment like this? # Note: Python uses tuples internally for multiple return values def foo() -> Tuple[int, int] return 0, 1 I can go ahead and add that and send a v2 out. FYI, if you do this in a REPL >>> a = foo() >>> type(a) <class 'tuple'> The syntax for `a, b = foo()` is just using Python's unpacking feature, i.e. b, c = (1, 2) So it's all just syntactic sugar around tuples. > > Also ah, I thought we had more instances of multiple return in kunit.py. > Looks like the only other is get_source_tree_ops_from_qemu_config(). > isolate_ktap_output() technically shows this off as well, but via yields. > > > > > Thoughts? > > > > > > -- David
On Wed, Jan 26, 2022 at 2:55 PM Daniel Latypov <dlatypov@google.com> wrote: > > On Thu, Jan 20, 2022 at 9:19 AM Daniel Latypov <dlatypov@google.com> wrote: > > > That being said, I can live with the current solution, but'd ideally > > > like a comment or something to make the return value Tuple a bit more > > > obvious. > > > > A comment to explain that Tuple == multiple return values from a func? > > Or something else? > > Friendly ping. > Do we want a comment like this? > > # Note: Python uses tuples internally for multiple return values > def foo() -> Tuple[int, int] > return 0, 1 I don't feel that's necessary. I think the use of tuple return types in Python is fairly common and don't require a comment, but I don't feel strongly about it either way. > I can go ahead and add that and send a v2 out. > > FYI, if you do this in a REPL > >>> a = foo() > >>> type(a) > <class 'tuple'> > > The syntax for `a, b = foo()` is just using Python's unpacking feature, i.e. > b, c = (1, 2) > > So it's all just syntactic sugar around tuples. > > > > > Also ah, I thought we had more instances of multiple return in kunit.py. > > Looks like the only other is get_source_tree_ops_from_qemu_config(). > > isolate_ktap_output() technically shows this off as well, but via yields. > > > > > > > > Thoughts? Personally, I think the change as is.
On 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>
On Thu, Jan 27, 2022 at 3:55 AM 'Daniel Latypov' via KUnit Development <kunit-dev@googlegroups.com> wrote: > > On Thu, Jan 20, 2022 at 9:19 AM Daniel Latypov <dlatypov@google.com> wrote: > > > That being said, I can live with the current solution, but'd ideally > > > like a comment or something to make the return value Tuple a bit more > > > obvious. > > > > A comment to explain that Tuple == multiple return values from a func? > > Or something else? > > Friendly ping. > Do we want a comment like this? > > # Note: Python uses tuples internally for multiple return values > def foo() -> Tuple[int, int] > return 0, 1 > Whoops -- forgot to send my response to this. I was less worried about explaining the concept of multiple return values, and more about naming what the return values were: that the first one is the result information, and the second is the parsed test. That being said, it's reasonably obvious from the types in this case, so I'm okay leaving this as-is, though in general I'm wary of tuples when the order doesn't matter, and a struct-style thing (with named members) fits that better. I'm no Python expert though, so don't let my whinging get too much in the way. -- David
On Wed, Jan 26, 2022 at 6:20 PM David Gow <davidgow@google.com> wrote: > > On Thu, Jan 27, 2022 at 3:55 AM 'Daniel Latypov' via KUnit Development > <kunit-dev@googlegroups.com> wrote: > > > > On Thu, Jan 20, 2022 at 9:19 AM Daniel Latypov <dlatypov@google.com> wrote: > > > > That being said, I can live with the current solution, but'd ideally > > > > like a comment or something to make the return value Tuple a bit more > > > > obvious. > > > > > > A comment to explain that Tuple == multiple return values from a func? > > > Or something else? > > > > Friendly ping. > > Do we want a comment like this? > > > > # Note: Python uses tuples internally for multiple return values > > def foo() -> Tuple[int, int] > > return 0, 1 > > > > Whoops -- forgot to send my response to this. > > I was less worried about explaining the concept of multiple return > values, and more about naming what the return values were: that the > first one is the result information, and the second is the parsed > test. > > That being said, it's reasonably obvious from the types in this case, > so I'm okay leaving this as-is, though in general I'm wary of tuples > when the order doesn't matter, and a struct-style thing (with named > members) fits that better. Ack. Yeah, in this case I don't think creating a new type to name each value is worth it. From what I've seen of python codebases, this info is usually captured in docstrings, but yeah, this particular case seems straightforward enough that it doesn't need it. > > I'm no Python expert though, so don't let my whinging get too much in the way. > > -- David
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:
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(-) base-commit: f079ab01b5609fb0c9acc52c88168bf1eed82373