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
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
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
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@go...
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:
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@go...
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@go...
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@go...
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@go...
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@go...
linux-kselftest-mirror@lists.linaro.org
-
Daniel Latypov -
David Gow