[1/3] kunit: tool: readability tweaks in KernelCI json generation logic

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(-)

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
> >