| Message ID | 20210810212107.2237868-4-fallentree@fb.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | selftests/bpf: Improve the usability of test_progs | expand |
On Tue, Aug 10, 2021 at 2:21 PM Yucong Sun <fallentree@fb.com> 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@fb.com> > --- 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, '/'); > [...]
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 +static bool __match_glob(const char *str, const char *pat) +{ + while (*str && *pat && *pat != '*') { + if (*str != *pat) + return false; + str++; + pat++; + } + /* Check wild card */ + if (*pat == '*') { + while (*pat == '*') + pat++; + if (!*pat) /* Tail wild card matches all */ + return true; + while (*str) + if (__match_glob(str++, pat)) + return true; + } + return !*str && !*pat; +} + #define EXIT_NO_TEST 2 #define EXIT_ERR_SETUP_INFRA 3 @@ -55,13 +77,23 @@ static bool should_run(struct test_selector *sel, int num, const char *name) int i; for (i = 0; i < sel->blacklist.cnt; i++) { - if (strstr(name, sel->blacklist.strs[i])) - return false; + if (sel->blacklist.is_glob_pattern) { + if (__match_glob(name, sel->blacklist.strs[i])) + return false; + } else { + if (strstr(name, sel->blacklist.strs[i])) + return false; + } } for (i = 0; i < sel->whitelist.cnt; i++) { - if (strstr(name, sel->whitelist.strs[i])) - return true; + if (sel->whitelist.is_glob_pattern) { + if (__match_glob(name, sel->whitelist.strs[i])) + return true; + } else { + if (strstr(name, sel->whitelist.strs[i])) + return true; + } } if (!sel->whitelist.cnt && !sel->num_set) @@ -450,6 +482,8 @@ enum ARG_KEYS { ARG_VERBOSE = 'v', ARG_GET_TEST_CNT = 'c', ARG_LIST_TEST_NAMES = 'l', + ARG_TEST_NAME_GLOB_ALLOWLIST = 'a', + ARG_TEST_NAME_GLOB_DENYLIST = 'd', }; static const struct argp_option opts[] = { @@ -467,6 +501,10 @@ static const struct argp_option opts[] = { "Get number of selected top-level tests " }, { "list", ARG_LIST_TEST_NAMES, NULL, 0, "List test names that would run (without running them) " }, + { "allow", ARG_TEST_NAME_GLOB_ALLOWLIST, "NAMES", 0, + "Run tests with name matching the pattern (support *)." }, + { "deny", ARG_TEST_NAME_GLOB_DENYLIST, "NAMES", 0, + "Don't run tests with name matching the pattern (support *)." }, {}, }; @@ -491,7 +529,7 @@ static void free_str_set(const struct str_set *set) free(set->strs); } -static int parse_str_list(const char *s, struct str_set *set) +static int parse_str_list(const char *s, struct str_set *set, bool is_glob_pattern) { char *input, *state = NULL, *next, **tmp, **strs = NULL; int cnt = 0; @@ -516,6 +554,7 @@ static int parse_str_list(const char *s, struct str_set *set) cnt++; } + set->is_glob_pattern = is_glob_pattern; set->cnt = cnt; set->strs = (const char **)strs; free(input); @@ -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"); + return -EINVAL; + } char *subtest_str = strchr(arg, '/'); if (subtest_str) { *subtest_str = '\0'; if (parse_str_list(subtest_str + 1, - &env->subtest_selector.whitelist)) + &env->subtest_selector.whitelist, + key == ARG_TEST_NAME_GLOB_ALLOWLIST)) return -ENOMEM; } - if (parse_str_list(arg, &env->test_selector.whitelist)) + if (parse_str_list(arg, &env->test_selector.whitelist, + key == ARG_TEST_NAME_GLOB_ALLOWLIST)) return -ENOMEM; break; } + case ARG_TEST_NAME_GLOB_DENYLIST: case ARG_TEST_NAME_BLACKLIST: { + if (env->test_selector.blacklist.cnt || env->subtest_selector.blacklist.cnt) { + fprintf(stderr, "-d and -b are mutually exclusive, you can only specific one.\n"); + return -EINVAL; + } char *subtest_str = strchr(arg, '/'); if (subtest_str) { *subtest_str = '\0'; if (parse_str_list(subtest_str + 1, - &env->subtest_selector.blacklist)) + &env->subtest_selector.blacklist, + key == ARG_TEST_NAME_GLOB_DENYLIST)) return -ENOMEM; } - if (parse_str_list(arg, &env->test_selector.blacklist)) + if (parse_str_list(arg, &env->test_selector.blacklist, + key == ARG_TEST_NAME_GLOB_DENYLIST)) return -ENOMEM; break; } diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h index c8c2bf878f67..c475d65dce4f 100644 --- a/tools/testing/selftests/bpf/test_progs.h +++ b/tools/testing/selftests/bpf/test_progs.h @@ -49,6 +49,7 @@ enum verbosity { struct str_set { const char **strs; int cnt; + bool is_glob_pattern; }; struct test_selector {
This patch adds '-a' and '-b' arguments, supporting exact string match, as well as using '*' wildchar in test/subtests selection. 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@fb.com> --- tools/testing/selftests/bpf/test_progs.c | 71 +++++++++++++++++++++--- tools/testing/selftests/bpf/test_progs.h | 1 + 2 files changed, 63 insertions(+), 9 deletions(-)