| Message ID | 20190409235556.3967-2-keescook@chromium.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | selftests: Move test output to diagnostic lines | expand |
Hi Kees, Thanks for the patch. On 4/9/19 5:55 PM, Kees Cook wrote: > In order to improve the reusability of the kselftest test running logic, > this extracts the single-test logic from lib.mk into kselftest/runner.sh > which lib.mk can call directly. No changes in output. > > As part of the change, this removes the unused "summary" Makefile variable > (and tests). However, future merging with the "emit_tests" target needs > to be able to redirect output, so a new "logfile" variable is introduced. Shouldn't the selftests/Makefile need update for "summary" removal?? > > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > tools/testing/selftests/.gitignore | 1 - > tools/testing/selftests/kselftest/runner.sh | 31 +++++++++++++++++++ > tools/testing/selftests/lib.mk | 33 ++------------------- > 3 files changed, 34 insertions(+), 31 deletions(-) > create mode 100644 tools/testing/selftests/kselftest/runner.sh > > diff --git a/tools/testing/selftests/.gitignore b/tools/testing/selftests/.gitignore > index 91750352459d..8059ce834247 100644 > --- a/tools/testing/selftests/.gitignore > +++ b/tools/testing/selftests/.gitignore > @@ -1,4 +1,3 @@ > -kselftest > gpiogpio-event-mon > gpiogpio-hammer > gpioinclude/ Please don't include this .gitignore change here. These are generated in tools/gpio and this .gitignore isn't the right place for them. > diff --git a/tools/testing/selftests/kselftest/runner.sh b/tools/testing/selftests/kselftest/runner.sh > new file mode 100644 > index 000000000000..77d5510ac4c5 > --- /dev/null > +++ b/tools/testing/selftests/kselftest/runner.sh > @@ -0,0 +1,31 @@ > +#!/bin/sh > +# > +# Runs a set of tests in a given subdirectory. > +export skip_rc=4 > +export logfile=/dev/stdout > + > +run_one() > +{ > + TEST="$1" > + NUM="$2" > + > + BASENAME_TEST=$(basename $TEST) > + > + TEST_HDR_MSG="selftests: "`basename $PWD`:" $BASENAME_TEST" > + echo "$TEST_HDR_MSG" > + echo "========================================" > + if [ ! -x "$TEST" ]; then > + echo "$TEST_HDR_MSG: Warning: file $TEST is not executable, correct this." > + echo "not ok 1..$test_num $TEST_HDR_MSG [FAIL]" > + else > + cd `dirname $TEST` > /dev/null > + (./$BASENAME_TEST >> "$logfile" 2>&1 && > + echo "ok 1..$test_num $TEST_HDR_MSG [PASS]") || > + (if [ $? -eq $skip_rc ]; then \ > + echo "not ok 1..$test_num $TEST_HDR_MSG [SKIP]" > + else > + echo "not ok 1..$test_num $TEST_HDR_MSG [FAIL]" > + fi) > + cd - >/dev/null > + fi > +} > diff --git a/tools/testing/selftests/lib.mk b/tools/testing/selftests/lib.mk > index 8b0f16409ed7..7da79fe0bb78 100644 > --- a/tools/testing/selftests/lib.mk > +++ b/tools/testing/selftests/lib.mk > @@ -5,6 +5,7 @@ CC := $(CROSS_COMPILE)gcc > ifeq (0,$(MAKELEVEL)) > OUTPUT := $(shell pwd) > endif > +selfdir = $(realpath $(dir $(filter %/lib.mk,$(MAKEFILE_LIST)))) > > # The following are built by lib.mk common compile rules. > # TEST_CUSTOM_PROGS should be used by tests that require > @@ -31,43 +32,15 @@ all: $(TEST_GEN_PROGS) $(TEST_GEN_PROGS_EXTENDED) $(TEST_GEN_FILES) > endif > > .ONESHELL: > -define RUN_TEST_PRINT_RESULT > - TEST_HDR_MSG="selftests: "`basename $$PWD`:" $$BASENAME_TEST"; \ > - echo $$TEST_HDR_MSG; \ > - echo "========================================"; \ > - if [ ! -x $$TEST ]; then \ > - echo "$$TEST_HDR_MSG: Warning: file $$BASENAME_TEST is not executable, correct this.";\ > - echo "not ok 1..$$test_num $$TEST_HDR_MSG [FAIL]"; \ > - else \ > - cd `dirname $$TEST` > /dev/null; \ > - if [ "X$(summary)" != "X" ]; then \ > - (./$$BASENAME_TEST > /tmp/$$BASENAME_TEST 2>&1 && \ > - echo "ok 1..$$test_num $$TEST_HDR_MSG [PASS]") || \ > - (if [ $$? -eq $$skip ]; then \ > - echo "not ok 1..$$test_num $$TEST_HDR_MSG [SKIP]"; \ > - else echo "not ok 1..$$test_num $$TEST_HDR_MSG [FAIL]"; \ > - fi;) \ > - else \ > - (./$$BASENAME_TEST && \ > - echo "ok 1..$$test_num $$TEST_HDR_MSG [PASS]") || \ > - (if [ $$? -eq $$skip ]; then \ > - echo "not ok 1..$$test_num $$TEST_HDR_MSG [SKIP]"; \ > - else echo "not ok 1..$$test_num $$TEST_HDR_MSG [FAIL]"; \ > - fi;) \ > - fi; \ > - cd - > /dev/null; \ > - fi; > -endef > - > define RUN_TESTS > @export KSFT_TAP_LEVEL=`echo 1`; \ > test_num=`echo 0`; \ > - skip=`echo 4`; \ > + . $(selfdir)/kselftest/runner.sh; \ > echo "TAP version 13"; \ > for TEST in $(1); do \ > BASENAME_TEST=`basename $$TEST`; \ > test_num=`echo $$test_num+1 | bc`; \ > - $(call RUN_TEST_PRINT_RESULT,$(TEST),$(BASENAME_TEST),$(test_num),$(skip)) \ > + run_one "$$BASENAME_TEST" "$$test_num"; \ > done; > endef > > thanks, -- Shuah
On Tue, Apr 16, 2019 at 6:11 PM shuah <shuah@kernel.org> wrote: > > Hi Kees, > > Thanks for the patch. > > On 4/9/19 5:55 PM, Kees Cook wrote: > > In order to improve the reusability of the kselftest test running logic, > > this extracts the single-test logic from lib.mk into kselftest/runner.sh > > which lib.mk can call directly. No changes in output. > > > > As part of the change, this removes the unused "summary" Makefile variable > > (and tests). However, future merging with the "emit_tests" target needs > > to be able to redirect output, so a new "logfile" variable is introduced. > > Shouldn't the selftests/Makefile need update for "summary" removal?? > I didn't see anything using "summary" except as a --summary argument to the run_kselftests.sh script. Maybe I missed it? > > > > Signed-off-by: Kees Cook <keescook@chromium.org> > > --- > > tools/testing/selftests/.gitignore | 1 - > > tools/testing/selftests/kselftest/runner.sh | 31 +++++++++++++++++++ > > tools/testing/selftests/lib.mk | 33 ++------------------- > > 3 files changed, 34 insertions(+), 31 deletions(-) > > create mode 100644 tools/testing/selftests/kselftest/runner.sh > > > > diff --git a/tools/testing/selftests/.gitignore b/tools/testing/selftests/.gitignore > > index 91750352459d..8059ce834247 100644 > > --- a/tools/testing/selftests/.gitignore > > +++ b/tools/testing/selftests/.gitignore > > @@ -1,4 +1,3 @@ > > -kselftest > > gpiogpio-event-mon > > gpiogpio-hammer > > gpioinclude/ > > Please don't include this .gitignore change here. These are generated > in tools/gpio and this .gitignore isn't the right place for them. This change is only removing the "kselftest" entry, for which the target is long gone. Since I was adding a directory by that name, I needed to remove it from the .gitignore file. I have nothing to do with the gpio stuff. :) Thanks for looking this over!
On 4/16/19 5:16 PM, Kees Cook wrote: > On Tue, Apr 16, 2019 at 6:11 PM shuah <shuah@kernel.org> wrote: >> >> Hi Kees, >> >> Thanks for the patch. >> >> On 4/9/19 5:55 PM, Kees Cook wrote: >>> In order to improve the reusability of the kselftest test running logic, >>> this extracts the single-test logic from lib.mk into kselftest/runner.sh >>> which lib.mk can call directly. No changes in output. >>> >>> As part of the change, this removes the unused "summary" Makefile variable >>> (and tests). However, future merging with the "emit_tests" target needs >>> to be able to redirect output, so a new "logfile" variable is introduced. >> >> Shouldn't the selftests/Makefile need update for "summary" removal?? >> > > I didn't see anything using "summary" except as a --summary argument > to the run_kselftests.sh script. Maybe I missed it? > It is in the selftests/Makefile install target. >>> >>> Signed-off-by: Kees Cook <keescook@chromium.org> >>> --- >>> tools/testing/selftests/.gitignore | 1 - >>> tools/testing/selftests/kselftest/runner.sh | 31 +++++++++++++++++++ >>> tools/testing/selftests/lib.mk | 33 ++------------------- >>> 3 files changed, 34 insertions(+), 31 deletions(-) >>> create mode 100644 tools/testing/selftests/kselftest/runner.sh >>> >>> diff --git a/tools/testing/selftests/.gitignore b/tools/testing/selftests/.gitignore >>> index 91750352459d..8059ce834247 100644 >>> --- a/tools/testing/selftests/.gitignore >>> +++ b/tools/testing/selftests/.gitignore >>> @@ -1,4 +1,3 @@ >>> -kselftest >>> gpiogpio-event-mon >>> gpiogpio-hammer >>> gpioinclude/ >> >> Please don't include this .gitignore change here. These are generated >> in tools/gpio and this .gitignore isn't the right place for them. > > This change is only removing the "kselftest" entry, for which the > target is long gone. Since I was adding a directory by that name, I > needed to remove it from the .gitignore file. I have nothing to do > with the gpio stuff. :) > Yeah my bad! :) > Thanks for looking this over! > thanks, -- Shuah
On Tue, Apr 16, 2019 at 4:21 PM shuah <shuah@kernel.org> wrote: > > On 4/16/19 5:16 PM, Kees Cook wrote: > > On Tue, Apr 16, 2019 at 6:11 PM shuah <shuah@kernel.org> wrote: > >> > >> Hi Kees, > >> > >> Thanks for the patch. > >> > >> On 4/9/19 5:55 PM, Kees Cook wrote: > >>> In order to improve the reusability of the kselftest test running logic, > >>> this extracts the single-test logic from lib.mk into kselftest/runner.sh > >>> which lib.mk can call directly. No changes in output. > >>> > >>> As part of the change, this removes the unused "summary" Makefile variable > >>> (and tests). However, future merging with the "emit_tests" target needs > >>> to be able to redirect output, so a new "logfile" variable is introduced. > >> > >> Shouldn't the selftests/Makefile need update for "summary" removal?? > >> > > > > I didn't see anything using "summary" except as a --summary argument > > to the run_kselftests.sh script. Maybe I missed it? > > > > It is in the selftests/Makefile install target. Right: it's used only by the run_kselftest.sh script: ALL_SCRIPT := $(INSTALL_PATH)/run_kselftest.sh install: ... echo "if [ \"\$$1\" = \"--summary\" ]; then" >> $(ALL_SCRIPT) So, I think this entire series can land. Is there other feedback I should incorporate? I'd like to see it get some -next testing... Thanks!
On 4/23/19 4:31 PM, Kees Cook wrote: > On Tue, Apr 16, 2019 at 4:21 PM shuah <shuah@kernel.org> wrote: >> >> On 4/16/19 5:16 PM, Kees Cook wrote: >>> On Tue, Apr 16, 2019 at 6:11 PM shuah <shuah@kernel.org> wrote: >>>> >>>> Hi Kees, >>>> >>>> Thanks for the patch. >>>> >>>> On 4/9/19 5:55 PM, Kees Cook wrote: >>>>> In order to improve the reusability of the kselftest test running logic, >>>>> this extracts the single-test logic from lib.mk into kselftest/runner.sh >>>>> which lib.mk can call directly. No changes in output. >>>>> >>>>> As part of the change, this removes the unused "summary" Makefile variable >>>>> (and tests). However, future merging with the "emit_tests" target needs >>>>> to be able to redirect output, so a new "logfile" variable is introduced. >>>> >>>> Shouldn't the selftests/Makefile need update for "summary" removal?? >>>> >>> >>> I didn't see anything using "summary" except as a --summary argument >>> to the run_kselftests.sh script. Maybe I missed it? >>> >> >> It is in the selftests/Makefile install target. > > Right: it's used only by the run_kselftest.sh script: > > ALL_SCRIPT := $(INSTALL_PATH)/run_kselftest.sh > > install: > ... > echo "if [ \"\$$1\" = \"--summary\" ]; then" >> $(ALL_SCRIPT) > > > So, I think this entire series can land. Is there other feedback I > should incorporate? I'd like to see it get some -next testing... > > Thanks! > Kees, Yes I was planning to get this into next and did some testing. I found that the first patch breaks the summary option. You can see it by running make summary=1 TARGETS="breakpoints" kselftest with and without your first patch. Ca you fix the regression and send me revised patches. Sorry, I didn't get back to you sooner. A few bugs I found kept me busy. While I was testing your patch series, I found test build failures due to circular dependency that needed fixing. In addition, the same headers_install dependency, also broke O= and KBUILD_OUTPUT builds and fixed those as well. Long story short, these fixes might conflict with your work. thanks, -- Shuah
On Tue, Apr 23, 2019 at 3:47 PM shuah <shuah@kernel.org> wrote: > You can see it by running > > make summary=1 TARGETS="breakpoints" kselftest Okay, thanks for this! I'll give it a spin. > with and without your first patch. Ca you fix the regression and send > me revised patches. Sorry, I didn't get back to you sooner. A few bugs > I found kept me busy. > > While I was testing your patch series, I found test build failures due > to circular dependency that needed fixing. In addition, the same > headers_install dependency, also broke O= and KBUILD_OUTPUT builds > and fixed those as well. Long story short, these fixes might conflict > with your work. I can rebase the series -- which tree should I look at? (Is it all in -next already?) Thanks!
On 4/23/19 8:43 PM, Kees Cook wrote: > On Tue, Apr 23, 2019 at 3:47 PM shuah <shuah@kernel.org> wrote: >> You can see it by running >> >> make summary=1 TARGETS="breakpoints" kselftest > > Okay, thanks for this! I'll give it a spin. > >> with and without your first patch. Ca you fix the regression and send >> me revised patches. Sorry, I didn't get back to you sooner. A few bugs >> I found kept me busy. >> >> While I was testing your patch series, I found test build failures due >> to circular dependency that needed fixing. In addition, the same >> headers_install dependency, also broke O= and KBUILD_OUTPUT builds >> and fixed those as well. Long story short, these fixes might conflict >> with your work. > > I can rebase the series -- which tree should I look at? (Is it all in > -next already?) > Yes - the two patches are in linux-kselftest next thanks, -- Shuah
diff --git a/tools/testing/selftests/.gitignore b/tools/testing/selftests/.gitignore index 91750352459d..8059ce834247 100644 --- a/tools/testing/selftests/.gitignore +++ b/tools/testing/selftests/.gitignore @@ -1,4 +1,3 @@ -kselftest gpiogpio-event-mon gpiogpio-hammer gpioinclude/ diff --git a/tools/testing/selftests/kselftest/runner.sh b/tools/testing/selftests/kselftest/runner.sh new file mode 100644 index 000000000000..77d5510ac4c5 --- /dev/null +++ b/tools/testing/selftests/kselftest/runner.sh @@ -0,0 +1,31 @@ +#!/bin/sh +# +# Runs a set of tests in a given subdirectory. +export skip_rc=4 +export logfile=/dev/stdout + +run_one() +{ + TEST="$1" + NUM="$2" + + BASENAME_TEST=$(basename $TEST) + + TEST_HDR_MSG="selftests: "`basename $PWD`:" $BASENAME_TEST" + echo "$TEST_HDR_MSG" + echo "========================================" + if [ ! -x "$TEST" ]; then + echo "$TEST_HDR_MSG: Warning: file $TEST is not executable, correct this." + echo "not ok 1..$test_num $TEST_HDR_MSG [FAIL]" + else + cd `dirname $TEST` > /dev/null + (./$BASENAME_TEST >> "$logfile" 2>&1 && + echo "ok 1..$test_num $TEST_HDR_MSG [PASS]") || + (if [ $? -eq $skip_rc ]; then \ + echo "not ok 1..$test_num $TEST_HDR_MSG [SKIP]" + else + echo "not ok 1..$test_num $TEST_HDR_MSG [FAIL]" + fi) + cd - >/dev/null + fi +} diff --git a/tools/testing/selftests/lib.mk b/tools/testing/selftests/lib.mk index 8b0f16409ed7..7da79fe0bb78 100644 --- a/tools/testing/selftests/lib.mk +++ b/tools/testing/selftests/lib.mk @@ -5,6 +5,7 @@ CC := $(CROSS_COMPILE)gcc ifeq (0,$(MAKELEVEL)) OUTPUT := $(shell pwd) endif +selfdir = $(realpath $(dir $(filter %/lib.mk,$(MAKEFILE_LIST)))) # The following are built by lib.mk common compile rules. # TEST_CUSTOM_PROGS should be used by tests that require @@ -31,43 +32,15 @@ all: $(TEST_GEN_PROGS) $(TEST_GEN_PROGS_EXTENDED) $(TEST_GEN_FILES) endif .ONESHELL: -define RUN_TEST_PRINT_RESULT - TEST_HDR_MSG="selftests: "`basename $$PWD`:" $$BASENAME_TEST"; \ - echo $$TEST_HDR_MSG; \ - echo "========================================"; \ - if [ ! -x $$TEST ]; then \ - echo "$$TEST_HDR_MSG: Warning: file $$BASENAME_TEST is not executable, correct this.";\ - echo "not ok 1..$$test_num $$TEST_HDR_MSG [FAIL]"; \ - else \ - cd `dirname $$TEST` > /dev/null; \ - if [ "X$(summary)" != "X" ]; then \ - (./$$BASENAME_TEST > /tmp/$$BASENAME_TEST 2>&1 && \ - echo "ok 1..$$test_num $$TEST_HDR_MSG [PASS]") || \ - (if [ $$? -eq $$skip ]; then \ - echo "not ok 1..$$test_num $$TEST_HDR_MSG [SKIP]"; \ - else echo "not ok 1..$$test_num $$TEST_HDR_MSG [FAIL]"; \ - fi;) \ - else \ - (./$$BASENAME_TEST && \ - echo "ok 1..$$test_num $$TEST_HDR_MSG [PASS]") || \ - (if [ $$? -eq $$skip ]; then \ - echo "not ok 1..$$test_num $$TEST_HDR_MSG [SKIP]"; \ - else echo "not ok 1..$$test_num $$TEST_HDR_MSG [FAIL]"; \ - fi;) \ - fi; \ - cd - > /dev/null; \ - fi; -endef - define RUN_TESTS @export KSFT_TAP_LEVEL=`echo 1`; \ test_num=`echo 0`; \ - skip=`echo 4`; \ + . $(selfdir)/kselftest/runner.sh; \ echo "TAP version 13"; \ for TEST in $(1); do \ BASENAME_TEST=`basename $$TEST`; \ test_num=`echo $$test_num+1 | bc`; \ - $(call RUN_TEST_PRINT_RESULT,$(TEST),$(BASENAME_TEST),$(test_num),$(skip)) \ + run_one "$$BASENAME_TEST" "$$test_num"; \ done; endef
In order to improve the reusability of the kselftest test running logic, this extracts the single-test logic from lib.mk into kselftest/runner.sh which lib.mk can call directly. No changes in output. As part of the change, this removes the unused "summary" Makefile variable (and tests). However, future merging with the "emit_tests" target needs to be able to redirect output, so a new "logfile" variable is introduced. Signed-off-by: Kees Cook <keescook@chromium.org> --- tools/testing/selftests/.gitignore | 1 - tools/testing/selftests/kselftest/runner.sh | 31 +++++++++++++++++++ tools/testing/selftests/lib.mk | 33 ++------------------- 3 files changed, 34 insertions(+), 31 deletions(-) create mode 100644 tools/testing/selftests/kselftest/runner.sh