diff mbox series

[2/2] shell: Extend timeout tests, to run on multiple shells

Message ID 20210519063109.224352-2-lkml@jv-coder.de
State Rejected
Headers show
Series [1/2] shell: Fix timeout process termination for zsh | expand

Commit Message

Joerg Vehlow May 19, 2021, 6:31 a.m. UTC
From: Joerg Vehlow <joerg.vehlow@aox-tech.de>

There are some differences especially in signal handling
between the shells, so execute the tests on as many
shells as possible.

Signed-off-by: Joerg Vehlow <joerg.vehlow@aox-tech.de>
---
 lib/newlib_tests/shell/test_timeout.sh | 242 +++++++++++++++++--------
 1 file changed, 169 insertions(+), 73 deletions(-)

Comments

Cyril Hrubis May 19, 2021, 10:28 a.m. UTC | #1
Hi!
> There are some differences especially in signal handling
> between the shells, so execute the tests on as many
> shells as possible.

I'm not sure that we want to support anything but bash, dash and
busybox and even these three are enough trouble.

One of my friends once told me that it's easier to write a portable
shell than portable shell code and it looks like he was right...
Joerg Vehlow May 19, 2021, 11:03 a.m. UTC | #2
HI Cyril,

On 5/19/2021 12:28 PM, Cyril Hrubis wrote:
> Hi!
>> There are some differences especially in signal handling
>> between the shells, so execute the tests on as many
>> shells as possible.
> I'm not sure that we want to support anything but bash, dash and
> busybox and even these three are enough trouble.
>
> One of my friends once told me that it's easier to write a portable
> shell than portable shell code and it looks like he was right...
>
In general I would say: YES
But if at some point in the future there are features used, that really 
only work for one shell,
we can still reduce the tested shells for this script.

Btw: ksh is not really support, it complains about all local variables,
because it only allows them in "function <name>"-style functions.
But the timeout code still works even there.

Jörg
Petr Vorel May 19, 2021, 12:57 p.m. UTC | #3
> HI Cyril,

> On 5/19/2021 12:28 PM, Cyril Hrubis wrote:
> > Hi!
> > > There are some differences especially in signal handling
> > > between the shells, so execute the tests on as many
> > > shells as possible.
> > I'm not sure that we want to support anything but bash, dash and
> > busybox and even these three are enough trouble.

> > One of my friends once told me that it's easier to write a portable
> > shell than portable shell code and it looks like he was right...

> In general I would say: YES
> But if at some point in the future there are features used, that really only
> work for one shell,
> we can still reduce the tested shells for this script.

> Btw: ksh is not really support, it complains about all local variables,
> because it only allows them in "function <name>"-style functions.
> But the timeout code still works even there.

Although I generally agree that scrips should be portable to some extent
(it's already hard trying to keep it POSIX), it should be for shells which are
used on Linux distros as a default shell (/bin/sh). That's why besides the
default bash on most of distros we also care about dash (Debian/Ubuntu by
default) and busybox sh implementation (embedded distros and Alpine).
I'm not aware of any distro using ksh, csh or zsh as a default.

Trying to remember when I met ksh as a default, I guess it was on some old
Solaris, more than decade ago :).

We state this in shell shoding style chapter [1]. Maybe I should have added it
into Supported kernel ... page [2], but not many people run shell tests anyway.

Kind regards,
Petr

[1] https://github.com/linux-test-project/ltp/wiki/Test-Writing-Guidelines#132-shell-coding-style
[2] https://github.com/linux-test-project/ltp/wiki/Supported-kernel,-libc,-toolchain-versions

> Jörg
Petr Vorel June 21, 2021, 7:25 a.m. UTC | #4
Hi all,

I wonder what is the state of this patchset?
Do we still consider it?
Apart from reducing shells (i.e. which distro has ash, which is not
alias to other shell in the list) I'm not against non-default shells, but I
don't like how whole test gets complicated by this.

Also we're reinventing wheel with printing results, checking whether test exist
etc. Maybe using test API for this?

Kind regards,
Petr
Joerg Vehlow June 21, 2021, 8:01 a.m. UTC | #5
Hi Petr,

On 6/21/2021 9:25 AM, Petr Vorel wrote:
> Hi all,
>
> I wonder what is the state of this patchset?
> Do we still consider it?
I still think this should be included, but we can drop shells if you like.
> Apart from reducing shells (i.e. which distro has ash, which is not
> alias to other shell in the list) I'm not against non-default shells, but I
> don't like how whole test gets complicated by this.
>
> Also we're reinventing wheel with printing results, checking whether test exist
> etc. Maybe using test API for this?
I know that this is reimplementing a lot of stuff, but I think I tried 
using the lib (a bit strange using the object under test to test it, but 
would be ok for me), but failed to do so.
I guess this was because it uses traps and the set -m stuff, but I am 
not sure anymore.

Jörg
Petr Vorel June 21, 2021, 8:19 a.m. UTC | #6
Hi Joerg,

> Hi Petr,

> On 6/21/2021 9:25 AM, Petr Vorel wrote:
> > Hi all,

> > I wonder what is the state of this patchset?
> > Do we still consider it?
> I still think this should be included, but we can drop shells if you like.
> > Apart from reducing shells (i.e. which distro has ash, which is not
> > alias to other shell in the list) I'm not against non-default shells, but I
> > don't like how whole test gets complicated by this.

> > Also we're reinventing wheel with printing results, checking whether test exist
> > etc. Maybe using test API for this?
> I know that this is reimplementing a lot of stuff, but I think I tried using
> the lib (a bit strange using the object under test to test it, but would be
> ok for me), but failed to do so.
> I guess this was because it uses traps and the set -m stuff, but I am not
> sure anymore.
Yes, using lib for testing it's a bit strange. But test_timeout.sh is already
messy and patch increases it even more. I'd prefer to implement the test
coverage in a cleaner way. Probably to create some very basic testing library,
a wrapper of actual tests.

There was some effort [1], based on my previous work, which concentrated on
having metadata of expected output. That allows to run tests which TBROK by
purpose (quite a lot of tests). I plan to get back to it, but but first I'd like
to create make target for running tests [2].

Kind regards,
Petr

> Jörg

[1] https://patchwork.ozlabs.org/project/ltp/patch/ce675759672af52bea02c11d51bd7d10f0bcb5cb.1566500817.git.clanig@suse.com/
[2] https://patchwork.ozlabs.org/project/ltp/patch/20210618191252.12403-1-pvorel@suse.cz/
diff mbox series

Patch

diff --git a/lib/newlib_tests/shell/test_timeout.sh b/lib/newlib_tests/shell/test_timeout.sh
index b05680cb1..152f51c32 100755
--- a/lib/newlib_tests/shell/test_timeout.sh
+++ b/lib/newlib_tests/shell/test_timeout.sh
@@ -5,6 +5,15 @@ 
 
 PATH="$(dirname $0):$(dirname $0)/../../../testcases/lib/:$PATH"
 
+SHELLS="
+bash
+dash
+busybox sh
+zsh
+ash
+ksh
+"
+
 # Test cases are separated by newlines.
 # Every test has the following fields in this order:
 # file
@@ -33,12 +42,23 @@  timeout04.sh|     |0|  |  2|0|0|1|Verify that timeout is enforced
 timeout02.sh|    2|1| 2|   |1|0|0|Test termination of timeout process
 "
 
+run_shell()
+{
+	local shell=$1
+	shift
+
+	eval $shell "$@"
+	return $?
+}
+
 # Executes a test
 # Parameter:
-#  - test:    The test to execute
-#  - timeout: The timeout multiplicator (optional)
-#  - use_cat: Pipe the output of the command through cat (optional)
-#             If this is used, the exit code is NOT returned!
+#  - shell:    The shell to be used while executing the test
+#  - shellarg: First parameter of shell (e.g. for busybox sh)
+#  - test:     The test to execute
+#  - timeout:  The timeout multiplicator (optional)
+#  - use_cat:  Pipe the output of the command through cat (optional)
+#              If this is used, the exit code is NOT returned!
 #
 # The function returns the following global variables:
 # - test_exit:     The exit code of the test
@@ -47,11 +67,13 @@  timeout02.sh|    2|1| 2|   |1|0|0|Test termination of timeout process
 # - test_passed:   The number of passed tests parsed from the summary
 # - test_failed:   The number of failed tests parsed from the summary
 # - test_broken:   The number of broken tests parsed from the summary
-run_test()
+exec_test()
 {
-	local test=$1
-	local timeout=$2
-	local use_cat=$3
+	local shell=$1
+	local shellarg=$2
+	local test=$3
+	local timeout=$4
+	local use_cat=$5
 	local tmpfile start end;
 
 	tmpfile=$(mktemp -t ltp_timeout_XXXXXXXX)
@@ -70,9 +92,9 @@  run_test()
 	# the sleep process was never killed, when a test finished
 	# before the timeout.
 	if [ "$use_cat" = "1" ]; then
-		LTP_TIMEOUT_MUL=$timeout $test 2>&1 | cat >$tmpfile
+		LTP_TIMEOUT_MUL=$timeout $shell $shellarg $test 2>&1 | cat >$tmpfile
 	else
-		LTP_TIMEOUT_MUL=$timeout $test 1>$tmpfile 2>&1
+		LTP_TIMEOUT_MUL=$timeout $shell $shellarg $test 1>$tmpfile 2>&1
 	fi
 	test_exit=$?
 	set +m
@@ -82,6 +104,9 @@  run_test()
 	test_output=$(cat $tmpfile)
 	rm $tmpfile
 
+	test_passed=-1
+	test_failed=-1
+	test_broken=-1
 	eval $(echo "$test_output" | awk '
 		BEGIN {sum=0}
 		$1 == "Summary:" {
@@ -96,83 +121,154 @@  run_test()
 	')
 }
 
-echo "Testing timeout in shell API"
-echo
+do_test()
+{
+	local failed test_nr old_ifs shell CLEANED_DATA test_max
+	local file cur_fails timeout use_cat max_runtime exp_exit
+	local exp_passed exp_failed exp_broken description
+	
+	shell="$1"
+	shellarg="$2"
 
-failed=0
-test_nr=0
+	failed=0
+	test_nr=0
 
-old_ifs="$IFS"
-IFS=$(printf "\n\b")
+	old_ifs="$IFS"
+	IFS=$(printf "\n\b")
 
-# Remove comments and empty lines
-CLEANED_DATA=$(echo "$DATA" | sed '/^\s*#/d;/^\s*$/d')
-test_max=$(echo "$CLEANED_DATA" | wc -l)
-for d in $CLEANED_DATA; do
+	# Remove comments and empty lines
+	CLEANED_DATA=$(echo "$DATA" | sed '/^\s*#/d;/^\s*$/d')
+	test_max=$(echo "$CLEANED_DATA" | wc -l)
+	for d in $CLEANED_DATA; do
+		IFS="$old_ifs"
+
+		file=$(echo $d | cut -d'|' -f1 | xargs)
+		timeout=$(echo $d | cut -d'|' -f2 | xargs)
+		use_cat=$(echo $d | cut -d'|' -f3 | xargs)
+		max_runtime=$(echo $d | cut -d'|' -f4 | xargs)
+		max_runtime=${max_runtime:--1}
+		exp_exit=$(echo $d | cut -d'|' -f5 | xargs)
+		exp_exit=${exp_exit:--1}
+		exp_passed=$(echo $d | cut -d'|' -f6 | xargs)
+		exp_passed=${exp_passed:--1}
+		exp_failed=$(echo $d | cut -d'|' -f7 | xargs)
+		exp_failed=${exp_failed:--1}
+		exp_broken=$(echo $d | cut -d'|' -f8 | xargs)
+		exp_broken=${exp_broken:--1}
+		description=$(echo $d | cut -d'|' -f9)
+
+		test_nr=$(($test_nr + 1))
+
+		cur_fails=0
+
+		if [ -z "$description" ]; then
+			description="$file (LTP_TIMEOUT_MUL='$timeout')"
+		fi
+
+		echo "=== $test_nr/$test_max $description ==="
+		exec_test "$shell" "$shellarg" "$file" "$timeout" "$use_cat"
+
+		if [ $max_runtime -ne -1 ] && [ $test_duration -gt $max_runtime ]; then
+			echo "FAILED (runtime: $test_duration, expected less than $max_runtime)"
+			cur_fails=$((cur_fails + 1))
+		fi
+
+		if [ $exp_passed -ne -1 ] && [ $exp_passed -ne $test_passed ]; then
+			echo "FAILED (passes: $test_passed, expected $exp_passed)"
+			cur_fails=$((cur_fails + 1))
+		fi
+
+		if [ $exp_failed -ne -1 ] && [ $exp_failed -ne $test_failed ]; then
+			echo "FAILED (failed: $test_failed, expected $exp_failed)"
+			cur_fails=$((cur_fails + 1))
+		fi
+
+		if [ $exp_broken -ne -1 ] && [ $exp_broken -ne $test_broken ]; then
+			echo "FAILED (broken: $test_broken, expected $exp_broken)"
+			cur_fails=$((cur_fails + 1))
+		fi
+
+		if [ $exp_exit -ne -1 ] && [ $test_exit -ne $exp_exit ]; then
+			echo "FAILED (exit code: $test_exit, expected $exp_exit)"
+			cur_fails=$((cur_fails + 1))
+		fi
+
+		if [ $cur_fails -gt 0 ]; then
+			failed=$((failed + 1))
+			echo "--------"
+			echo "$test_output"
+			echo "--------"
+		else
+			echo "PASSED"
+		fi
+		echo
+	done
 	IFS="$old_ifs"
 
-	file=$(echo $d | cut -d'|' -f1 | xargs)
-	timeout=$(echo $d | cut -d'|' -f2 | xargs)
-	use_cat=$(echo $d | cut -d'|' -f3 | xargs)
-	max_runtime=$(echo $d | cut -d'|' -f4 | xargs)
-	max_runtime=${max_runtime:--1}
-	exp_exit=$(echo $d | cut -d'|' -f5 | xargs)
-	exp_exit=${exp_exit:--1}
-	exp_passed=$(echo $d | cut -d'|' -f6 | xargs)
-	exp_passed=${exp_passed:--1}
-	exp_failed=$(echo $d | cut -d'|' -f7 | xargs)
-	exp_failed=${exp_failed:--1}
-	exp_broken=$(echo $d | cut -d'|' -f8 | xargs)
-	exp_broken=${exp_broken:--1}
-	description=$(echo $d | cut -d'|' -f9)
-
-	test_nr=$(($test_nr + 1))
-
-	cur_fails=0
-
-	if [ -z "$description" ]; then
-		description="$file (LTP_TIMEOUT_MUL='$timeout')"
-	fi
+	echo "Failed tests: $failed"
+	return $failed
+}
 
-	echo "=== $test_nr/$test_max $description ==="
-	run_test "$file" "$timeout" "$use_cat"
 
-	if [ $max_runtime -ne -1 ] && [ $test_duration -gt $max_runtime ]; then
-		echo "FAILED (runtime: $test_duration, expected less than $max_runtime)"
-		cur_fails=$((cur_fails + 1))
+print_results()
+{
+	echo
+	if [ -n "$raw_shell" ]; then
+		result=$(printf "%s\n%-15s %s" "$result" "$raw_shell" "INTERRUPTED")
+		failed_shells=$((failed_shells + 1))
 	fi
-
-	if [ $exp_passed -ne -1 ] && [ $exp_passed -ne $test_passed ]; then
-		echo "FAILED (passes: $test_passed, expected $exp_passed)"
-		cur_fails=$((cur_fails + 1))
+	echo
+	echo "----------------------------------------"
+	echo
+	echo "Summary:"
+	echo "$result"
+	echo
+	if [ $failed_shells -ne 0 ]; then
+		echo "A total number of $total_fails failed for $failed_shells shells"
+	else
+		echo "All tests passed"
 	fi
+}
 
-	if [ $exp_failed -ne -1 ] && [ $exp_failed -ne $test_failed ]; then
-		echo "FAILED (failed: $test_failed, expected $exp_failed)"
-		cur_fails=$((cur_fails + 1))
-	fi
+# For some reason at least in zsh, it can happen, that the whole
+# testrunner is killed, when the test result is piped through cat.
+# If the test was aborted using CTRL^C or kill, the output can be ignored,
+# otherwise these messages should never be visible.
+trap 'echo; echo "Test unexpectedly killed by SIGINT."; print_results; exit 1' INT
+trap 'echo; echo "Test unexpectedly killed by SIGTERM."; print_results; exit 1' TERM
 
-	if [ $exp_broken -ne -1 ] && [ $exp_broken -ne $test_broken ]; then
-		echo "FAILED (broken: $test_broken, expected $exp_broken)"
-		cur_fails=$((cur_fails + 1))
-	fi
+old_ifs="$IFS"
+IFS=$(printf "\n\b")
 
-	if [ $exp_exit -ne -1 ] && [ $test_exit -ne $exp_exit ]; then
-		echo "FAILED (exit code: $test_exit, expected $exp_exit)"
-		cur_fails=$((cur_fails + 1))
-	fi
+failed_shells=0
+total_fails=0
 
-	if [ $cur_fails -gt 0 ]; then
-		failed=$((failed + 1))
-		echo "--------"
-		echo "$test_output"
-		echo "--------"
+# Remove comments and empty lines
+CLEANED_SHELLS=$(echo "$SHELLS" | sed '/^\s*#/d;/^\s*$/d')
+shell_max=$(echo "$CLEANED_SHELLS" | wc -l)
+shell_nr=0
+result=""
+for raw_shell in $CLEANED_SHELLS; do
+	shell_nr=$(( shell_nr + 1 ))
+	echo "($shell_nr/$shell_max) Testing timeout in shell API with '$raw_shell'"
+	shellarg=$(echo "$raw_shell" | cut -sd' ' -f2)
+	shell=$(echo "$raw_shell" | cut -d' ' -f1)
+	res="BROKEN"
+	if ! $shell $shellarg -c true 2>/dev/null; then
+		echo "SKIPED: Shell not found"
+		res="SKIPED"
 	else
-		echo "PASSED"
+		res="PASSED"
+		do_test "$shell" "$shellarg"
+		if [ $? -ne 0 ]; then
+			res="FAILED ($?)"
+			total_fails=$((total_fails + $?))
+			failed_shells=$((failed_shells + 1))
+		fi
 	fi
-	echo
+	result=$(printf "%s\n%-15s %s" "$result" "$raw_shell" "$res")
 done
-IFS="$old_ifs"
+raw_shell=""
 
-echo "Failed tests: $failed"
-exit $failed
+print_results
+exit $failed_shells