| Message ID | 20220408034848.2081355-1-davidgow@google.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Brendan Higgins |
| Headers | show |
| Series | kunit: tool: Print a total count of tests. | expand |
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)
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
)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
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/
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:
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(-)