Message-ID: <CAGS_qxqmbgy6KOp1bPF4Pk5SZT+r8L7ASqEyNWF9BtCS2JgXaw@mail.gmail.com>
Date:   Fri, 8 Apr 2022 14:04:06 -0500
From:   Daniel Latypov <dlatypov@...gle.com>
To:     David Gow <davidgow@...gle.com>
Cc:     Brendan Higgins <brendanhiggins@...gle.com>,
        Shuah Khan <skhan@...uxfoundation.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        KUnit Development <kunit-dev@...glegroups.com>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@...r.kernel.org>
Subject: Re: [PATCH] kunit: tool: Print a total count of tests.

)On Thu, Apr 7, 2022 at 11:18 PM David Gow <davidgow@...gle.com> wrote:
>
> On Fri, Apr 8, 2022 at 11:59 AM 'Daniel Latypov' via KUnit Development
> <kunit-dev@...glegroups.com> wrote:
> >
> > On Thu, Apr 7, 2022 at 10:48 PM 'David Gow' via KUnit Development
> > <kunit-dev@...glegroups.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@...gle.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

Powered by blists - more mailing lists