| Message ID | 20190814164742.208909-3-sdf@google.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | selftests/bpf: test_progs: misc fixes | expand |
On Wed, Aug 14, 2019 at 9:48 AM Stanislav Fomichev <sdf@google.com> wrote: > > Export test__skip() to indicate skipped tests and use it in > test_send_signal_nmi(). > > Cc: Andrii Nakryiko <andriin@fb.com> > Signed-off-by: Stanislav Fomichev <sdf@google.com> > --- For completeness, we should probably also support test__skip_subtest() eventually, but it's fine until we don't have a use case. Acked-by: Andrii Nakryiko <andriin@fb.com> > tools/testing/selftests/bpf/prog_tests/send_signal.c | 1 + > tools/testing/selftests/bpf/test_progs.c | 9 +++++++-- > tools/testing/selftests/bpf/test_progs.h | 2 ++ > 3 files changed, 10 insertions(+), 2 deletions(-) > > diff --git a/tools/testing/selftests/bpf/prog_tests/send_signal.c b/tools/testing/selftests/bpf/prog_tests/send_signal.c > index 1575f0a1f586..40c2c5efdd3e 100644 > --- a/tools/testing/selftests/bpf/prog_tests/send_signal.c > +++ b/tools/testing/selftests/bpf/prog_tests/send_signal.c > @@ -204,6 +204,7 @@ static int test_send_signal_nmi(void) > if (errno == ENOENT) { > printf("%s:SKIP:no PERF_COUNT_HW_CPU_CYCLES\n", > __func__); > + test__skip(); > return 0; > } > /* Let the test fail with a more informative message */ > diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c > index 1a7a2a0c0a11..1993f2ce0d23 100644 > --- a/tools/testing/selftests/bpf/test_progs.c > +++ b/tools/testing/selftests/bpf/test_progs.c > @@ -121,6 +121,11 @@ void test__force_log() { > env.test->force_log = true; > } > > +void test__skip(void) > +{ > + env.skip_cnt++; > +} > + > struct ipv4_packet pkt_v4 = { > .eth.h_proto = __bpf_constant_htons(ETH_P_IP), > .iph.ihl = 5, > @@ -535,8 +540,8 @@ int main(int argc, char **argv) > test->test_name); > } > stdio_restore(); > - printf("Summary: %d/%d PASSED, %d FAILED\n", > - env.succ_cnt, env.sub_succ_cnt, env.fail_cnt); > + printf("Summary: %d/%d PASSED, %d SKIPPED, %d FAILED\n", > + env.succ_cnt, env.sub_succ_cnt, env.skip_cnt, env.fail_cnt); > > free(env.test_selector.num_set); > free(env.subtest_selector.num_set); > diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h > index 37d427f5a1e5..9defd35cb6c0 100644 > --- a/tools/testing/selftests/bpf/test_progs.h > +++ b/tools/testing/selftests/bpf/test_progs.h > @@ -64,6 +64,7 @@ struct test_env { > int succ_cnt; /* successful tests */ > int sub_succ_cnt; /* successful sub-tests */ > int fail_cnt; /* total failed tests + sub-tests */ > + int skip_cnt; /* skipped tests */ > }; > > extern int error_cnt; > @@ -72,6 +73,7 @@ extern struct test_env env; > > extern void test__force_log(); > extern bool test__start_subtest(const char *name); > +extern void test__skip(void); > > #define MAGIC_BYTES 123 > > -- > 2.23.0.rc1.153.gdeed80330f-goog >
On Wed, Aug 14, 2019 at 12:22 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Wed, Aug 14, 2019 at 9:48 AM Stanislav Fomichev <sdf@google.com> wrote: > > > > Export test__skip() to indicate skipped tests and use it in > > test_send_signal_nmi(). > > > > Cc: Andrii Nakryiko <andriin@fb.com> > > Signed-off-by: Stanislav Fomichev <sdf@google.com> > > --- > > For completeness, we should probably also support test__skip_subtest() > eventually, but it's fine until we don't have a use case. Hm.. so I think we don't need separate test__skip_subtest(). test__skip() should skip either test or sub-test, depending on which context we are running in. So maybe just make sure this is handled correctly? > > Acked-by: Andrii Nakryiko <andriin@fb.com> > > > tools/testing/selftests/bpf/prog_tests/send_signal.c | 1 + > > tools/testing/selftests/bpf/test_progs.c | 9 +++++++-- > > tools/testing/selftests/bpf/test_progs.h | 2 ++ > > 3 files changed, 10 insertions(+), 2 deletions(-) > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/send_signal.c b/tools/testing/selftests/bpf/prog_tests/send_signal.c > > index 1575f0a1f586..40c2c5efdd3e 100644 > > --- a/tools/testing/selftests/bpf/prog_tests/send_signal.c > > +++ b/tools/testing/selftests/bpf/prog_tests/send_signal.c > > @@ -204,6 +204,7 @@ static int test_send_signal_nmi(void) > > if (errno == ENOENT) { > > printf("%s:SKIP:no PERF_COUNT_HW_CPU_CYCLES\n", > > __func__); > > + test__skip(); > > return 0; > > } > > /* Let the test fail with a more informative message */ > > diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c > > index 1a7a2a0c0a11..1993f2ce0d23 100644 > > --- a/tools/testing/selftests/bpf/test_progs.c > > +++ b/tools/testing/selftests/bpf/test_progs.c > > @@ -121,6 +121,11 @@ void test__force_log() { > > env.test->force_log = true; > > } > > > > +void test__skip(void) > > +{ > > + env.skip_cnt++; > > +} > > + > > struct ipv4_packet pkt_v4 = { > > .eth.h_proto = __bpf_constant_htons(ETH_P_IP), > > .iph.ihl = 5, > > @@ -535,8 +540,8 @@ int main(int argc, char **argv) > > test->test_name); > > } > > stdio_restore(); > > - printf("Summary: %d/%d PASSED, %d FAILED\n", > > - env.succ_cnt, env.sub_succ_cnt, env.fail_cnt); > > + printf("Summary: %d/%d PASSED, %d SKIPPED, %d FAILED\n", So because some sub-tests might be skipped, while others will be running, let's keep output consistent with SUCCESS and use <test>/<subtests> format for SKIPPED as well? > > + env.succ_cnt, env.sub_succ_cnt, env.skip_cnt, env.fail_cnt); > > > > free(env.test_selector.num_set); > > free(env.subtest_selector.num_set); > > diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h > > index 37d427f5a1e5..9defd35cb6c0 100644 > > --- a/tools/testing/selftests/bpf/test_progs.h > > +++ b/tools/testing/selftests/bpf/test_progs.h > > @@ -64,6 +64,7 @@ struct test_env { > > int succ_cnt; /* successful tests */ > > int sub_succ_cnt; /* successful sub-tests */ > > int fail_cnt; /* total failed tests + sub-tests */ > > + int skip_cnt; /* skipped tests */ > > }; > > > > extern int error_cnt; > > @@ -72,6 +73,7 @@ extern struct test_env env; > > > > extern void test__force_log(); > > extern bool test__start_subtest(const char *name); > > +extern void test__skip(void); > > > > #define MAGIC_BYTES 123 > > > > -- > > 2.23.0.rc1.153.gdeed80330f-goog > >
On 08/14, Andrii Nakryiko wrote: > On Wed, Aug 14, 2019 at 12:22 PM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > > > On Wed, Aug 14, 2019 at 9:48 AM Stanislav Fomichev <sdf@google.com> wrote: > > > > > > Export test__skip() to indicate skipped tests and use it in > > > test_send_signal_nmi(). > > > > > > Cc: Andrii Nakryiko <andriin@fb.com> > > > Signed-off-by: Stanislav Fomichev <sdf@google.com> > > > --- > > > > For completeness, we should probably also support test__skip_subtest() > > eventually, but it's fine until we don't have a use case. > > Hm.. so I think we don't need separate test__skip_subtest(). > test__skip() should skip either test or sub-test, depending on which > context we are running in. So maybe just make sure this is handled > correctly? Do we care if it's a test or a subtest skip? My motivation was to have a counter that can be examined to make sure we have a full test coverage, so when people run the tests they can be sure that nothing is skipped due to missing config or something else. Let me know if you see a value in highlighting test vs subtest skip. Other related question is: should we do verbose output in case of a skip? Right now we don't do it. > > > > Acked-by: Andrii Nakryiko <andriin@fb.com> > > > > > tools/testing/selftests/bpf/prog_tests/send_signal.c | 1 + > > > tools/testing/selftests/bpf/test_progs.c | 9 +++++++-- > > > tools/testing/selftests/bpf/test_progs.h | 2 ++ > > > 3 files changed, 10 insertions(+), 2 deletions(-) > > > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/send_signal.c b/tools/testing/selftests/bpf/prog_tests/send_signal.c > > > index 1575f0a1f586..40c2c5efdd3e 100644 > > > --- a/tools/testing/selftests/bpf/prog_tests/send_signal.c > > > +++ b/tools/testing/selftests/bpf/prog_tests/send_signal.c > > > @@ -204,6 +204,7 @@ static int test_send_signal_nmi(void) > > > if (errno == ENOENT) { > > > printf("%s:SKIP:no PERF_COUNT_HW_CPU_CYCLES\n", > > > __func__); > > > + test__skip(); > > > return 0; > > > } > > > /* Let the test fail with a more informative message */ > > > diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c > > > index 1a7a2a0c0a11..1993f2ce0d23 100644 > > > --- a/tools/testing/selftests/bpf/test_progs.c > > > +++ b/tools/testing/selftests/bpf/test_progs.c > > > @@ -121,6 +121,11 @@ void test__force_log() { > > > env.test->force_log = true; > > > } > > > > > > +void test__skip(void) > > > +{ > > > + env.skip_cnt++; > > > +} > > > + > > > struct ipv4_packet pkt_v4 = { > > > .eth.h_proto = __bpf_constant_htons(ETH_P_IP), > > > .iph.ihl = 5, > > > @@ -535,8 +540,8 @@ int main(int argc, char **argv) > > > test->test_name); > > > } > > > stdio_restore(); > > > - printf("Summary: %d/%d PASSED, %d FAILED\n", > > > - env.succ_cnt, env.sub_succ_cnt, env.fail_cnt); > > > + printf("Summary: %d/%d PASSED, %d SKIPPED, %d FAILED\n", > > So because some sub-tests might be skipped, while others will be > running, let's keep output consistent with SUCCESS and use > <test>/<subtests> format for SKIPPED as well? > > > > + env.succ_cnt, env.sub_succ_cnt, env.skip_cnt, env.fail_cnt); > > > > > > free(env.test_selector.num_set); > > > free(env.subtest_selector.num_set); > > > diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h > > > index 37d427f5a1e5..9defd35cb6c0 100644 > > > --- a/tools/testing/selftests/bpf/test_progs.h > > > +++ b/tools/testing/selftests/bpf/test_progs.h > > > @@ -64,6 +64,7 @@ struct test_env { > > > int succ_cnt; /* successful tests */ > > > int sub_succ_cnt; /* successful sub-tests */ > > > int fail_cnt; /* total failed tests + sub-tests */ > > > + int skip_cnt; /* skipped tests */ > > > }; > > > > > > extern int error_cnt; > > > @@ -72,6 +73,7 @@ extern struct test_env env; > > > > > > extern void test__force_log(); > > > extern bool test__start_subtest(const char *name); > > > +extern void test__skip(void); > > > > > > #define MAGIC_BYTES 123 > > > > > > -- > > > 2.23.0.rc1.153.gdeed80330f-goog > > >
On Wed, Aug 14, 2019 at 12:53 PM Stanislav Fomichev <sdf@fomichev.me> wrote: > > On 08/14, Andrii Nakryiko wrote: > > On Wed, Aug 14, 2019 at 12:22 PM Andrii Nakryiko > > <andrii.nakryiko@gmail.com> wrote: > > > > > > On Wed, Aug 14, 2019 at 9:48 AM Stanislav Fomichev <sdf@google.com> wrote: > > > > > > > > Export test__skip() to indicate skipped tests and use it in > > > > test_send_signal_nmi(). > > > > > > > > Cc: Andrii Nakryiko <andriin@fb.com> > > > > Signed-off-by: Stanislav Fomichev <sdf@google.com> > > > > --- > > > > > > For completeness, we should probably also support test__skip_subtest() > > > eventually, but it's fine until we don't have a use case. > > > > Hm.. so I think we don't need separate test__skip_subtest(). > > test__skip() should skip either test or sub-test, depending on which > > context we are running in. So maybe just make sure this is handled > > correctly? > Do we care if it's a test or a subtest skip? My motivation was to > have a counter that can be examined to make sure we have a full test > coverage, so when people run the tests they can be sure that nothing > is skipped due to missing config or something else. I think we do. We might convert, e.g., test_btf to be one big test with lots of sub-tests. Some of those might be legitimately skipped. Having just "1 test skipped" message is not helpful, when there are 170 sub-tests that were not. > > Let me know if you see a value in highlighting test vs subtest skip. > > Other related question is: should we do verbose output in case > of a skip? Right now we don't do it. It might be useful, I guess, especially if it's not too common. But Alexei is way more picky about stuff like that, so I'd defer to him. I have no problem with a clean "SKIPPED: <test>/<subtest> (maybe some reason for skipping here)" message. > > > > > > > Acked-by: Andrii Nakryiko <andriin@fb.com> > > > > > > > tools/testing/selftests/bpf/prog_tests/send_signal.c | 1 + > > > > tools/testing/selftests/bpf/test_progs.c | 9 +++++++-- > > > > tools/testing/selftests/bpf/test_progs.h | 2 ++ > > > > 3 files changed, 10 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/send_signal.c b/tools/testing/selftests/bpf/prog_tests/send_signal.c > > > > index 1575f0a1f586..40c2c5efdd3e 100644 > > > > --- a/tools/testing/selftests/bpf/prog_tests/send_signal.c > > > > +++ b/tools/testing/selftests/bpf/prog_tests/send_signal.c > > > > @@ -204,6 +204,7 @@ static int test_send_signal_nmi(void) > > > > if (errno == ENOENT) { > > > > printf("%s:SKIP:no PERF_COUNT_HW_CPU_CYCLES\n", > > > > __func__); > > > > + test__skip(); > > > > return 0; > > > > } > > > > /* Let the test fail with a more informative message */ > > > > diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c > > > > index 1a7a2a0c0a11..1993f2ce0d23 100644 > > > > --- a/tools/testing/selftests/bpf/test_progs.c > > > > +++ b/tools/testing/selftests/bpf/test_progs.c > > > > @@ -121,6 +121,11 @@ void test__force_log() { > > > > env.test->force_log = true; > > > > } > > > > > > > > +void test__skip(void) > > > > +{ > > > > + env.skip_cnt++; > > > > +} > > > > + > > > > struct ipv4_packet pkt_v4 = { > > > > .eth.h_proto = __bpf_constant_htons(ETH_P_IP), > > > > .iph.ihl = 5, > > > > @@ -535,8 +540,8 @@ int main(int argc, char **argv) > > > > test->test_name); > > > > } > > > > stdio_restore(); > > > > - printf("Summary: %d/%d PASSED, %d FAILED\n", > > > > - env.succ_cnt, env.sub_succ_cnt, env.fail_cnt); > > > > + printf("Summary: %d/%d PASSED, %d SKIPPED, %d FAILED\n", > > > > So because some sub-tests might be skipped, while others will be > > running, let's keep output consistent with SUCCESS and use > > <test>/<subtests> format for SKIPPED as well? > > > > > > + env.succ_cnt, env.sub_succ_cnt, env.skip_cnt, env.fail_cnt); > > > > > > > > free(env.test_selector.num_set); > > > > free(env.subtest_selector.num_set); > > > > diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h > > > > index 37d427f5a1e5..9defd35cb6c0 100644 > > > > --- a/tools/testing/selftests/bpf/test_progs.h > > > > +++ b/tools/testing/selftests/bpf/test_progs.h > > > > @@ -64,6 +64,7 @@ struct test_env { > > > > int succ_cnt; /* successful tests */ > > > > int sub_succ_cnt; /* successful sub-tests */ > > > > int fail_cnt; /* total failed tests + sub-tests */ > > > > + int skip_cnt; /* skipped tests */ > > > > }; > > > > > > > > extern int error_cnt; > > > > @@ -72,6 +73,7 @@ extern struct test_env env; > > > > > > > > extern void test__force_log(); > > > > extern bool test__start_subtest(const char *name); > > > > +extern void test__skip(void); > > > > > > > > #define MAGIC_BYTES 123 > > > > > > > > -- > > > > 2.23.0.rc1.153.gdeed80330f-goog > > > >
On Wed, Aug 14, 2019 at 1:01 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > > > Let me know if you see a value in highlighting test vs subtest skip. > > > > Other related question is: should we do verbose output in case > > of a skip? Right now we don't do it. > > It might be useful, I guess, especially if it's not too common. But > Alexei is way more picky about stuff like that, so I'd defer to him. I > have no problem with a clean "SKIPPED: <test>/<subtest> (maybe some > reason for skipping here)" message. Since test_progs prints single number for FAILED tests then single number for SKIPPED tests is fine as well.
On Thu, Aug 15, 2019 at 10:16 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Wed, Aug 14, 2019 at 1:01 PM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > > > > > Let me know if you see a value in highlighting test vs subtest skip. > > > > > > Other related question is: should we do verbose output in case > > > of a skip? Right now we don't do it. > > > > It might be useful, I guess, especially if it's not too common. But > > Alexei is way more picky about stuff like that, so I'd defer to him. I > > have no problem with a clean "SKIPPED: <test>/<subtest> (maybe some > > reason for skipping here)" message. > > Since test_progs prints single number for FAILED tests then single number > for SKIPPED tests is fine as well. I'm fine with single number, but it should count number of subtests skipped, if there are subtests within test, same as for FAILED.
diff --git a/tools/testing/selftests/bpf/prog_tests/send_signal.c b/tools/testing/selftests/bpf/prog_tests/send_signal.c index 1575f0a1f586..40c2c5efdd3e 100644 --- a/tools/testing/selftests/bpf/prog_tests/send_signal.c +++ b/tools/testing/selftests/bpf/prog_tests/send_signal.c @@ -204,6 +204,7 @@ static int test_send_signal_nmi(void) if (errno == ENOENT) { printf("%s:SKIP:no PERF_COUNT_HW_CPU_CYCLES\n", __func__); + test__skip(); return 0; } /* Let the test fail with a more informative message */ diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c index 1a7a2a0c0a11..1993f2ce0d23 100644 --- a/tools/testing/selftests/bpf/test_progs.c +++ b/tools/testing/selftests/bpf/test_progs.c @@ -121,6 +121,11 @@ void test__force_log() { env.test->force_log = true; } +void test__skip(void) +{ + env.skip_cnt++; +} + struct ipv4_packet pkt_v4 = { .eth.h_proto = __bpf_constant_htons(ETH_P_IP), .iph.ihl = 5, @@ -535,8 +540,8 @@ int main(int argc, char **argv) test->test_name); } stdio_restore(); - printf("Summary: %d/%d PASSED, %d FAILED\n", - env.succ_cnt, env.sub_succ_cnt, env.fail_cnt); + printf("Summary: %d/%d PASSED, %d SKIPPED, %d FAILED\n", + env.succ_cnt, env.sub_succ_cnt, env.skip_cnt, env.fail_cnt); free(env.test_selector.num_set); free(env.subtest_selector.num_set); diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h index 37d427f5a1e5..9defd35cb6c0 100644 --- a/tools/testing/selftests/bpf/test_progs.h +++ b/tools/testing/selftests/bpf/test_progs.h @@ -64,6 +64,7 @@ struct test_env { int succ_cnt; /* successful tests */ int sub_succ_cnt; /* successful sub-tests */ int fail_cnt; /* total failed tests + sub-tests */ + int skip_cnt; /* skipped tests */ }; extern int error_cnt; @@ -72,6 +73,7 @@ extern struct test_env env; extern void test__force_log(); extern bool test__start_subtest(const char *name); +extern void test__skip(void); #define MAGIC_BYTES 123
Export test__skip() to indicate skipped tests and use it in test_send_signal_nmi(). Cc: Andrii Nakryiko <andriin@fb.com> Signed-off-by: Stanislav Fomichev <sdf@google.com> --- tools/testing/selftests/bpf/prog_tests/send_signal.c | 1 + tools/testing/selftests/bpf/test_progs.c | 9 +++++++-- tools/testing/selftests/bpf/test_progs.h | 2 ++ 3 files changed, 10 insertions(+), 2 deletions(-)