| Message ID | 20211022223228.99920-3-andrii@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | BPF |
| Headers | show |
| Series | Parallelize verif_scale selftests | expand |
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. > + $(TRUNNER_TESTS_DIR)/*.c | sort ; \ to be super safe : maybe add a check here to ensure each file contains at least one test function. > ) > $$@) > endif > > -- > 2.30.2 >
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? > > > ) > $$@) > > endif > > > > -- > > 2.30.2 > >
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 > > >
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 > > > >
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' \ + $(TRUNNER_TESTS_DIR)/*.c | sort ; \ ) > $$@) endif
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(-)