| Message ID | 20190904162509.199561-1-sdf@google.com |
|---|---|
| Headers | show |
| Series | selftests/bpf: move sockopt tests under test_progs | expand |
On Wed, Sep 04, 2019 at 09:25:03AM -0700, Stanislav Fomichev wrote: > Now that test_progs is shaping into more generic test framework, > let's convert sockopt tests to it. This requires adding > a helper to create and join a cgroup first (test__join_cgroup). > Since we already hijack stdout/stderr that shouldn't be > a problem (cgroup helpers log to stderr). > > The rest of the patches just move sockopt tests files under prog_tests/ > and do the required small adjustments. Looks good. Thank you for working on it. Could you de-verbose setsockopt test a bit? #23/32 setsockopt: deny write ctx->retval:OK #23/33 setsockopt: deny read ctx->retval:OK #23/34 setsockopt: deny writing to ctx->optval:OK #23/35 setsockopt: deny writing to ctx->optval_end:OK #23/36 setsockopt: allow IP_TOS <= 128:OK #23/37 setsockopt: deny IP_TOS > 128:OK 37 subtests is a bit too much spam.
On Wed, Sep 4, 2019 at 4:03 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Wed, Sep 04, 2019 at 09:25:03AM -0700, Stanislav Fomichev wrote: > > Now that test_progs is shaping into more generic test framework, > > let's convert sockopt tests to it. This requires adding > > a helper to create and join a cgroup first (test__join_cgroup). > > Since we already hijack stdout/stderr that shouldn't be > > a problem (cgroup helpers log to stderr). > > > > The rest of the patches just move sockopt tests files under prog_tests/ > > and do the required small adjustments. > > Looks good. Thank you for working on it. > Could you de-verbose setsockopt test a bit? > #23/32 setsockopt: deny write ctx->retval:OK > #23/33 setsockopt: deny read ctx->retval:OK > #23/34 setsockopt: deny writing to ctx->optval:OK > #23/35 setsockopt: deny writing to ctx->optval_end:OK > #23/36 setsockopt: allow IP_TOS <= 128:OK > #23/37 setsockopt: deny IP_TOS > 128:OK > 37 subtests is a bit too much spam. If we merged test_btf into test_progs, we'd have >150 subtests, which would be pretty verbose as well. But the benefit of subtest is that you can run just that sub-test and debug/verify just it, without all the rest stuff. So I'm wondering, if too many lines of default output is the only problem, should we just not output per-subtest line by default? >
On 09/06, Andrii Nakryiko wrote: > On Wed, Sep 4, 2019 at 4:03 PM Alexei Starovoitov > <alexei.starovoitov@gmail.com> wrote: > > > > On Wed, Sep 04, 2019 at 09:25:03AM -0700, Stanislav Fomichev wrote: > > > Now that test_progs is shaping into more generic test framework, > > > let's convert sockopt tests to it. This requires adding > > > a helper to create and join a cgroup first (test__join_cgroup). > > > Since we already hijack stdout/stderr that shouldn't be > > > a problem (cgroup helpers log to stderr). > > > > > > The rest of the patches just move sockopt tests files under prog_tests/ > > > and do the required small adjustments. > > > > Looks good. Thank you for working on it. > > Could you de-verbose setsockopt test a bit? > > #23/32 setsockopt: deny write ctx->retval:OK > > #23/33 setsockopt: deny read ctx->retval:OK > > #23/34 setsockopt: deny writing to ctx->optval:OK > > #23/35 setsockopt: deny writing to ctx->optval_end:OK > > #23/36 setsockopt: allow IP_TOS <= 128:OK > > #23/37 setsockopt: deny IP_TOS > 128:OK > > 37 subtests is a bit too much spam. > > If we merged test_btf into test_progs, we'd have >150 subtests, which > would be pretty verbose as well. But the benefit of subtest is that > you can run just that sub-test and debug/verify just it, without all > the rest stuff. > > So I'm wondering, if too many lines of default output is the only > problem, should we just not output per-subtest line by default? Ack, we can output per-subtest line if it fails so it's easy to re-run; otherwise, hiding by default sounds good. I'll prepare a v3 sometime today; Alexei, let us know if you disagree.
On Fri, Sep 06, 2019 at 08:18:08AM -0700, Stanislav Fomichev wrote: > On 09/06, Andrii Nakryiko wrote: > > On Wed, Sep 4, 2019 at 4:03 PM Alexei Starovoitov > > <alexei.starovoitov@gmail.com> wrote: > > > > > > On Wed, Sep 04, 2019 at 09:25:03AM -0700, Stanislav Fomichev wrote: > > > > Now that test_progs is shaping into more generic test framework, > > > > let's convert sockopt tests to it. This requires adding > > > > a helper to create and join a cgroup first (test__join_cgroup). > > > > Since we already hijack stdout/stderr that shouldn't be > > > > a problem (cgroup helpers log to stderr). > > > > > > > > The rest of the patches just move sockopt tests files under prog_tests/ > > > > and do the required small adjustments. > > > > > > Looks good. Thank you for working on it. > > > Could you de-verbose setsockopt test a bit? > > > #23/32 setsockopt: deny write ctx->retval:OK > > > #23/33 setsockopt: deny read ctx->retval:OK > > > #23/34 setsockopt: deny writing to ctx->optval:OK > > > #23/35 setsockopt: deny writing to ctx->optval_end:OK > > > #23/36 setsockopt: allow IP_TOS <= 128:OK > > > #23/37 setsockopt: deny IP_TOS > 128:OK > > > 37 subtests is a bit too much spam. > > > > If we merged test_btf into test_progs, we'd have >150 subtests, which > > would be pretty verbose as well. But the benefit of subtest is that > > you can run just that sub-test and debug/verify just it, without all > > the rest stuff. > > > > So I'm wondering, if too many lines of default output is the only > > problem, should we just not output per-subtest line by default? > Ack, we can output per-subtest line if it fails so it's easy to re-run; > otherwise, hiding by default sounds good. I'll prepare a v3 sometime > today; Alexei, let us know if you disagree. If the subtests are runnable and useful individually it's good to have them as subtests. I think in the above I misread them as a sequence of sub-checks that needs to happen before actual test result. Looking at test_sockopt.c I see that they're separate tests, so yeah keep them. No need to hide by default or extra flags. Let me look at v1 and v2 again...
On Fri, Sep 6, 2019 at 9:42 AM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Fri, Sep 06, 2019 at 08:18:08AM -0700, Stanislav Fomichev wrote: > > On 09/06, Andrii Nakryiko wrote: > > > On Wed, Sep 4, 2019 at 4:03 PM Alexei Starovoitov > > > <alexei.starovoitov@gmail.com> wrote: > > > > > > > > On Wed, Sep 04, 2019 at 09:25:03AM -0700, Stanislav Fomichev wrote: > > > > > Now that test_progs is shaping into more generic test framework, > > > > > let's convert sockopt tests to it. This requires adding > > > > > a helper to create and join a cgroup first (test__join_cgroup). > > > > > Since we already hijack stdout/stderr that shouldn't be > > > > > a problem (cgroup helpers log to stderr). > > > > > > > > > > The rest of the patches just move sockopt tests files under prog_tests/ > > > > > and do the required small adjustments. > > > > > > > > Looks good. Thank you for working on it. > > > > Could you de-verbose setsockopt test a bit? > > > > #23/32 setsockopt: deny write ctx->retval:OK > > > > #23/33 setsockopt: deny read ctx->retval:OK > > > > #23/34 setsockopt: deny writing to ctx->optval:OK > > > > #23/35 setsockopt: deny writing to ctx->optval_end:OK > > > > #23/36 setsockopt: allow IP_TOS <= 128:OK > > > > #23/37 setsockopt: deny IP_TOS > 128:OK > > > > 37 subtests is a bit too much spam. > > > > > > If we merged test_btf into test_progs, we'd have >150 subtests, which > > > would be pretty verbose as well. But the benefit of subtest is that > > > you can run just that sub-test and debug/verify just it, without all > > > the rest stuff. > > > > > > So I'm wondering, if too many lines of default output is the only > > > problem, should we just not output per-subtest line by default? > > Ack, we can output per-subtest line if it fails so it's easy to re-run; > > otherwise, hiding by default sounds good. I'll prepare a v3 sometime > > today; Alexei, let us know if you disagree. > > If the subtests are runnable and useful individually it's good to have > them as subtests. > I think in the above I misread them as a sequence of sub-checks that needs > to happen before actual test result. > Looking at test_sockopt.c I see that they're separate tests, > so yeah keep them. > No need to hide by default or extra flags. > Let me look at v1 and v2 again... I applied v1 set. Thanks!
Now that test_progs is shaping into more generic test framework, let's convert sockopt tests to it. This requires adding a helper to create and join a cgroup first (test__join_cgroup). Since we already hijack stdout/stderr that shouldn't be a problem (cgroup helpers log to stderr). The rest of the patches just move sockopt tests files under prog_tests/ and do the required small adjustments. Stanislav Fomichev (6): selftests/bpf: test_progs: add test__join_cgroup helper selftests/bpf: test_progs: convert test_sockopt selftests/bpf: test_progs: convert test_sockopt_sk selftests/bpf: test_progs: convert test_sockopt_multi selftests/bpf: test_progs: convert test_sockopt_inherit selftests/bpf: test_progs: convert test_tcp_rtt tools/testing/selftests/bpf/.gitignore | 5 - tools/testing/selftests/bpf/Makefile | 12 +-- .../{test_sockopt.c => prog_tests/sockopt.c} | 50 ++------- .../sockopt_inherit.c} | 102 ++++++++---------- .../sockopt_multi.c} | 62 ++--------- .../sockopt_sk.c} | 60 +++-------- .../{test_tcp_rtt.c => prog_tests/tcp_rtt.c} | 83 +++++--------- tools/testing/selftests/bpf/test_progs.c | 38 +++++++ tools/testing/selftests/bpf/test_progs.h | 4 +- 9 files changed, 142 insertions(+), 274 deletions(-) rename tools/testing/selftests/bpf/{test_sockopt.c => prog_tests/sockopt.c} (96%) rename tools/testing/selftests/bpf/{test_sockopt_inherit.c => prog_tests/sockopt_inherit.c} (72%) rename tools/testing/selftests/bpf/{test_sockopt_multi.c => prog_tests/sockopt_multi.c} (83%) rename tools/testing/selftests/bpf/{test_sockopt_sk.c => prog_tests/sockopt_sk.c} (79%) rename tools/testing/selftests/bpf/{test_tcp_rtt.c => prog_tests/tcp_rtt.c} (76%)