On Tue, Aug 10, 2021 at 4:23 AM Daniel Borkmann <daniel@xxxxxxxxxxxxx> 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@xxxxxx>
>
> 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;
> >
>
