| Message ID | 20210810001625.1140255-6-fallentree@fb.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | Improve the usability of test_progs | expand |
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; >
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; > > >
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 >
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).
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;
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(-)