[RFC PATCH v2 0/6] check: add option to rerun failed tests

[RFC PATCH v2 0/6] check: add option to rerun failed tests

[David Disseldorp]

This RFC patchset adds support to loop on failed tests, as proposed by
Ted Ts'o in https://lwn.net/Articles/897061/:
  add a mode that will immediately rerun a failed test 25 or 100 times
  to establish a failure percentage.

Changes since previous version (v1):
- rebased atop upstream v2022.06.26
- added a few extra cleanup commits
- append details for every rerun to xunit output
  + provide aggregate stats via <testcase status=X>
- extend _stash_test_status to call report hook, as well as save failure
  artifacts with a .rerun# suffix

Still flagged as RFC as I'd like to do a bit more testing and work out
whether the regular / xunit reporting output makes sense. Feedback
appreciated.

Diffstat:
 check         | 146 ++++++++++++++++++++++++++++++++++++++++++--------
 common/report |  96 ++++++++++++++++-----------------
 2 files changed, 170 insertions(+), 72 deletions(-)

Cheers, David

[RFC PATCH v2 1/6] report: use array for REPORT_ENV_LIST

[David Disseldorp]

There's no need for multiple assignments.

Signed-off-by: David Disseldorp <ddiss@suse.de>
---
 common/report | 26 +++++---------------------
 1 file changed, 5 insertions(+), 21 deletions(-)

diff --git a/common/report b/common/report
index 84d9e0a7..2b8285d8 100644
--- a/common/report
+++ b/common/report
@@ -4,26 +4,10 @@
 
 # List of xfstests's enviroment variables to include reports
 ## TODO automate list population inside common/conf
-REPORT_ENV_LIST="$REPORT_ENV_LIST SECTION"
-REPORT_ENV_LIST="$REPORT_ENV_LIST FSTYP"
-REPORT_ENV_LIST="$REPORT_ENV_LIST PLATFORM"
-REPORT_ENV_LIST="$REPORT_ENV_LIST MKFS_OPTIONS"
-REPORT_ENV_LIST="$REPORT_ENV_LIST MOUNT_OPTIONS"
-
-REPORT_ENV_LIST="$REPORT_ENV_LIST HOST_OPTIONS"
-REPORT_ENV_LIST="$REPORT_ENV_LIST CHECK_OPTIONS"
-REPORT_ENV_LIST="$REPORT_ENV_LIST XFS_MKFS_OPTIONS"
-REPORT_ENV_LIST="$REPORT_ENV_LIST TIME_FACTOR"
-REPORT_ENV_LIST="$REPORT_ENV_LIST LOAD_FACTOR"
-
-REPORT_ENV_LIST="$REPORT_ENV_LIST TEST_DIR"
-REPORT_ENV_LIST="$REPORT_ENV_LIST TEST_DEV"
-REPORT_ENV_LIST="$REPORT_ENV_LIST SCRATCH_DEV"
-REPORT_ENV_LIST="$REPORT_ENV_LIST SCRATCH_MNT"
-
-REPORT_ENV_LIST="$REPORT_ENV_LIST OVL_UPPER"
-REPORT_ENV_LIST="$REPORT_ENV_LIST OVL_LOWER"
-REPORT_ENV_LIST="$REPORT_ENV_LIST OVL_WORK"
+REPORT_ENV_LIST=("SECTION" "FSTYP" "PLATFORM" "MKFS_OPTIONS" "MOUNT_OPTIONS" \
+		 "HOST_OPTIONS" "CHECK_OPTIONS" "XFS_MKFS_OPTIONS" \
+		 "TIME_FACTOR" "LOAD_FACTOR" "TEST_DIR" "TEST_DEV" \
+		 "SCRATCH_DEV" "SCRATCH_MNT" "OVL_UPPER" "OVL_LOWER" "OVL_WORK")
 
 encode_xml()
 {
@@ -70,7 +54,7 @@ _xunit_make_section_report()
 
 	# Properties
 	echo -e "\t<properties>" >> $REPORT_DIR/result.xml
-	for p in $REPORT_ENV_LIST;do
+	for p in "${REPORT_ENV_LIST[@]}"; do
 		_xunit_add_property "$p"
 	done
 	echo -e "\t</properties>" >> $REPORT_DIR/result.xml
-- 
2.35.3

[RFC PATCH v2 2/6] report: pass through most details as function parameters

[David Disseldorp]

Report generation currently involves reaching into a whole bunch of
globals for things like section name and start/end times. Pass these
through as explicit function parameters to avoid unintentional breakage.

One minor fix included is the default xunit error message, which used
$sequm instead of $seqnum.

Signed-off-by: David Disseldorp <ddiss@suse.de>
---
 check         | 14 +++++++----
 common/report | 64 +++++++++++++++++++++++++++++----------------------
 2 files changed, 47 insertions(+), 31 deletions(-)

diff --git a/check b/check
index 2ea2920f..9d60df45 100755
--- a/check
+++ b/check
@@ -429,7 +429,9 @@ _wrapup()
 	if $showme && $needwrap; then
 		if $do_report; then
 			# $showme = all selected tests are notrun (no tries)
-			_make_section_report "${#notrun[*]}" "0" "${#notrun[*]}"
+			_make_section_report "$section" "${#notrun[*]}" "0" \
+					     "${#notrun[*]}" \
+					     "$((sect_stop - sect_start))"
 		fi
 		needwrap=false
 	elif $needwrap; then
@@ -490,7 +492,9 @@ _wrapup()
 		fi
 		echo "" >>$tmp.summary
 		if $do_report; then
-			_make_section_report "${#try[*]}" "${#bad[*]}" "${#notrun[*]}"
+			_make_section_report "$section" "${#try[*]}" \
+					     "${#bad[*]}" "${#notrun[*]}" \
+					     "$((sect_stop - sect_start))"
 		fi
 		needwrap=false
 	fi
@@ -733,7 +737,8 @@ function run_section()
 			bad+=("$seqnum")
 		fi
 		if $do_report && [[ ! $tc_status =~ ^(init|expunge)$ ]]; then
-			_make_testcase_report "$prev_seq" "$tc_status"
+			_make_testcase_report "$section" "$seqnum" \
+					      "$tc_status" "$((stop - start))"
 		fi
 
 		prev_seq="$seq"
@@ -937,7 +942,8 @@ function run_section()
 		bad+=("$seqnum")
 	fi
 	if $do_report && [[ ! $tc_status =~ ^(init|expunge)$ ]]; then
-		_make_testcase_report "$prev_seq" "$tc_status"
+		_make_testcase_report "$section" "$seqnum" "$tc_status" \
+				      "$((stop - start))"
 	fi
 
 	sect_stop=`_wallclock`
diff --git a/common/report b/common/report
index 2b8285d8..5ca41bc4 100644
--- a/common/report
+++ b/common/report
@@ -33,11 +33,11 @@ _xunit_add_property()
 _xunit_make_section_report()
 {
 	# xfstest:section ==> xunit:testsuite
-	local tests_count="$1"
-	local bad_count="$2"
-	local notrun_count="$3"
-	local sect_name=$section
-	local sect_time=`expr $sect_stop - $sect_start`
+	local sect_name="$1"
+	local tests_count="$2"
+	local bad_count="$3"
+	local notrun_count="$4"
+	local sect_time="$5"
 
 	if [ $sect_name == '-no-sections-' ]; then
 		sect_name='global'
@@ -67,12 +67,10 @@ _xunit_make_section_report()
 
 _xunit_make_testcase_report()
 {
-	local test_seq="$1"
-	local test_status="$2"
-	local test_time=`expr $stop - $start`
-	local strip="$SRC_DIR/"
-	local test_name=${test_seq#$strip}
-	local sect_name=$section
+	local sect_name="$1"
+	local test_name="$2"
+	local test_status="$3"
+	local test_time="$4"
 
 	# TODO: other places may also win if no-section mode will be named like 'default/global'
 	if [ $sect_name == '-no-sections-' ]; then
@@ -86,8 +84,9 @@ _xunit_make_testcase_report()
 	"pass")
 		;;
 	"notrun")
-		if [ -f $seqres.notrun ]; then
-			local msg=`cat $seqres.notrun | encode_xml`
+		local notrun_file="${REPORT_DIR}/${test_name}.notrun"
+		if [ -f "$notrun_file" ]; then
+			local msg=`cat "$notrun_file" | encode_xml`
 			echo -e "\t\t<skipped message=\"$msg\" />" >> $report
 		else
 			echo -e "\t\t<skipped/>" >> $report
@@ -97,27 +96,31 @@ _xunit_make_testcase_report()
 		echo -e "\t\t<skipped/>" >> $report
 		;;
 	"fail")
+		local out_src="${SRC_DIR}/${test_name}.out"
+		local full_file="${REPORT_DIR}/${test_name}.full"
+		local dmesg_file="${REPORT_DIR}/${test_name}.dmesg"
+		local outbad_file="${REPORT_DIR}/${test_name}.out.bad"
 		if [ -z "$_err_msg" ]; then
-			_err_msg="Test $sequm failed, reason unknown"
+			_err_msg="Test $test_name failed, reason unknown"
 		fi
 		echo -e "\t\t<failure message=\"$_err_msg\" type=\"TestFail\" />" >> $report
-		if [ -s $seqres.full ]; then
+		if [ -s "$full_file" ]; then
 			echo -e "\t\t<system-out>" >> $report
 			printf	'<![CDATA[\n' >>$report
-			cat $seqres.full | tr -dc '[:print:][:space:]' | encode_xml >>$report
+			cat "$full_file" | tr -dc '[:print:][:space:]' | encode_xml >>$report
 			printf ']]>\n'	>>$report
 			echo -e "\t\t</system-out>" >> $report
 		fi
-		if [ -f $seqres.dmesg ]; then
+		if [ -f "$dmesg_file" ]; then
 			echo -e "\t\t<system-err>" >> $report
 			printf	'<![CDATA[\n' >>$report
-			cat $seqres.dmesg | tr -dc '[:print:][:space:]' | encode_xml >>$report
+			cat "$dmesg_file" | tr -dc '[:print:][:space:]' | encode_xml >>$report
 			printf ']]>\n'	>>$report
 			echo -e "\t\t</system-err>" >> $report
-		elif [ -s $seqres.out.bad ]; then
+		elif [ -s "$outbad_file" ]; then
 			echo -e "\t\t<system-err>" >> $report
 			printf	'<![CDATA[\n' >>$report
-			$diff $test_seq.out $seqres.out.bad | encode_xml >>$report
+			$diff "$out_src" "$outbad_file" | encode_xml >>$report
 			printf ']]>\n'	>>$report
 			echo -e "\t\t</system-err>" >> $report
 		fi
@@ -134,13 +137,17 @@ _xunit_make_testcase_report()
 #  Common report generator entry points
 _make_section_report()
 {
-	local tests_count="$1"
-	local bad_count="$2"
-	local notrun_count="$3"
+	local sect_name="$1"
+	local tests_count="$2"
+	local bad_count="$3"
+	local notrun_count="$4"
+	local sect_time="$5"
 	for report in $REPORT_LIST; do
 		case "$report" in
 		"xunit")
-			_xunit_make_section_report "$tests_count" "$bad_count" "$notrun_count"
+			_xunit_make_section_report "$sect_name" "$tests_count" \
+						   "$bad_count" "$notrun_count" \
+						   "$sect_time"
 			;;
 		*)
 			_dump_err "format '$report' is not supported"
@@ -151,12 +158,15 @@ _make_section_report()
 
 _make_testcase_report()
 {
-	local test_seq="$1"
-	local test_status="$2"
+	local sect_name="$1"
+	local test_seq="$2"
+	local test_status="$3"
+	local test_time="$4"
 	for report in $REPORT_LIST; do
 		case "$report" in
 		"xunit")
-			_xunit_make_testcase_report "$test_seq" "$test_status"
+			_xunit_make_testcase_report "$sect_name" "$test_seq" \
+						    "$test_status" "$test_time"
 			;;
 		*)
 			_dump_err "report format '$report' is not supported"
-- 
2.35.3

[RFC PATCH v2 3/6] check: make a few variables local

[David Disseldorp]

The variables aren't used outside of function scope. Also convert one
timestamp output to use the helper.

Signed-off-by: David Disseldorp <ddiss@suse.de>
---
 check | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/check b/check
index 9d60df45..f973dd28 100755
--- a/check
+++ b/check
@@ -176,10 +176,10 @@ get_all_tests()
 # the function from that list.
 trim_test_list()
 {
-	test_list="$*"
+	local test_list="$*"
 
 	rm -f $tmp.grep
-	numsed=0
+	local numsed=0
 	for t in $test_list
 	do
 	    if [ $numsed -gt 100 ]; then
@@ -204,7 +204,7 @@ _wallclock()
 
 _timestamp()
 {
-    now=`date "+%T"`
+    local now=`date "+%T"`
     echo -n " [$now]"
 }
 
@@ -600,7 +600,7 @@ fi
 
 function run_section()
 {
-	local section=$1
+	local section=$1 skip
 
 	OLD_FSTYP=$FSTYP
 	OLD_TEST_FS_MOUNT_OPTS=$TEST_FS_MOUNT_OPTS
@@ -817,7 +817,7 @@ function run_section()
 		rm -f core $seqres.notrun
 
 		start=`_wallclock`
-		$timestamp && echo -n "	["`date "+%T"`"]"
+		$timestamp && _timestamp
 		[ ! -x $seq ] && chmod u+x $seq # ensure we can run it
 		$LOGGER_PROG "run xfstest $seqnum"
 		if [ -w /dev/kmsg ]; then
-- 
2.35.3

[RFC PATCH v2 4/6] check: append bad / notrun arrays in helper function

[David Disseldorp]

Currently the @try, @bad and @notrun arrays are appended with seqnum at
different points in the main run_section() loop:
- @try: shortly prior to test script execution
- @notrun: on list (check -n), or after .notrun flagged test completion
- @bad: at the start of subsequent test loop and loop exit

For future loop-test-following-failure functionality it makes sense to
combine some of these steps. This change moves both @notrun and @bad
appends into a helper function.

Signed-off-by: David Disseldorp <ddiss@suse.de>
---
 check | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/check b/check
index f973dd28..aa7dac2f 100755
--- a/check
+++ b/check
@@ -550,6 +550,27 @@ _expunge_test()
 	return 0
 }
 
+# Retain in @bad / @notrun the result of previously run @test_seq. @try array
+# entries are added prior to execution.
+_stash_test_status() {
+	local test_seq="$1"
+	local test_status="$2"
+
+	case "$test_status" in
+	fail)
+		bad+=("$test_seq")
+		;;
+	list|notrun)
+		notrun+=("$test_seq")
+		;;
+	pass|expunge|init)
+		;;
+	*)
+		echo "Unexpected test $test_seq status: $test_status"
+		;;
+	esac
+}
+
 # Can we run systemd scopes?
 HAVE_SYSTEMD_SCOPES=
 systemctl reset-failed "fstests-check" &>/dev/null
@@ -733,9 +754,7 @@ function run_section()
 	prev_seq=""
 	for seq in $list ; do
 		# Run report for previous test!
-		if [ "$tc_status" == "fail" ]; then
-			bad+=("$seqnum")
-		fi
+		_stash_test_status "$seqnum" "$tc_status"
 		if $do_report && [[ ! $tc_status =~ ^(init|expunge)$ ]]; then
 			_make_testcase_report "$section" "$seqnum" \
 					      "$tc_status" "$((stop - start))"
@@ -788,7 +807,6 @@ function run_section()
 			start=0
 			stop=0
 			tc_status="list"
-			notrun+=("$seqnum")
 			continue
 		fi
 
@@ -854,7 +872,6 @@ function run_section()
 			$timestamp && echo " [not run]" && \
 				      echo -n "	$seqnum -- "
 			cat $seqres.notrun
-			notrun+=("$seqnum")
 			tc_status="notrun"
 
 			# Unmount the scratch fs so that we can wipe the scratch
@@ -938,9 +955,7 @@ function run_section()
 	done
 
 	# make sure we record the status of the last test we ran.
-	if [ "$tc_status" == "fail" ]; then
-		bad+=("$seqnum")
-	fi
+	_stash_test_status "$seqnum" "$tc_status"
 	if $do_report && [[ ! $tc_status =~ ^(init|expunge)$ ]]; then
 		_make_testcase_report "$section" "$seqnum" "$tc_status" \
 				      "$((stop - start))"
-- 
2.35.3

[RFC PATCH v2 5/6] check: add -L <n> parameter to rerun failed tests

[David Disseldorp]

If check is run with -L <n>, then a failed test will be rerun <n> times
before proceeding to the next test. Following completion of the rerun
loop, aggregate pass/fail statistics are printed.

Rerun tests will be tracked as a single failure in overall pass/fail
metrics (via @try and @bad), with .out.bad, .dmesg and .full saved using
a .rerun# suffix.

Suggested-by: Theodore Ts'o <tytso@mit.edu>
Link: https://lwn.net/Articles/897061/
Signed-off-by: David Disseldorp <ddiss@suse.de>
---
 check         | 94 ++++++++++++++++++++++++++++++++++++++++++++-------
 common/report |  8 +++--
 2 files changed, 88 insertions(+), 14 deletions(-)

diff --git a/check b/check
index aa7dac2f..726c83d9 100755
--- a/check
+++ b/check
@@ -26,6 +26,7 @@ do_report=false
 DUMP_OUTPUT=false
 iterations=1
 istop=false
+loop_on_fail=0
 
 # This is a global variable used to pass test failure text to reporting gunk
 _err_msg=""
@@ -75,6 +76,7 @@ check options
     --large-fs		optimise scratch device for large filesystems
     -s section		run only specified section from config file
     -S section		exclude the specified section from the config file
+    -L <n>		loop tests <n> times following a failure, measuring aggregate pass/fail metrics
 
 testlist options
     -g group[,group...]	include tests from these groups
@@ -333,6 +335,9 @@ while [ $# -gt 0 ]; do
 		;;
 	--large-fs) export LARGE_SCRATCH_DEV=yes ;;
 	--extra-space=*) export SCRATCH_DEV_EMPTY_SPACE=${r#*=} ;;
+	-L)	[[ $2 =~ ^[0-9]+$ ]] || usage
+		loop_on_fail=$2; shift
+		;;
 
 	-*)	usage ;;
 	*)	# not an argument, we've got tests now.
@@ -555,6 +560,18 @@ _expunge_test()
 _stash_test_status() {
 	local test_seq="$1"
 	local test_status="$2"
+	local test_time="$3"
+	local loop_num="$4"
+	local report_msg="$5"
+
+	if $do_report && [[ ! $test_status =~ ^(init|expunge)$ ]]; then
+		_make_testcase_report "$section" "$test_seq" \
+				      "$test_status" "$test_time" \
+				      "$report_msg"
+	fi
+
+	# only stash result for first failure (triggering loop)
+	((loop_num > 1)) && return
 
 	case "$test_status" in
 	fail)
@@ -610,6 +627,38 @@ _run_seq() {
 	fi
 }
 
+# Check whether the last test should be rerun according to loop-on-error state
+# and return "0" if so, otherwise return "1".
+_ix_inc() {
+	local test_status="$1"
+	local loop_len="$2"
+
+	if ((!loop_on_fail)); then
+		echo 1
+		return
+	fi
+
+	if [ "$test_status" == "fail" ] && ((!loop_len)); then
+		echo 0	# initial failure of this test, start loop-on-fail
+	elif ((loop_len > 0)) && ((loop_len < loop_on_fail)); then
+		echo 0	# continue loop following initial failure
+	else
+		echo 1	# completed or not currently in a failure loop
+	fi
+}
+
+_failure_loop_dump_stats() {
+	awk "BEGIN {
+		n=split(\"$*\", arr);"'
+		for (i = 1; i <= n; i++)
+			stats[arr[i]]++;
+		printf("aggregate results across %d runs: ", n);
+		for (x in stats)
+			printf("%s=%d (%.1f%%)", (i-- > n ? x : ", " x),
+			       stats[x], 100 * stats[x] / n);
+	     }'
+}
+
 _detect_kmemleak
 _prepare_test_list
 
@@ -750,14 +799,29 @@ function run_section()
 	seqres="$check"
 	_check_test_fs
 
-	local tc_status="init"
+	local tc_status="init" ix agg_msg
 	prev_seq=""
-	for seq in $list ; do
+	local -a _list=( $list ) loop_status=()
+	for ((ix = 0; ix < ${#_list[*]};
+	      ix += $(_ix_inc "$tc_status" "${#loop_status[*]}"))); do
+		seq="${_list[$ix]}"
+
+		if [ "$seq" == "$prev_seq" ]; then
+			loop_status+=("$tc_status")
+		elif ((${#loop_status[*]})); then
+			# leaving rerun-on-failure loop
+			loop_status+=("$tc_status")
+			agg_msg=$(_failure_loop_dump_stats "${loop_status[@]}")
+			echo "$seqnum $agg_msg"
+		fi
+
 		# Run report for previous test!
-		_stash_test_status "$seqnum" "$tc_status"
-		if $do_report && [[ ! $tc_status =~ ^(init|expunge)$ ]]; then
-			_make_testcase_report "$section" "$seqnum" \
-					      "$tc_status" "$((stop - start))"
+		_stash_test_status "$seqnum" "$tc_status" "$((stop - start))" \
+				   "${#loop_status[*]}" "$agg_msg"
+
+		if [ -n "$agg_msg" ]; then
+			loop_status=()
+			agg_msg=""
 		fi
 
 		prev_seq="$seq"
@@ -827,7 +891,9 @@ function run_section()
 		fi
 
 		# record that we really tried to run this test.
-		try+=("$seqnum")
+		if ((!${#loop_status[*]})); then
+			try+=("$seqnum")
+		fi
 
 		awk 'BEGIN {lasttime="       "} \
 		     $1 == "'$seqnum'" {lasttime=" " $2 "s ... "; exit} \
@@ -954,13 +1020,17 @@ function run_section()
 		fi
 	done
 
-	# make sure we record the status of the last test we ran.
-	_stash_test_status "$seqnum" "$tc_status"
-	if $do_report && [[ ! $tc_status =~ ^(init|expunge)$ ]]; then
-		_make_testcase_report "$section" "$seqnum" "$tc_status" \
-				      "$((stop - start))"
+	if ((${#loop_status[*]})); then
+		# leaving rerun-on-failure loop
+		loop_status+=("$tc_status")
+		agg_msg=$(_failure_loop_dump_stats "${loop_status[@]}")
+		echo "$seqnum $agg_msg"
 	fi
 
+	# Run report for previous test!
+	_stash_test_status "$seqnum" "$tc_status" "$((stop - start))" \
+			   "${#loop_status[*]}" "$agg_msg"
+
 	sect_stop=`_wallclock`
 	interrupt=false
 	_wrapup
diff --git a/common/report b/common/report
index 5ca41bc4..cede4987 100644
--- a/common/report
+++ b/common/report
@@ -71,6 +71,7 @@ _xunit_make_testcase_report()
 	local test_name="$2"
 	local test_status="$3"
 	local test_time="$4"
+	local test_md="$5"
 
 	# TODO: other places may also win if no-section mode will be named like 'default/global'
 	if [ $sect_name == '-no-sections-' ]; then
@@ -79,7 +80,8 @@ _xunit_make_testcase_report()
 	fi
 	local report=$tmp.report.xunit.$sect_name.xml
 
-	echo -e "\t<testcase classname=\"xfstests.$sect_name\" name=\"$test_name\" time=\"$test_time\">" >> $report
+	[ -n "$test_md" ] && test_md=" status=\"$(echo "$test_md"|encode_xml)\""
+	echo -e "\t<testcase classname=\"xfstests.$sect_name\" name=\"$test_name\" time=\"$test_time\"${test_md}>" >> $report
 	case $test_status in
 	"pass")
 		;;
@@ -162,11 +164,13 @@ _make_testcase_report()
 	local test_seq="$2"
 	local test_status="$3"
 	local test_time="$4"
+	local test_md="$5"
 	for report in $REPORT_LIST; do
 		case "$report" in
 		"xunit")
 			_xunit_make_testcase_report "$sect_name" "$test_seq" \
-						    "$test_status" "$test_time"
+						    "$test_status" \
+						    "$test_time" "$test_md"
 			;;
 		*)
 			_dump_err "report format '$report' is not supported"
-- 
2.35.3

[RFC PATCH v2 6/6] check: stash full/dmesg/out.bad files on rerun

[David Disseldorp]

These files would otherwise be overwritten.

Signed-off-by: David Disseldorp <ddiss@suse.de>
---
 check | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/check b/check
index 726c83d9..baf336da 100755
--- a/check
+++ b/check
@@ -570,8 +570,17 @@ _stash_test_status() {
 				      "$report_msg"
 	fi
 
-	# only stash result for first failure (triggering loop)
-	((loop_num > 1)) && return
+	if ((loop_num > 0)); then
+		# retain files which would be overwritten in subsequent reruns
+		for i in "${REPORT_DIR}/${test_seq}.full" \
+			 "${REPORT_DIR}/${test_seq}.dmesg" \
+			 "${REPORT_DIR}/${test_seq}.out.bad"; do
+			[ -f "$i" ] || continue
+			cp "$i" "${i}.rerun$((loop_num - 1))"
+		done
+		# only stash result for first failure (triggering loop)
+		((loop_num != 1)) && return
+	fi
 
 	case "$test_status" in
 	fail)
-- 
2.35.3

Re: [RFC PATCH v2 1/6] report: use array for REPORT_ENV_LIST

[Darrick J. Wong]

On Tue, Jun 28, 2022 at 12:22:51AM +0200, David Disseldorp wrote:
> There's no need for multiple assignments.
> 
> Signed-off-by: David Disseldorp <ddiss@suse.de>
> ---
>  common/report | 26 +++++---------------------
>  1 file changed, 5 insertions(+), 21 deletions(-)
> 
> diff --git a/common/report b/common/report
> index 84d9e0a7..2b8285d8 100644
> --- a/common/report
> +++ b/common/report
> @@ -4,26 +4,10 @@
>  
>  # List of xfstests's enviroment variables to include reports
>  ## TODO automate list population inside common/conf
> -REPORT_ENV_LIST="$REPORT_ENV_LIST SECTION"
> -REPORT_ENV_LIST="$REPORT_ENV_LIST FSTYP"
> -REPORT_ENV_LIST="$REPORT_ENV_LIST PLATFORM"
> -REPORT_ENV_LIST="$REPORT_ENV_LIST MKFS_OPTIONS"
> -REPORT_ENV_LIST="$REPORT_ENV_LIST MOUNT_OPTIONS"
> -
> -REPORT_ENV_LIST="$REPORT_ENV_LIST HOST_OPTIONS"
> -REPORT_ENV_LIST="$REPORT_ENV_LIST CHECK_OPTIONS"
> -REPORT_ENV_LIST="$REPORT_ENV_LIST XFS_MKFS_OPTIONS"
> -REPORT_ENV_LIST="$REPORT_ENV_LIST TIME_FACTOR"
> -REPORT_ENV_LIST="$REPORT_ENV_LIST LOAD_FACTOR"
> -
> -REPORT_ENV_LIST="$REPORT_ENV_LIST TEST_DIR"
> -REPORT_ENV_LIST="$REPORT_ENV_LIST TEST_DEV"
> -REPORT_ENV_LIST="$REPORT_ENV_LIST SCRATCH_DEV"
> -REPORT_ENV_LIST="$REPORT_ENV_LIST SCRATCH_MNT"
> -
> -REPORT_ENV_LIST="$REPORT_ENV_LIST OVL_UPPER"
> -REPORT_ENV_LIST="$REPORT_ENV_LIST OVL_LOWER"
> -REPORT_ENV_LIST="$REPORT_ENV_LIST OVL_WORK"
> +REPORT_ENV_LIST=("SECTION" "FSTYP" "PLATFORM" "MKFS_OPTIONS" "MOUNT_OPTIONS" \
> +		 "HOST_OPTIONS" "CHECK_OPTIONS" "XFS_MKFS_OPTIONS" \
> +		 "TIME_FACTOR" "LOAD_FACTOR" "TEST_DIR" "TEST_DEV" \
> +		 "SCRATCH_DEV" "SCRATCH_MNT" "OVL_UPPER" "OVL_LOWER" "OVL_WORK")

Cheers for the end of a stringbuilder pattern,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

>  
>  encode_xml()
>  {
> @@ -70,7 +54,7 @@ _xunit_make_section_report()
>  
>  	# Properties
>  	echo -e "\t<properties>" >> $REPORT_DIR/result.xml
> -	for p in $REPORT_ENV_LIST;do
> +	for p in "${REPORT_ENV_LIST[@]}"; do
>  		_xunit_add_property "$p"
>  	done
>  	echo -e "\t</properties>" >> $REPORT_DIR/result.xml
> -- 
> 2.35.3
> 

Re: [RFC PATCH v2 2/6] report: pass through most details as function parameters

[Darrick J. Wong]

On Tue, Jun 28, 2022 at 12:22:52AM +0200, David Disseldorp wrote:
> Report generation currently involves reaching into a whole bunch of
> globals for things like section name and start/end times. Pass these
> through as explicit function parameters to avoid unintentional breakage.
> 
> One minor fix included is the default xunit error message, which used
> $sequm instead of $seqnum.

I wish bash would just error out on undefined variables...

> Signed-off-by: David Disseldorp <ddiss@suse.de>

Looks ok,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  check         | 14 +++++++----
>  common/report | 64 +++++++++++++++++++++++++++++----------------------
>  2 files changed, 47 insertions(+), 31 deletions(-)
> 
> diff --git a/check b/check
> index 2ea2920f..9d60df45 100755
> --- a/check
> +++ b/check
> @@ -429,7 +429,9 @@ _wrapup()
>  	if $showme && $needwrap; then
>  		if $do_report; then
>  			# $showme = all selected tests are notrun (no tries)
> -			_make_section_report "${#notrun[*]}" "0" "${#notrun[*]}"
> +			_make_section_report "$section" "${#notrun[*]}" "0" \
> +					     "${#notrun[*]}" \
> +					     "$((sect_stop - sect_start))"
>  		fi
>  		needwrap=false
>  	elif $needwrap; then
> @@ -490,7 +492,9 @@ _wrapup()
>  		fi
>  		echo "" >>$tmp.summary
>  		if $do_report; then
> -			_make_section_report "${#try[*]}" "${#bad[*]}" "${#notrun[*]}"
> +			_make_section_report "$section" "${#try[*]}" \
> +					     "${#bad[*]}" "${#notrun[*]}" \
> +					     "$((sect_stop - sect_start))"
>  		fi
>  		needwrap=false
>  	fi
> @@ -733,7 +737,8 @@ function run_section()
>  			bad+=("$seqnum")
>  		fi
>  		if $do_report && [[ ! $tc_status =~ ^(init|expunge)$ ]]; then
> -			_make_testcase_report "$prev_seq" "$tc_status"
> +			_make_testcase_report "$section" "$seqnum" \
> +					      "$tc_status" "$((stop - start))"
>  		fi
>  
>  		prev_seq="$seq"
> @@ -937,7 +942,8 @@ function run_section()
>  		bad+=("$seqnum")
>  	fi
>  	if $do_report && [[ ! $tc_status =~ ^(init|expunge)$ ]]; then
> -		_make_testcase_report "$prev_seq" "$tc_status"
> +		_make_testcase_report "$section" "$seqnum" "$tc_status" \
> +				      "$((stop - start))"
>  	fi
>  
>  	sect_stop=`_wallclock`
> diff --git a/common/report b/common/report
> index 2b8285d8..5ca41bc4 100644
> --- a/common/report
> +++ b/common/report
> @@ -33,11 +33,11 @@ _xunit_add_property()
>  _xunit_make_section_report()
>  {
>  	# xfstest:section ==> xunit:testsuite
> -	local tests_count="$1"
> -	local bad_count="$2"
> -	local notrun_count="$3"
> -	local sect_name=$section
> -	local sect_time=`expr $sect_stop - $sect_start`
> +	local sect_name="$1"
> +	local tests_count="$2"
> +	local bad_count="$3"
> +	local notrun_count="$4"
> +	local sect_time="$5"
>  
>  	if [ $sect_name == '-no-sections-' ]; then
>  		sect_name='global'
> @@ -67,12 +67,10 @@ _xunit_make_section_report()
>  
>  _xunit_make_testcase_report()
>  {
> -	local test_seq="$1"
> -	local test_status="$2"
> -	local test_time=`expr $stop - $start`
> -	local strip="$SRC_DIR/"
> -	local test_name=${test_seq#$strip}
> -	local sect_name=$section
> +	local sect_name="$1"
> +	local test_name="$2"
> +	local test_status="$3"
> +	local test_time="$4"
>  
>  	# TODO: other places may also win if no-section mode will be named like 'default/global'
>  	if [ $sect_name == '-no-sections-' ]; then
> @@ -86,8 +84,9 @@ _xunit_make_testcase_report()
>  	"pass")
>  		;;
>  	"notrun")
> -		if [ -f $seqres.notrun ]; then
> -			local msg=`cat $seqres.notrun | encode_xml`
> +		local notrun_file="${REPORT_DIR}/${test_name}.notrun"
> +		if [ -f "$notrun_file" ]; then
> +			local msg=`cat "$notrun_file" | encode_xml`
>  			echo -e "\t\t<skipped message=\"$msg\" />" >> $report
>  		else
>  			echo -e "\t\t<skipped/>" >> $report
> @@ -97,27 +96,31 @@ _xunit_make_testcase_report()
>  		echo -e "\t\t<skipped/>" >> $report
>  		;;
>  	"fail")
> +		local out_src="${SRC_DIR}/${test_name}.out"
> +		local full_file="${REPORT_DIR}/${test_name}.full"
> +		local dmesg_file="${REPORT_DIR}/${test_name}.dmesg"
> +		local outbad_file="${REPORT_DIR}/${test_name}.out.bad"
>  		if [ -z "$_err_msg" ]; then
> -			_err_msg="Test $sequm failed, reason unknown"
> +			_err_msg="Test $test_name failed, reason unknown"
>  		fi
>  		echo -e "\t\t<failure message=\"$_err_msg\" type=\"TestFail\" />" >> $report
> -		if [ -s $seqres.full ]; then
> +		if [ -s "$full_file" ]; then
>  			echo -e "\t\t<system-out>" >> $report
>  			printf	'<![CDATA[\n' >>$report
> -			cat $seqres.full | tr -dc '[:print:][:space:]' | encode_xml >>$report
> +			cat "$full_file" | tr -dc '[:print:][:space:]' | encode_xml >>$report
>  			printf ']]>\n'	>>$report
>  			echo -e "\t\t</system-out>" >> $report
>  		fi
> -		if [ -f $seqres.dmesg ]; then
> +		if [ -f "$dmesg_file" ]; then
>  			echo -e "\t\t<system-err>" >> $report
>  			printf	'<![CDATA[\n' >>$report
> -			cat $seqres.dmesg | tr -dc '[:print:][:space:]' | encode_xml >>$report
> +			cat "$dmesg_file" | tr -dc '[:print:][:space:]' | encode_xml >>$report
>  			printf ']]>\n'	>>$report
>  			echo -e "\t\t</system-err>" >> $report
> -		elif [ -s $seqres.out.bad ]; then
> +		elif [ -s "$outbad_file" ]; then
>  			echo -e "\t\t<system-err>" >> $report
>  			printf	'<![CDATA[\n' >>$report
> -			$diff $test_seq.out $seqres.out.bad | encode_xml >>$report
> +			$diff "$out_src" "$outbad_file" | encode_xml >>$report
>  			printf ']]>\n'	>>$report
>  			echo -e "\t\t</system-err>" >> $report
>  		fi
> @@ -134,13 +137,17 @@ _xunit_make_testcase_report()
>  #  Common report generator entry points
>  _make_section_report()
>  {
> -	local tests_count="$1"
> -	local bad_count="$2"
> -	local notrun_count="$3"
> +	local sect_name="$1"
> +	local tests_count="$2"
> +	local bad_count="$3"
> +	local notrun_count="$4"
> +	local sect_time="$5"
>  	for report in $REPORT_LIST; do
>  		case "$report" in
>  		"xunit")
> -			_xunit_make_section_report "$tests_count" "$bad_count" "$notrun_count"
> +			_xunit_make_section_report "$sect_name" "$tests_count" \
> +						   "$bad_count" "$notrun_count" \
> +						   "$sect_time"
>  			;;
>  		*)
>  			_dump_err "format '$report' is not supported"
> @@ -151,12 +158,15 @@ _make_section_report()
>  
>  _make_testcase_report()
>  {
> -	local test_seq="$1"
> -	local test_status="$2"
> +	local sect_name="$1"
> +	local test_seq="$2"
> +	local test_status="$3"
> +	local test_time="$4"
>  	for report in $REPORT_LIST; do
>  		case "$report" in
>  		"xunit")
> -			_xunit_make_testcase_report "$test_seq" "$test_status"
> +			_xunit_make_testcase_report "$sect_name" "$test_seq" \
> +						    "$test_status" "$test_time"
>  			;;
>  		*)
>  			_dump_err "report format '$report' is not supported"
> -- 
> 2.35.3
> 

Re: [RFC PATCH v2 3/6] check: make a few variables local

[Darrick J. Wong]

On Tue, Jun 28, 2022 at 12:22:53AM +0200, David Disseldorp wrote:
> The variables aren't used outside of function scope. Also convert one
> timestamp output to use the helper.
> 
> Signed-off-by: David Disseldorp <ddiss@suse.de>

Looks good,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  check | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/check b/check
> index 9d60df45..f973dd28 100755
> --- a/check
> +++ b/check
> @@ -176,10 +176,10 @@ get_all_tests()
>  # the function from that list.
>  trim_test_list()
>  {
> -	test_list="$*"
> +	local test_list="$*"
>  
>  	rm -f $tmp.grep
> -	numsed=0
> +	local numsed=0
>  	for t in $test_list
>  	do
>  	    if [ $numsed -gt 100 ]; then
> @@ -204,7 +204,7 @@ _wallclock()
>  
>  _timestamp()
>  {
> -    now=`date "+%T"`
> +    local now=`date "+%T"`
>      echo -n " [$now]"
>  }
>  
> @@ -600,7 +600,7 @@ fi
>  
>  function run_section()
>  {
> -	local section=$1
> +	local section=$1 skip
>  
>  	OLD_FSTYP=$FSTYP
>  	OLD_TEST_FS_MOUNT_OPTS=$TEST_FS_MOUNT_OPTS
> @@ -817,7 +817,7 @@ function run_section()
>  		rm -f core $seqres.notrun
>  
>  		start=`_wallclock`
> -		$timestamp && echo -n "	["`date "+%T"`"]"
> +		$timestamp && _timestamp
>  		[ ! -x $seq ] && chmod u+x $seq # ensure we can run it
>  		$LOGGER_PROG "run xfstest $seqnum"
>  		if [ -w /dev/kmsg ]; then
> -- 
> 2.35.3
> 

Re: [RFC PATCH v2 4/6] check: append bad / notrun arrays in helper function

[Darrick J. Wong]

On Tue, Jun 28, 2022 at 12:22:54AM +0200, David Disseldorp wrote:
> Currently the @try, @bad and @notrun arrays are appended with seqnum at
> different points in the main run_section() loop:
> - @try: shortly prior to test script execution
> - @notrun: on list (check -n), or after .notrun flagged test completion
> - @bad: at the start of subsequent test loop and loop exit
> 
> For future loop-test-following-failure functionality it makes sense to
> combine some of these steps. This change moves both @notrun and @bad
> appends into a helper function.
> 
> Signed-off-by: David Disseldorp <ddiss@suse.de>
> ---
>  check | 31 +++++++++++++++++++++++--------
>  1 file changed, 23 insertions(+), 8 deletions(-)
> 
> diff --git a/check b/check
> index f973dd28..aa7dac2f 100755
> --- a/check
> +++ b/check
> @@ -550,6 +550,27 @@ _expunge_test()
>  	return 0
>  }
>  
> +# Retain in @bad / @notrun the result of previously run @test_seq. @try array
> +# entries are added prior to execution.
> +_stash_test_status() {
> +	local test_seq="$1"
> +	local test_status="$2"
> +
> +	case "$test_status" in
> +	fail)
> +		bad+=("$test_seq")
> +		;;
> +	list|notrun)
> +		notrun+=("$test_seq")
> +		;;
> +	pass|expunge|init)
> +		;;
> +	*)
> +		echo "Unexpected test $test_seq status: $test_status"
> +		;;
> +	esac
> +}
> +
>  # Can we run systemd scopes?
>  HAVE_SYSTEMD_SCOPES=
>  systemctl reset-failed "fstests-check" &>/dev/null
> @@ -733,9 +754,7 @@ function run_section()
>  	prev_seq=""
>  	for seq in $list ; do
>  		# Run report for previous test!
> -		if [ "$tc_status" == "fail" ]; then
> -			bad+=("$seqnum")
> -		fi
> +		_stash_test_status "$seqnum" "$tc_status"
>  		if $do_report && [[ ! $tc_status =~ ^(init|expunge)$ ]]; then
>  			_make_testcase_report "$section" "$seqnum" \
>  					      "$tc_status" "$((stop - start))"
> @@ -788,7 +807,6 @@ function run_section()
>  			start=0
>  			stop=0
>  			tc_status="list"
> -			notrun+=("$seqnum")
>  			continue
>  		fi
>  
> @@ -854,7 +872,6 @@ function run_section()
>  			$timestamp && echo " [not run]" && \
>  				      echo -n "	$seqnum -- "
>  			cat $seqres.notrun
> -			notrun+=("$seqnum")

Ahh, it took me a minute to figure out that removing this line was ok
because we'll either jump back to the top of the loop or fall out below.

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D


>  			tc_status="notrun"
>  
>  			# Unmount the scratch fs so that we can wipe the scratch
> @@ -938,9 +955,7 @@ function run_section()
>  	done
>  
>  	# make sure we record the status of the last test we ran.
> -	if [ "$tc_status" == "fail" ]; then
> -		bad+=("$seqnum")
> -	fi
> +	_stash_test_status "$seqnum" "$tc_status"
>  	if $do_report && [[ ! $tc_status =~ ^(init|expunge)$ ]]; then
>  		_make_testcase_report "$section" "$seqnum" "$tc_status" \
>  				      "$((stop - start))"
> -- 
> 2.35.3
> 

Re: [RFC PATCH v2 5/6] check: add -L <n> parameter to rerun failed tests

[Darrick J. Wong]

On Tue, Jun 28, 2022 at 12:22:55AM +0200, David Disseldorp wrote:
> If check is run with -L <n>, then a failed test will be rerun <n> times
> before proceeding to the next test. Following completion of the rerun
> loop, aggregate pass/fail statistics are printed.
> 
> Rerun tests will be tracked as a single failure in overall pass/fail
> metrics (via @try and @bad), with .out.bad, .dmesg and .full saved using
> a .rerun# suffix.
> 
> Suggested-by: Theodore Ts'o <tytso@mit.edu>
> Link: https://lwn.net/Articles/897061/
> Signed-off-by: David Disseldorp <ddiss@suse.de>
> ---
>  check         | 94 ++++++++++++++++++++++++++++++++++++++++++++-------
>  common/report |  8 +++--
>  2 files changed, 88 insertions(+), 14 deletions(-)
> 
> diff --git a/check b/check
> index aa7dac2f..726c83d9 100755
> --- a/check
> +++ b/check
> @@ -26,6 +26,7 @@ do_report=false
>  DUMP_OUTPUT=false
>  iterations=1
>  istop=false
> +loop_on_fail=0
>  
>  # This is a global variable used to pass test failure text to reporting gunk
>  _err_msg=""
> @@ -75,6 +76,7 @@ check options
>      --large-fs		optimise scratch device for large filesystems
>      -s section		run only specified section from config file
>      -S section		exclude the specified section from the config file
> +    -L <n>		loop tests <n> times following a failure, measuring aggregate pass/fail metrics
>  
>  testlist options
>      -g group[,group...]	include tests from these groups
> @@ -333,6 +335,9 @@ while [ $# -gt 0 ]; do
>  		;;
>  	--large-fs) export LARGE_SCRATCH_DEV=yes ;;
>  	--extra-space=*) export SCRATCH_DEV_EMPTY_SPACE=${r#*=} ;;
> +	-L)	[[ $2 =~ ^[0-9]+$ ]] || usage
> +		loop_on_fail=$2; shift
> +		;;
>  
>  	-*)	usage ;;
>  	*)	# not an argument, we've got tests now.
> @@ -555,6 +560,18 @@ _expunge_test()
>  _stash_test_status() {
>  	local test_seq="$1"
>  	local test_status="$2"
> +	local test_time="$3"
> +	local loop_num="$4"
> +	local report_msg="$5"
> +
> +	if $do_report && [[ ! $test_status =~ ^(init|expunge)$ ]]; then
> +		_make_testcase_report "$section" "$test_seq" \
> +				      "$test_status" "$test_time" \
> +				      "$report_msg"
> +	fi
> +
> +	# only stash result for first failure (triggering loop)
> +	((loop_num > 1)) && return
>  
>  	case "$test_status" in
>  	fail)
> @@ -610,6 +627,38 @@ _run_seq() {
>  	fi
>  }
>  
> +# Check whether the last test should be rerun according to loop-on-error state
> +# and return "0" if so, otherwise return "1".

Er... this echoes 0 and 1, and the return value of the function is
neither checked nor set to anything other than zero, right?

I'm also not sure what 'ix' stands for?

> +_ix_inc() {
> +	local test_status="$1"
> +	local loop_len="$2"
> +
> +	if ((!loop_on_fail)); then
> +		echo 1
> +		return
> +	fi
> +
> +	if [ "$test_status" == "fail" ] && ((!loop_len)); then
> +		echo 0	# initial failure of this test, start loop-on-fail
> +	elif ((loop_len > 0)) && ((loop_len < loop_on_fail)); then
> +		echo 0	# continue loop following initial failure
> +	else
> +		echo 1	# completed or not currently in a failure loop
> +	fi
> +}
> +
> +_failure_loop_dump_stats() {
> +	awk "BEGIN {
> +		n=split(\"$*\", arr);"'
> +		for (i = 1; i <= n; i++)
> +			stats[arr[i]]++;
> +		printf("aggregate results across %d runs: ", n);
> +		for (x in stats)
> +			printf("%s=%d (%.1f%%)", (i-- > n ? x : ", " x),
> +			       stats[x], 100 * stats[x] / n);
> +	     }'
> +}
> +
>  _detect_kmemleak
>  _prepare_test_list
>  
> @@ -750,14 +799,29 @@ function run_section()
>  	seqres="$check"
>  	_check_test_fs
>  
> -	local tc_status="init"
> +	local tc_status="init" ix agg_msg
>  	prev_seq=""
> -	for seq in $list ; do
> +	local -a _list=( $list ) loop_status=()
> +	for ((ix = 0; ix < ${#_list[*]};
> +	      ix += $(_ix_inc "$tc_status" "${#loop_status[*]}"))); do
> +		seq="${_list[$ix]}"
> +
> +		if [ "$seq" == "$prev_seq" ]; then
> +			loop_status+=("$tc_status")
> +		elif ((${#loop_status[*]})); then
> +			# leaving rerun-on-failure loop
> +			loop_status+=("$tc_status")
> +			agg_msg=$(_failure_loop_dump_stats "${loop_status[@]}")
> +			echo "$seqnum $agg_msg"
> +		fi
> +
>  		# Run report for previous test!
> -		_stash_test_status "$seqnum" "$tc_status"
> -		if $do_report && [[ ! $tc_status =~ ^(init|expunge)$ ]]; then
> -			_make_testcase_report "$section" "$seqnum" \
> -					      "$tc_status" "$((stop - start))"
> +		_stash_test_status "$seqnum" "$tc_status" "$((stop - start))" \
> +				   "${#loop_status[*]}" "$agg_msg"
> +
> +		if [ -n "$agg_msg" ]; then
> +			loop_status=()
> +			agg_msg=""
>  		fi
>  
>  		prev_seq="$seq"
> @@ -827,7 +891,9 @@ function run_section()
>  		fi
>  
>  		# record that we really tried to run this test.
> -		try+=("$seqnum")
> +		if ((!${#loop_status[*]})); then
> +			try+=("$seqnum")
> +		fi
>  
>  		awk 'BEGIN {lasttime="       "} \
>  		     $1 == "'$seqnum'" {lasttime=" " $2 "s ... "; exit} \
> @@ -954,13 +1020,17 @@ function run_section()
>  		fi
>  	done
>  
> -	# make sure we record the status of the last test we ran.
> -	_stash_test_status "$seqnum" "$tc_status"
> -	if $do_report && [[ ! $tc_status =~ ^(init|expunge)$ ]]; then
> -		_make_testcase_report "$section" "$seqnum" "$tc_status" \
> -				      "$((stop - start))"
> +	if ((${#loop_status[*]})); then
> +		# leaving rerun-on-failure loop
> +		loop_status+=("$tc_status")
> +		agg_msg=$(_failure_loop_dump_stats "${loop_status[@]}")
> +		echo "$seqnum $agg_msg"
>  	fi
>  
> +	# Run report for previous test!
> +	_stash_test_status "$seqnum" "$tc_status" "$((stop - start))" \
> +			   "${#loop_status[*]}" "$agg_msg"
> +
>  	sect_stop=`_wallclock`
>  	interrupt=false
>  	_wrapup
> diff --git a/common/report b/common/report
> index 5ca41bc4..cede4987 100644
> --- a/common/report
> +++ b/common/report
> @@ -71,6 +71,7 @@ _xunit_make_testcase_report()
>  	local test_name="$2"
>  	local test_status="$3"
>  	local test_time="$4"
> +	local test_md="$5"

...or what "md" here means.

>  
>  	# TODO: other places may also win if no-section mode will be named like 'default/global'
>  	if [ $sect_name == '-no-sections-' ]; then
> @@ -79,7 +80,8 @@ _xunit_make_testcase_report()
>  	fi
>  	local report=$tmp.report.xunit.$sect_name.xml
>  
> -	echo -e "\t<testcase classname=\"xfstests.$sect_name\" name=\"$test_name\" time=\"$test_time\">" >> $report
> +	[ -n "$test_md" ] && test_md=" status=\"$(echo "$test_md"|encode_xml)\""

AFAICT the junit xml schema defines a @status attribute for <testcase>.

https://raw.githubusercontent.com/windyroad/JUnit-Schema/master/JUnit.xsd

And yes, it's confusing that fstests namespaced all this code with
'xunit', since there's a separate xunit test reporting schema too!

That said -- I'm not sure what's supposed to end up in this attribute?
It looks like it's supposed to capture the aggregation report?

But then I wonder, why not stick to adding a separate <testcase> element
for each test run, even the repeated ones?  And let the xml consumer
compute aggregation statistics from the elements?

--D

> +	echo -e "\t<testcase classname=\"xfstests.$sect_name\" name=\"$test_name\" time=\"$test_time\"${test_md}>" >> $report
>  	case $test_status in
>  	"pass")
>  		;;
> @@ -162,11 +164,13 @@ _make_testcase_report()
>  	local test_seq="$2"
>  	local test_status="$3"
>  	local test_time="$4"
> +	local test_md="$5"
>  	for report in $REPORT_LIST; do
>  		case "$report" in
>  		"xunit")
>  			_xunit_make_testcase_report "$sect_name" "$test_seq" \
> -						    "$test_status" "$test_time"
> +						    "$test_status" \
> +						    "$test_time" "$test_md"
>  			;;
>  		*)
>  			_dump_err "report format '$report' is not supported"
> -- 
> 2.35.3
> 

Re: [RFC PATCH v2 6/6] check: stash full/dmesg/out.bad files on rerun

[Darrick J. Wong]

On Tue, Jun 28, 2022 at 12:22:56AM +0200, David Disseldorp wrote:
> These files would otherwise be overwritten.
> 
> Signed-off-by: David Disseldorp <ddiss@suse.de>

Why not fold this into the previous patch?

--D

> ---
>  check | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/check b/check
> index 726c83d9..baf336da 100755
> --- a/check
> +++ b/check
> @@ -570,8 +570,17 @@ _stash_test_status() {
>  				      "$report_msg"
>  	fi
>  
> -	# only stash result for first failure (triggering loop)
> -	((loop_num > 1)) && return
> +	if ((loop_num > 0)); then
> +		# retain files which would be overwritten in subsequent reruns
> +		for i in "${REPORT_DIR}/${test_seq}.full" \
> +			 "${REPORT_DIR}/${test_seq}.dmesg" \
> +			 "${REPORT_DIR}/${test_seq}.out.bad"; do
> +			[ -f "$i" ] || continue
> +			cp "$i" "${i}.rerun$((loop_num - 1))"
> +		done
> +		# only stash result for first failure (triggering loop)
> +		((loop_num != 1)) && return
> +	fi
>  
>  	case "$test_status" in
>  	fail)
> -- 
> 2.35.3
> 

Re: [RFC PATCH v2 5/6] check: add -L <n> parameter to rerun failed tests

[David Disseldorp]

Thanks for the review, Darrick...

On Tue, 28 Jun 2022 08:15:07 -0700, Darrick J. Wong wrote:

> On Tue, Jun 28, 2022 at 12:22:55AM +0200, David Disseldorp wrote:
> > If check is run with -L <n>, then a failed test will be rerun <n> times
> > before proceeding to the next test. Following completion of the rerun
> > loop, aggregate pass/fail statistics are printed.
> > 
> > Rerun tests will be tracked as a single failure in overall pass/fail
> > metrics (via @try and @bad), with .out.bad, .dmesg and .full saved using
> > a .rerun# suffix.
> > 
> > Suggested-by: Theodore Ts'o <tytso@mit.edu>
> > Link: https://lwn.net/Articles/897061/
> > Signed-off-by: David Disseldorp <ddiss@suse.de>
> > ---
> >  check         | 94 ++++++++++++++++++++++++++++++++++++++++++++-------
> >  common/report |  8 +++--
> >  2 files changed, 88 insertions(+), 14 deletions(-)
> > 
> > diff --git a/check b/check
> > index aa7dac2f..726c83d9 100755
> > --- a/check
> > +++ b/check
> > @@ -26,6 +26,7 @@ do_report=false
> >  DUMP_OUTPUT=false
> >  iterations=1
> >  istop=false
> > +loop_on_fail=0
> >  
> >  # This is a global variable used to pass test failure text to reporting gunk
> >  _err_msg=""
> > @@ -75,6 +76,7 @@ check options
> >      --large-fs		optimise scratch device for large filesystems
> >      -s section		run only specified section from config file
> >      -S section		exclude the specified section from the config file
> > +    -L <n>		loop tests <n> times following a failure, measuring aggregate pass/fail metrics
> >  
> >  testlist options
> >      -g group[,group...]	include tests from these groups
> > @@ -333,6 +335,9 @@ while [ $# -gt 0 ]; do
> >  		;;
> >  	--large-fs) export LARGE_SCRATCH_DEV=yes ;;
> >  	--extra-space=*) export SCRATCH_DEV_EMPTY_SPACE=${r#*=} ;;
> > +	-L)	[[ $2 =~ ^[0-9]+$ ]] || usage
> > +		loop_on_fail=$2; shift
> > +		;;
> >  
> >  	-*)	usage ;;
> >  	*)	# not an argument, we've got tests now.
> > @@ -555,6 +560,18 @@ _expunge_test()
> >  _stash_test_status() {
> >  	local test_seq="$1"
> >  	local test_status="$2"
> > +	local test_time="$3"
> > +	local loop_num="$4"
> > +	local report_msg="$5"
> > +
> > +	if $do_report && [[ ! $test_status =~ ^(init|expunge)$ ]]; then
> > +		_make_testcase_report "$section" "$test_seq" \
> > +				      "$test_status" "$test_time" \
> > +				      "$report_msg"
> > +	fi
> > +
> > +	# only stash result for first failure (triggering loop)
> > +	((loop_num > 1)) && return
> >  
> >  	case "$test_status" in
> >  	fail)
> > @@ -610,6 +627,38 @@ _run_seq() {
> >  	fi
> >  }
> >  
> > +# Check whether the last test should be rerun according to loop-on-error state
> > +# and return "0" if so, otherwise return "1".  
> 
> Er... this echoes 0 and 1, and the return value of the function is
> neither checked nor set to anything other than zero, right?

Right. run_section() test list iteration uses this helper to
conditionally increment the test index, depending on whether or not a
rerun (following test failure) is required.

> I'm also not sure what 'ix' stands for?

Index... I figured 'i' would get stomped on and didn't come up with a
better name at the time :)

> > +_ix_inc() {
> > +	local test_status="$1"
> > +	local loop_len="$2"
> > +
> > +	if ((!loop_on_fail)); then
> > +		echo 1
> > +		return
> > +	fi
> > +
> > +	if [ "$test_status" == "fail" ] && ((!loop_len)); then
> > +		echo 0	# initial failure of this test, start loop-on-fail
> > +	elif ((loop_len > 0)) && ((loop_len < loop_on_fail)); then
> > +		echo 0	# continue loop following initial failure
> > +	else
> > +		echo 1	# completed or not currently in a failure loop
> > +	fi
> > +}
> > +
> > +_failure_loop_dump_stats() {
> > +	awk "BEGIN {
> > +		n=split(\"$*\", arr);"'
> > +		for (i = 1; i <= n; i++)
> > +			stats[arr[i]]++;
> > +		printf("aggregate results across %d runs: ", n);
> > +		for (x in stats)
> > +			printf("%s=%d (%.1f%%)", (i-- > n ? x : ", " x),
> > +			       stats[x], 100 * stats[x] / n);
> > +	     }'
> > +}
> > +
> >  _detect_kmemleak
> >  _prepare_test_list
> >  
> > @@ -750,14 +799,29 @@ function run_section()
> >  	seqres="$check"
> >  	_check_test_fs
> >  
> > -	local tc_status="init"
> > +	local tc_status="init" ix agg_msg
> >  	prev_seq=""
> > -	for seq in $list ; do
> > +	local -a _list=( $list ) loop_status=()
> > +	for ((ix = 0; ix < ${#_list[*]};
> > +	      ix += $(_ix_inc "$tc_status" "${#loop_status[*]}"))); do

Here is the _ix_inc caller ^^. It's a bit convoluted so I could probably
turn it into a while loop and put the increment logic here. Not sure how
much simpler it'd be though.

> > +		seq="${_list[$ix]}"
> > +
> > +		if [ "$seq" == "$prev_seq" ]; then
> > +			loop_status+=("$tc_status")
> > +		elif ((${#loop_status[*]})); then
> > +			# leaving rerun-on-failure loop
> > +			loop_status+=("$tc_status")
> > +			agg_msg=$(_failure_loop_dump_stats "${loop_status[@]}")
> > +			echo "$seqnum $agg_msg"
> > +		fi
> > +
> >  		# Run report for previous test!
> > -		_stash_test_status "$seqnum" "$tc_status"
> > -		if $do_report && [[ ! $tc_status =~ ^(init|expunge)$ ]]; then
> > -			_make_testcase_report "$section" "$seqnum" \
> > -					      "$tc_status" "$((stop - start))"
> > +		_stash_test_status "$seqnum" "$tc_status" "$((stop - start))" \
> > +				   "${#loop_status[*]}" "$agg_msg"
> > +
> > +		if [ -n "$agg_msg" ]; then
> > +			loop_status=()
> > +			agg_msg=""
> >  		fi
> >  
> >  		prev_seq="$seq"
> > @@ -827,7 +891,9 @@ function run_section()
> >  		fi
> >  
> >  		# record that we really tried to run this test.
> > -		try+=("$seqnum")
> > +		if ((!${#loop_status[*]})); then
> > +			try+=("$seqnum")
> > +		fi
> >  
> >  		awk 'BEGIN {lasttime="       "} \
> >  		     $1 == "'$seqnum'" {lasttime=" " $2 "s ... "; exit} \
> > @@ -954,13 +1020,17 @@ function run_section()
> >  		fi
> >  	done
> >  
> > -	# make sure we record the status of the last test we ran.
> > -	_stash_test_status "$seqnum" "$tc_status"
> > -	if $do_report && [[ ! $tc_status =~ ^(init|expunge)$ ]]; then
> > -		_make_testcase_report "$section" "$seqnum" "$tc_status" \
> > -				      "$((stop - start))"
> > +	if ((${#loop_status[*]})); then
> > +		# leaving rerun-on-failure loop
> > +		loop_status+=("$tc_status")
> > +		agg_msg=$(_failure_loop_dump_stats "${loop_status[@]}")
> > +		echo "$seqnum $agg_msg"
> >  	fi
> >  
> > +	# Run report for previous test!
> > +	_stash_test_status "$seqnum" "$tc_status" "$((stop - start))" \
> > +			   "${#loop_status[*]}" "$agg_msg"
> > +
> >  	sect_stop=`_wallclock`
> >  	interrupt=false
> >  	_wrapup
> > diff --git a/common/report b/common/report
> > index 5ca41bc4..cede4987 100644
> > --- a/common/report
> > +++ b/common/report
> > @@ -71,6 +71,7 @@ _xunit_make_testcase_report()
> >  	local test_name="$2"
> >  	local test_status="$3"
> >  	local test_time="$4"
> > +	local test_md="$5"  
> 
> ...or what "md" here means.

Test (result) metadata.

> >  	# TODO: other places may also win if no-section mode will be named like 'default/global'
> >  	if [ $sect_name == '-no-sections-' ]; then
> > @@ -79,7 +80,8 @@ _xunit_make_testcase_report()
> >  	fi
> >  	local report=$tmp.report.xunit.$sect_name.xml
> >  
> > -	echo -e "\t<testcase classname=\"xfstests.$sect_name\" name=\"$test_name\" time=\"$test_time\">" >> $report
> > +	[ -n "$test_md" ] && test_md=" status=\"$(echo "$test_md"|encode_xml)\""  
> 
> AFAICT the junit xml schema defines a @status attribute for <testcase>.
> 
> https://raw.githubusercontent.com/windyroad/JUnit-Schema/master/JUnit.xsd

^ hmm, I don't see a @status attribute under your link. I was using a
different reference...

> And yes, it's confusing that fstests namespaced all this code with
> 'xunit', since there's a separate xunit test reporting schema too!

It is absolutely confusing. I've been using
https://llg.cubic.org/docs/junit/ but only came across that by searching
for specific attribute names.

> That said -- I'm not sure what's supposed to end up in this attribute?
> It looks like it's supposed to capture the aggregation report?

Correct, it's just the aggregation report for now.

> But then I wonder, why not stick to adding a separate <testcase> element
> for each test run, even the repeated ones?  And let the xml consumer
> compute aggregation statistics from the elements?

This v2 patchset does add a <testcase> element for each rerun, with the
aggregation stats attached to the last rerun . The
aggregation stats are already calculated for non-xunit output, and
passing it through for xunit only costs a few lines of code. However, if
the @status attribute isn't an option then I suppose I could drop it -
Ted also mentioned that it's unnecessary.

Cheers, David

Re: [RFC PATCH v2 6/6] check: stash full/dmesg/out.bad files on rerun

[David Disseldorp]

On Tue, 28 Jun 2022 08:16:08 -0700, Darrick J. Wong wrote:

> On Tue, Jun 28, 2022 at 12:22:56AM +0200, David Disseldorp wrote:
> > These files would otherwise be overwritten.
> > 
> > Signed-off-by: David Disseldorp <ddiss@suse.de>  
> 
> Why not fold this into the previous patch?

Gladly. Will do so in a subsequent round.

Cheers, David