| Message ID | 159353162763.912056.3435319848074491018.stgit@firesoul |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [bpf-next] selftests/bpf: test_progs option for listing test names | expand |
On Tue, Jun 30, 2020 at 8:40 AM Jesper Dangaard Brouer <brouer@redhat.com> wrote: > > The program test_progs have some very useful ability to specify a list of > test name substrings for selecting which tests to run. > > This patch add the ability to list the selected test names without running > them. This is practical for seeing which tests gets selected with given > select arguments (which can also contain a exclude list via --name-blacklist). > > This output can also be used by shell-scripts in a for-loop: > > for N in $(./test_progs --list -t xdp); do \ > ./test_progs -t $N 2>&1 > result_test_${N}.log & \ > done ; wait > > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> > --- > tools/testing/selftests/bpf/test_progs.c | 20 ++++++++++++++++++++ > tools/testing/selftests/bpf/test_progs.h | 1 + > 2 files changed, 21 insertions(+) > > diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c > index 1aa5360c427f..36abc3d4a8e2 100644 > --- a/tools/testing/selftests/bpf/test_progs.c > +++ b/tools/testing/selftests/bpf/test_progs.c > @@ -367,6 +367,7 @@ enum ARG_KEYS { > ARG_VERIFIER_STATS = 's', > ARG_VERBOSE = 'v', > ARG_GET_TEST_CNT = 'c', > + ARG_LIST_TEST_NAMES = 'l', > }; > > static const struct argp_option opts[] = { > @@ -382,6 +383,8 @@ static const struct argp_option opts[] = { > "Verbose output (use -vv or -vvv for progressively verbose output)" }, > { "count", ARG_GET_TEST_CNT, NULL, 0, > "Get number of top-level tests " }, > + { "list", ARG_LIST_TEST_NAMES, NULL, 0, > + "List test names that would run (without running them) " }, > {}, > }; > > @@ -517,6 +520,9 @@ static error_t parse_arg(int key, char *arg, struct argp_state *state) > case ARG_GET_TEST_CNT: > env->get_test_cnt = true; > break; > + case ARG_LIST_TEST_NAMES: > + env->list_test_names = true; > + break; > case ARGP_KEY_ARG: > argp_usage(state); > break; > @@ -665,6 +671,12 @@ int main(int argc, char **argv) > test->test_num, test->test_name)) > continue; > > + if (env.list_test_names) { > + fprintf(env.stdout, "%s\n", test->test_name); > + env.succ_cnt++; > + continue; > + } > + > test->run_test(); > /* ensure last sub-test is finalized properly */ > if (test->subtest_name) > @@ -688,9 +700,17 @@ int main(int argc, char **argv) > cleanup_cgroup_environment(); > } > stdio_restore(); > + > + if (env.list_test_names) { > + if (env.succ_cnt == 0) > + env.fail_cnt = 1; > + goto out; > + } > + Why failure if no test matched? Is that to catch bugs in whitelisting? > fprintf(stdout, "Summary: %d/%d PASSED, %d SKIPPED, %d FAILED\n", > env.succ_cnt, env.sub_succ_cnt, env.skip_cnt, env.fail_cnt); > > +out: > free_str_set(&env.test_selector.blacklist); > free_str_set(&env.test_selector.whitelist); > free(env.test_selector.num_set); > diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h > index 0030584619c3..ec31f382e7fd 100644 > --- a/tools/testing/selftests/bpf/test_progs.h > +++ b/tools/testing/selftests/bpf/test_progs.h > @@ -67,6 +67,7 @@ struct test_env { > > bool jit_enabled; > bool get_test_cnt; > + bool list_test_names; > > struct prog_test_def *test; > FILE *stdout; > >
On Tue, 30 Jun 2020 08:46:01 -0700 Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > @@ -688,9 +700,17 @@ int main(int argc, char **argv) > > cleanup_cgroup_environment(); > > } > > stdio_restore(); > > + > > + if (env.list_test_names) { > > + if (env.succ_cnt == 0) > > + env.fail_cnt = 1; > > + goto out; > > + } > > + > > Why failure if no test matched? Is that to catch bugs in whitelisting? I would not call it catch bugs, but sort of. The purpose is to know if requested test is valid. This can be used to e.g. run through all the tests numbers, and stopping when a test number (-n) is no-longer valid, by using this shell exit value as a test, like: n=1; while [ $(./test_progs --list -n $n) ] ; do \ echo "./test_progs -n $n" ; n=$(( n+1 )); \ done Notice that this features that be used for looking up a test number, and returning a testname, which was the original request from CI. I choose this implementation as it more generic and generally useful. $ ./test_progs --list -n 89 xdp_adjust_tail
On Tue, Jun 30, 2020 at 1:32 PM Jesper Dangaard Brouer <brouer@redhat.com> wrote: > > On Tue, 30 Jun 2020 08:46:01 -0700 > Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > > > @@ -688,9 +700,17 @@ int main(int argc, char **argv) > > > cleanup_cgroup_environment(); > > > } > > > stdio_restore(); > > > + > > > + if (env.list_test_names) { > > > + if (env.succ_cnt == 0) > > > + env.fail_cnt = 1; > > > + goto out; > > > + } > > > + > > > > Why failure if no test matched? Is that to catch bugs in whitelisting? > > I would not call it catch bugs, but sort of. The purpose is to know if > requested test is valid. This can be used to e.g. run through all the > tests numbers, and stopping when a test number (-n) is no-longer valid, > by using this shell exit value as a test, like: > > n=1; > while [ $(./test_progs --list -n $n) ] ; do \ > echo "./test_progs -n $n" ; n=$(( n+1 )); \ > done > > Notice that this features that be used for looking up a test number, > and returning a testname, which was the original request from CI. I > choose this implementation as it more generic and generally useful. > > $ ./test_progs --list -n 89 > xdp_adjust_tail > Yeah, it has a nice querying effect. Makes sense. Acked-by: Andrii Nakryiko <andriin@fb.com> > > -- > Best regards, > Jesper Dangaard Brouer > MSc.CS, Principal Kernel Engineer at Red Hat > LinkedIn: http://www.linkedin.com/in/brouer >
On Tue, Jun 30, 2020 at 2:19 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Tue, Jun 30, 2020 at 1:32 PM Jesper Dangaard Brouer > <brouer@redhat.com> wrote: > > > > On Tue, 30 Jun 2020 08:46:01 -0700 > > Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > > > > > @@ -688,9 +700,17 @@ int main(int argc, char **argv) > > > > cleanup_cgroup_environment(); > > > > } > > > > stdio_restore(); > > > > + > > > > + if (env.list_test_names) { > > > > + if (env.succ_cnt == 0) > > > > + env.fail_cnt = 1; > > > > + goto out; > > > > + } > > > > + > > > > > > Why failure if no test matched? Is that to catch bugs in whitelisting? > > > > I would not call it catch bugs, but sort of. The purpose is to know if > > requested test is valid. This can be used to e.g. run through all the > > tests numbers, and stopping when a test number (-n) is no-longer valid, > > by using this shell exit value as a test, like: > > > > n=1; > > while [ $(./test_progs --list -n $n) ] ; do \ > > echo "./test_progs -n $n" ; n=$(( n+1 )); \ > > done > > > > Notice that this features that be used for looking up a test number, > > and returning a testname, which was the original request from CI. I > > choose this implementation as it more generic and generally useful. > > > > $ ./test_progs --list -n 89 > > xdp_adjust_tail > > > > Yeah, it has a nice querying effect. Makes sense. > > Acked-by: Andrii Nakryiko <andriin@fb.com> hmm. it doesn't apply. Applying: selftests/bpf: Test_progs option for listing test names error: sha1 information is lacking or useless (tools/testing/selftests/bpf/test_progs.c). error: could not build fake ancestor Patch failed at 0001 selftests/bpf: Test_progs option for listing test names Could you please respin. Thanks
On Wed, 1 Jul 2020 08:36:08 -0700 Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > On Tue, Jun 30, 2020 at 2:19 PM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > > > On Tue, Jun 30, 2020 at 1:32 PM Jesper Dangaard Brouer > > <brouer@redhat.com> wrote: > > > > > > On Tue, 30 Jun 2020 08:46:01 -0700 > > > Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > > > > > > > @@ -688,9 +700,17 @@ int main(int argc, char **argv) > > > > > cleanup_cgroup_environment(); > > > > > } > > > > > stdio_restore(); > > > > > + > > > > > + if (env.list_test_names) { > > > > > + if (env.succ_cnt == 0) > > > > > + env.fail_cnt = 1; > > > > > + goto out; > > > > > + } > > > > > + > > > > > > > > Why failure if no test matched? Is that to catch bugs in whitelisting? > > > > > > I would not call it catch bugs, but sort of. The purpose is to know if > > > requested test is valid. This can be used to e.g. run through all the > > > tests numbers, and stopping when a test number (-n) is no-longer valid, > > > by using this shell exit value as a test, like: > > > > > > n=1; > > > while [ $(./test_progs --list -n $n) ] ; do \ > > > echo "./test_progs -n $n" ; n=$(( n+1 )); \ > > > done > > > > > > Notice that this features that be used for looking up a test number, > > > and returning a testname, which was the original request from CI. I > > > choose this implementation as it more generic and generally useful. > > > > > > $ ./test_progs --list -n 89 > > > xdp_adjust_tail > > > > > > > Yeah, it has a nice querying effect. Makes sense. > > > > Acked-by: Andrii Nakryiko <andriin@fb.com> > > hmm. it doesn't apply. > Applying: selftests/bpf: Test_progs option for listing test names > error: sha1 information is lacking or useless > (tools/testing/selftests/bpf/test_progs.c). > error: could not build fake ancestor > Patch failed at 0001 selftests/bpf: Test_progs option for listing test names It doesn't apply because it depend on my previous changes, that Daniel said he applied: https://lore.kernel.org/bpf/6e7543fa-f496-a6d2-a6d5-70dff9f84090@iogearbox.net/ But I can see that it is not in the net-next git tree. > Could you please respin. I will respin together with the other unapplied patch. Which is actually fine, as I have an improvement for the previous patch, that I can squash.
On Wed, Jul 1, 2020 at 9:23 AM Jesper Dangaard Brouer <brouer@redhat.com> wrote: > > On Wed, 1 Jul 2020 08:36:08 -0700 > Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > > On Tue, Jun 30, 2020 at 2:19 PM Andrii Nakryiko > > <andrii.nakryiko@gmail.com> wrote: > > > > > > On Tue, Jun 30, 2020 at 1:32 PM Jesper Dangaard Brouer > > > <brouer@redhat.com> wrote: > > > > > > > > On Tue, 30 Jun 2020 08:46:01 -0700 > > > > Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > > > > > > > > > @@ -688,9 +700,17 @@ int main(int argc, char **argv) > > > > > > cleanup_cgroup_environment(); > > > > > > } > > > > > > stdio_restore(); > > > > > > + > > > > > > + if (env.list_test_names) { > > > > > > + if (env.succ_cnt == 0) > > > > > > + env.fail_cnt = 1; > > > > > > + goto out; > > > > > > + } > > > > > > + > > > > > > > > > > Why failure if no test matched? Is that to catch bugs in whitelisting? > > > > > > > > I would not call it catch bugs, but sort of. The purpose is to know if > > > > requested test is valid. This can be used to e.g. run through all the > > > > tests numbers, and stopping when a test number (-n) is no-longer valid, > > > > by using this shell exit value as a test, like: > > > > > > > > n=1; > > > > while [ $(./test_progs --list -n $n) ] ; do \ > > > > echo "./test_progs -n $n" ; n=$(( n+1 )); \ > > > > done > > > > > > > > Notice that this features that be used for looking up a test number, > > > > and returning a testname, which was the original request from CI. I > > > > choose this implementation as it more generic and generally useful. > > > > > > > > $ ./test_progs --list -n 89 > > > > xdp_adjust_tail > > > > > > > > > > Yeah, it has a nice querying effect. Makes sense. > > > > > > Acked-by: Andrii Nakryiko <andriin@fb.com> > > > > hmm. it doesn't apply. > > Applying: selftests/bpf: Test_progs option for listing test names > > error: sha1 information is lacking or useless > > (tools/testing/selftests/bpf/test_progs.c). > > error: could not build fake ancestor > > Patch failed at 0001 selftests/bpf: Test_progs option for listing test names > > It doesn't apply because it depend on my previous changes, that Daniel > said he applied: > > https://lore.kernel.org/bpf/6e7543fa-f496-a6d2-a6d5-70dff9f84090@iogearbox.net/ > > But I can see that it is not in the net-next git tree. oops. sorry about this. I guess to due taking out Jakub's set out of bpf-next and moving into bpf some patches got lost. :( > > Could you please respin. > > I will respin together with the other unapplied patch. Which is > actually fine, as I have an improvement for the previous patch, that I > can squash. Awesome. thanks
diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c index 1aa5360c427f..36abc3d4a8e2 100644 --- a/tools/testing/selftests/bpf/test_progs.c +++ b/tools/testing/selftests/bpf/test_progs.c @@ -367,6 +367,7 @@ enum ARG_KEYS { ARG_VERIFIER_STATS = 's', ARG_VERBOSE = 'v', ARG_GET_TEST_CNT = 'c', + ARG_LIST_TEST_NAMES = 'l', }; static const struct argp_option opts[] = { @@ -382,6 +383,8 @@ static const struct argp_option opts[] = { "Verbose output (use -vv or -vvv for progressively verbose output)" }, { "count", ARG_GET_TEST_CNT, NULL, 0, "Get number of top-level tests " }, + { "list", ARG_LIST_TEST_NAMES, NULL, 0, + "List test names that would run (without running them) " }, {}, }; @@ -517,6 +520,9 @@ static error_t parse_arg(int key, char *arg, struct argp_state *state) case ARG_GET_TEST_CNT: env->get_test_cnt = true; break; + case ARG_LIST_TEST_NAMES: + env->list_test_names = true; + break; case ARGP_KEY_ARG: argp_usage(state); break; @@ -665,6 +671,12 @@ int main(int argc, char **argv) test->test_num, test->test_name)) continue; + if (env.list_test_names) { + fprintf(env.stdout, "%s\n", test->test_name); + env.succ_cnt++; + continue; + } + test->run_test(); /* ensure last sub-test is finalized properly */ if (test->subtest_name) @@ -688,9 +700,17 @@ int main(int argc, char **argv) cleanup_cgroup_environment(); } stdio_restore(); + + if (env.list_test_names) { + if (env.succ_cnt == 0) + env.fail_cnt = 1; + goto out; + } + fprintf(stdout, "Summary: %d/%d PASSED, %d SKIPPED, %d FAILED\n", env.succ_cnt, env.sub_succ_cnt, env.skip_cnt, env.fail_cnt); +out: free_str_set(&env.test_selector.blacklist); free_str_set(&env.test_selector.whitelist); free(env.test_selector.num_set); diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h index 0030584619c3..ec31f382e7fd 100644 --- a/tools/testing/selftests/bpf/test_progs.h +++ b/tools/testing/selftests/bpf/test_progs.h @@ -67,6 +67,7 @@ struct test_env { bool jit_enabled; bool get_test_cnt; + bool list_test_names; struct prog_test_def *test; FILE *stdout;
The program test_progs have some very useful ability to specify a list of test name substrings for selecting which tests to run. This patch add the ability to list the selected test names without running them. This is practical for seeing which tests gets selected with given select arguments (which can also contain a exclude list via --name-blacklist). This output can also be used by shell-scripts in a for-loop: for N in $(./test_progs --list -t xdp); do \ ./test_progs -t $N 2>&1 > result_test_${N}.log & \ done ; wait Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> --- tools/testing/selftests/bpf/test_progs.c | 20 ++++++++++++++++++++ tools/testing/selftests/bpf/test_progs.h | 1 + 2 files changed, 21 insertions(+)