[PATCH] selftests/kselftest/runner.sh: Add optional command parameters in settings

[PATCH] selftests/kselftest/runner.sh: Add optional command parameters in settings

[Cristian Marussi]

Some testcases allow for optional commandline parameters but as of now
there is now way to provide such arguments to the runner script.

Add support to the per-test-directory "settings" file to provide such
optional arguments; two new optional fields can now be defined in
"settings":

 - args="<options>": general arguments common to all testcase commands in
   the test directory

 - <BASENAME_TEST>_args="<options>": custom arguments specific to only one
   specific testcase command

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
Used to configure the use of a specific rtc device on CI systems with:
 tools/testing/selftests/rtc/settings:
   timeout=90
   rtctest_args="/dev/rtc1"
---
 tools/testing/selftests/kselftest/runner.sh | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/kselftest/runner.sh b/tools/testing/selftests/kselftest/runner.sh
index a9ba782d8ca0..f877a8571927 100644
--- a/tools/testing/selftests/kselftest/runner.sh
+++ b/tools/testing/selftests/kselftest/runner.sh
@@ -49,6 +49,15 @@ run_one()
 
 	# Reset any "settings"-file variables.
 	export kselftest_timeout="$kselftest_default_timeout"
+
+	# Optional arguments for any command, possibly defined in settings
+	# as args="<options>"
+	kselftest_args=""
+
+	# Optional arguments for this command, possibly defined in settings
+	# as <$BASENAME_TEST>_args="<options>"
+	kselftest_cmd_args_ref="kselftest_${BASENAME_TEST}_args"
+
 	# Load per-test-directory kselftest "settings" file.
 	settings="$BASE_DIR/$DIR/settings"
 	if [ -r "$settings" ] ; then
@@ -69,7 +78,8 @@ run_one()
 		echo "# Warning: file $TEST is missing!"
 		echo "not ok $test_num $TEST_HDR_MSG"
 	else
-		cmd="./$BASENAME_TEST"
+		eval kselftest_cmd_args="\$$kselftest_cmd_args_ref"
+		cmd="./$BASENAME_TEST $kselftest_cmd_args $kselftest_args"
 		if [ ! -x "$TEST" ]; then
 			echo "# Warning: file $TEST is not executable"
 
-- 
2.17.1

Re: [PATCH] selftests/kselftest/runner.sh: Add optional command parameters in settings

[Kees Cook]

On Thu, Dec 02, 2021 at 02:20:56PM +0000, Cristian Marussi wrote:
> Some testcases allow for optional commandline parameters but as of now
> there is now way to provide such arguments to the runner script.
> 
> Add support to the per-test-directory "settings" file to provide such
> optional arguments; two new optional fields can now be defined in
> "settings":
> 
>  - args="<options>": general arguments common to all testcase commands in
>    the test directory
> 
>  - <BASENAME_TEST>_args="<options>": custom arguments specific to only one
>    specific testcase command
> 
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
> Used to configure the use of a specific rtc device on CI systems with:
>  tools/testing/selftests/rtc/settings:
>    timeout=90
>    rtctest_args="/dev/rtc1"

I like this idea generally, but I have some concern that this is
muddling the test's settings ("do not expect me to finish before
timeout=90") vs the local system's settings ("here is where to find the
rtc to test"). I can't, however, think of a better way to handle this
currently. :P

Is this case common enough that a given test shouldn't, instead, just
take config from environment variables set by the CI?

(Also, will we need to worry in the future about running the same test
multiple times with different system settings? ("try each of these /dev
nodes...")

Is there a patch for the changes to the RTC test?

> ---
>  tools/testing/selftests/kselftest/runner.sh | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/kselftest/runner.sh b/tools/testing/selftests/kselftest/runner.sh
> index a9ba782d8ca0..f877a8571927 100644
> --- a/tools/testing/selftests/kselftest/runner.sh
> +++ b/tools/testing/selftests/kselftest/runner.sh
> @@ -49,6 +49,15 @@ run_one()
>  
>  	# Reset any "settings"-file variables.
>  	export kselftest_timeout="$kselftest_default_timeout"
> +
> +	# Optional arguments for any command, possibly defined in settings
> +	# as args="<options>"
> +	kselftest_args=""
> +
> +	# Optional arguments for this command, possibly defined in settings
> +	# as <$BASENAME_TEST>_args="<options>"
> +	kselftest_cmd_args_ref="kselftest_${BASENAME_TEST}_args"
> +
>  	# Load per-test-directory kselftest "settings" file.
>  	settings="$BASE_DIR/$DIR/settings"
>  	if [ -r "$settings" ] ; then
> @@ -69,7 +78,8 @@ run_one()
>  		echo "# Warning: file $TEST is missing!"
>  		echo "not ok $test_num $TEST_HDR_MSG"
>  	else
> -		cmd="./$BASENAME_TEST"
> +		eval kselftest_cmd_args="\$$kselftest_cmd_args_ref"

nitpit: Just to avoid tripping any future work to gracefully handle
unset variables, maybe this could specify an empty-string default:

		eval kselftest_cmd_args="\${$kselftest_cmd_args_ref:-}"

> +		cmd="./$BASENAME_TEST $kselftest_cmd_args $kselftest_args"
>  		if [ ! -x "$TEST" ]; then
>  			echo "# Warning: file $TEST is not executable"
>  
> -- 
> 2.17.1
> 

-- 
Kees Cook

Re: [PATCH] selftests/kselftest/runner.sh: Add optional command parameters in settings

[Cristian Marussi]

On Thu, Dec 02, 2021 at 01:18:35PM -0800, Kees Cook wrote:
> On Thu, Dec 02, 2021 at 02:20:56PM +0000, Cristian Marussi wrote:
> > Some testcases allow for optional commandline parameters but as of now
> > there is now way to provide such arguments to the runner script.
> > 
> > Add support to the per-test-directory "settings" file to provide such
> > optional arguments; two new optional fields can now be defined in
> > "settings":
> > 
> >  - args="<options>": general arguments common to all testcase commands in
> >    the test directory
> > 
> >  - <BASENAME_TEST>_args="<options>": custom arguments specific to only one
> >    specific testcase command
> > 
> > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> > ---

Hi Kees,

thanks for the review.

> > Used to configure the use of a specific rtc device on CI systems with:
> >  tools/testing/selftests/rtc/settings:
> >    timeout=90
> >    rtctest_args="/dev/rtc1"
> 
> I like this idea generally, but I have some concern that this is
> muddling the test's settings ("do not expect me to finish before
> timeout=90") vs the local system's settings ("here is where to find the
> rtc to test"). I can't, however, think of a better way to handle this
> currently. :P
> 

The idea stems from the need to workaround some broken setup in CI, but
beside this (which is opinable) then I realized that, indeed, the rtctest
could already accept as first argument the rtc to use but simply there's
no way as of now to pass any argument down to the tests through the
run_kselftest/runner scripts, so, it seemed to me a general nice to have
for any kind of test that needs local configuration, beside the specific
issue I was facing.

So basically enabling a mechanism that allows me to tell the CI automation
guys how to overlay a specific platform tests-config on top of the default
one found in setting, right before the test starts.

At the end, both timeouts and test-specific args are configuration params
with the only difference that the first is general the second address
test specific capabilities.
(there are quite a few: egrep -R "argv\[1\]" tools/testing/selftests/)

> Is this case common enough that a given test shouldn't, instead, just
> take config from environment variables set by the CI?
> 
That would mean patch every single test though, right ? or do you mean
maybe instead using the same variable indirection mechanism as it is now
but let the CI system provide the exported var instead of picking it from
the overlayed settings file like:

export kselftest_rtctest_args=/dev/rtc1; /opt/ksft/run_kselftest.sh -c rtc

...in fact it works fine on my local setup and maybe it's better than the
overlaying thing for the CI perspective

> (Also, will we need to worry in the future about running the same test
> multiple times with different system settings? ("try each of these /dev
> nodes...")
> 

I would let this to be in charge of the CI automation machinery (if they
want to run multiple runs with different setups and so different ENVs)

> Is there a patch for the changes to the RTC test?
> 

The rtctest already interprets argv[1] as the rtc to use, if provided and,
as said, I was thinking to use this as a CI-directed overlaying mechanism,
so that's no need of further patches to the test to be commited upstream
(if this was what you meant)

> > ---
> >  tools/testing/selftests/kselftest/runner.sh | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/testing/selftests/kselftest/runner.sh b/tools/testing/selftests/kselftest/runner.sh
> > index a9ba782d8ca0..f877a8571927 100644
> > --- a/tools/testing/selftests/kselftest/runner.sh
> > +++ b/tools/testing/selftests/kselftest/runner.sh
> > @@ -49,6 +49,15 @@ run_one()
> >  
> >  	# Reset any "settings"-file variables.
> >  	export kselftest_timeout="$kselftest_default_timeout"
> > +
> > +	# Optional arguments for any command, possibly defined in settings
> > +	# as args="<options>"
> > +	kselftest_args=""
> > +
> > +	# Optional arguments for this command, possibly defined in settings
> > +	# as <$BASENAME_TEST>_args="<options>"
> > +	kselftest_cmd_args_ref="kselftest_${BASENAME_TEST}_args"
> > +
> >  	# Load per-test-directory kselftest "settings" file.
> >  	settings="$BASE_DIR/$DIR/settings"
> >  	if [ -r "$settings" ] ; then
> > @@ -69,7 +78,8 @@ run_one()
> >  		echo "# Warning: file $TEST is missing!"
> >  		echo "not ok $test_num $TEST_HDR_MSG"
> >  	else
> > -		cmd="./$BASENAME_TEST"
> > +		eval kselftest_cmd_args="\$$kselftest_cmd_args_ref"
> 
> nitpit: Just to avoid tripping any future work to gracefully handle
> unset variables, maybe this could specify an empty-string default:
> 
> 		eval kselftest_cmd_args="\${$kselftest_cmd_args_ref:-}"
> 

Right, I'll fix.

Thanks again,
Cristian

Re: [selftests/kselftest/runner.sh] 3226b4a464: kernel-selftests.cpu-hotplug.cpu-on-off-test.sh.fail

[Cristian Marussi]

On Mon, Dec 13, 2021 at 01:20:44PM +0800, kernel test robot wrote:
> 
> 
> Greeting,
> 
> FYI, we noticed the following commit (built with gcc-9):
> 

Hi,

> commit: 3226b4a4648c6562c642947cee8b90335df911f5 ("[PATCH] selftests/kselftest/runner.sh: Add optional command parameters in settings")
> url: https://github.com/0day-ci/linux/commits/Cristian-Marussi/selftests-kselftest-runner-sh-Add-optional-command-parameters-in-settings/20211202-222205
> base: https://git.kernel.org/cgit/linux/kernel/git/shuah/linux-kselftest.git next
> patch link: https://lore.kernel.org/linux-kselftest/20211202142056.17386-1-cristian.marussi@arm.com
> 
> in testcase: kernel-selftests
> version: kernel-selftests-x86_64-99d09ee9-1_20211206
> with following parameters:
> 
> 	group: group-01
> 	ucode: 0xde
> 
> test-description: The kernel contains a set of "self tests" under the tools/testing/selftests/ directory. These are intended to be small unit tests to exercise individual code paths in the kernel.
> test-url: https://www.kernel.org/doc/Documentation/kselftest.txt
> 
> 

This commit was under discussion/not acked nor fully reviewed, not ready
for upstream queuing really.

Indeed I posted today a V2 that take a different approach:

https://lore.kernel.org/linux-kselftest/20211215095340.50717-1-cristian.marussi@arm.com/T/#u

and should solve also the issue mentioned in this report.

Thanks,
Cristian