On Tue, Aug 10, 2021 at 2:21 PM Yucong Sun <fallentree@xxxxxx> wrote: > > This patch adds '-a' and '-b' arguments, supporting exact string match, as well typo: you probably meant '-d' > as using '*' wildchar in test/subtests selection. typo: wildcard > > Caveat: As same as the current substring matching mechanism, test and subtest > selector applies independently, 'a*/b*' will execute all tests matches "a*", > and with subtest name matches "b*", but tests matches "a*" but has no subtests > will also be executed. > > Signed-off-by: Yucong Sun <fallentree@xxxxxx> > --- This looks good, see proposal at the end to simplify internals by always using glob form. But there is another problem with test/subtest selection. While you are looking at this topic, maybe you can deal with that as well? Basically: [vmuser@archvm bpf]$ sudo ./test_progs -t core_reloc/arrays -t core_reloc/enumval #32/68 enumval:OK #32/69 enumval___diff:OK #32/70 enumval___val3_missing:OK #32/71 enumval___err_missing:OK #32 core_reloc:OK Summary: 1/4 PASSED, 0 SKIPPED, 0 FAILED [vmuser@archvm bpf]$ sudo ./test_progs -t core_reloc/arrays,core_reloc/enumval #32/19 arrays:OK #32/20 arrays___diff_arr_dim:OK #32/21 arrays___diff_arr_val_sz:OK #32/22 arrays___equiv_zero_sz_arr:OK #32/23 arrays___fixed_arr:OK #32/24 arrays___err_too_small:OK #32/25 arrays___err_too_shallow:OK #32/26 arrays___err_non_array:OK #32/27 arrays___err_wrong_val_type:OK #32/28 arrays___err_bad_zero_sz_arr:OK #32 core_reloc:OK Summary: 1/10 PASSED, 0 SKIPPED, 0 FAILED The intent was to run two sets of subtests, but we get either one or another... For test selection it's a bit better. sudo ./test_progs -t core_reloc,core_extern will run both tests, but ./test_progs -t core_reloc and -t core_extern will run just core_extern. This is not a theoretical problem, I ran into these limitations when trying to disable only some of the subtests on older kernels (see [0]). It would be great to support both -t abc -t def to be interpreted as -t abc,def (concatenation of specifications), and fix multiple specifiers with subtests. It's actually ambiguous, because -t foo/bar,baz can be interpreted as either "test foo, subtests bar and baz" (i.e., -t foo/bar,foo/baz) or "test foo and subtest bar within it, and separately test baz" (so, -t foo/bar -t baz). I think the second one is less surprising and it still allows to specify multiple subtests with more verbose form, if necessary. See if you can tackle that, but it doesn't have to be in the same patch set. [0] https://github.com/libbpf/libbpf/blob/master/travis-ci/vmtest/configs/blacklist/BLACKLIST-5.5.0#L106-L108 > tools/testing/selftests/bpf/test_progs.c | 71 +++++++++++++++++++++--- > tools/testing/selftests/bpf/test_progs.h | 1 + > 2 files changed, 63 insertions(+), 9 deletions(-) > > diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c > index f0fbead40883..af43e206a806 100644 > --- a/tools/testing/selftests/bpf/test_progs.c > +++ b/tools/testing/selftests/bpf/test_progs.c > @@ -13,6 +13,28 @@ > #include <execinfo.h> /* backtrace */ > #include <linux/membarrier.h> > > +// Adapted from perf/util/string.c no C++ comments, please > +static bool __match_glob(const char *str, const char *pat) we (almost) never use __funcname in selftest, is there anything wrong with just "match_glob"? Better still "matches_glob" reads more meaningfully. > +{ > + while (*str && *pat && *pat != '*') { > + if (*str != *pat) > + return false; > + str++; > + pat++; > + } [...] > @@ -553,29 +592,43 @@ static error_t parse_arg(int key, char *arg, struct argp_state *state) > } > break; > } > + case ARG_TEST_NAME_GLOB_ALLOWLIST: > case ARG_TEST_NAME: { > + if (env->test_selector.whitelist.cnt || env->subtest_selector.whitelist.cnt) { > + fprintf(stderr, "-a and -t are mutually exclusive, you can only specific one.\n"); typo: specify but also, it just occurred to me. Isn't `-t whatever` same as `-a '*whatever*'`? We can do that transparently and make -a and -t co-exist nicely. Basically, let's do globs only internally. > + return -EINVAL; > + } > char *subtest_str = strchr(arg, '/'); > [...]
