| Message ID | 20220217205227.4098452-1-dlatypov@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 6bd0f52ee8f400a558f1c0f33e1f3fd3ef4922a8 |
| Delegated to: | Brendan Higgins |
| Headers | show |
| Series | [1/3] kunit: tool: readability tweaks in KernelCI json generation logic | expand |
On Fri, Feb 18, 2022 at 4:52 AM Daniel Latypov <dlatypov@google.com> wrote: > > Use a more idiomatic check that a list is non-empty (`if mylist:`) and > sinmplify the function body by dedenting and using a dict to map between Nit: spelling of "simplify". (This is also the first time I've seen "dedenting" as a word, which I thought was a typo for a while, too...) > the kunit TestStatus enum => KernelCI json status string. > > The dict hopefully makes it less likely to have bugs like commit > 9a6bb30a8830 ("kunit: tool: fix --json output for skipped tests"). > > Signed-off-by: Daniel Latypov <dlatypov@google.com> > --- > > Note: this series is based on my earlier set of kunit tool cleanups for > 5.18, https://lore.kernel.org/linux-kselftest/20220118190922.1557074-1-dlatypov@google.com/ > > There's no interesting semantic dependency, just some boring merge > conflicts, specifically with patch #4 there, https://lore.kernel.org/linux-kselftest/20220118190922.1557074-5-dlatypov@google.com/ > > --- Looks good to me. While in general, I think I prefer an extra level of indentation to using "continue", it doesn't worry me either way here. The use of a Dict is definitely an improvement. Reviewed-by: David Gow <davidgow@google.com> -- David > tools/testing/kunit/kunit_json.py | 20 ++++++++++---------- > 1 file changed, 10 insertions(+), 10 deletions(-) > > diff --git a/tools/testing/kunit/kunit_json.py b/tools/testing/kunit/kunit_json.py > index 24d103049bca..14a480d3308a 100644 > --- a/tools/testing/kunit/kunit_json.py > +++ b/tools/testing/kunit/kunit_json.py > @@ -16,24 +16,24 @@ from typing import Any, Dict > > JsonObj = Dict[str, Any] > > +_status_map: Dict[TestStatus, str] = { > + TestStatus.SUCCESS: "PASS", > + TestStatus.SKIPPED: "SKIP", > + TestStatus.TEST_CRASHED: "ERROR", > +} > + > def _get_group_json(test: Test, def_config: str, build_dir: str) -> JsonObj: > sub_groups = [] # List[JsonObj] > test_cases = [] # List[JsonObj] > > for subtest in test.subtests: > - if len(subtest.subtests): > + if subtest.subtests: > sub_group = _get_group_json(subtest, def_config, > build_dir) > sub_groups.append(sub_group) > - else: > - test_case = {"name": subtest.name, "status": "FAIL"} > - if subtest.status == TestStatus.SUCCESS: > - test_case["status"] = "PASS" > - elif subtest.status == TestStatus.SKIPPED: > - test_case["status"] = "SKIP" > - elif subtest.status == TestStatus.TEST_CRASHED: > - test_case["status"] = "ERROR" > - test_cases.append(test_case) > + continue > + status = _status_map.get(subtest.status, "FAIL") > + test_cases.append({"name": subtest.name, "status": status}) > > test_group = { > "name": test.name, > -- > 2.35.1.473.g83b2b277ed-goog >
On Wed, Feb 23, 2022 at 10:26 PM David Gow <davidgow@google.com> wrote: > > On Fri, Feb 18, 2022 at 4:52 AM Daniel Latypov <dlatypov@google.com> wrote: > > > > Use a more idiomatic check that a list is non-empty (`if mylist:`) and > > sinmplify the function body by dedenting and using a dict to map between > > Nit: spelling of "simplify". (This is also the first time I've seen Good catch, fixed. > "dedenting" as a word, which I thought was a typo for a while, too...) "outdent" is the more proper word here. Afaik programmers invented "dedent", e.g. https://docs.python.org/3/library/textwrap.html#textwrap.dedent I found "dedent" more obvious and used it enough since that I've forgotten it's not really a word. > > > the kunit TestStatus enum => KernelCI json status string. > > > > The dict hopefully makes it less likely to have bugs like commit > > 9a6bb30a8830 ("kunit: tool: fix --json output for skipped tests"). > > > > Signed-off-by: Daniel Latypov <dlatypov@google.com> > > --- > > > > Note: this series is based on my earlier set of kunit tool cleanups for > > 5.18, https://lore.kernel.org/linux-kselftest/20220118190922.1557074-1-dlatypov@google.com/ > > > > There's no interesting semantic dependency, just some boring merge > > conflicts, specifically with patch #4 there, https://lore.kernel.org/linux-kselftest/20220118190922.1557074-5-dlatypov@google.com/ > > > > --- > > Looks good to me. While in general, I think I prefer an extra level of > indentation to using "continue", it doesn't worry me either way here. It's not really a concern here, but I'm always wary given python has significant whitespace. I've seen enough instances of moved code where the indentation wasn't properly updated. > The use of a Dict is definitely an improvement. > > Reviewed-by: David Gow <davidgow@google.com> > > > -- David > > > tools/testing/kunit/kunit_json.py | 20 ++++++++++---------- > > 1 file changed, 10 insertions(+), 10 deletions(-) > > > > diff --git a/tools/testing/kunit/kunit_json.py b/tools/testing/kunit/kunit_json.py > > index 24d103049bca..14a480d3308a 100644 > > --- a/tools/testing/kunit/kunit_json.py > > +++ b/tools/testing/kunit/kunit_json.py > > @@ -16,24 +16,24 @@ from typing import Any, Dict > > > > JsonObj = Dict[str, Any] > > > > +_status_map: Dict[TestStatus, str] = { > > + TestStatus.SUCCESS: "PASS", > > + TestStatus.SKIPPED: "SKIP", > > + TestStatus.TEST_CRASHED: "ERROR", > > +} > > + > > def _get_group_json(test: Test, def_config: str, build_dir: str) -> JsonObj: > > sub_groups = [] # List[JsonObj] > > test_cases = [] # List[JsonObj] > > > > for subtest in test.subtests: > > - if len(subtest.subtests): > > + if subtest.subtests: > > sub_group = _get_group_json(subtest, def_config, > > build_dir) > > sub_groups.append(sub_group) > > - else: > > - test_case = {"name": subtest.name, "status": "FAIL"} > > - if subtest.status == TestStatus.SUCCESS: > > - test_case["status"] = "PASS" > > - elif subtest.status == TestStatus.SKIPPED: > > - test_case["status"] = "SKIP" > > - elif subtest.status == TestStatus.TEST_CRASHED: > > - test_case["status"] = "ERROR" > > - test_cases.append(test_case) > > + continue > > + status = _status_map.get(subtest.status, "FAIL") > > + test_cases.append({"name": subtest.name, "status": status}) > > > > test_group = { > > "name": test.name, > > -- > > 2.35.1.473.g83b2b277ed-goog > >
diff --git a/tools/testing/kunit/kunit_json.py b/tools/testing/kunit/kunit_json.py index 24d103049bca..14a480d3308a 100644 --- a/tools/testing/kunit/kunit_json.py +++ b/tools/testing/kunit/kunit_json.py @@ -16,24 +16,24 @@ from typing import Any, Dict JsonObj = Dict[str, Any] +_status_map: Dict[TestStatus, str] = { + TestStatus.SUCCESS: "PASS", + TestStatus.SKIPPED: "SKIP", + TestStatus.TEST_CRASHED: "ERROR", +} + def _get_group_json(test: Test, def_config: str, build_dir: str) -> JsonObj: sub_groups = [] # List[JsonObj] test_cases = [] # List[JsonObj] for subtest in test.subtests: - if len(subtest.subtests): + if subtest.subtests: sub_group = _get_group_json(subtest, def_config, build_dir) sub_groups.append(sub_group) - else: - test_case = {"name": subtest.name, "status": "FAIL"} - if subtest.status == TestStatus.SUCCESS: - test_case["status"] = "PASS" - elif subtest.status == TestStatus.SKIPPED: - test_case["status"] = "SKIP" - elif subtest.status == TestStatus.TEST_CRASHED: - test_case["status"] = "ERROR" - test_cases.append(test_case) + continue + status = _status_map.get(subtest.status, "FAIL") + test_cases.append({"name": subtest.name, "status": status}) test_group = { "name": test.name,
Use a more idiomatic check that a list is non-empty (`if mylist:`) and sinmplify the function body by dedenting and using a dict to map between the kunit TestStatus enum => KernelCI json status string. The dict hopefully makes it less likely to have bugs like commit 9a6bb30a8830 ("kunit: tool: fix --json output for skipped tests"). Signed-off-by: Daniel Latypov <dlatypov@google.com> --- Note: this series is based on my earlier set of kunit tool cleanups for 5.18, https://lore.kernel.org/linux-kselftest/20220118190922.1557074-1-dlatypov@google.com/ There's no interesting semantic dependency, just some boring merge conflicts, specifically with patch #4 there, https://lore.kernel.org/linux-kselftest/20220118190922.1557074-5-dlatypov@google.com/ --- tools/testing/kunit/kunit_json.py | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)