[PATCH v2 bpf-next 0/5] Improve the usability of test_progs

[PATCH v2 bpf-next 0/5] Improve the usability of test_progs

[Yucong Sun]

This short series focus on improving the usability of test_progs, making
the output cleaner, easier to select test to run, and adding a summary of
all failed the tests at the end of the output.

Yucong Sun (5):
  Skip loading bpf_testmod when using -l to list tests.
  Add glob matching for test selector in test_progs.
  Correctly display subtest skip status
  Display test number when listing test names
  Record all failed tests and output after the summary line.

 tools/testing/selftests/bpf/test_progs.c | 144 ++++++++++++++++++++---
 tools/testing/selftests/bpf/test_progs.h |   2 +
 2 files changed, 132 insertions(+), 14 deletions(-)

-- 
2.30.2

[PATCH v2 bpf-next 1/5] Skip loading bpf_testmod when using -l to list tests.

[Yucong Sun]

This patch remove bpf_testmod load test when using "-l", making output
cleaner.

Signed-off-by: Yucong Sun <fallentree@fb.com>
---
 tools/testing/selftests/bpf/test_progs.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index 6f103106a39b..74dde0af1592 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -754,10 +754,12 @@ int main(int argc, char **argv)
 
 	save_netns();
 	stdio_hijack();
-	env.has_testmod = true;
-	if (load_bpf_testmod()) {
-		fprintf(env.stderr, "WARNING! Selftests relying on bpf_testmod.ko will be skipped.\n");
-		env.has_testmod = false;
+	if (!env.list_test_names) {
+		env.has_testmod = true;
+		if (load_bpf_testmod()) {
+			fprintf(env.stderr, "WARNING! Selftests relying on bpf_testmod.ko will be skipped.\n");
+			env.has_testmod = false;
+		}
 	}
 	for (i = 0; i < prog_test_cnt; i++) {
 		struct prog_test_def *test = &prog_test_defs[i];
-- 
2.30.2

Re: [PATCH v2 bpf-next 1/5] Skip loading bpf_testmod when using -l to list tests.

[Andrii Nakryiko]

On Mon, Aug 9, 2021 at 5:17 PM Yucong Sun <fallentree@fb.com> wrote:
>
> This patch remove bpf_testmod load test when using "-l", making output
> cleaner.
>
> Signed-off-by: Yucong Sun <fallentree@fb.com>
> ---
>  tools/testing/selftests/bpf/test_progs.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> index 6f103106a39b..74dde0af1592 100644
> --- a/tools/testing/selftests/bpf/test_progs.c
> +++ b/tools/testing/selftests/bpf/test_progs.c
> @@ -754,10 +754,12 @@ int main(int argc, char **argv)
>
>         save_netns();
>         stdio_hijack();
> -       env.has_testmod = true;
> -       if (load_bpf_testmod()) {

could keep this to minimal changes by just doing

if (!env.list_test_names && load_bpf_testmod()) { ... }

env.has_testmod = true doesn't make difference for listing tests (and
in the future we might want to assume that testmod is available to be
able to list all tests and subtests, including those that depend on
testmod).

> -               fprintf(env.stderr, "WARNING! Selftests relying on bpf_testmod.ko will be skipped.\n");
> -               env.has_testmod = false;
> +       if (!env.list_test_names) {
> +               env.has_testmod = true;
> +               if (load_bpf_testmod()) {
> +                       fprintf(env.stderr, "WARNING! Selftests relying on bpf_testmod.ko will be skipped.\n");
> +                       env.has_testmod = false;
> +               }
>         }
>         for (i = 0; i < prog_test_cnt; i++) {
>                 struct prog_test_def *test = &prog_test_defs[i];
> --
> 2.30.2
>

[PATCH v2 bpf-next 2/5] Add glob matching for test selector in test_progs.

[Yucong Sun]

This patch adds glob matching to test selector, it accepts
simple glob pattern "*", "?", "[]" to match the test names to run.

The glob matching function is copied from perf/util/string.c

Signed-off-by: Yucong Sun <fallentree@fb.com>
---
 tools/testing/selftests/bpf/test_progs.c | 94 +++++++++++++++++++++++-
 1 file changed, 92 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index 74dde0af1592..c5bffd2e78ae 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -13,6 +13,96 @@
 #include <execinfo.h> /* backtrace */
 #include <linux/membarrier.h>
 
+// Copied from perf/util/string.c
+
+/* Character class matching */
+static bool __match_charclass(const char *pat, char c, const char **npat)
+{
+	bool complement = false, ret = true;
+
+	if (*pat == '!') {
+		complement = true;
+		pat++;
+	}
+	if (*pat++ == c) /* First character is special */
+		goto end;
+
+	while (*pat && *pat != ']') { /* Matching */
+		if (*pat == '-' && *(pat + 1) != ']') { /* Range */
+			if (*(pat - 1) <= c && c <= *(pat + 1))
+				goto end;
+			if (*(pat - 1) > *(pat + 1))
+				goto error;
+			pat += 2;
+		} else if (*pat++ == c)
+			goto end;
+	}
+	if (!*pat)
+		goto error;
+	ret = false;
+
+end:
+	while (*pat && *pat != ']') /* Searching closing */
+		pat++;
+	if (!*pat)
+		goto error;
+	*npat = pat + 1;
+	return complement ? !ret : ret;
+
+error:
+	return false;
+}
+
+// Copied from perf/util/string.c
+/* Glob/lazy pattern matching */
+static bool __match_glob(const char *str, const char *pat, bool ignore_space,
+			 bool case_ins)
+{
+	while (*str && *pat && *pat != '*') {
+		if (ignore_space) {
+			/* Ignore spaces for lazy matching */
+			if (isspace(*str)) {
+				str++;
+				continue;
+			}
+			if (isspace(*pat)) {
+				pat++;
+				continue;
+			}
+		}
+		if (*pat == '?') { /* Matches any single character */
+			str++;
+			pat++;
+			continue;
+		} else if (*pat == '[') /* Character classes/Ranges */
+			if (__match_charclass(pat + 1, *str, &pat)) {
+				str++;
+				continue;
+			} else
+				return false;
+		else if (*pat == '\\') /* Escaped char match as normal char */
+			pat++;
+		if (case_ins) {
+			if (tolower(*str) != tolower(*pat))
+				return false;
+		} else if (*str != *pat)
+			return false;
+		str++;
+		pat++;
+	}
+	/* Check wild card */
+	if (*pat == '*') {
+		while (*pat == '*')
+			pat++;
+		if (!*pat) /* Tail wild card matches all */
+			return true;
+		while (*str)
+			if (__match_glob(str++, pat, ignore_space, case_ins))
+				return true;
+	}
+	return !*str && !*pat;
+}
+
 #define EXIT_NO_TEST		2
 #define EXIT_ERR_SETUP_INFRA	3
 
@@ -55,12 +145,12 @@ static bool should_run(struct test_selector *sel, int num, const char *name)
 	int i;
 
 	for (i = 0; i < sel->blacklist.cnt; i++) {
-		if (strstr(name, sel->blacklist.strs[i]))
+		if (__match_glob(name, sel->blacklist.strs[i], false, false))
 			return false;
 	}
 
 	for (i = 0; i < sel->whitelist.cnt; i++) {
-		if (strstr(name, sel->whitelist.strs[i]))
+		if (__match_glob(name, sel->whitelist.strs[i], false, false))
 			return true;
 	}
 
-- 
2.30.2

Re: [PATCH v2 bpf-next 2/5] Add glob matching for test selector in test_progs.

[Andrii Nakryiko]

On Mon, Aug 9, 2021 at 5:17 PM Yucong Sun <fallentree@fb.com> wrote:
>
> This patch adds glob matching to test selector, it accepts
> simple glob pattern "*", "?", "[]" to match the test names to run.

do we really need ? and []? I've been pretty happy so far in retsnoop
with supporting just these patterns:

1. exact match ('abc')
2. prefix match ('abc*')
3. suffix match ('*abc')
4. substring match ('*abc*')

See [0] for a naive but simple implementation for that logic. So far
with test_progs I've only needed two cases from the above: exact and
substring matches. So I'm leaning towards keeping it simple, actually.

But there is also an issue of backwards compatibility. People using
`test_progs -t substr` are used to substring matching logic, so with
this change you are breaking this, which will cause frustration, most
probably. So maybe let's add a new parameter to specify these globs.
E.g., maybe `-a <glob>` for whitelisting (allowlisting), and `-d
<glob>` for blacklisting (denylisting)? Also, instead of parsing
comma-separated lists as I did initially with -t, we should probably
just allow multiple occurences of -a and -d:

./test_progs -a '*core*' -a '*linux' -d 'core_autosize'

will allow all CO-RE tests except autosize one, plus will admit
vmlinux selftest.

Also, keep in mind that there are subtests within some tests, and
those should be matches with the same logic as well:

./test_progs -a 'core_reloc/size*'

should run:

#32/58 size:OK
#32/59 size___diff_sz:OK
#32/60 size___err_ambiguous:OK


It gets a bit trickier with globs for both test and subtest, e.g.
'*core*/size*' -- should it match just core_reloc/size* tests as
above? Or also core_retro, core_extern, etc tests even though they
don't have subtests starting with 'size'? I'd say the latter is more
desirable, but I haven't checked how hard that would be to support.


  [0] https://github.com/anakryiko/retsnoop/blob/2e6217f7a82f421fcf3481cc401390605066ab26/src/mass_attacher.c#L982-L1015

>
> The glob matching function is copied from perf/util/string.c
>
> Signed-off-by: Yucong Sun <fallentree@fb.com>
> ---
>  tools/testing/selftests/bpf/test_progs.c | 94 +++++++++++++++++++++++-
>  1 file changed, 92 insertions(+), 2 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> index 74dde0af1592..c5bffd2e78ae 100644
> --- a/tools/testing/selftests/bpf/test_progs.c
> +++ b/tools/testing/selftests/bpf/test_progs.c
> @@ -13,6 +13,96 @@
>  #include <execinfo.h> /* backtrace */
>  #include <linux/membarrier.h>
>
> +// Copied from perf/util/string.c
> +
> +/* Character class matching */
> +static bool __match_charclass(const char *pat, char c, const char **npat)
> +{
> +       bool complement = false, ret = true;
> +
> +       if (*pat == '!') {
> +               complement = true;
> +               pat++;
> +       }
> +       if (*pat++ == c) /* First character is special */
> +               goto end;
> +
> +       while (*pat && *pat != ']') { /* Matching */
> +               if (*pat == '-' && *(pat + 1) != ']') { /* Range */
> +                       if (*(pat - 1) <= c && c <= *(pat + 1))
> +                               goto end;
> +                       if (*(pat - 1) > *(pat + 1))
> +                               goto error;
> +                       pat += 2;
> +               } else if (*pat++ == c)
> +                       goto end;
> +       }
> +       if (!*pat)
> +               goto error;
> +       ret = false;
> +
> +end:
> +       while (*pat && *pat != ']') /* Searching closing */
> +               pat++;
> +       if (!*pat)
> +               goto error;
> +       *npat = pat + 1;
> +       return complement ? !ret : ret;
> +
> +error:
> +       return false;
> +}
> +
> +// Copied from perf/util/string.c
> +/* Glob/lazy pattern matching */
> +static bool __match_glob(const char *str, const char *pat, bool ignore_space,
> +                        bool case_ins)
> +{
> +       while (*str && *pat && *pat != '*') {
> +               if (ignore_space) {
> +                       /* Ignore spaces for lazy matching */
> +                       if (isspace(*str)) {
> +                               str++;
> +                               continue;
> +                       }
> +                       if (isspace(*pat)) {
> +                               pat++;
> +                               continue;
> +                       }
> +               }
> +               if (*pat == '?') { /* Matches any single character */
> +                       str++;
> +                       pat++;
> +                       continue;
> +               } else if (*pat == '[') /* Character classes/Ranges */
> +                       if (__match_charclass(pat + 1, *str, &pat)) {
> +                               str++;
> +                               continue;
> +                       } else
> +                               return false;
> +               else if (*pat == '\\') /* Escaped char match as normal char */
> +                       pat++;
> +               if (case_ins) {
> +                       if (tolower(*str) != tolower(*pat))
> +                               return false;
> +               } else if (*str != *pat)
> +                       return false;
> +               str++;
> +               pat++;
> +       }
> +       /* Check wild card */
> +       if (*pat == '*') {
> +               while (*pat == '*')
> +                       pat++;
> +               if (!*pat) /* Tail wild card matches all */
> +                       return true;
> +               while (*str)
> +                       if (__match_glob(str++, pat, ignore_space, case_ins))
> +                               return true;
> +       }
> +       return !*str && !*pat;
> +}
> +
>  #define EXIT_NO_TEST           2
>  #define EXIT_ERR_SETUP_INFRA   3
>
> @@ -55,12 +145,12 @@ static bool should_run(struct test_selector *sel, int num, const char *name)
>         int i;
>
>         for (i = 0; i < sel->blacklist.cnt; i++) {
> -               if (strstr(name, sel->blacklist.strs[i]))
> +               if (__match_glob(name, sel->blacklist.strs[i], false, false))
>                         return false;
>         }
>
>         for (i = 0; i < sel->whitelist.cnt; i++) {
> -               if (strstr(name, sel->whitelist.strs[i]))
> +               if (__match_glob(name, sel->whitelist.strs[i], false, false))
>                         return true;
>         }
>
> --
> 2.30.2
>

[PATCH v2 bpf-next 3/5] Correctly display subtest skip status

[Yucong Sun]

In skip_account(), test->skip_cnt is set to 0 at the end, this makes
next print statement never display SKIP status for the subtest. This
patch moves the accounting logic after the print statement, fixing the
issue.

Signed-off-by: Yucong Sun <fallentree@fb.com>
---
 tools/testing/selftests/bpf/test_progs.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index c5bffd2e78ae..82d012671552 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -238,18 +238,18 @@ void test__end_subtest()
 	struct prog_test_def *test = env.test;
 	int sub_error_cnt = test->error_cnt - test->old_error_cnt;
 
-	if (sub_error_cnt)
-		env.fail_cnt++;
-	else if (test->skip_cnt == 0)
-		env.sub_succ_cnt++;
-	skip_account();
-
 	dump_test_log(test, sub_error_cnt);
 
 	fprintf(env.stdout, "#%d/%d %s:%s\n",
 	       test->test_num, test->subtest_num, test->subtest_name,
 	       sub_error_cnt ? "FAIL" : (test->skip_cnt ? "SKIP" : "OK"));
 
+	if (sub_error_cnt)
+		env.fail_cnt++;
+	else if (test->skip_cnt == 0)
+		env.sub_succ_cnt++;
+	skip_account();
+
 	free(test->subtest_name);
 	test->subtest_name = NULL;
 }
-- 
2.30.2

Re: [PATCH v2 bpf-next 3/5] Correctly display subtest skip status

[Andrii Nakryiko]

On Mon, Aug 9, 2021 at 5:17 PM Yucong Sun <fallentree@fb.com> wrote:
>
> In skip_account(), test->skip_cnt is set to 0 at the end, this makes
> next print statement never display SKIP status for the subtest. This
> patch moves the accounting logic after the print statement, fixing the
> issue.
>
> Signed-off-by: Yucong Sun <fallentree@fb.com>
> ---

Looks good, but seems like we are not printing SKIP for normal tests,
let's do that while we are at fixing SKIP reporting?

>  tools/testing/selftests/bpf/test_progs.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> index c5bffd2e78ae..82d012671552 100644
> --- a/tools/testing/selftests/bpf/test_progs.c
> +++ b/tools/testing/selftests/bpf/test_progs.c
> @@ -238,18 +238,18 @@ void test__end_subtest()
>         struct prog_test_def *test = env.test;
>         int sub_error_cnt = test->error_cnt - test->old_error_cnt;
>
> -       if (sub_error_cnt)
> -               env.fail_cnt++;
> -       else if (test->skip_cnt == 0)
> -               env.sub_succ_cnt++;
> -       skip_account();
> -
>         dump_test_log(test, sub_error_cnt);
>
>         fprintf(env.stdout, "#%d/%d %s:%s\n",
>                test->test_num, test->subtest_num, test->subtest_name,
>                sub_error_cnt ? "FAIL" : (test->skip_cnt ? "SKIP" : "OK"));
>
> +       if (sub_error_cnt)
> +               env.fail_cnt++;
> +       else if (test->skip_cnt == 0)
> +               env.sub_succ_cnt++;
> +       skip_account();
> +
>         free(test->subtest_name);
>         test->subtest_name = NULL;
>  }
> --
> 2.30.2
>

[PATCH v2 bpf-next 4/5] Display test number when listing test names

[Yucong Sun]

This patch adds tests number to the output when using "test_prog -l".

Signed-off-by: Yucong Sun <fallentree@fb.com>
---
 tools/testing/selftests/bpf/test_progs.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index 82d012671552..5cc808992b00 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -867,7 +867,8 @@ int main(int argc, char **argv)
 		}
 
 		if (env.list_test_names) {
-			fprintf(env.stdout, "%s\n", test->test_name);
+			fprintf(env.stdout, "# %d %s\n",
+				test->test_num, test->test_name);
 			env.succ_cnt++;
 			continue;
 		}
-- 
2.30.2

Re: [PATCH v2 bpf-next 4/5] Display test number when listing test names

[Andrii Nakryiko]

On Mon, Aug 9, 2021 at 5:17 PM Yucong Sun <fallentree@fb.com> wrote:
>
> This patch adds tests number to the output when using "test_prog -l".

Yes, but why? Commit message should contain the motivation for the
change, not just description of what's being changed.

Also, this changes the output format and there are systems that might
rely on this specific format (I remember Red Hat were using
./test_progs -l, for example). So without a good reason, we shouldn't
change the format.

>
> Signed-off-by: Yucong Sun <fallentree@fb.com>
> ---
>  tools/testing/selftests/bpf/test_progs.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> index 82d012671552..5cc808992b00 100644
> --- a/tools/testing/selftests/bpf/test_progs.c
> +++ b/tools/testing/selftests/bpf/test_progs.c
> @@ -867,7 +867,8 @@ int main(int argc, char **argv)
>                 }
>
>                 if (env.list_test_names) {
> -                       fprintf(env.stdout, "%s\n", test->test_name);
> +                       fprintf(env.stdout, "# %d %s\n",
> +                               test->test_num, test->test_name);
>                         env.succ_cnt++;
>                         continue;
>                 }
> --
> 2.30.2
>

[PATCH v2 bpf-next 5/5] Record all failed tests and output after the summary line.

[Yucong Sun]

This patch records all failed tests and subtests during the run, output
them after the summary line, making it easier to identify failed tests
in the long output.

Signed-off-by: Yucong Sun <fallentree@fb.com>
---
 tools/testing/selftests/bpf/test_progs.c | 25 +++++++++++++++++++++++-
 tools/testing/selftests/bpf/test_progs.h |  2 ++
 2 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index 5cc808992b00..51a70031f07e 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -244,6 +244,11 @@ void test__end_subtest()
 	       test->test_num, test->subtest_num, test->subtest_name,
 	       sub_error_cnt ? "FAIL" : (test->skip_cnt ? "SKIP" : "OK"));
 
+	if (sub_error_cnt) {
+		fprintf(env.summary_errors, "#%d/%d %s: FAIL\n",
+			test->test_num, test->subtest_num, test->subtest_name);
+	}
+
 	if (sub_error_cnt)
 		env.fail_cnt++;
 	else if (test->skip_cnt == 0)
@@ -816,6 +821,10 @@ int main(int argc, char **argv)
 		.sa_flags = SA_RESETHAND,
 	};
 	int err, i;
+	/* record errors to print after summary line */
+	char *summary_errors_buf;
+	size_t summary_errors_cnt;
+
 
 	sigaction(SIGSEGV, &sigact, NULL);
 
@@ -823,6 +832,9 @@ int main(int argc, char **argv)
 	if (err)
 		return err;
 
+	env.summary_errors = open_memstream(
+		&summary_errors_buf, &summary_errors_cnt);
+
 	err = cd_flavor_subdir(argv[0]);
 	if (err)
 		return err;
@@ -891,6 +903,11 @@ int main(int argc, char **argv)
 			test->test_num, test->test_name,
 			test->error_cnt ? "FAIL" : "OK");
 
+		if(test->error_cnt) {
+			fprintf(env.summary_errors, "#%d %s: FAIL\n",
+				test->test_num, test->test_name);
+		}
+
 		reset_affinity();
 		restore_netns();
 		if (test->need_cgroup_cleanup)
@@ -908,9 +925,14 @@ int main(int argc, char **argv)
 	if (env.list_test_names)
 		goto out;
 
-	fprintf(stdout, "Summary: %d/%d PASSED, %d SKIPPED, %d FAILED\n",
+	fprintf(stdout, "\nSummary: %d/%d PASSED, %d SKIPPED, %d FAILED\n\n",
 		env.succ_cnt, env.sub_succ_cnt, env.skip_cnt, env.fail_cnt);
 
+	fclose(env.summary_errors);
+	if(env.fail_cnt) {
+		fprintf(stdout, "%s", summary_errors_buf);
+	}
+
 out:
 	free_str_set(&env.test_selector.blacklist);
 	free_str_set(&env.test_selector.whitelist);
@@ -919,6 +941,7 @@ int main(int argc, char **argv)
 	free_str_set(&env.subtest_selector.whitelist);
 	free(env.subtest_selector.num_set);
 	close(env.saved_netns_fd);
+	free(summary_errors_buf);
 
 	if (env.succ_cnt + env.fail_cnt + env.skip_cnt == 0)
 		return EXIT_NO_TEST;
diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
index c8c2bf878f67..63f4e534c6e5 100644
--- a/tools/testing/selftests/bpf/test_progs.h
+++ b/tools/testing/selftests/bpf/test_progs.h
@@ -82,6 +82,8 @@ struct test_env {
 	int skip_cnt; /* skipped tests */
 
 	int saved_netns_fd;
+
+	FILE* summary_errors;
 };
 
 extern struct test_env env;
-- 
2.30.2

Re: [PATCH v2 bpf-next 5/5] Record all failed tests and output after the summary line.

[Daniel Borkmann]

On 8/10/21 2:16 AM, Yucong Sun wrote:
> This patch records all failed tests and subtests during the run, output
> them after the summary line, making it easier to identify failed tests
> in the long output.
> 
> Signed-off-by: Yucong Sun <fallentree@fb.com>

nit: please prefix all $subjects with e.g. 'bpf, selftests:'. for example, here should
be 'bpf, selftests: Record all failed tests and output after the summary line' so it's
more clear in the git log which subsystem is meant.

> ---
>   tools/testing/selftests/bpf/test_progs.c | 25 +++++++++++++++++++++++-
>   tools/testing/selftests/bpf/test_progs.h |  2 ++
>   2 files changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> index 5cc808992b00..51a70031f07e 100644
> --- a/tools/testing/selftests/bpf/test_progs.c
> +++ b/tools/testing/selftests/bpf/test_progs.c
> @@ -244,6 +244,11 @@ void test__end_subtest()
>   	       test->test_num, test->subtest_num, test->subtest_name,
>   	       sub_error_cnt ? "FAIL" : (test->skip_cnt ? "SKIP" : "OK"));
>   
> +	if (sub_error_cnt) {
> +		fprintf(env.summary_errors, "#%d/%d %s: FAIL\n",
> +			test->test_num, test->subtest_num, test->subtest_name);
> +	}
> +
>   	if (sub_error_cnt)
>   		env.fail_cnt++;
>   	else if (test->skip_cnt == 0)
> @@ -816,6 +821,10 @@ int main(int argc, char **argv)
>   		.sa_flags = SA_RESETHAND,
>   	};
>   	int err, i;
> +	/* record errors to print after summary line */
> +	char *summary_errors_buf;
> +	size_t summary_errors_cnt;
> +
>   

nit: double newline

>   	sigaction(SIGSEGV, &sigact, NULL);
>   
> @@ -823,6 +832,9 @@ int main(int argc, char **argv)
>   	if (err)
>   		return err;
>   
> +	env.summary_errors = open_memstream(
> +		&summary_errors_buf, &summary_errors_cnt);

Test for env.summary_errors being NULL missing.

> +
>   	err = cd_flavor_subdir(argv[0]);
>   	if (err)
>   		return err;
> @@ -891,6 +903,11 @@ int main(int argc, char **argv)
>   			test->test_num, test->test_name,
>   			test->error_cnt ? "FAIL" : "OK");
>   
> +		if(test->error_cnt) {
> +			fprintf(env.summary_errors, "#%d %s: FAIL\n",
> +				test->test_num, test->test_name);
> +		}
> +
>   		reset_affinity();
>   		restore_netns();
>   		if (test->need_cgroup_cleanup)
> @@ -908,9 +925,14 @@ int main(int argc, char **argv)
>   	if (env.list_test_names)
>   		goto out;
>   
> -	fprintf(stdout, "Summary: %d/%d PASSED, %d SKIPPED, %d FAILED\n",
> +	fprintf(stdout, "\nSummary: %d/%d PASSED, %d SKIPPED, %d FAILED\n\n",
>   		env.succ_cnt, env.sub_succ_cnt, env.skip_cnt, env.fail_cnt);
>   
> +	fclose(env.summary_errors);
> +	if(env.fail_cnt) {
> +		fprintf(stdout, "%s", summary_errors_buf);
> +	}
> +
>   out:
>   	free_str_set(&env.test_selector.blacklist);
>   	free_str_set(&env.test_selector.whitelist);
> @@ -919,6 +941,7 @@ int main(int argc, char **argv)
>   	free_str_set(&env.subtest_selector.whitelist);
>   	free(env.subtest_selector.num_set);
>   	close(env.saved_netns_fd);
> +	free(summary_errors_buf);
>   
>   	if (env.succ_cnt + env.fail_cnt + env.skip_cnt == 0)
>   		return EXIT_NO_TEST;
> diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
> index c8c2bf878f67..63f4e534c6e5 100644
> --- a/tools/testing/selftests/bpf/test_progs.h
> +++ b/tools/testing/selftests/bpf/test_progs.h
> @@ -82,6 +82,8 @@ struct test_env {
>   	int skip_cnt; /* skipped tests */
>   
>   	int saved_netns_fd;
> +
> +	FILE* summary_errors;

nit: FILE *summary_errors;

>   };
>   
>   extern struct test_env env;
> 

Re: [PATCH v2 bpf-next 5/5] Record all failed tests and output after the summary line.

[Andrii Nakryiko]

On Tue, Aug 10, 2021 at 4:23 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 8/10/21 2:16 AM, Yucong Sun wrote:
> > This patch records all failed tests and subtests during the run, output
> > them after the summary line, making it easier to identify failed tests
> > in the long output.
> >
> > Signed-off-by: Yucong Sun <fallentree@fb.com>
>
> nit: please prefix all $subjects with e.g. 'bpf, selftests:'. for example, here should
> be 'bpf, selftests: Record all failed tests and output after the summary line' so it's
> more clear in the git log which subsystem is meant.

Thank, Daniel, for catching this!

We've more or less consistently used these prefixes (with the emphasis
on "more or less", of course):

1. 'bpf:', for BPF-related kernel proper patches
2. 'libbpf:', for libbpf patches
3. 'selftests/bpf:'. for BPF selftests
4. 'bpftool:', for bpftool-specific patches
5. 'samples/bpf', for, you guessed it, samples/bpf :)

I don't know how much value it is to record this convention in our
docs Q&A doc, but it's worth keeping this convention consistent.

Haven't checked the logic of this patch yet, but thought I'll comment
on this convention (and a minor styling nit below).

>
> > ---
> >   tools/testing/selftests/bpf/test_progs.c | 25 +++++++++++++++++++++++-
> >   tools/testing/selftests/bpf/test_progs.h |  2 ++
> >   2 files changed, 26 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> > index 5cc808992b00..51a70031f07e 100644
> > --- a/tools/testing/selftests/bpf/test_progs.c
> > +++ b/tools/testing/selftests/bpf/test_progs.c
> > @@ -244,6 +244,11 @@ void test__end_subtest()
> >              test->test_num, test->subtest_num, test->subtest_name,
> >              sub_error_cnt ? "FAIL" : (test->skip_cnt ? "SKIP" : "OK"));
> >
> > +     if (sub_error_cnt) {
> > +             fprintf(env.summary_errors, "#%d/%d %s: FAIL\n",
> > +                     test->test_num, test->subtest_num, test->subtest_name);
> > +     }
> > +
> >       if (sub_error_cnt)
> >               env.fail_cnt++;
> >       else if (test->skip_cnt == 0)
> > @@ -816,6 +821,10 @@ int main(int argc, char **argv)
> >               .sa_flags = SA_RESETHAND,
> >       };
> >       int err, i;
> > +     /* record errors to print after summary line */
> > +     char *summary_errors_buf;
> > +     size_t summary_errors_cnt;
> > +
> >
>
> nit: double newline
>
> >       sigaction(SIGSEGV, &sigact, NULL);
> >
> > @@ -823,6 +832,9 @@ int main(int argc, char **argv)
> >       if (err)
> >               return err;
> >
> > +     env.summary_errors = open_memstream(
> > +             &summary_errors_buf, &summary_errors_cnt);
>
> Test for env.summary_errors being NULL missing.
>
> > +
> >       err = cd_flavor_subdir(argv[0]);
> >       if (err)
> >               return err;
> > @@ -891,6 +903,11 @@ int main(int argc, char **argv)
> >                       test->test_num, test->test_name,
> >                       test->error_cnt ? "FAIL" : "OK");
> >
> > +             if(test->error_cnt) {

styling nit: if<space>(

please run checkpatches.pl before submitting patches

> > +                     fprintf(env.summary_errors, "#%d %s: FAIL\n",
> > +                             test->test_num, test->test_name);
> > +             }
> > +
> >               reset_affinity();
> >               restore_netns();
> >               if (test->need_cgroup_cleanup)
> > @@ -908,9 +925,14 @@ int main(int argc, char **argv)
> >       if (env.list_test_names)
> >               goto out;
> >
> > -     fprintf(stdout, "Summary: %d/%d PASSED, %d SKIPPED, %d FAILED\n",
> > +     fprintf(stdout, "\nSummary: %d/%d PASSED, %d SKIPPED, %d FAILED\n\n",
> >               env.succ_cnt, env.sub_succ_cnt, env.skip_cnt, env.fail_cnt);
> >
> > +     fclose(env.summary_errors);
> > +     if(env.fail_cnt) {
> > +             fprintf(stdout, "%s", summary_errors_buf);
> > +     }
> > +
> >   out:
> >       free_str_set(&env.test_selector.blacklist);
> >       free_str_set(&env.test_selector.whitelist);
> > @@ -919,6 +941,7 @@ int main(int argc, char **argv)
> >       free_str_set(&env.subtest_selector.whitelist);
> >       free(env.subtest_selector.num_set);
> >       close(env.saved_netns_fd);
> > +     free(summary_errors_buf);
> >
> >       if (env.succ_cnt + env.fail_cnt + env.skip_cnt == 0)
> >               return EXIT_NO_TEST;
> > diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
> > index c8c2bf878f67..63f4e534c6e5 100644
> > --- a/tools/testing/selftests/bpf/test_progs.h
> > +++ b/tools/testing/selftests/bpf/test_progs.h
> > @@ -82,6 +82,8 @@ struct test_env {
> >       int skip_cnt; /* skipped tests */
> >
> >       int saved_netns_fd;
> > +
> > +     FILE* summary_errors;
>
> nit: FILE *summary_errors;
>
> >   };
> >
> >   extern struct test_env env;
> >
>

Re: [PATCH v2 bpf-next 5/5] Record all failed tests and output after the summary line.

[Daniel Borkmann]

On 8/10/21 6:25 PM, Andrii Nakryiko wrote:
> On Tue, Aug 10, 2021 at 4:23 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>>
>> On 8/10/21 2:16 AM, Yucong Sun wrote:
>>> This patch records all failed tests and subtests during the run, output
>>> them after the summary line, making it easier to identify failed tests
>>> in the long output.
>>>
>>> Signed-off-by: Yucong Sun <fallentree@fb.com>
>>
>> nit: please prefix all $subjects with e.g. 'bpf, selftests:'. for example, here should
>> be 'bpf, selftests: Record all failed tests and output after the summary line' so it's
>> more clear in the git log which subsystem is meant.
> 
> Thank, Daniel, for catching this!
> 
> We've more or less consistently used these prefixes (with the emphasis
> on "more or less", of course):
> 
> 1. 'bpf:', for BPF-related kernel proper patches
> 2. 'libbpf:', for libbpf patches
> 3. 'selftests/bpf:'. for BPF selftests
> 4. 'bpftool:', for bpftool-specific patches
> 5. 'samples/bpf', for, you guessed it, samples/bpf :)
> 
> I don't know how much value it is to record this convention in our
> docs Q&A doc, but it's worth keeping this convention consistent.

Agree, it somewhat evolved into these 5 main areas above. Might be worth putting
a note into q&a doc or at least tweak $subject line while applying if it's too
far off. :)

> Haven't checked the logic of this patch yet, but thought I'll comment
> on this convention (and a minor styling nit below).

Re: [PATCH v2 bpf-next 5/5] Record all failed tests and output after the summary line.

[Andrii Nakryiko]

On Mon, Aug 9, 2021 at 5:17 PM Yucong Sun <fallentree@fb.com> wrote:
>
> This patch records all failed tests and subtests during the run, output
> them after the summary line, making it easier to identify failed tests
> in the long output.

Well, but then you still have to search back to see what exactly
happened with that test. So it's definitely more useful, but not
completely useful for dealing with CI test failures. E.g., here's the
output I get:

.... lots of lines here ....
#167 xdp_link:OK
#168 xdp_noinline:OK
#169 xdp_perf:OK

Summary: 166/947 PASSED, 3 SKIPPED, 3 FAILED

#4 attach_probe: FAIL
#134 tc_bpf: FAIL
#153 trace_printk: FAIL


Now I need to go and grep trace_printk to see what exactly failed
about that test.

I think the best output would be the one that will repeat the entire
log of each failed test/subtest. This is great for CI, because we can
have a pretty simple post-processing script capturing relevant error
logs and posting them in the email, eventually.

>
> Signed-off-by: Yucong Sun <fallentree@fb.com>
> ---
>  tools/testing/selftests/bpf/test_progs.c | 25 +++++++++++++++++++++++-
>  tools/testing/selftests/bpf/test_progs.h |  2 ++
>  2 files changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> index 5cc808992b00..51a70031f07e 100644
> --- a/tools/testing/selftests/bpf/test_progs.c
> +++ b/tools/testing/selftests/bpf/test_progs.c
> @@ -244,6 +244,11 @@ void test__end_subtest()
>                test->test_num, test->subtest_num, test->subtest_name,
>                sub_error_cnt ? "FAIL" : (test->skip_cnt ? "SKIP" : "OK"));
>
> +       if (sub_error_cnt) {
> +               fprintf(env.summary_errors, "#%d/%d %s: FAIL\n",
> +                       test->test_num, test->subtest_num, test->subtest_name);
> +       }
> +
>         if (sub_error_cnt)
>                 env.fail_cnt++;
>         else if (test->skip_cnt == 0)
> @@ -816,6 +821,10 @@ int main(int argc, char **argv)
>                 .sa_flags = SA_RESETHAND,
>         };
>         int err, i;
> +       /* record errors to print after summary line */
> +       char *summary_errors_buf;
> +       size_t summary_errors_cnt;
> +
>
>         sigaction(SIGSEGV, &sigact, NULL);
>
> @@ -823,6 +832,9 @@ int main(int argc, char **argv)
>         if (err)
>                 return err;
>
> +       env.summary_errors = open_memstream(
> +               &summary_errors_buf, &summary_errors_cnt);
> +
>         err = cd_flavor_subdir(argv[0]);
>         if (err)
>                 return err;
> @@ -891,6 +903,11 @@ int main(int argc, char **argv)
>                         test->test_num, test->test_name,
>                         test->error_cnt ? "FAIL" : "OK");
>
> +               if(test->error_cnt) {
> +                       fprintf(env.summary_errors, "#%d %s: FAIL\n",
> +                               test->test_num, test->test_name);
> +               }
> +
>                 reset_affinity();
>                 restore_netns();
>                 if (test->need_cgroup_cleanup)
> @@ -908,9 +925,14 @@ int main(int argc, char **argv)
>         if (env.list_test_names)
>                 goto out;
>
> -       fprintf(stdout, "Summary: %d/%d PASSED, %d SKIPPED, %d FAILED\n",
> +       fprintf(stdout, "\nSummary: %d/%d PASSED, %d SKIPPED, %d FAILED\n\n",
>                 env.succ_cnt, env.sub_succ_cnt, env.skip_cnt, env.fail_cnt);

this will emit an extra empty line even if no tests failed; let's just
do if (env.fail_cnt) fprintf(stdout, "\n"); ?

As for the empty line before Summary, is it really necessary?

>
> +       fclose(env.summary_errors);
> +       if(env.fail_cnt) {
> +               fprintf(stdout, "%s", summary_errors_buf);
> +       }

kernel style says that if/for/while statements with a single statement
shouldn't use {} around that statement. And `if` is not a function, so
there should be a space between if and ().

> +
>  out:
>         free_str_set(&env.test_selector.blacklist);
>         free_str_set(&env.test_selector.whitelist);
> @@ -919,6 +941,7 @@ int main(int argc, char **argv)
>         free_str_set(&env.subtest_selector.whitelist);
>         free(env.subtest_selector.num_set);
>         close(env.saved_netns_fd);
> +       free(summary_errors_buf);
>
>         if (env.succ_cnt + env.fail_cnt + env.skip_cnt == 0)
>                 return EXIT_NO_TEST;
> diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
> index c8c2bf878f67..63f4e534c6e5 100644
> --- a/tools/testing/selftests/bpf/test_progs.h
> +++ b/tools/testing/selftests/bpf/test_progs.h
> @@ -82,6 +82,8 @@ struct test_env {
>         int skip_cnt; /* skipped tests */
>
>         int saved_netns_fd;
> +
> +       FILE* summary_errors;
>  };
>
>  extern struct test_env env;
> --
> 2.30.2
>