[PATCH 0/2] Changes to timers Maefile to use shared logic for run_tests and install

This patch set:

adds ability to run and emit script for tests args to lib.mk.
lib.mk provides a way to override the default RUN_TESTS and
EMIT_TESTS, however in the cases where a test has several
executables that don't require any special args and/or handling
and some that do, overrding will be cumbersome. Add variables
RUN_TESTS_WITH_ARGS, and EMIT_TESTS_WITH_ARGS to enable a test to
run and exmit script for such special executables.

Timers test Makefile is changed to use the shared logic for running
and installing tests.

Shuah Khan (2):
  selftests: add ability to run and emit script for tests args to lib.mk
  selftests/timers: change to use shared logic to run and install tests

 tools/testing/selftests/lib.mk          |  6 ++++++
 tools/testing/selftests/timers/Makefile | 20 +++++++++++---------
 2 files changed, 17 insertions(+), 9 deletions(-)

-- 
2.1.0

Add ability to run, emit, install tests take arguments to the
shared logic in lib.mk. lib.mk provides a way to override the
default RUN_TESTS and EMIT_TESTS, however in the cases where a
test has several executables that don't require any special args
and/or handling and some that do, overrding will be cumbersome.
Add variables RUN_TESTS_WITH_ARGS, and EMIT_TESTS_WITH_ARGS to
enable a test to run and exmit script for such special executables.

Signed-off-by: Shuah Khan <redacted>
---
 tools/testing/selftests/lib.mk | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/testing/selftests/lib.mk b/tools/testing/selftests/lib.mk
index 7bd3dab..78cdf91 100644
--- a/tools/testing/selftests/lib.mk
+++ b/tools/testing/selftests/lib.mk

@@ -6,6 +6,9 @@ endef
 
 run_tests: all
 	$(RUN_TESTS)
+ifdef RUN_TESTS_WITH_ARGS
+	$(RUN_TESTS_WITH_ARGS)
+endif
 
 define INSTALL_RULE
 	mkdir -p $(INSTALL_PATH)

@@ -27,5 +30,8 @@ endef
 
 emit_tests:
 	$(EMIT_TESTS)
+ifdef EMIT_TESTS_WITH_ARGS
+	$(EMIT_TESTS_WITH_ARGS)
+endif
 
 .PHONY: run_tests all clean install emit_tests

-- 
2.1.0

Change the timers Makefile to make use of shared run and install
logic in lib.mk. Destructive tests are installed. Regular tests
are emited to run_kselftest script to match the run_tests behavior.

Signed-off-by: Shuah Khan <redacted>
---
 tools/testing/selftests/timers/Makefile | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/tools/testing/selftests/timers/Makefile b/tools/testing/selftests/timers/Makefile
index 9da3498..61e7284 100644
--- a/tools/testing/selftests/timers/Makefile
+++ b/tools/testing/selftests/timers/Makefile

@@ -7,19 +7,21 @@ bins = posix_timers nanosleep inconsistency-check nsleep-lat raw_skew \
 	alarmtimer-suspend change_skew skew_consistency clocksource-switch \
 	leap-a-day leapcrash set-tai set-2038
 
+TEST_PROGS = posix_timers nanosleep nsleep-lat set-timer-lat mqueue-lat \
+		inconsistency-check raw_skew
+TEST_FILES = threadtest alarmtimer-suspend valid-adjtimex change_skew \
+		skew_consistency clocksource-switch leap-a-day leapcrash \
+		set-tai set-2038
+
+RUN_TESTS_WITH_ARGS := ./threadtest -t 30 -n 8 || echo "selftests: threadtest [FAIL]"
+
+EMIT_TESTS_WITH_ARGS := echo "$(RUN_TESTS_WITH_ARGS)"
+
 all: ${bins}
 
 # these are all "safe" tests that don't modify
 # system time or require escalated privledges
-run_tests: all
-	./posix_timers
-	./nanosleep
-	./nsleep-lat
-	./set-timer-lat
-	./mqueue-lat
-	./inconsistency-check
-	./raw_skew
-	./threadtest -t 30 -n 8
+include ../lib.mk
 
 # these tests require escalated privledges
 # and may modify the system time or trigger

-- 
2.1.0

On Fri, Mar 13, 2015 at 3:57 PM, Shuah Khan [off-list ref] wrote:

So my make-foo isn't very strong, but no objections from me.

My only thoughts:
1) Would it be better if threadtest can be made to have better
defaults for kselftest so you don't need that extra logic?
2) While I get that TEST_FILES is likely going to be used to copy the
destructive tests over, It feels a little like its being bundled in
with something like data files that tests might need, which seems sort
of hackish. Would TEST_PROGS_EXTENDED or something be more clear and
make more sense?

thanks
-john

On 14 March 2015 09:57:51 GMT+11:00, Shuah Khan [off-list ref] wrote:

You shouldn't need this separate variable. As long as you override RUN_TESTS after you include lib.mk you can include the default value, eg:

override RUN_TESTS := $(RUN_TESTS) ./threadtest -t 30 -n 8 || echo "selftests: threadtest [FAIL]"

I'll test that in the morning and send a proper patch.

cheers
-- 
Sent from my Android phone with K-9 Mail. Please excuse my brevity.

On Sun, 2015-03-15 at 19:42 +1100, Michael Ellerman wrote:

On 14 March 2015 09:57:51 GMT+11:00, Shuah Khan [off-list ref] wrote:

quoted

Change the timers Makefile to make use of shared run and install
logic in lib.mk. Destructive tests are installed. Regular tests
are emited to run_kselftest script to match the run_tests behavior.

Signed-off-by: Shuah Khan <redacted>
---
tools/testing/selftests/timers/Makefile | 20 +++++++++++---------
1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/tools/testing/selftests/timers/Makefile

b/tools/testing/selftests/timers/Makefile
index 9da3498..61e7284 100644

--- a/tools/testing/selftests/timers/Makefile
+++ b/tools/testing/selftests/timers/Makefile

@@ -7,19 +7,21 @@ bins = posix_timers nanosleep inconsistency-check

nsleep-lat raw_skew \
	alarmtimer-suspend change_skew skew_consistency clocksource-switch \
	leap-a-day leapcrash set-tai set-2038

+TEST_PROGS = posix_timers nanosleep nsleep-lat set-timer-lat
mqueue-lat \
+		inconsistency-check raw_skew
+TEST_FILES = threadtest alarmtimer-suspend valid-adjtimex change_skew
\
+		skew_consistency clocksource-switch leap-a-day leapcrash \
+		set-tai set-2038
+
+RUN_TESTS_WITH_ARGS := ./threadtest -t 30 -n 8 || echo "selftests:
threadtest [FAIL]"

You shouldn't need this separate variable. As long as you override RUN_TESTS after you include lib.mk you can include the default value, eg:

override RUN_TESTS := $(RUN_TESTS) ./threadtest -t 30 -n 8 || echo "selftests: threadtest [FAIL]"

I'll test that in the morning and send a proper patch.

How's this look?

Contents of install/timers (same as with your patch):

$ ls install/timers/
alarmtimer-suspend*  clocksource-switch*   leap-a-day*  mqueue-lat*  nsleep-lat*    raw_skew*  set-tai*        skew_consistency*  valid-adjtimex*
change_skew*         inconsistency-check*  leapcrash*   nanosleep*   posix_timers*  set-2038*  set-timer-lat*  threadtest*

And in run_kselftest.sh:

echo ; echo Running tests in timers
echo ========================================
cd timers
(./posix_timers && echo "selftests: posix_timers [PASS]") || echo "selftests: posix_timers [FAIL]"
(./nanosleep && echo "selftests: nanosleep [PASS]") || echo "selftests: nanosleep [FAIL]"
(./nsleep-lat && echo "selftests: nsleep-lat [PASS]") || echo "selftests: nsleep-lat [FAIL]"
(./set-timer-lat && echo "selftests: set-timer-lat [PASS]") || echo "selftests: set-timer-lat [FAIL]"
(./mqueue-lat && echo "selftests: mqueue-lat [PASS]") || echo "selftests: mqueue-lat [FAIL]"
(./inconsistency-check && echo "selftests: inconsistency-check [PASS]") || echo "selftests: inconsistency-check [FAIL]"
(./raw_skew && echo "selftests: raw_skew [PASS]") || echo "selftests: raw_skew [FAIL]"
./threadtest -t 30 -n 8 || echo selftests: threadtest [FAIL]
cd $ROOT


cheers

From 5ea5ea0e5adee5317978965acc4594dce648da63 Mon Sep 17 00:00:00 2001

From: Michael Ellerman <mpe-Gsx/Oe8HsFggBc27wqDAHg@public.gmane.org>
Date: Mon, 16 Mar 2015 13:38:06 +1100
Subject: [PATCH] selftests/timers: Use shared logic to run and install tests

Change the timers Makefile to make use of shared run and install logic
in lib.mk. Destructive tests are installed. Regular tests are emited to
run_kselftest script to match the run_tests behavior.

Signed-off-by: Michael Ellerman <mpe-Gsx/Oe8HsFggBc27wqDAHg@public.gmane.org>
---
 tools/testing/selftests/timers/Makefile | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/tools/testing/selftests/timers/Makefile b/tools/testing/selftests/timers/Makefile
index 9da3498987c8..8ba056ee8b48 100644
--- a/tools/testing/selftests/timers/Makefile
+++ b/tools/testing/selftests/timers/Makefile

@@ -1,25 +1,26 @@
-CC = $(CROSS_COMPILE)gcc
 BUILD_FLAGS = -DKTEST
 CFLAGS += -O3 -Wl,-no-as-needed -Wall $(BUILD_FLAGS)
 LDFLAGS += -lrt -lpthread
-bins = posix_timers nanosleep inconsistency-check nsleep-lat raw_skew \
-	set-timer-lat threadtest mqueue-lat valid-adjtimex \
-	alarmtimer-suspend change_skew skew_consistency clocksource-switch \
-	leap-a-day leapcrash set-tai set-2038
-
-all: ${bins}
 
 # these are all "safe" tests that don't modify
 # system time or require escalated privledges
-run_tests: all
-	./posix_timers
-	./nanosleep
-	./nsleep-lat
-	./set-timer-lat
-	./mqueue-lat
-	./inconsistency-check
-	./raw_skew
-	./threadtest -t 30 -n 8
+TEST_PROGS = posix_timers nanosleep nsleep-lat set-timer-lat mqueue-lat \
+		inconsistency-check raw_skew
+
+TEST_FILES = threadtest alarmtimer-suspend valid-adjtimex change_skew \
+		skew_consistency clocksource-switch leap-a-day leapcrash \
+		set-tai set-2038
+
+bins = $(TEST_PROGS) $(TEST_FILES)
+
+all: ${bins}
+
+include ../lib.mk
+
+THREADTEST := ./threadtest -t 30 -n 8 || echo "selftests: threadtest [FAIL]"
+
+override RUN_TESTS := $(RUN_TESTS) $(THREADTEST)
+override EMIT_TESTS := $(EMIT_TESTS) echo "$(THREADTEST)"
 
 # these tests require escalated privledges
 # and may modify the system time or trigger

-- 
2.1.0

On Fri, 2015-03-13 at 20:14 -0700, John Stultz wrote:

On Fri, Mar 13, 2015 at 3:57 PM, Shuah Khan [off-list ref] wrote:

quoted

Change the timers Makefile to make use of shared run and install
logic in lib.mk. Destructive tests are installed. Regular tests
are emited to run_kselftest script to match the run_tests behavior.

Signed-off-by: Shuah Khan <redacted>
---
 tools/testing/selftests/timers/Makefile | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/tools/testing/selftests/timers/Makefile b/tools/testing/selftests/timers/Makefile
index 9da3498..61e7284 100644
--- a/tools/testing/selftests/timers/Makefile
+++ b/tools/testing/selftests/timers/Makefile

@@ -7,19 +7,21 @@ bins = posix_timers nanosleep inconsistency-check nsleep-lat raw_skew \
        alarmtimer-suspend change_skew skew_consistency clocksource-switch \
        leap-a-day leapcrash set-tai set-2038

+TEST_PROGS = posix_timers nanosleep nsleep-lat set-timer-lat mqueue-lat \
+               inconsistency-check raw_skew
+TEST_FILES = threadtest alarmtimer-suspend valid-adjtimex change_skew \
+               skew_consistency clocksource-switch leap-a-day leapcrash \
+               set-tai set-2038
+
+RUN_TESTS_WITH_ARGS := ./threadtest -t 30 -n 8 || echo "selftests: threadtest [FAIL]"
+
+EMIT_TESTS_WITH_ARGS := echo "$(RUN_TESTS_WITH_ARGS)"
+

So my make-foo isn't very strong, but no objections from me.

My only thoughts:
1) Would it be better if threadtest can be made to have better
defaults for kselftest so you don't need that extra logic?

That would help. But with the patch I just sent I think it's no bother, it's
only a little extra logic and it's only in the timers Makefile.

2) While I get that TEST_FILES is likely going to be used to copy the
destructive tests over, It feels a little like its being bundled in
with something like data files that tests might need, which seems sort
of hackish. Would TEST_PROGS_EXTENDED or something be more clear and
make more sense?

That doesn't really bother me. You're right that TEST_FILES is originally
intended for data files etc. but I don't think it's a big hack to use it for
other tests that shouldn't be run by default. Still if it bothers you I'm happy
to add a separate variable for it, they are cheap :)

cheers

On 03/13/2015 09:14 PM, John Stultz wrote:

On Fri, Mar 13, 2015 at 3:57 PM, Shuah Khan [off-list ref] wrote:

quoted

Change the timers Makefile to make use of shared run and install
logic in lib.mk. Destructive tests are installed. Regular tests
are emited to run_kselftest script to match the run_tests behavior.

Signed-off-by: Shuah Khan <redacted>
---
 tools/testing/selftests/timers/Makefile | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/tools/testing/selftests/timers/Makefile b/tools/testing/selftests/timers/Makefile
index 9da3498..61e7284 100644
--- a/tools/testing/selftests/timers/Makefile
+++ b/tools/testing/selftests/timers/Makefile

@@ -7,19 +7,21 @@ bins = posix_timers nanosleep inconsistency-check nsleep-lat raw_skew \
        alarmtimer-suspend change_skew skew_consistency clocksource-switch \
        leap-a-day leapcrash set-tai set-2038

+TEST_PROGS = posix_timers nanosleep nsleep-lat set-timer-lat mqueue-lat \
+               inconsistency-check raw_skew
+TEST_FILES = threadtest alarmtimer-suspend valid-adjtimex change_skew \
+               skew_consistency clocksource-switch leap-a-day leapcrash \
+               set-tai set-2038
+
+RUN_TESTS_WITH_ARGS := ./threadtest -t 30 -n 8 || echo "selftests: threadtest [FAIL]"
+
+EMIT_TESTS_WITH_ARGS := echo "$(RUN_TESTS_WITH_ARGS)"
+

So my make-foo isn't very strong, but no objections from me.

My only thoughts:
1) Would it be better if threadtest can be made to have better
defaults for kselftest so you don't need that extra logic?

Yes. If you can change the threadset to run with -t 30 -n 8
in default case, that would make the logic simpler. Can you
send me a patch to do this?

2) While I get that TEST_FILES is likely going to be used to copy the
destructive tests over, It feels a little like its being bundled in
with something like data files that tests might need, which seems sort
of hackish. Would TEST_PROGS_EXTENDED or something be more clear and
make more sense?

I agree with you on this. TEST_FILES usage is an overload.

-- Shuah


-- 
Shuah Khan
Sr. Linux Kernel Developer
Open Source Innovation Group
Samsung Research America (Silicon Valley)
shuahkh@osg.samsung.com | (970) 217-8978

On 03/15/2015 08:48 PM, Michael Ellerman wrote:

On Fri, 2015-03-13 at 20:14 -0700, John Stultz wrote:

quoted

On Fri, Mar 13, 2015 at 3:57 PM, Shuah Khan [off-list ref] wrote:

quoted

Change the timers Makefile to make use of shared run and install
logic in lib.mk. Destructive tests are installed. Regular tests
are emited to run_kselftest script to match the run_tests behavior.

Signed-off-by: Shuah Khan <redacted>
---
 tools/testing/selftests/timers/Makefile | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/tools/testing/selftests/timers/Makefile b/tools/testing/selftests/timers/Makefile
index 9da3498..61e7284 100644
--- a/tools/testing/selftests/timers/Makefile
+++ b/tools/testing/selftests/timers/Makefile

@@ -7,19 +7,21 @@ bins = posix_timers nanosleep inconsistency-check nsleep-lat raw_skew \
        alarmtimer-suspend change_skew skew_consistency clocksource-switch \
        leap-a-day leapcrash set-tai set-2038

+TEST_PROGS = posix_timers nanosleep nsleep-lat set-timer-lat mqueue-lat \
+               inconsistency-check raw_skew
+TEST_FILES = threadtest alarmtimer-suspend valid-adjtimex change_skew \
+               skew_consistency clocksource-switch leap-a-day leapcrash \
+               set-tai set-2038
+
+RUN_TESTS_WITH_ARGS := ./threadtest -t 30 -n 8 || echo "selftests: threadtest [FAIL]"
+
+EMIT_TESTS_WITH_ARGS := echo "$(RUN_TESTS_WITH_ARGS)"
+

So my make-foo isn't very strong, but no objections from me.

My only thoughts:
1) Would it be better if threadtest can be made to have better
defaults for kselftest so you don't need that extra logic?

That would help. But with the patch I just sent I think it's no bother, it's
only a little extra logic and it's only in the timers Makefile.

Let's go with a threadtest patch with better defaults. It will emit
scripts logic as well. Awesome. I just saw John's new patch in my
Inbox. Thanks.

quoted

2) While I get that TEST_FILES is likely going to be used to copy the
destructive tests over, It feels a little like its being bundled in
with something like data files that tests might need, which seems sort
of hackish. Would TEST_PROGS_EXTENDED or something be more clear and
make more sense?

That doesn't really bother me. You're right that TEST_FILES is originally
intended for data files etc. but I don't think it's a big hack to use it for
other tests that shouldn't be run by default. Still if it bothers you I'm happy
to add a separate variable for it, they are cheap :)

Michael,

Could you please make the change from TEST_FILES to TEST_PROGS_EXTENDED
which is definitely better than overloading TEST_FILES?

I think this would probably only change Add Install target patch. You
can send me patch v5 with that change and I will override the next with
your new one. Thanks for doing this.

thanks,
-- Shuah

-- 
Shuah Khan
Sr. Linux Kernel Developer
Open Source Innovation Group
Samsung Research America (Silicon Valley)
shuahkh-JPH+aEBZ4P+UEJcrhfAQsw@public.gmane.org | (970) 217-8978

On Wed, 2015-03-18 at 09:55 -0600, Shuah Khan wrote:

On 03/15/2015 08:48 PM, Michael Ellerman wrote:

quoted

On Fri, 2015-03-13 at 20:14 -0700, John Stultz wrote:

quoted

My only thoughts:
1) Would it be better if threadtest can be made to have better
defaults for kselftest so you don't need that extra logic?

That would help. But with the patch I just sent I think it's no bother, it's
only a little extra logic and it's only in the timers Makefile.

Let's go with a threadtest patch with better defaults. It will emit
scripts logic as well. Awesome. I just saw John's new patch in my
Inbox. Thanks.

Fine by me.

quoted

quoted

2) While I get that TEST_FILES is likely going to be used to copy the
destructive tests over, It feels a little like its being bundled in
with something like data files that tests might need, which seems sort
of hackish. Would TEST_PROGS_EXTENDED or something be more clear and
make more sense?

That doesn't really bother me. You're right that TEST_FILES is originally
intended for data files etc. but I don't think it's a big hack to use it for
other tests that shouldn't be run by default. Still if it bothers you I'm happy
to add a separate variable for it, they are cheap :)

Could you please make the change from TEST_FILES to TEST_PROGS_EXTENDED
which is definitely better than overloading TEST_FILES?

I don't see the point, but I don't care that much.

I think this would probably only change Add Install target patch. You
can send me patch v5 with that change and I will override the next with
your new one. Thanks for doing this.

You shouldn't rebase your next branch, people may have already merged it (like
me). You should apply it as an additional patch on top.

Patch sent.

cheers