Re: [PATCH bpf-next 2/4] selftests/bpf: support multiple tests per file

From: "sunyucong@gmail.com" <sunyucong@gmail.com>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: Andrii Nakryiko <andrii@kernel.org>, bpf <bpf@vger.kernel.org>,
	Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Kernel Team <kernel-team@fb.com>
Subject: Re: [PATCH bpf-next 2/4] selftests/bpf: support multiple tests per file
Date: Mon, 25 Oct 2021 13:55:35 -0700	[thread overview]
Message-ID: <CAJygYd1ctJpaNmL8eyGs66UpFCrSdRo7SQZbnpUnpMufKfcnug@mail.gmail.com> (raw)
In-Reply-To: <CAEf4BzZ3_COLB32D7oktOPKBGzU3LZ7N=Bd-H-Lf0KWb-Qc7NA@mail.gmail.com>

On Mon, Oct 25, 2021 at 1:39 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Mon, Oct 25, 2021 at 1:13 PM sunyucong@gmail.com <sunyucong@gmail.com> wrote:
> >
> > On Fri, Oct 22, 2021 at 3:33 PM Andrii Nakryiko <andrii@kernel.org> wrote:
> > >
> > > Revamp how test discovery works for test_progs and allow multiple test
> > > entries per file. Any global void function with no arguments and
> > > serial_test_ or test_ prefix is considered a test.
> > >
> > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org>
> > > ---
> > >  tools/testing/selftests/bpf/Makefile | 7 +++----
> > >  1 file changed, 3 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> > > index 498222543c37..ac47cf9760fc 100644
> > > --- a/tools/testing/selftests/bpf/Makefile
> > > +++ b/tools/testing/selftests/bpf/Makefile
> > > @@ -421,10 +421,9 @@ ifeq ($($(TRUNNER_TESTS_DIR)-tests-hdr),)
> > >  $(TRUNNER_TESTS_DIR)-tests-hdr := y
> > >  $(TRUNNER_TESTS_HDR): $(TRUNNER_TESTS_DIR)/*.c
> > >         $$(call msg,TEST-HDR,$(TRUNNER_BINARY),$$@)
> > > -       $$(shell ( cd $(TRUNNER_TESTS_DIR);                             \
> > > -                 echo '/* Generated header, do not edit */';           \
> > > -                 ls *.c 2> /dev/null |                                 \
> > > -                       sed -e 's@\([^\.]*\)\.c@DEFINE_TEST(\1)@';      \
> > > +       $$(shell (echo '/* Generated header, do not edit */';                                   \
> > > +                 sed -n -E 's/^void (serial_)?test_([a-zA-Z0-9_]+)\((void)?\).*/DEFINE_TEST(\2)/p'     \
> >
> > probably not that important :  allow \s* before void and after void.
> > Or,  maybe we can just  (?!static)  instead of anchoring to line
> > start.
>
> Selftests source code is pretty strict with formatting, so I don't
> think we'll deviate from the strict `^void <name>` pattern (and we
> certainly don't want to deviate). So I didn't want to overcomplicate
> regexes unnecessarily.
>
> >
> > > +                       $(TRUNNER_TESTS_DIR)/*.c | sort ;       \
> >
> > to be super safe : maybe add a check here to ensure each file contains
> > at least one test function.
>
> It's actually a useful property to have .c files that don't have
> tests. This can be used for adding various shared helpers. Currently
> all *_helpers.c are in selftests/bpf/ directory and have to be
> explicitly wired in Makefile, which is a bit annoying. With this setup
> we can just put a new .c file in the selftests/bpf/prog_tests/ and it
> will be automatically compiled and linked.
>
> It also will significantly hurt readability to add some sort of
> per-file check in there, do you think it's worth it?

You are right, probably not really worth it. we just have to watch the
total test numbers, it should always goes up :-D

>
> >
> > >                  ) > $$@)
> > >  endif
> > >
> > > --
> > > 2.30.2
> > >