[PATCH v1 0/5] selftests/nolibc: report: print test status

[PATCH v1 0/5] selftests/nolibc: report: print test status

[Zhangjin Wu]

Hi, Willy

As you suggested, the 'status: [success|warning|failure]' info is added
to the summary line, with additional newlines around this line to
extrude the status info. at the same time, the total tests is printed,
the passed, skipped and failed values are aligned with '%03d'.

This patchset is based on 20230705-nolibc-series2 of nolibc repo[1].

The test result looks like:

    ...

    138 test(s): 135 passed, 002 skipped, 001 failed => status: failure

    See all results in /labs/linux-lab/src/linux-stable/tools/testing/selftests/nolibc/run.out

Or:

    ...

    137 test(s): 134 passed, 003 skipped, 000 failed => status: warning

    See all results in /labs/linux-lab/src/linux-stable/tools/testing/selftests/nolibc/run.out

Best regards,
Zhangjin
---
[1]: https://git.kernel.org/pub/scm/linux/kernel/git/wtarreau/nolibc.git

Zhangjin Wu (5):
  selftests/nolibc: report: print a summarized test status
  selftests/nolibc: report: print total tests
  selftests/nolibc: report: align passed, skipped and failed
  selftests/nolibc: report: extrude the test status line
  selftests/nolibc: report: add newline before test failures

 tools/testing/selftests/nolibc/Makefile | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

-- 
2.25.1

[PATCH v1 1/5] selftests/nolibc: report: print a summarized test status

[Zhangjin Wu]

one of the test status: success, warning and failure is printed to
summarize the passed, skipped and failed values.

- "success" means no skipped and no failed.
- "warning" means has at least one skipped and no failed.
- "failure" means all tests are failed.

Suggested-by: Willy Tarreau <w@1wt.eu>
Link: https://lore.kernel.org/lkml/20230702164358.GB16233@1wt.eu/
Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
---
 tools/testing/selftests/nolibc/Makefile | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile
index d408b688b291..84b9a46ad678 100644
--- a/tools/testing/selftests/nolibc/Makefile
+++ b/tools/testing/selftests/nolibc/Makefile
@@ -85,7 +85,8 @@ CFLAGS  ?= -Os -fno-ident -fno-asynchronous-unwind-tables -std=c89 \
 LDFLAGS := -s
 
 REPORT  ?= awk '/\[OK\][\r]*$$/{p++} /\[FAIL\][\r]*$$/{f++;print} /\[SKIPPED\][\r]*$$/{s++} \
-		END{ printf("%d test(s) passed, %d skipped, %d failed.\n", p, s, f); \
+		END{ printf("%d test(s) passed, %d skipped, %d failed => status: ", p, s, f); \
+		if (f) printf("failure\n"); else if (s) printf("warning\n"); else printf("success\n");; \
 		printf("See all results in %s\n", ARGV[1]); }'
 
 help:
-- 
2.25.1

[PATCH v1 2/5] selftests/nolibc: report: print total tests

[Zhangjin Wu]

Let's count and print the total number of tests, now, the data of
passed, skipped and failed have the same format.

Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
---
 tools/testing/selftests/nolibc/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile
index 84b9a46ad678..a02be8b0a569 100644
--- a/tools/testing/selftests/nolibc/Makefile
+++ b/tools/testing/selftests/nolibc/Makefile
@@ -85,7 +85,7 @@ CFLAGS  ?= -Os -fno-ident -fno-asynchronous-unwind-tables -std=c89 \
 LDFLAGS := -s
 
 REPORT  ?= awk '/\[OK\][\r]*$$/{p++} /\[FAIL\][\r]*$$/{f++;print} /\[SKIPPED\][\r]*$$/{s++} \
-		END{ printf("%d test(s) passed, %d skipped, %d failed => status: ", p, s, f); \
+		END{ printf("%d test(s): %d passed, %d skipped, %d failed => status: ", p+s+f, p, s, f); \
 		if (f) printf("failure\n"); else if (s) printf("warning\n"); else printf("success\n");; \
 		printf("See all results in %s\n", ARGV[1]); }'
 
-- 
2.25.1

[PATCH v1 3/5] selftests/nolibc: report: align passed, skipped and failed

[Zhangjin Wu]

align the test values for different runs and different architectures.

Since the total number of tests is not bigger than 1000 currently, let's
align them with "%03d".

Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
---
 tools/testing/selftests/nolibc/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile
index a02be8b0a569..2c53bf41967b 100644
--- a/tools/testing/selftests/nolibc/Makefile
+++ b/tools/testing/selftests/nolibc/Makefile
@@ -85,7 +85,7 @@ CFLAGS  ?= -Os -fno-ident -fno-asynchronous-unwind-tables -std=c89 \
 LDFLAGS := -s
 
 REPORT  ?= awk '/\[OK\][\r]*$$/{p++} /\[FAIL\][\r]*$$/{f++;print} /\[SKIPPED\][\r]*$$/{s++} \
-		END{ printf("%d test(s): %d passed, %d skipped, %d failed => status: ", p+s+f, p, s, f); \
+		END{ printf("%03d test(s): %03d passed, %03d skipped, %03d failed => status: ", p+s+f, p, s, f); \
 		if (f) printf("failure\n"); else if (s) printf("warning\n"); else printf("success\n");; \
 		printf("See all results in %s\n", ARGV[1]); }'
 
-- 
2.25.1

Re: [PATCH v1 3/5] selftests/nolibc: report: align passed, skipped and failed

[Willy Tarreau]

On Thu, Jul 06, 2023 at 05:10:08PM +0800, Zhangjin Wu wrote:
> align the test values for different runs and different architectures.
> 
> Since the total number of tests is not bigger than 1000 currently, let's
> align them with "%03d".

%03d is not great for those who want to use them in scripts because it will
prepend zeroes. Better use %3d. Look for example:

  $ x=$(printf "%03d\n" 19)
  $ echo $x
  019
  $ echo $((x+1))
  -bash: 019: value too great for base (error token is "019")

Instead:

  $ printf "%3d\n" 19
   19
  $ x=$(printf "%3d\n" 19)
  $ echo $x
  19
  $ echo $((x+1))
  20

If you're fine with it I'll change your patch and commit message
accordingly.

Willy

Re: [PATCH v1 3/5] selftests/nolibc: report: align passed, skipped and failed

[Zhangjin Wu]

Hi, Willy

> On Thu, Jul 06, 2023 at 05:10:08PM +0800, Zhangjin Wu wrote:
> > align the test values for different runs and different architectures.
> > 
> > Since the total number of tests is not bigger than 1000 currently, let's
> > align them with "%03d".
> 
> %03d is not great for those who want to use them in scripts because it will
> prepend zeroes. Better use %3d. Look for example:
> 
>   $ x=$(printf "%03d\n" 19)
>   $ echo $x
>   019
>   $ echo $((x+1))
>   -bash: 019: value too great for base (error token is "019")
>

I have tried both '%03d' and '%3d' locally, but used '%03d' at last.

    190 passed, 021 skipped, 001 failed
    190 passed,  21 skipped,   1 failed

Beside the calculate issue you pointed out, the 0 prefix although align
all of them with 'numbers' but also bring us some noises, filling the
left parts as whitespaces really looks better.

> Instead:
> 
>   $ printf "%3d\n" 19
>    19
>   $ x=$(printf "%3d\n" 19)
>   $ echo $x
>   19
>   $ echo $((x+1))
>   20
> 
> If you're fine with it I'll change your patch and commit message
> accordingly.

Ok, let's use '%3d' instead of '%03d'.

Thanks,
Zhangjin

> 
> Willy

[PATCH v1 4/5] selftests/nolibc: report: extrude the test status line

[Zhangjin Wu]

two newlines are added around the test summary line to extrude the test
status.

Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
---
 tools/testing/selftests/nolibc/Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile
index 2c53bf41967b..10e9e5c1bdd0 100644
--- a/tools/testing/selftests/nolibc/Makefile
+++ b/tools/testing/selftests/nolibc/Makefile
@@ -85,9 +85,9 @@ CFLAGS  ?= -Os -fno-ident -fno-asynchronous-unwind-tables -std=c89 \
 LDFLAGS := -s
 
 REPORT  ?= awk '/\[OK\][\r]*$$/{p++} /\[FAIL\][\r]*$$/{f++;print} /\[SKIPPED\][\r]*$$/{s++} \
-		END{ printf("%03d test(s): %03d passed, %03d skipped, %03d failed => status: ", p+s+f, p, s, f); \
+		END{ printf("\n%03d test(s): %03d passed, %03d skipped, %03d failed => status: ", p+s+f, p, s, f); \
 		if (f) printf("failure\n"); else if (s) printf("warning\n"); else printf("success\n");; \
-		printf("See all results in %s\n", ARGV[1]); }'
+		printf("\nSee all results in %s\n", ARGV[1]); }'
 
 help:
 	@echo "Supported targets under selftests/nolibc:"
-- 
2.25.1

Re: [PATCH v1 4/5] selftests/nolibc: report: extrude the test status line

[Willy Tarreau]

On Thu, Jul 06, 2023 at 05:11:17PM +0800, Zhangjin Wu wrote:
> two newlines are added around the test summary line to extrude the test
> status.

But then we're back to making it annoying to check, having to figure
if we need to grep -A or grep -B etc. With grep 'status:' we would get
a synthetic status and the counters together. Why do you think it's
not convenient ? Or am I the only one considering it useful to just
run grep "status:" on all output files and figure a global status at
once ?

Willy

Re: [PATCH v1 4/5] selftests/nolibc: report: extrude the test status line

[Zhangjin Wu]

Hi, Willy

> On Thu, Jul 06, 2023 at 05:11:17PM +0800, Zhangjin Wu wrote:
> > two newlines are added around the test summary line to extrude the test
> > status.
> 
> But then we're back to making it annoying to check, having to figure
> if we need to grep -A or grep -B etc. With grep 'status:' we would get
> a synthetic status and the counters together. Why do you think it's
> not convenient ? Or am I the only one considering it useful to just
> run grep "status:" on all output files and figure a global status at
> once ?

Sorry, Willy, my commit message may mislead you a little.

The newlines are added around the whole test summary line (with the
status info), not only around the 'status info' ;-)

An example is added in our cover-letter (use '%3d' instead of '%03d'
here):

    ...
                                                 <-- newline here -->
    138 test(s): 135 passed,   2 skipped,   1 failed => status: failure
                                                 <-- newline here -->
    See all results in /labs/linux-lab/src/linux-stable/tools/testing/selftests/nolibc/run.out

Or:

    ...
                                                 <-- newline here -->
    137 test(s): 134 passed,   3 skipped,   0 failed => status: warning
                                                 <-- newline here -->
    See all results in /labs/linux-lab/src/linux-stable/tools/testing/selftests/nolibc/run.out

It is not for status grep, it is for developers to easily see the whole
summary line at a glance (I should add this in the commit message),
especially when we run tests for lots of architectures one by one
automatically, during the tests running, these newlines may help us to
see the status at a glance.

And further, if not consider pure-text, the colors may be more helpful,
for example, red for failed/failure, yellow for skipped/warning, green
for passed/success, for example:

    $ echo | awk 'END{printf("138 test(s): \033[32m135\033[0m passed, \033[33m  2\033[0m skipped, \033[31m  1\033[0m failed => status: \033[31mfailure\033[0m\n");}'
    138 test(s): 135 passed,   2 skipped,   1 failed => status: failure

But as we can see, the color control code is not readable and it may
break the simple "status: failure" grep, we should use something like
"status: .*failure" ;-)

It is possible to filter out the color info in the last run.out and only
reserve the colors info in the console.

    $ cat run.tmp.out | sed 's/\x1b\[[0-9;]*m//g' | col -bp > run.out

As a summary, the "status info grep" you proposed is very helpful to
summarize all of the tests after the testing finish, I do like it:

    $ grep "status: " /path/to/all/run.out
    138 test(s): 135 passed,   2 skipped,   1 failed => status: failure
    137 test(s): 134 passed,   3 skipped,   0 failed => status: warning

And these newlines (and even further with colors) are added to help
developers to see what happens during the tests running at a glance.

Thanks,
Zhangjin

> 
> Willy

Re: [PATCH v1 4/5] selftests/nolibc: report: extrude the test status line

[Willy Tarreau]

Hi Zhangjin,

On Mon, Jul 10, 2023 at 03:26:52AM +0800, Zhangjin Wu wrote:
> > On Thu, Jul 06, 2023 at 05:11:17PM +0800, Zhangjin Wu wrote:
> > > two newlines are added around the test summary line to extrude the test
> > > status.
> > 
> > But then we're back to making it annoying to check, having to figure
> > if we need to grep -A or grep -B etc. With grep 'status:' we would get
> > a synthetic status and the counters together. Why do you think it's
> > not convenient ? Or am I the only one considering it useful to just
> > run grep "status:" on all output files and figure a global status at
> > once ?
> 
> Sorry, Willy, my commit message may mislead you a little.
> 
> The newlines are added around the whole test summary line (with the
> status info), not only around the 'status info' ;-)

Ah OK, thanks for clarifying this!

> It is not for status grep, it is for developers to easily see the whole
> summary line at a glance

I understand but both work hand-in-hand, as every time you'll perform
a slight change, you'll necessarily rerun the whole series on all archs
to confirm, which is why I'm particularly annoying about the ability to
grep!

> And further, if not consider pure-text, the colors may be more helpful,
> for example, red for failed/failure, yellow for skipped/warning, green
> for passed/success, for example:
> 
>     $ echo | awk 'END{printf("138 test(s): \033[32m135\033[0m passed, \033[33m  2\033[0m skipped, \033[31m  1\033[0m failed => status: \033[31mfailure\033[0m\n");}'
>     138 test(s): 135 passed,   2 skipped,   1 failed => status: failure
> 
> But as we can see, the color control code is not readable and it may
> break the simple "status: failure" grep, we should use something like
> "status: .*failure" ;-)

Colors may only be used when stdout is a terminal, and still, some might
find it annonying (for example some distros use unreadably dark colors
that were apparently never tested over a black background, forcing users
to highlight the text by selecting it with the mouse to read it). Better
not start to play with this IMO, that's not really needed and may be more
annoying to some than helpful to most.

Thanks,
Willy

[PATCH v1 5/5] selftests/nolibc: report: add newline before test failures

[Zhangjin Wu]

a newline is inserted just before the test failures to avoid mixing the
test failures with the raw test log.

Signed-off-by: Zhangjin Wu <falcon@tinylab.org>
---
 tools/testing/selftests/nolibc/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/nolibc/Makefile b/tools/testing/selftests/nolibc/Makefile
index 10e9e5c1bdd0..f5c9c6bf7a19 100644
--- a/tools/testing/selftests/nolibc/Makefile
+++ b/tools/testing/selftests/nolibc/Makefile
@@ -84,7 +84,7 @@ CFLAGS  ?= -Os -fno-ident -fno-asynchronous-unwind-tables -std=c89 \
 		$(CFLAGS_$(ARCH)) $(CFLAGS_STACKPROTECTOR)
 LDFLAGS := -s
 
-REPORT  ?= awk '/\[OK\][\r]*$$/{p++} /\[FAIL\][\r]*$$/{f++;print} /\[SKIPPED\][\r]*$$/{s++} \
+REPORT  ?= awk '/\[OK\][\r]*$$/{p++} /\[FAIL\][\r]*$$/{if (!f) printf("\n"); f++; print;} /\[SKIPPED\][\r]*$$/{s++} \
 		END{ printf("\n%03d test(s): %03d passed, %03d skipped, %03d failed => status: ", p+s+f, p, s, f); \
 		if (f) printf("failure\n"); else if (s) printf("warning\n"); else printf("success\n");; \
 		printf("\nSee all results in %s\n", ARGV[1]); }'
-- 
2.25.1

Re: [PATCH v1 0/5] selftests/nolibc: report: print test status

[Willy Tarreau]

Hi Zhangjin,

On Thu, Jul 06, 2023 at 05:02:26PM +0800, Zhangjin Wu wrote:
> Hi, Willy
> 
> As you suggested, the 'status: [success|warning|failure]' info is added
> to the summary line, with additional newlines around this line to
> extrude the status info. at the same time, the total tests is printed,
> the passed, skipped and failed values are aligned with '%03d'.

So as I mentioned with some commits, I *do* find it important to
preserve the convenience of grepping for a single word to from 20
test reports at once and visually check all statuses (and in this
sense I like your preference for aligning the words to make them
more readable). But having to guess some grep context and see the
output garbled clearly does the opposite of what we were looking
for in my opinion. Also, I think there's no need for having 5
separate patches to add/remove a line feed. Better discuss an
output format that matches everyone's needs and change it at once,
this will make the patch more reviewable than having individual
changes like this.

thanks,
willy

Re: [PATCH v1 0/5] selftests/nolibc: report: print test status

[Zhangjin Wu]

Hi, Willy

> Hi Zhangjin,
> 
> On Thu, Jul 06, 2023 at 05:02:26PM +0800, Zhangjin Wu wrote:
> > Hi, Willy
> > 
> > As you suggested, the 'status: [success|warning|failure]' info is added
> > to the summary line, with additional newlines around this line to
> > extrude the status info. at the same time, the total tests is printed,
> > the passed, skipped and failed values are aligned with '%03d'.
> 
> So as I mentioned with some commits, I *do* find it important to
> preserve the convenience of grepping for a single word to from 20
> test reports at once and visually check all statuses (and in this
> sense I like your preference for aligning the words to make them
> more readable). But having to guess some grep context and see the
> output garbled clearly does the opposite of what we were looking
> for in my opinion.

Sorry for confusing you, hope my just reply [1] explained the 'newlines'
patch, as you pointed out in another reply, perhaps I need to write more
about the deeper 'background' idea of the patch, but sometimes, I'm also
worried about writing too much, for example, some info may be 'obvious'
but I spent too much statements, I will improve as possible as I can,
thanks.

[1]: https://lore.kernel.org/lkml/20230709192652.97668-1-falcon@tinylab.org/

> Also, I think there's no need for having 5
> separate patches to add/remove a line feed. Better discuss an
> output format that matches everyone's needs and change it at once,
> this will make the patch more reviewable than having individual
> changes like this.

That's right, the patches are split here is just for the last three are
new to our previous discuss, perhaps need more discuss, in the future, I
will propose the ideas before send the patches, just as we did for some
other patches.

Thanks,
Zhangjin

> 
> thanks,
> willy

Re: [PATCH v1 0/5] selftests/nolibc: report: print test status

[Willy Tarreau]

Series finally queued as well with your explanation :-)

Thanks,
Willy