Thanks for the super quick review Andrii!
> On Apr 5, 2022, at 4:14 PM, Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Tue, Apr 5, 2022 at 1:45 PM Mykola Lysenko <mykolal@xxxxxx> wrote:
>>
>> Improve subtest selection logic when using -t/-a/-d parameters.
>> In particular, more than one subtest can be specified or a
>> combination of tests / subtests.
>>
>> -a send_signal -d send_signal/send_signal_nmi* - runs send_signal
>> test without nmi tests
>>
>> -a send_signal/send_signal_nmi*,find_vma - runs two send_signal
>> subtests and find_vma test
>>
>
> Only somewhat related, but while we are at this topic. Can you please
> check whether equivalent approach works:
>
> -a send_signal/send_signal_nmi* -a find_vma
>
> i.e., multi -a/-d/-t/-b are concatenating their selectors.
Yes, these options can be combined. I will add a test for the sequence of parse_test_list calls in v2.
>
>> This will allow us to have granular control over which subtests
>> to disable in the CI system instead of disabling whole tests.
>>
>> Also, add new selftest to avoid possible regression when
>> changing prog_test test name selection logic.
>>
>> Signed-off-by: Mykola Lysenko <mykolal@xxxxxx>
>> ---
>> .../selftests/bpf/prog_tests/arg_parsing.c | 88 ++++++++++
>> tools/testing/selftests/bpf/test_progs.c | 157 +++++++++---------
>> tools/testing/selftests/bpf/test_progs.h | 16 +-
>> tools/testing/selftests/bpf/testing_helpers.c | 87 ++++++++++
>> tools/testing/selftests/bpf/testing_helpers.h | 6 +
>> 5 files changed, 266 insertions(+), 88 deletions(-)
>> create mode 100644 tools/testing/selftests/bpf/prog_tests/arg_parsing.c
>>
>
> [...]
>
>> +static void test_parse_test_list(void)
>> +{
>> + struct test_set test_set;
>> +
>> + init_test_set(&test_set);
>> +
>> + parse_test_list("arg_parsing", &test_set, true);
>
> ASSER_OK() return value?
Will fix
>
>> + if (CHECK(test_set.cnt != 1, "parse_test_list subtest argument", "Unexpected number of tests in num table %d\n", test_set.cnt))
>
> please don't use CHECK()s in new tests, we only add ASSERT_xxx() now.
> Also line length should be under 100 characters (except for the case
> when we have a long string literal, we don't break string literals, no
> matter how long).
Will fix
>
>> + goto error;
>> + if (!ASSERT_OK_PTR(test_set.tests, "tests__initialized"))
>> + goto error;
>> + if (CHECK(!test_set.tests[0].whole_test, "parse_test_list subtest argument", "Expected test 0 to be initialized"))
>> + goto error;
>> + if (CHECK(strcmp("arg_parsing", test_set.tests[0].name), "parse_test_list subtest argument", "Expected test 0 to be initialized"))
>> + goto error;
>> + free_test_set(&test_set);
>> +
>> + parse_test_list("arg_parsing,bpf_cookie", &test_set, true);
>> + if (CHECK(test_set.cnt != 2, "parse_test_list subtest argument", "Unexpected number of tests in num table %d\n", test_set.cnt))
>> + goto error;
>> + if (!ASSERT_OK_PTR(test_set.tests, "tests__initialized"))
>> + goto error;
>> + if (CHECK(!test_set.tests[0].whole_test, "parse_test_list subtest argument", "Expected test 0 to be fully runnable"))
>> + goto error;
>> + if (CHECK(!test_set.tests[1].whole_test, "parse_test_list subtest argument", "Expected test 1 to be fully runnable"))
>> + goto error;
>
> nit: I think there is no need to goto error after each check. We need
> goto error if subsequent checks can crash due to some invalid state.
> In this case, validating the value of few independent values is ok to
> always check without goto error jumps. Makes tests shorter and
> simpler.
Will fix
>
>
>> + if (CHECK(strcmp("arg_parsing", test_set.tests[0].name), "parse_test_list subtest argument", "Expected test 0 to be arg_parsing"))
>> + goto error;
>> + if (CHECK(strcmp("bpf_cookie", test_set.tests[1].name), "parse_test_list subtest argument", "Expected test 1 to be bpf_cookie"))
>> + goto error;
>> + free_test_set(&test_set);
>> +
>> + parse_test_list("arg_parsing/test_parse_test_list,bpf_cookie", &test_set, true);
>> + if (CHECK(test_set.cnt != 2, "parse_test_list subtest argument", "Unexpected number of tests in num table %d\n", test_set.cnt))
>> + goto error;
>
> [...]
>
>> diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
>> index 0a4b45d7b515..671e37cada4b 100644
>> --- a/tools/testing/selftests/bpf/test_progs.c
>> +++ b/tools/testing/selftests/bpf/test_progs.c
>> @@ -3,6 +3,7 @@
>> */
>> #define _GNU_SOURCE
>> #include "test_progs.h"
>> +#include "testing_helpers.h"
>> #include "cgroup_helpers.h"
>> #include <argp.h>
>> #include <pthread.h>
>> @@ -82,14 +83,14 @@ int usleep(useconds_t usec)
>> static bool should_run(struct test_selector *sel, int num, const char *name)
>> {
>> int i;
>> -
>
> keep empty line between variable declaration block and first statement
Will fix, curious though why it is not caught by the script. Probably because this line is not modified
>
>> for (i = 0; i < sel->blacklist.cnt; i++) {
>> - if (glob_match(name, sel->blacklist.strs[i]))
>> + if (glob_match(name, sel->blacklist.tests[i].name) &&
>> + sel->blacklist.tests[i].whole_test)
>> return false;
>> }
>>
>> for (i = 0; i < sel->whitelist.cnt; i++) {
>> - if (glob_match(name, sel->whitelist.strs[i]))
>> + if (glob_match(name, sel->whitelist.tests[i].name))
>> return true;
>> }
>>
>
> [...]
>
>> -bool test__start_subtest(const char *name)
>> +bool test__start_subtest(const char *subtest_name)
>> {
>> struct prog_test_def *test = env.test;
>>
>> @@ -205,17 +246,21 @@ bool test__start_subtest(const char *name)
>>
>> test->subtest_num++;
>>
>> - if (!name || !name[0]) {
>> + if (!subtest_name || !subtest_name[0]) {
>> fprintf(env.stderr,
>> "Subtest #%d didn't provide sub-test name!\n",
>> test->subtest_num);
>> return false;
>> }
>>
>> - if (!should_run(&env.subtest_selector, test->subtest_num, name))
>> + if (!should_run_subtest(&env.test_selector,
>> + &env.subtest_selector,
>
> we don't have subtest_selector anymore, do we?
Unfortunately, we do use subtest selector for -n option. I do have a change for -n option, but I want to review it separately. Otherwise it makes the change way to big
>
> also, do you think that maybe combining should_run and
> should_rub_subtest would be a good idea? You can distinguish by having
> subtest_name NULL when you need to check only test?
Similar to above, because of -n option, combining two makes logic too convoluted. Happy to address it as a separate change once we figure out -n option behavior
>
>> + test->subtest_num,
>> + test->test_name,
>> + subtest_name))
>> return false;
>>
>> - test->subtest_name = strdup(name);
>> + test->subtest_name = strdup(subtest_name);
>> if (!test->subtest_name) {
>> fprintf(env.stderr,
>> "Subtest #%d: failed to copy subtest name!\n",
>> @@ -527,63 +572,29 @@ static int libbpf_print_fn(enum libbpf_print_level level,
>> return 0;
>> }
>>
>
> [...]
>
>> diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
>> index eec4c7385b14..6a465a98341e 100644
>> --- a/tools/testing/selftests/bpf/test_progs.h
>> +++ b/tools/testing/selftests/bpf/test_progs.h
>> @@ -37,7 +37,6 @@ typedef __u16 __sum16;
>> #include <bpf/bpf_endian.h>
>> #include "trace_helpers.h"
>> #include "testing_helpers.h"
>> -#include "flow_dissector_load.h"
>>
>> enum verbosity {
>> VERBOSE_NONE,
>> @@ -46,14 +45,21 @@ enum verbosity {
>> VERBOSE_SUPER,
>> };
>>
>> -struct str_set {
>> - const char **strs;
>> +struct prog_test {
>> + char *name;
>> + char **subtests;
>> + int subtest_cnt;
>> + bool whole_test;
>> +};
>> +
>> +struct test_set {
>> + struct prog_test *tests;
>
> prog_test is a bad name, IMO. it's a "test filter", right? Let's call it that?
>
> test_set isn't most accurate as well, maybe test_filter_set or test_set_filter?
Will change
>
>> int cnt;
>> };
>>
>> struct test_selector {
>> - struct str_set whitelist;
>> - struct str_set blacklist;
>> + struct test_set whitelist;
>> + struct test_set blacklist;
>> bool *num_set;
>> int num_set_len;
>> };
>> diff --git a/tools/testing/selftests/bpf/testing_helpers.c b/tools/testing/selftests/bpf/testing_helpers.c
>> index 795b6798ccee..d2160d2a1303 100644
>> --- a/tools/testing/selftests/bpf/testing_helpers.c
>> +++ b/tools/testing/selftests/bpf/testing_helpers.c
>> @@ -6,6 +6,7 @@
>> #include <errno.h>
>> #include <bpf/bpf.h>
>> #include <bpf/libbpf.h>
>> +#include "test_progs.h"
>> #include "testing_helpers.h"
>>
>> int parse_num_list(const char *s, bool **num_set, int *num_set_len)
>> @@ -69,6 +70,92 @@ int parse_num_list(const char *s, bool **num_set, int *num_set_len)
>> return 0;
>> }
>>
>> +int parse_test_list(const char *s,
>> + struct test_set *test_set,
>> + bool is_glob_pattern)
>> +{
>> + char *input, *state = NULL, *next;
>> + struct prog_test *tmp, *tests = NULL;
>> + int i, j, cnt = 0;
>> +
>> + input = strdup(s);
>> + if (!input)
>> + return -ENOMEM;
>> +
>> + while ((next = strtok_r(state ? NULL : input, ",", &state))) {
>> + char *subtest_str = strchr(next, '/');
>> + char *pattern = NULL;
>> +
>> + tmp = realloc(tests, sizeof(*tests) * (cnt + 1));
>> + if (!tmp)
>> + goto err;
>> + tests = tmp;
>> +
>> + tests[cnt].subtest_cnt = 0;
>> + tests[cnt].subtests = NULL;
>> + tests[cnt].whole_test = false;
>> +
>> + if (subtest_str) {
>> + char **tmp_subtests = NULL;
>
> need an empty line between variable declarations and statements
Will fix
>
>> + *subtest_str = '\0';
>> + int subtest_cnt = tests[cnt].subtest_cnt;
>> +
>> + tmp_subtests = realloc(tests[cnt].subtests,
>> + sizeof(*tmp_subtests) *
>> + (subtest_cnt + 1));
>> + if (!tmp_subtests)
>> + goto err;
>> + tests[cnt].subtests = tmp_subtests;
>> +
>> + tests[cnt].subtests[subtest_cnt] = strdup(subtest_str + 1);
>> + if (!tests[cnt].subtests[subtest_cnt])
>> + goto err;
>> +
>> + tests[cnt].subtest_cnt++;
>> + } else {
>> + tests[cnt].whole_test = true;
>
> isn't whole_test equivalent to subtest_cnt > 0? Why keeping extra bool
> of state then?
Will fix. Overthought it here a bit
>
>> + }
>> +
>> + if (is_glob_pattern) {
>> + pattern = "%s";
>> + tests[cnt].name = malloc(strlen(next) + 1);
>> + } else {
>> + pattern = "*%s*";
>> + tests[cnt].name = malloc(strlen(next) + 2 + 1);
>> + }
>> +
>> + if (!tests[cnt].name)
>> + goto err;
>> +
>> + sprintf(tests[cnt].name, pattern, next);
>> +
>> + cnt++;
>> + }
>> +
>> + tmp = realloc(test_set->tests, sizeof(*tests) * (cnt + test_set->cnt));
>> + if (!tmp)
>> + goto err;
>> +
>> + memcpy(tmp + test_set->cnt, tests, sizeof(*tests) * cnt);
>> + test_set->tests = tmp;
>> + test_set->cnt += cnt;
>> +
>> + free(tests);
>> + free(input);
>> + return 0;
>> +
>> +err:
>> + for (i = 0; i < cnt; i++) {
>> + for (j = 0; j < tests[i].subtest_cnt; j++)
>> + free(tests[i].subtests[j]);
>> +
>> + free(tests[i].name);
>> + }
>> + free(tests);
>> + free(input);
>> + return -ENOMEM;
>> +}
>> +
>> __u32 link_info_prog_id(const struct bpf_link *link, struct bpf_link_info *info)
>> {
>> __u32 info_len = sizeof(*info);
>> diff --git a/tools/testing/selftests/bpf/testing_helpers.h b/tools/testing/selftests/bpf/testing_helpers.h
>> index f46ebc476ee8..d2f502184cd1 100644
>> --- a/tools/testing/selftests/bpf/testing_helpers.h
>> +++ b/tools/testing/selftests/bpf/testing_helpers.h
>> @@ -12,3 +12,9 @@ int bpf_test_load_program(enum bpf_prog_type type, const struct bpf_insn *insns,
>> size_t insns_cnt, const char *license,
>> __u32 kern_version, char *log_buf,
>> size_t log_buf_sz);
>> +
>> +/*
>> + * below function is exported for testing in prog_test test
>> + */
>> +struct test_set;
>> +int parse_test_list(const char *s, struct test_set *test_set, bool is_glob_pattern);
>> --
>> 2.30.2
