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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> > ---
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
Greeting,
FYI, we noticed the following commit (built with gcc-9):
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/[email protected]
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
on test machine: 8 threads 1 sockets Intel(R) Core(TM) i7-7700K CPU @ 4.20GHz with 32G memory
caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace):
If you fix the issue, kindly add following tag
Reported-by: kernel test robot <[email protected]>
make: Leaving directory '/usr/src/perf_selftests-x86_64-rhel-8.3-kselftests-3226b4a4648c6562c642947cee8b90335df911f5/tools/testing/selftests/cpu-hotplug'
2021-12-08 12:21:53 make run_tests -C cpu-hotplug
make: Entering directory '/usr/src/perf_selftests-x86_64-rhel-8.3-kselftests-3226b4a4648c6562c642947cee8b90335df911f5/tools/testing/selftests/cpu-hotplug'
TAP version 13
1..1
# selftests: cpu-hotplug: cpu-on-off-test.sh
# ./cpu-on-off-test.sh: illegal option -- o
# ./cpu-on-off-test.sh: illegal option -- n
# ./cpu-on-off-test.sh: illegal option -- -
# ./cpu-on-off-test.sh: illegal option -- o
# ./cpu-on-off-test.sh: illegal option -- f
# ./cpu-on-off-test.sh: illegal option -- f
# ./cpu-on-off-test.sh: illegal option -- -
# ./cpu-on-off-test.sh: illegal option -- t
# ./cpu-on-off-test.sh: line 181: [: st.sh_args: integer expression expected
# error code must be -4095 <= errno < 0
not ok 1 selftests: cpu-hotplug: cpu-on-off-test.sh # exit=1
make: Leaving directory '/usr/src/perf_selftests-x86_64-rhel-8.3-kselftests-3226b4a4648c6562c642947cee8b90335df911f5/tools/testing/selftests/cpu-hotplug'
To reproduce:
git clone https://github.com/intel/lkp-tests.git
cd lkp-tests
sudo bin/lkp install job.yaml # job file is attached in this email
bin/lkp split-job --compatible job.yaml # generate the yaml file for lkp run
sudo bin/lkp run generated-yaml-file
# if come across any failure that blocks the test,
# please remove ~/.lkp and /lkp dir to run from a clean state.
---
0DAY/LKP+ Test Infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/[email protected] Intel Corporation
Thanks,
Oliver Sang
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/[email protected]
>
> 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/[email protected]/T/#u
and should solve also the issue mentioned in this report.
Thanks,
Cristian