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

From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: "sunyucong@gmail.com" <sunyucong@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 14:09:05 -0700	[thread overview]
Message-ID: <CAEf4BzYoGY_eBP0vjb8W7VRgeZjo=49_eqFwPSeA0VwAJHKAZA@mail.gmail.com> (raw)
In-Reply-To: <CAJygYd1ctJpaNmL8eyGs66UpFCrSdRo7SQZbnpUnpMufKfcnug@mail.gmail.com>

On Mon, Oct 25, 2021 at 1:56 PM sunyucong@gmail.com <sunyucong@gmail.com> wrote:
>
> 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

Yep. We should be able to automate this once we have some sort of
baseline comparison functionality in BPF CI.

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