[PATCH] kunit: tool: don't print out test statuses w/ 0s in summary

[PATCH] kunit: tool: don't print out test statuses w/ 0s in summary

[Daniel Latypov]

Before:
> Testing complete. Passed: 137, Failed: 0, Crashed: 0, Skipped: 36, Errors: 0

After:
> Testing complete. Passed: 137, Skipped: 36

Even with our current set of statuses, the output is a bit verbose.
It could get worse in the future if we add more (e.g. timeout, kasan).
Let's only print the relevant ones.

I had previously been sympathetic to the argument that always
printing out all the statuses would make it easier to parse results.
But now we have commit acd8e8407b8f ("kunit: Print test statistics on
failure"), there are test counts printed out in the raw output.
We don't currently print out an overall total across all suites, but it
would be easy to add, if we see a need for that.

Signed-off-by: Daniel Latypov <dlatypov@google.com>
---
 tools/testing/kunit/kunit_parser.py | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/tools/testing/kunit/kunit_parser.py b/tools/testing/kunit/kunit_parser.py
index 807ed2bd6832..957907105429 100644
--- a/tools/testing/kunit/kunit_parser.py
+++ b/tools/testing/kunit/kunit_parser.py
@@ -94,11 +94,10 @@ class TestCounts:
 	def __str__(self) -> str:
 		"""Returns the string representation of a TestCounts object.
 		"""
-		return ('Passed: ' + str(self.passed) +
-			', Failed: ' + str(self.failed) +
-			', Crashed: ' + str(self.crashed) +
-			', Skipped: ' + str(self.skipped) +
-			', Errors: ' + str(self.errors))
+		statuses = [('Passed', self.passed), ('Failed', self.failed),
+			('Crashed', self.crashed), ('Skipped', self.skipped),
+			('Errors', self.errors)]
+		return ', '.join('{}: {}'.format(s, n) for s, n in statuses if n > 0)
 
 	def total(self) -> int:
 		"""Returns the total number of test cases within a test

base-commit: b04d1a8dc7e7ff7ca91a20bef053bcc04265d83a
-- 
2.35.1.1178.g4f1659d476-goog

Re: [PATCH] kunit: tool: don't print out test statuses w/ 0s in summary

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 1095 bytes --]

On Fri, Apr 8, 2022 at 6:30 AM Daniel Latypov <dlatypov@google.com> wrote:
>
> Before:
> > Testing complete. Passed: 137, Failed: 0, Crashed: 0, Skipped: 36, Errors: 0
>
> After:
> > Testing complete. Passed: 137, Skipped: 36
>
> Even with our current set of statuses, the output is a bit verbose.
> It could get worse in the future if we add more (e.g. timeout, kasan).
> Let's only print the relevant ones.
>
> I had previously been sympathetic to the argument that always
> printing out all the statuses would make it easier to parse results.
> But now we have commit acd8e8407b8f ("kunit: Print test statistics on
> failure"), there are test counts printed out in the raw output.
> We don't currently print out an overall total across all suites, but it
> would be easy to add, if we see a need for that.
>
> Signed-off-by: Daniel Latypov <dlatypov@google.com>
> ---

Looks good to me. I agree that we should add a total, too. I was
thinking of adding one anyway, but now there's more space for it, I've
just sent a patch out.

Reviewed-by: David Gow <davidgow@google.com>

Cheers,
-- David

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

[PATCH] kunit: tool: Print a total count of tests.

[David Gow]

Add a count of the total number of tests run (including skipped tests,
which do run a little bit until they decide to skip themselves) to the
summary line.

Signed-off-by: David Gow <davidgow@google.com>
---

This patch depends on:
https://lore.kernel.org/linux-kselftest/20220407223019.2066361-1-dlatypov@google.com/

 tools/testing/kunit/kunit_parser.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/kunit/kunit_parser.py b/tools/testing/kunit/kunit_parser.py
index 957907105429..da01998d29b1 100644
--- a/tools/testing/kunit/kunit_parser.py
+++ b/tools/testing/kunit/kunit_parser.py
@@ -96,7 +96,7 @@ class TestCounts:
 		"""
 		statuses = [('Passed', self.passed), ('Failed', self.failed),
 			('Crashed', self.crashed), ('Skipped', self.skipped),
-			('Errors', self.errors)]
+			('Errors', self.errors), ('Total', self.total())]
 		return ', '.join('{}: {}'.format(s, n) for s, n in statuses if n > 0)
 
 	def total(self) -> int:
-- 
2.35.1.1178.g4f1659d476-goog

Re: [PATCH] kunit: tool: don't print out test statuses w/ 0s in summary

[Daniel Latypov]

On Thu, Apr 7, 2022 at 10:48 PM David Gow <davidgow@google.com> wrote:
>
> On Fri, Apr 8, 2022 at 6:30 AM Daniel Latypov <dlatypov@google.com> wrote:
> >
> > Before:
> > > Testing complete. Passed: 137, Failed: 0, Crashed: 0, Skipped: 36, Errors: 0
> >
> > After:
> > > Testing complete. Passed: 137, Skipped: 36
> >
> > Even with our current set of statuses, the output is a bit verbose.
> > It could get worse in the future if we add more (e.g. timeout, kasan).
> > Let's only print the relevant ones.
> >
> > I had previously been sympathetic to the argument that always
> > printing out all the statuses would make it easier to parse results.
> > But now we have commit acd8e8407b8f ("kunit: Print test statistics on
> > failure"), there are test counts printed out in the raw output.
> > We don't currently print out an overall total across all suites, but it
> > would be easy to add, if we see a need for that.
> >
> > Signed-off-by: Daniel Latypov <dlatypov@google.com>
> > ---
>
> Looks good to me. I agree that we should add a total, too. I was
> thinking of adding one anyway, but now there's more space for it, I've
> just sent a patch out.

I was specifically referring to the test statistics in the kernel output.
We print out the counts per suite, but we don't print out the total count.

But a total in the kunit.py parsed output might be useful as well.

>
> Reviewed-by: David Gow <davidgow@google.com>
>
> Cheers,
> -- David

Re: [PATCH] kunit: tool: Print a total count of tests.

[Daniel Latypov]

On Thu, Apr 7, 2022 at 10:48 PM 'David Gow' via KUnit Development
<kunit-dev@googlegroups.com> wrote:
>
> Add a count of the total number of tests run (including skipped tests,
> which do run a little bit until they decide to skip themselves) to the
> summary line.
>
> Signed-off-by: David Gow <davidgow@google.com>
> ---
>
> This patch depends on:
> https://lore.kernel.org/linux-kselftest/20220407223019.2066361-1-dlatypov@google.com/
>
>  tools/testing/kunit/kunit_parser.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/testing/kunit/kunit_parser.py b/tools/testing/kunit/kunit_parser.py
> index 957907105429..da01998d29b1 100644
> --- a/tools/testing/kunit/kunit_parser.py
> +++ b/tools/testing/kunit/kunit_parser.py
> @@ -96,7 +96,7 @@ class TestCounts:
>                 """
>                 statuses = [('Passed', self.passed), ('Failed', self.failed),
>                         ('Crashed', self.crashed), ('Skipped', self.skipped),
> -                       ('Errors', self.errors)]
> +                       ('Errors', self.errors), ('Total', self.total())]

Hmm, I've never really felt the need for a total to be printed out.
We've had few enough tests and different statuses that the mental
addition is easy enough.

Bikeshedding:
This current output of
  Passed: 40, Skipped: 2, Total: 42
feels a bit awkward to me.
If we did print one out, I think it should probably go first, e.g.
  Ran 42 tests: 40 passed, 2 skipped.

Wdyt?

>                 return ', '.join('{}: {}'.format(s, n) for s, n in statuses if n > 0)

Re: [PATCH] kunit: tool: Print a total count of tests.

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 2886 bytes --]

On Fri, Apr 8, 2022 at 11:59 AM 'Daniel Latypov' via KUnit Development
<kunit-dev@googlegroups.com> wrote:
>
> On Thu, Apr 7, 2022 at 10:48 PM 'David Gow' via KUnit Development
> <kunit-dev@googlegroups.com> wrote:
> >
> > Add a count of the total number of tests run (including skipped tests,
> > which do run a little bit until they decide to skip themselves) to the
> > summary line.
> >
> > Signed-off-by: David Gow <davidgow@google.com>
> > ---
> >
> > This patch depends on:
> > https://lore.kernel.org/linux-kselftest/20220407223019.2066361-1-dlatypov@google.com/
> >
> >  tools/testing/kunit/kunit_parser.py | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/tools/testing/kunit/kunit_parser.py b/tools/testing/kunit/kunit_parser.py
> > index 957907105429..da01998d29b1 100644
> > --- a/tools/testing/kunit/kunit_parser.py
> > +++ b/tools/testing/kunit/kunit_parser.py
> > @@ -96,7 +96,7 @@ class TestCounts:
> >                 """
> >                 statuses = [('Passed', self.passed), ('Failed', self.failed),
> >                         ('Crashed', self.crashed), ('Skipped', self.skipped),
> > -                       ('Errors', self.errors)]
> > +                       ('Errors', self.errors), ('Total', self.total())]
>
> Hmm, I've never really felt the need for a total to be printed out.
> We've had few enough tests and different statuses that the mental
> addition is easy enough.

It's useful just often enough as a sanity check (were those failures /
skipped tests fixed, or did we just stop running them): having one
number to check for "did some more tests run at all" is quite
convenient. Particularly when dealing with nasty dependency chains and
"all tests" builds.

This is also particularly useful when running on setups where
scrollback is more of a pain, as the summary line is absolutely
invaluable there.

>
> Bikeshedding:
> This current output of
>   Passed: 40, Skipped: 2, Total: 42
> feels a bit awkward to me.
> If we did print one out, I think it should probably go first, e.g.
>   Ran 42 tests: 40 passed, 2 skipped.
>
> Wdyt?

I personally don't find having "Total" at the end awkward -- putting
the sum at the end has been done on ledgers for years -- but do admit
it's even more convenient to have it first (so it's at the same place
on the screen every run, regardless of the rest). So "Ran 42 tests: 40
passed..." would emphasise the "total" over the "passed" count here.
Personally, I think that's probably a good thing: I think what most
people really want at a glance is effectively "Failed / Total" (or,
more realistically a sort-of "Problems / Total", where problems is
Failed + Error). But the combination of the colour (did it pass
overall) and the total are the things I'd usually want to look for
first.

So, tl;dr: I'd be all for the "Ran n tests: a passed, b failed, etc" wording.

Cheers,
-- David

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

Re: [PATCH] kunit: tool: Print a total count of tests.

[Daniel Latypov]

)On Thu, Apr 7, 2022 at 11:18 PM David Gow <davidgow@google.com> wrote:
>
> On Fri, Apr 8, 2022 at 11:59 AM 'Daniel Latypov' via KUnit Development
> <kunit-dev@googlegroups.com> wrote:
> >
> > On Thu, Apr 7, 2022 at 10:48 PM 'David Gow' via KUnit Development
> > <kunit-dev@googlegroups.com> wrote:
> > >
> > > Add a count of the total number of tests run (including skipped tests,
> > > which do run a little bit until they decide to skip themselves) to the
> > > summary line.
> > >
> > > Signed-off-by: David Gow <davidgow@google.com>
> > > ---
> > >
> > > This patch depends on:
> > > https://lore.kernel.org/linux-kselftest/20220407223019.2066361-1-dlatypov@google.com/
> > >
> > >  tools/testing/kunit/kunit_parser.py | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/tools/testing/kunit/kunit_parser.py b/tools/testing/kunit/kunit_parser.py
> > > index 957907105429..da01998d29b1 100644
> > > --- a/tools/testing/kunit/kunit_parser.py
> > > +++ b/tools/testing/kunit/kunit_parser.py
> > > @@ -96,7 +96,7 @@ class TestCounts:
> > >                 """
> > >                 statuses = [('Passed', self.passed), ('Failed', self.failed),
> > >                         ('Crashed', self.crashed), ('Skipped', self.skipped),
> > > -                       ('Errors', self.errors)]
> > > +                       ('Errors', self.errors), ('Total', self.total())]
> >
> > Hmm, I've never really felt the need for a total to be printed out.
> > We've had few enough tests and different statuses that the mental
> > addition is easy enough.
>
> It's useful just often enough as a sanity check (were those failures /
> skipped tests fixed, or did we just stop running them): having one
> number to check for "did some more tests run at all" is quite
> convenient. Particularly when dealing with nasty dependency chains and
> "all tests" builds.
>
> This is also particularly useful when running on setups where
> scrollback is more of a pain, as the summary line is absolutely
> invaluable there.

Ack. My point was about the summary line.
We have so few tests that only a handful statuses, that adding up 2-3
small numbers always felt simple enough.
Esp. since the previous patch skips printing out the statues with 0s,
that becomes even easier.

But I'm not against having the total.
I just personally find the current output looks very awkward and would
prefer the status-quo over that specific output format.

>
> >
> > Bikeshedding:
> > This current output of
> >   Passed: 40, Skipped: 2, Total: 42
> > feels a bit awkward to me.
> > If we did print one out, I think it should probably go first, e.g.
> >   Ran 42 tests: 40 passed, 2 skipped.
> >
> > Wdyt?
>
> I personally don't find having "Total" at the end awkward -- putting
> the sum at the end has been done on ledgers for years -- but do admit
> it's even more convenient to have it first (so it's at the same place
> on the screen every run, regardless of the rest). So "Ran 42 tests: 40
> passed..." would emphasise the "total" over the "passed" count here.
> Personally, I think that's probably a good thing: I think what most
> people really want at a glance is effectively "Failed / Total" (or,

I think a reader can already easily note the difference between
  Ran 42 tests: 40 passed, 2 skipped.
and
  Ran 42 tests: 20 passed, 20 failed, 2 skipped.

I.e. the mere existence of a second or third number in the breakdown
after "XX passed" feels like enough of an affordance.
(This is perhaps naively assuming that tests are kept healthy)

> more realistically a sort-of "Problems / Total", where problems is
> Failed + Error). But the combination of the colour (did it pass
> overall) and the total are the things I'd usually want to look for
> first.
>
> So, tl;dr: I'd be all for the "Ran n tests: a passed, b failed, etc" wording.

Ack.
Let's see if Brendan or others on the list have a preference.

If we want to go down that route, it might be easier if I combine this
in the previous patch
(https://lore.kernel.org/linux-kselftest/20220407223019.2066361-1-dlatypov@google.com/

E.g. I get this output
Ran 173 tests: passed: 137, skipped: 36

with a new combined patch of

diff --git a/tools/testing/kunit/kunit_parser.py
b/tools/testing/kunit/kunit_parser.py
index 807ed2bd6832..de1c0b7e14ed 100644
--- a/tools/testing/kunit/kunit_parser.py
+++ b/tools/testing/kunit/kunit_parser.py
@@ -94,11 +94,11 @@ class TestCounts:
        def __str__(self) -> str:
                """Returns the string representation of a TestCounts object.
                """
-               return ('Passed: ' + str(self.passed) +
-                       ', Failed: ' + str(self.failed) +
-                       ', Crashed: ' + str(self.crashed) +
-                       ', Skipped: ' + str(self.skipped) +
-                       ', Errors: ' + str(self.errors))
+               statuses = [('passed', self.passed), ('failed', self.failed),
+                       ('crashed', self.crashed), ('skipped', self.skipped),
+                       ('errors', self.errors)]
+               return f'Ran {self.total()} tests: ' + \
+                       ', '.join(f'{s}: {n}' for s, n in statuses if n > 0)

        def total(self) -> int:
                """Returns the total number of test cases within a test

>
> Cheers,
> -- David

Re: [PATCH] kunit: tool: Print a total count of tests.

[Daniel Latypov]

On Fri, Apr 8, 2022 at 2:04 PM Daniel Latypov <dlatypov@google.com> wrote:
<snip>

> E.g. I get this output
> Ran 173 tests: passed: 137, skipped: 36
>
> with a new combined patch of
>
> diff --git a/tools/testing/kunit/kunit_parser.py
> b/tools/testing/kunit/kunit_parser.py
> index 807ed2bd6832..de1c0b7e14ed 100644
> --- a/tools/testing/kunit/kunit_parser.py
> +++ b/tools/testing/kunit/kunit_parser.py
> @@ -94,11 +94,11 @@ class TestCounts:
>         def __str__(self) -> str:
>                 """Returns the string representation of a TestCounts object.
>                 """
> -               return ('Passed: ' + str(self.passed) +
> -                       ', Failed: ' + str(self.failed) +
> -                       ', Crashed: ' + str(self.crashed) +
> -                       ', Skipped: ' + str(self.skipped) +
> -                       ', Errors: ' + str(self.errors))
> +               statuses = [('passed', self.passed), ('failed', self.failed),
> +                       ('crashed', self.crashed), ('skipped', self.skipped),
> +                       ('errors', self.errors)]
> +               return f'Ran {self.total()} tests: ' + \
> +                       ', '.join(f'{s}: {n}' for s, n in statuses if n > 0)
>
>         def total(self) -> int:
>                 """Returns the total number of test cases within a test
>

Sent this patch out as a v2,
https://lore.kernel.org/linux-kselftest/20220408215105.2332902-1-dlatypov@google.com/