* [PATCH] kunit: tool: don't print out test statuses w/ 0s in summary @ 2022-04-07 22:30 Daniel Latypov 2022-04-08 3:48 ` David Gow 2022-04-08 3:48 ` [PATCH] kunit: tool: Print a total count of tests David Gow 0 siblings, 2 replies; 8+ messages in thread From: Daniel Latypov @ 2022-04-07 22:30 UTC (permalink / raw) To: brendanhiggins, davidgow Cc: linux-kernel, kunit-dev, linux-kselftest, skhan, 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] kunit: tool: don't print out test statuses w/ 0s in summary 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 1 sibling, 1 reply; 8+ messages in thread From: David Gow @ 2022-04-08 3:48 UTC (permalink / raw) To: Daniel Latypov Cc: Brendan Higgins, Linux Kernel Mailing List, KUnit Development, open list:KERNEL SELFTEST FRAMEWORK, Shuah Khan [-- 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 --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] kunit: tool: don't print out test statuses w/ 0s in summary 2022-04-08 3:48 ` David Gow @ 2022-04-08 3:54 ` Daniel Latypov 0 siblings, 0 replies; 8+ messages in thread From: Daniel Latypov @ 2022-04-08 3:54 UTC (permalink / raw) To: David Gow Cc: Brendan Higgins, Linux Kernel Mailing List, KUnit Development, open list:KERNEL SELFTEST FRAMEWORK, Shuah Khan 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] kunit: tool: Print a total count of tests. 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:48 ` David Gow 2022-04-08 3:59 ` Daniel Latypov 1 sibling, 1 reply; 8+ messages in thread From: David Gow @ 2022-04-08 3:48 UTC (permalink / raw) To: Daniel Latypov, brendanhiggins, skhan Cc: David Gow, linux-kernel, kunit-dev, linux-kselftest 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] kunit: tool: Print a total count of tests. 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 0 siblings, 1 reply; 8+ messages in thread From: Daniel Latypov @ 2022-04-08 3:59 UTC (permalink / raw) To: David Gow; +Cc: brendanhiggins, skhan, linux-kernel, kunit-dev, linux-kselftest 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) ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] kunit: tool: Print a total count of tests. 2022-04-08 3:59 ` Daniel Latypov @ 2022-04-08 4:18 ` David Gow 2022-04-08 19:04 ` Daniel Latypov 0 siblings, 1 reply; 8+ messages in thread From: David Gow @ 2022-04-08 4:18 UTC (permalink / raw) To: Daniel Latypov Cc: Brendan Higgins, Shuah Khan, Linux Kernel Mailing List, KUnit Development, open list:KERNEL SELFTEST FRAMEWORK [-- 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 --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] kunit: tool: Print a total count of tests. 2022-04-08 4:18 ` David Gow @ 2022-04-08 19:04 ` Daniel Latypov 2022-04-08 21:54 ` Daniel Latypov 0 siblings, 1 reply; 8+ messages in thread From: Daniel Latypov @ 2022-04-08 19:04 UTC (permalink / raw) To: David Gow Cc: Brendan Higgins, Shuah Khan, Linux Kernel Mailing List, KUnit Development, open list:KERNEL SELFTEST FRAMEWORK )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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] kunit: tool: Print a total count of tests. 2022-04-08 19:04 ` Daniel Latypov @ 2022-04-08 21:54 ` Daniel Latypov 0 siblings, 0 replies; 8+ messages in thread From: Daniel Latypov @ 2022-04-08 21:54 UTC (permalink / raw) To: David Gow Cc: Brendan Higgins, Shuah Khan, Linux Kernel Mailing List, KUnit Development, open list:KERNEL SELFTEST FRAMEWORK 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/ ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-04-08 21:54 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 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 2022-04-08 21:54 ` Daniel Latypov
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).