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

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

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
>