[RFC,1/2] selftests: Start shell API
diff mbox series

Message ID 20190406214915.16914-2-pvorel@suse.cz
State New
Headers show
Series
  • Kselftest shell (or even C) API
Related show

Commit Message

Petr Vorel April 6, 2019, 9:49 p.m. UTC
kselftest.sh is a beginning of shell API.
ATM it's a stub (target could be as rich as LTP API), containing only:
* exit codes
* filling TEST variable
* logging functions
* requiring root function
* add script directory into PATH

Inspired by kexec functions (with some cleanup)
and LTP.

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 tools/testing/selftests/kselftest.sh | 53 ++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)
 create mode 100644 tools/testing/selftests/kselftest.sh

Comments

Mimi Zohar April 8, 2019, 11:06 a.m. UTC | #1
Hi Petr,

On Sat, 2019-04-06 at 23:49 +0200, Petr Vorel wrote:
> kselftest.sh is a beginning of shell API.
> ATM it's a stub (target could be as rich as LTP API), containing only:
> * exit codes
> * filling TEST variable
> * logging functions
> * requiring root function
> * add script directory into PATH
> 
> Inspired by kexec functions (with some cleanup)
> and LTP.
> 
> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
>  tools/testing/selftests/kselftest.sh | 53 ++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
>  create mode 100644 tools/testing/selftests/kselftest.sh
> 
> diff --git a/tools/testing/selftests/kselftest.sh b/tools/testing/selftests/kselftest.sh
> new file mode 100644
> index 000000000000..519ec2707dd8
> --- /dev/null
> +++ b/tools/testing/selftests/kselftest.sh
> @@ -0,0 +1,53 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2019 Petr Vorel <pvorel@suse.cz>
> +
> +PATH="$(dirname $0):$PATH"
> +
> +KSFT_PASS=0
> +KSFT_FAIL=1
> +KSFT_XFAIL=2
> +KSFT_XPASS=3
> +KSFT_SKIP=4

The kexec tests only defined functions for PASS, FAIL, and SKIP.  What
is the difference between KSFT_FAIL and KSFT_XFAIL, and similarly
between KSFT_PASS and KSFT_XPASS?  Either here or above the functions
should be a comment.

> +
> +TEST=$(basename $0)
> +
> +ksft_info()
> +{
> +	echo "[INFO] $TEST: $1"
> +}


The "ksft_" prefix is good.

Mimi

> +ksft_pass()
> +{
> +	echo "[PASS] $TEST: $1"
> +	exit $KSFT_PASS
> +}
> +
> +ksft_fail()
> +{
> +	echo "[FAIL] $TEST: $1"
> +	exit $KSFT_FAIL
> +}
> +
> +ksft_xfail()
> +{
> +	echo "[FAIL] $TEST: $1"
> +	exit $KSFT_XFAIL
> +}

> +ksft_xpass()
> +{
> +	echo "[PASS] $TEST: $1"
> +	exit $KSFT_XPASS
> +}
> +
> +ksft_skip()
> +{
> +	echo "[SKIP] $TEST: $1"
> +	exit $KSFT_SKIP
> +}
> +
> +ksft_require_root()
> +{
> +	[ $(id -ru) -eq 0 ] || ksft_skip "requires root privileges"
> +}
Cyril Hrubis April 8, 2019, 11:38 a.m. UTC | #2
Hi!
> +ksft_pass()
> +{
> +	echo "[PASS] $TEST: $1"
> +	exit $KSFT_PASS
> +}
> +
> +ksft_fail()
> +{
> +	echo "[FAIL] $TEST: $1"
> +	exit $KSFT_FAIL
> +}

I think that the main disadvantage here is that these functions call
exit instead of storing the results which leads to a common pattern of
passing the result up the function call chain which is prone to errors.

What I have learned the hard way over the years is that the result
reporting should be separated from the functions that exit the tests and
that the test code should not be trusted with passing the overall test
result at the end. I've seen too many cases where the actuall failure
was ignored becaues the failure was lost on it's way to the main
function.

Another lesson is that tests shouldn't implement the main() function,
that is something that the test library should do, which allows for
resources to be listed in a declarative way instead of calling init
funcitons at the start of the tests. Which means that in LTP you can say
"mount at least 512MB device formatted with ext4 to this mount point"
and all this handled in the test library before the actual test starts.

As the last point this completely misses a cleanup callback support,
i.e. function that is called to clean up if you need to exit in the
middle of a test in a case of an error.
Petr Vorel April 8, 2019, 12:22 p.m. UTC | #3
Hi Mimi,

> > +++ b/tools/testing/selftests/kselftest.sh
> > @@ -0,0 +1,53 @@
> > +#!/bin/sh
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Copyright (c) 2019 Petr Vorel <pvorel@suse.cz>
> > +
> > +PATH="$(dirname $0):$PATH"
> > +
> > +KSFT_PASS=0
> > +KSFT_FAIL=1
> > +KSFT_XFAIL=2
> > +KSFT_XPASS=3
> > +KSFT_SKIP=4

> The kexec tests only defined functions for PASS, FAIL, and SKIP.  What
> is the difference between KSFT_FAIL and KSFT_XFAIL, and similarly
> between KSFT_PASS and KSFT_XPASS?  Either here or above the functions
> should be a comment.
I guess xfail and xpass are taken from pytest [1].
I took them from kselftest.h, in order to be somehow compatible with existing C
API. But grepping code xpass is never used (not even in list of kselftest results [2]),
xfail is used in about 4 tests (binderfs, ftrace, pidfd, seccomp).

But I'm not a big fan of this pytest terminology "something is resulting the
opposite than expected", IMHO simple pass and fail are enough.
On the other hand I miss "test failed in preparation phase" (TBROK in LTP),
skip has different meaning.

Kind regards,
Petr

[1] https://docs.pytest.org/en/latest/skipping.html
[2] https://www.spinics.net/lists/linux-kselftest/msg06651.html
Petr Vorel April 8, 2019, 1:07 p.m. UTC | #4
Hi,

> > +ksft_pass()
> > +{
> > +	echo "[PASS] $TEST: $1"
> > +	exit $KSFT_PASS
> > +}
> > +
> > +ksft_fail()
> > +{
> > +	echo "[FAIL] $TEST: $1"
> > +	exit $KSFT_FAIL
> > +}

> I think that the main disadvantage here is that these functions call
> exit instead of storing the results which leads to a common pattern of
> passing the result up the function call chain which is prone to errors.

> What I have learned the hard way over the years is that the result
> reporting should be separated from the functions that exit the tests and
> that the test code should not be trusted with passing the overall test
> result at the end. I've seen too many cases where the actuall failure
> was ignored becaues the failure was lost on it's way to the main
> function.

> Another lesson is that tests shouldn't implement the main() function,
> that is something that the test library should do, which allows for
> resources to be listed in a declarative way instead of calling init
> funcitons at the start of the tests. Which means that in LTP you can say
> "mount at least 512MB device formatted with ext4 to this mount point"
> and all this handled in the test library before the actual test starts.

> As the last point this completely misses a cleanup callback support,
> i.e. function that is called to clean up if you need to exit in the
> middle of a test in a case of an error.

Agree with all mentioned. My patchset was mainly to bring the discussion.
Although library defining some general functions and constants to reduce
duplicity is itself a small improvement, kselftest deserves a proper API.
For both C and shell.

Kind regards,
Petr

Patch
diff mbox series

diff --git a/tools/testing/selftests/kselftest.sh b/tools/testing/selftests/kselftest.sh
new file mode 100644
index 000000000000..519ec2707dd8
--- /dev/null
+++ b/tools/testing/selftests/kselftest.sh
@@ -0,0 +1,53 @@ 
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2019 Petr Vorel <pvorel@suse.cz>
+
+PATH="$(dirname $0):$PATH"
+
+KSFT_PASS=0
+KSFT_FAIL=1
+KSFT_XFAIL=2
+KSFT_XPASS=3
+KSFT_SKIP=4
+
+TEST=$(basename $0)
+
+ksft_info()
+{
+	echo "[INFO] $TEST: $1"
+}
+
+ksft_pass()
+{
+	echo "[PASS] $TEST: $1"
+	exit $KSFT_PASS
+}
+
+ksft_fail()
+{
+	echo "[FAIL] $TEST: $1"
+	exit $KSFT_FAIL
+}
+
+ksft_xfail()
+{
+	echo "[FAIL] $TEST: $1"
+	exit $KSFT_XFAIL
+}
+
+ksft_xpass()
+{
+	echo "[PASS] $TEST: $1"
+	exit $KSFT_XPASS
+}
+
+ksft_skip()
+{
+	echo "[SKIP] $TEST: $1"
+	exit $KSFT_SKIP
+}
+
+ksft_require_root()
+{
+	[ $(id -ru) -eq 0 ] || ksft_skip "requires root privileges"
+}