Re: [PATCHv2] selftests: Use ktap helpers for runner.sh

[Brendan Jackman <jackmanb@google.com>]

On Tue Feb 24, 2026 at 4:10 AM UTC, Hangbin Liu wrote:
> Instead of manually writing ktap messages, we should use the formal
> ktap helpers in runner.sh. Brendan did some work in d9e6269e3303
> ("selftests/run_kselftest.sh: exit with error if tests fail") to make
> run_kselftest.sh exit with the correct return value. However, the output
> does not include the total results, such as how many tests passed or
> failed.
>
> Let’s convert all manually printed messages in runner.sh to use the
> formal ktap helpers. Here are what I changed:
>
>   1. Move TAP header from runner.sh to run_kselftest.sh, since run_kselftest.sh
>      is the only caller of run_many().
>   2. In run_kselftest.sh, call run_many() in main process to count the
>      pass/fail numbers.
>   3. In run_kselftest.sh, do not generate kselftest_failures_file. Just
>      use ktap_print_totals to report the result.
>   4. In runner.sh run_one(), get the return value and use ktap helpers for
>      all pass/fail reporting. This allows counting pass/fail numbers in the
>      main process.
>   5. In runner.sh run_in_netns(), also return the correct rc, so we can
>      count results during wait.
>
> After the change, the printed result looks like:
>
>   not ok 4 4 selftests: clone3: clone3_cap_checkpoint_restore # exit=1
>   # Totals: pass:3 fail:1 xfail:0 xpass:0 skip:0 error:0
>
>   ]# echo $?
>   1
>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
>
> v2: Update commit description and comments in code. (Brendan Jackman)

Cool thanks sorry I did not really review the code before but now I'm
taking a closer look.

I don't parse KTAP but I did check that this doesn't break my usecase
[0] so:

Tested-By: Brendan Jackman <jackmanb@google.com>

[0]: https://github.com/bjackman/limmat-kernel-nix/blob/master/ktests.nix

> ---
>  tools/testing/selftests/kselftest/runner.sh | 84 ++++++++++++---------
>  tools/testing/selftests/run_kselftest.sh    | 21 ++++--
>  2 files changed, 63 insertions(+), 42 deletions(-)
>
> diff --git a/tools/testing/selftests/kselftest/runner.sh b/tools/testing/selftests/kselftest/runner.sh
> index 3a62039fa621..dfd6fedf8911 100644
> --- a/tools/testing/selftests/kselftest/runner.sh
> +++ b/tools/testing/selftests/kselftest/runner.sh
> @@ -1,8 +1,8 @@
> -#!/bin/sh
> +#!/bin/bash
>  # SPDX-License-Identifier: GPL-2.0
>  #
>  # Runs a set of tests in a given subdirectory.
> -export skip_rc=4
> +. $(dirname "$(readlink -e "${BASH_SOURCE[0]}")")/ktap_helpers.sh
>  export timeout_rc=124
>  export logfile=/dev/stdout
>  export per_test_logging=
> @@ -44,17 +44,11 @@ tap_timeout()
>  	fi
>  }
>  
> -report_failure()
> -{
> -	echo "not ok $*"
> -	echo "$*" >> "$kselftest_failures_file"
> -}
> -
>  run_one()
>  {
>  	DIR="$1"
>  	TEST="$2"
> -	local test_num="$3"
> +	local rc test_num="$3"
>  
>  	BASENAME_TEST=$(basename $TEST)
>  
> @@ -102,16 +96,17 @@ run_one()
>  	# Command line timeout overrides the settings file
>  	if [ -n "$kselftest_override_timeout" ]; then
>  		kselftest_timeout="$kselftest_override_timeout"
> -		echo "# overriding timeout to $kselftest_timeout" >> "$logfile"
> +		ktap_print_msg "overriding timeout to $kselftest_timeout" >> "$logfile"
>  	else
> -		echo "# timeout set to $kselftest_timeout" >> "$logfile"
> +		ktap_print_msg "timeout set to $kselftest_timeout" >> "$logfile"
>  	fi
>  
>  	TEST_HDR_MSG="selftests: $DIR: $BASENAME_TEST"
>  	echo "# $TEST_HDR_MSG"
>  	if [ ! -e "$TEST" ]; then
> -		echo "# Warning: file $TEST is missing!"
> -		report_failure "$test_num $TEST_HDR_MSG"
> +		ktap_print_msg "Warning: file $TEST is missing!"
> +		ktap_test_fail "$test_num $TEST_HDR_MSG"
> +		rc=$KSFT_FAIL
>  	else
>  		if [ -x /usr/bin/stdbuf ]; then
>  			stdbuf="/usr/bin/stdbuf --output=L "
> @@ -122,33 +117,37 @@ run_one()
>  		elif [ -x "./ksft_runner.sh" ]; then
>  			cmd="$stdbuf ./ksft_runner.sh ./$BASENAME_TEST"
>  		else
> -			echo "# Warning: file $TEST is not executable"
> +			ktap_print_msg "Warning: file $TEST is not executable"
>  
>  			if [ $(head -n 1 "$TEST" | cut -c -2) = "#!" ]
>  			then
>  				interpreter=$(head -n 1 "$TEST" | cut -c 3-)
>  				cmd="$stdbuf $interpreter ./$BASENAME_TEST"
>  			else
> -				report_failure "$test_num $TEST_HDR_MSG"
> -				return
> +				ktap_test_fail "$test_num $TEST_HDR_MSG"
> +				return $KSFT_FAIL
>  			fi
>  		fi
>  		cd `dirname $TEST` > /dev/null
> -		((((( tap_timeout "$cmd" 2>&1; echo $? >&3) |
> +		(((( tap_timeout "$cmd" 2>&1; echo $? >&3) |
>  			tap_prefix >&4) 3>&1) |
> -			(read xs; exit $xs)) 4>>"$logfile" &&
> -		echo "ok $test_num $TEST_HDR_MSG") ||
> -		(rc=$?;	\
> -		if [ $rc -eq $skip_rc ]; then	\
> -			echo "ok $test_num $TEST_HDR_MSG # SKIP"
> -		elif [ $rc -eq $timeout_rc ]; then \
> -			echo "#"
> -			report_failure "$test_num $TEST_HDR_MSG # TIMEOUT $kselftest_timeout seconds"
> +			(read xs; exit $xs)) 4>>"$logfile"
> +		rc=$?
> +		if [ "$rc" -eq "$KSFT_PASS" ]; then
> +			ktap_test_pass "$test_num $TEST_HDR_MSG"
> +		elif [ "$rc" -eq "$KSFT_SKIP" ]; then
> +			ktap_test_skip "$test_num $TEST_HDR_MSG"
> +		elif [ "$rc" -eq "$KSFT_XFAIL" ]; then
> +			ktap_test_xfail "$test_num $TEST_HDR_MSG"
> +		elif [ "$rc" -eq "$timeout_rc" ]; then
> +			ktap_test_fail "$test_num $TEST_HDR_MSG # TIMEOUT $kselftest_timeout seconds"
>  		else
> -			report_failure "$test_num $TEST_HDR_MSG # exit=$rc"
> -		fi)
> +			ktap_test_fail "$test_num $TEST_HDR_MSG # exit=$rc"
> +		fi
>  		cd - >/dev/null
>  	fi
> +
> +	return $rc
>  }
>  
>  in_netns()
> @@ -164,27 +163,34 @@ in_netns()
>  
>  run_in_netns()
>  {
> -	local netns=$(mktemp -u ${BASENAME_TEST}-XXXXXX)
>  	local tmplog="/tmp/$(mktemp -u ${BASENAME_TEST}-XXXXXX)"
> +	local netns=$(mktemp -u ${BASENAME_TEST}-XXXXXX)

Nit (no need to respin just for this, but if you're already respinning):
Diff noise here.

> +	local rc
> +
>  	ip netns add $netns
>  	if [ $? -ne 0 ]; then
> -		echo "# Warning: Create namespace failed for $BASENAME_TEST"
> -		echo "not ok $test_num selftests: $DIR: $BASENAME_TEST # Create NS failed"
> +		ktap_print_msg "Warning: Create namespace failed for $BASENAME_TEST"
> +		ktap_test_fail "$test_num selftests: $DIR: $BASENAME_TEST # Create NS failed"
>  	fi
>  	ip -n $netns link set lo up
> +
>  	in_netns $netns &> $tmplog
> +	rc=$?
> +
>  	ip netns del $netns &> /dev/null
> +	# Cat the log at once to avoid parallel netns logs.
>  	cat $tmplog
>  	rm -f $tmplog
> +	return $rc
>  }
>  
>  run_many()
>  {
> -	echo "TAP version 13"
>  	DIR="${PWD#${BASE_DIR}/}"
>  	test_num=0
> -	total=$(echo "$@" | wc -w)
> -	echo "1..$total"
> +	local rc
> +	pids=()
> +
>  	for TEST in "$@"; do
>  		BASENAME_TEST=$(basename $TEST)
>  		test_num=$(( test_num + 1 ))
> @@ -194,10 +200,20 @@ run_many()
>  		fi
>  		if [ -n "$RUN_IN_NETNS" ]; then
>  			run_in_netns &
> +			pids+=($!)
>  		else
>  			run_one "$DIR" "$TEST" "$test_num"
>  		fi
>  	done
>  
> -	wait
> +	# These variables are outputs of ktap_helpers.sh but since we've
> +	# run the test in a subprocess we need to update them manually
> +	for pid in "${pids[@]}"; do
> +		wait "$pid"
> +		rc=$?
> +		[ "$rc" -eq "$KSFT_PASS" ] && KTAP_CNT_PASS=$((KTAP_CNT_PASS+1))
> +		[ "$rc" -eq "$KSFT_FAIL" ] && KTAP_CNT_FAIL=$((KTAP_CNT_FAIL+1))
> +		[ "$rc" -eq "$KSFT_SKIP" ] && KTAP_CNT_SKIP=$((KTAP_CNT_SKIP+1))
> +		[ "$rc" -eq "$KSFT_XFAIL" ] && KTAP_CNT_XFAIL=$((KTAP_CNT_XFAIL+1))
> +	done

Can we please be consistent about how we do these two big conditional
blocks? Also should they use `case`? Also please add something to not
fail silently if $rc has a garbage value (I guess just log a warning).

FWIW I still think this is very yucky but, it seems switching to the
helper library is a worthwhile cleanup (plus, this patch drops a level
of subshell nesting in the tap_timeout pipeline, which seems like a step
in the right direction!) so I think it would be irrational to block that
cleanup just coz of this yuckiness.

>  }
> diff --git a/tools/testing/selftests/run_kselftest.sh b/tools/testing/selftests/run_kselftest.sh
> index 84d45254675c..3ffd03f1334d 100755
> --- a/tools/testing/selftests/run_kselftest.sh
> +++ b/tools/testing/selftests/run_kselftest.sh
> @@ -121,18 +121,23 @@ if [ -n "$SKIP" ]; then
>  	done
>  fi
>  
> -kselftest_failures_file="$(mktemp --tmpdir kselftest-failures-XXXXXX)"
> -export kselftest_failures_file
> -
> +curdir=$(pwd)
> +total=$(echo "$available" | wc -w)
>  collections=$(echo "$available" | cut -d: -f1 | sort | uniq)
> +
> +ktap_print_header
> +ktap_set_plan "$total"
>
>  for collection in $collections ; do
>  	[ -w /dev/kmsg ] && echo "kselftest: Running tests in $collection" >> /dev/kmsg
>  	tests=$(echo "$available" | grep "^$collection:" | cut -d: -f2)
> -	($dryrun cd "$collection" && $dryrun run_many $tests)
> +	$dryrun cd "$collection" && $dryrun run_many $tests
> +	$dryrun cd "$curdir"
>  done
>  
> -failures="$(cat "$kselftest_failures_file")"
> -rm "$kselftest_failures_file"
> -if "$ERROR_ON_FAIL" && [ "$failures" ]; then
> -	exit 1
> +ktap_print_totals
> +if "$ERROR_ON_FAIL" && [ "$KTAP_CNT_FAIL" -ne 0 ]; then
> +	exit "$KSFT_FAIL"
> +else
> +	exit "$KSFT_PASS"
>  fi