From: Daniel Latypov <dlatypov@google.com>
To: David Gow <davidgow@google.com>
Cc: Brendan Higgins <brendanhiggins@google.com>,
Shuah Khan <skhan@linuxfoundation.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
KUnit Development <kunit-dev@googlegroups.com>,
"open list:KERNEL SELFTEST FRAMEWORK"
<linux-kselftest@vger.kernel.org>
Subject: Re: [PATCH] kunit: tool: Print a total count of tests.
Date: Fri, 8 Apr 2022 14:04:06 -0500 [thread overview]
Message-ID: <CAGS_qxqmbgy6KOp1bPF4Pk5SZT+r8L7ASqEyNWF9BtCS2JgXaw@mail.gmail.com> (raw)
In-Reply-To: <CABVgOSkKuHCkfpQhzNmzufaVw_6qO-Ve_t5oTub1JeweTiC9RQ@mail.gmail.com>
)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
next prev parent reply other threads:[~2022-04-08 19:04 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-04-07 22:30 [PATCH] kunit: tool: don't print out test statuses w/ 0s in summary Daniel Latypov
2022-04-08 3:48 ` David Gow
2022-04-08 3:54 ` Daniel Latypov
2022-04-08 3:48 ` [PATCH] kunit: tool: Print a total count of tests David Gow
2022-04-08 3:59 ` Daniel Latypov
2022-04-08 4:18 ` David Gow
2022-04-08 19:04 ` Daniel Latypov [this message]
2022-04-08 21:54 ` Daniel Latypov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAGS_qxqmbgy6KOp1bPF4Pk5SZT+r8L7ASqEyNWF9BtCS2JgXaw@mail.gmail.com \
--to=dlatypov@google.com \
--cc=brendanhiggins@google.com \
--cc=davidgow@google.com \
--cc=kunit-dev@googlegroups.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-kselftest@vger.kernel.org \
--cc=skhan@linuxfoundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).