[1/6] selftests: Extract single-test shell logic from lib.mk
diff mbox series

Message ID 20190409235556.3967-2-keescook@chromium.org
State New
Headers show
Series
  • selftests: Move test output to diagnostic lines
Related show

Commit Message

Kees Cook April 9, 2019, 11:55 p.m. UTC
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

Comments

shuah April 16, 2019, 11:11 p.m. UTC | #1
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
Kees Cook April 16, 2019, 11:16 p.m. UTC | #2
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!
shuah April 16, 2019, 11:21 p.m. UTC | #3
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
Kees Cook April 23, 2019, 10:31 p.m. UTC | #4
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!
shuah April 23, 2019, 10:47 p.m. UTC | #5
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
Kees Cook April 24, 2019, 2:43 a.m. UTC | #6
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!
shuah April 24, 2019, 2:46 a.m. UTC | #7
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

Patch
diff mbox series

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