[PATCH v2 0/3] Add support for -q <n> unconditional loop

[PATCH v2 0/3] Add support for -q <n> unconditional loop

[Nirjhar Roy (IBM)]

This series introduces "-q <n>" option to support unconditional looping.
Along with this, it also brings some improvements to output data both, in stdout
and in result.log.

-q <n> option can be useful for fast unconditional looping (compared to
-I <n>) to test a certain flakey test and get it's pass/fail metrics.

This is similar to -L <n>, although with -L the test only loops if it
fails the 1st time.
The individual patches can provide more details about other changes and
improvements.

[v1] -> [v2]
 1. This patch series removes the central config fs feature from [v1] (patch 4
    patch 5) so that this can be reviewed separately. No functional changes.
 2. Added R.B tag of Darrick is patch  1/5.

[v1] https://lore.kernel.org/all/cover.1736496620.git.nirjhar.roy.lists@gmail.com/

Nirjhar Roy (IBM) (3):
  tests/selftest: Add a new pseudo flaky test.
  check: Add -q <n> option to support unconditional looping.
  check: Improve pass/fail metrics and section config output

 check                  | 113 ++++++++++++++++++++++++++++++-----------
 tests/selftest/007     |  21 ++++++++
 tests/selftest/007.out |   2 +
 3 files changed, 105 insertions(+), 31 deletions(-)
 create mode 100755 tests/selftest/007
 create mode 100644 tests/selftest/007.out

--
2.34.1

[PATCH v2 1/3] tests/selftest: Add a new pseudo flaky test.

[Nirjhar Roy (IBM)]

This test is to simulate the behavior of a flaky test. This will be required
when we will make some modifications to the pass/fail metric calculation of
the test infrastructure where we will need a test with non-zero pass
and non-zero failure rate.

Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com>
---
 tests/selftest/007     | 21 +++++++++++++++++++++
 tests/selftest/007.out |  2 ++
 2 files changed, 23 insertions(+)
 create mode 100755 tests/selftest/007
 create mode 100644 tests/selftest/007.out

diff --git a/tests/selftest/007 b/tests/selftest/007
new file mode 100755
index 00000000..f100ec5f
--- /dev/null
+++ b/tests/selftest/007
@@ -0,0 +1,21 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2024 IBM Corporation.  All Rights Reserved.
+# Author: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com>
+#
+# FS QA Test 007
+#
+# This test is to simulate the behavior of a flakey test.
+#
+
+. ./common/preamble
+_begin_fstest selftest
+
+if (($RANDOM % 2)); then
+    echo "Silence is golden"
+else
+    echo "Silence is flakey"
+fi
+
+status=0
+exit
diff --git a/tests/selftest/007.out b/tests/selftest/007.out
new file mode 100644
index 00000000..fd3590e6
--- /dev/null
+++ b/tests/selftest/007.out
@@ -0,0 +1,2 @@
+QA output created by 007
+Silence is golden
-- 
2.34.1

[PATCH v2 2/3] check: Add -q <n> option to support unconditional looping.

[Nirjhar Roy (IBM)]

This patch adds -q <n> option through which one can run a given test <n>
times unconditionally. It also prints pass/fail metrics at the end.

The advantage of this over -L <n> and -i/-I <n> is that:
    a. -L <n> will not re-run a flakey test if the test passes for the first time.
    b. -I/-i <n> sets up devices during each iteration and hence slower.
Note -q <n> will override -L <n>.

Also rename _stash_fail_loop_files() to _stash_loop_files()
because this function will now be used even when the test doesn't fail
(i.e. when ran with -q <n>).

Suggested-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com>
Co-developed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
---
 check | 46 ++++++++++++++++++++++++++++++++++++----------
 1 file changed, 36 insertions(+), 10 deletions(-)

diff --git a/check b/check
index 32890470..f6007750 100755
--- a/check
+++ b/check
@@ -11,6 +11,7 @@ needsum=true
 try=()
 sum_bad=0
 bad=()
+is_bad_test=false
 notrun=()
 interrupt=true
 diff="diff -u"
@@ -27,6 +28,7 @@ DUMP_OUTPUT=false
 iterations=1
 istop=false
 loop_on_fail=0
+loop_unconditional=0
 exclude_tests=()
 
 # This is a global variable used to pass test failure text to reporting gunk
@@ -83,6 +85,7 @@ check options
     -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
+    -q <n>		loop tests <n> times irrespective of a pass or a failure, measuring aggregate pass/fail metrics
 
 testlist options
     -g group[,group...]	include tests from these groups
@@ -341,7 +344,9 @@ while [ $# -gt 0 ]; do
 	-L)	[[ $2 =~ ^[0-9]+$ ]] || usage
 		loop_on_fail=$2; shift
 		;;
-
+	-q)	[[ $2 =~ ^[0-9]+$ ]] || usage
+		loop_unconditional=$(($2 - 1)); shift
+		;;
 	-*)	usage ;;
 	*)	# not an argument, we've got tests now.
 		have_test_arg=true ;;
@@ -357,6 +362,11 @@ while [ $# -gt 0 ]; do
 	shift
 done
 
+# -q <n> overrides -L <m>
+if [ "$loop_unconditional" -gt 0 ]; then
+	loop_on_fail=0
+fi
+
 # we need common/rc, that also sources common/config. We need to source it
 # after processing args, overlay needs FSTYP set before sourcing common/config
 if ! . ./common/rc; then
@@ -609,7 +619,7 @@ _expunge_test()
 }
 
 # retain files which would be overwritten in subsequent reruns of the same test
-_stash_fail_loop_files() {
+_stash_loop_files() {
 	local seq_prefix="${REPORT_DIR}/${1}"
 	local cp_suffix="$2"
 
@@ -633,10 +643,18 @@ _stash_test_status() {
 	fi
 
 	if ((${#loop_status[*]} > 0)); then
-		# continuing or completing rerun-on-failure loop
-		_stash_fail_loop_files "$test_seq" ".rerun${#loop_status[*]}"
+		# continuing or completing rerun loop
+		_stash_loop_files "$test_seq" ".rerun${#loop_status[*]}"
 		loop_status+=("$test_status")
-		if ((${#loop_status[*]} > loop_on_fail)); then
+
+		# only stash @bad result for initial failure in loop
+		if [[ "$test_status" == "fail" ]] && ! $is_bad_test; then
+			bad+=("$test_seq")
+			is_bad_test=true
+		fi
+
+		if ((loop_on_fail && ${#loop_status[*]} > loop_on_fail)) || \
+		   ((loop_unconditional && ${#loop_status[*]} > loop_unconditional)); then
 			printf "%s aggregate results across %d runs: " \
 				"$test_seq" "${#loop_status[*]}"
 			awk "BEGIN {
@@ -650,23 +668,30 @@ _stash_test_status() {
 				}'
 			echo
 			loop_status=()
+			is_bad_test=false
 		fi
-		return	# only stash @bad result for initial failure in loop
+		return
 	fi
 
 	case "$test_status" in
 	fail)
-		if ((loop_on_fail > 0)); then
-			# initial failure, start rerun-on-failure loop
-			_stash_fail_loop_files "$test_seq" ".rerun0"
+		# re-run if either of the loop argument is set
+		if ((loop_on_fail > 0)) || ((loop_unconditional > 0)); then
+			_stash_loop_files "$test_seq" ".rerun0"
 			loop_status+=("$test_status")
 		fi
 		bad+=("$test_seq")
+		is_bad_test=true
 		;;
 	list|notrun)
 		notrun+=("$test_seq")
 		;;
 	pass|expunge)
+		# re-run if loop_unconditional argument is set
+		if ((loop_unconditional > 0)); then
+			_stash_loop_files "$test_seq" ".rerun0"
+			loop_status+=("$test_status")
+		fi
 		;;
 	*)
 		echo "Unexpected test $test_seq status: $test_status"
@@ -852,7 +877,8 @@ function run_section()
 	seqres="$check"
 	_check_test_fs
 
-	loop_status=()	# track rerun-on-failure state
+	is_bad_test=false
+	loop_status=()	# track loop rerun state
 	local tc_status ix
 	local -a _list=( $list )
 	for ((ix = 0; ix < ${#_list[*]}; !${#loop_status[*]} && ix++)); do
-- 
2.34.1

[PATCH v2 3/3] check: Improve pass/fail metrics and section config output

[Nirjhar Roy (IBM)]

This patch improves the output by:
- Adding pass/fail aggregate results in the stdout for each section's
  summary. Also adds the location of the xunit xml files in the
  section's summary.
- Adds section config details into the result's global log file.

e.g.

$ ./check -q 2 -R xunit-quiet   selftest/001

SECTION       -- s1
=========================
selftest/001 aggregate results across 2 runs: pass=2 (100.0%)
Ran: selftest/001
Passed all 1 tests
Xunit report: /home/ubuntu/xfstests/results//s1/result.xml

SECTION       -- s2
=========================
selftest/001 aggregate results across 2 runs: pass=2 (100.0%)
Ran: selftest/001
Passed all 1 tests
Xunit report: /home/ubuntu/xfstests/results//s2/result.xml

Output in results/check.log file

Kernel version: 6.13.0-rc1-00182-gb8f52214c61a-dirty
Wed Jan  8 11:23:57 UTC 2025
SECTION       -- s1
=========================
FSTYP         -- ext4
PLATFORM      -- Linux/x86_64 citest-1 6.13.0-rc1-00182-gb8f52214c61a-dirty #5 SMP PREEMPT_DYNAMIC Wed Dec 18 14:03:34 IST 2024
MKFS_OPTIONS  -- -F /dev/loop1
MOUNT_OPTIONS -- -o acl,user_xattr /dev/loop1 /mnt1/scratch
selftest/001 aggregate results across 2 runs: pass=2 (100.0%)
Ran: selftest/001
Passed all 1 tests
Xunit report: /home/ubuntu/xfstests/results//s1/result.xml

Kernel version: 6.13.0-rc1-00182-gb8f52214c61a-dirty
Wed Jan  8 11:24:02 UTC 2025
SECTION       -- s2
=========================
FSTYP         -- ext4
PLATFORM      -- Linux/x86_64 citest-1 6.13.0-rc1-00182-gb8f52214c61a-dirty #5 SMP PREEMPT_DYNAMIC Wed Dec 18 14:03:34 IST 2024
MKFS_OPTIONS  -- -F /dev/loop1
MOUNT_OPTIONS -- -o acl,user_xattr /dev/loop1 /mnt1/scratch
selftest/001 aggregate results across 2 runs: pass=2 (100.0%)
Ran: selftest/001
Passed all 1 tests
Xunit report: /home/ubuntu/xfstests/results//s2/result.xml

Suggested-by: Ritesh Harjani <ritesh.list@gmail.com>
Co-developed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com>
---
 check | 67 ++++++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 46 insertions(+), 21 deletions(-)

diff --git a/check b/check
index f6007750..bb91cf9d 100755
--- a/check
+++ b/check
@@ -468,6 +468,25 @@ if [ -n "$REPORT_GCOV" ]; then
 	_gcov_check_report_gcov
 fi
 
+_print_list()
+{
+	local -n list=$1
+	local item
+	for item in "${list[@]}"; do
+		echo "$item"
+	done
+}
+
+_display_test_configuration()
+{
+	echo "FSTYP         -- `_full_fstyp_details`"
+	echo "PLATFORM      -- `_full_platform_details`"
+	if [ ! -z "$SCRATCH_DEV" ]; then
+		echo "MKFS_OPTIONS  -- `_scratch_mkfs_options`"
+		echo "MOUNT_OPTIONS -- `_scratch_mount_options`"
+	fi
+}
+
 _wrapup()
 {
 	seq="check.$$"
@@ -505,11 +524,19 @@ _wrapup()
 
 		echo "SECTION       -- $section" >>$tmp.summary
 		echo "=========================" >>$tmp.summary
+
+		_global_log "SECTION       -- $section"
+		_global_log "=========================" >>$tmp.summary
+		_global_log "$(_display_test_configuration)"
 		if ((${#try[*]} > 0)); then
+			local test_aggr_stats=$(_print_list loop_test_stats_per_section)
 			if [ $brief_test_summary == "false" ]; then
+				[[ -n "$test_aggr_stats" ]] && echo "$test_aggr_stats" >> \
+					$tmp.summary
 				echo "Ran: ${try[*]}"
 				echo "Ran: ${try[*]}" >>$tmp.summary
 			fi
+			_global_log "$test_aggr_stats"
 			_global_log "Ran: ${try[*]}"
 		fi
 
@@ -539,12 +566,15 @@ _wrapup()
 			_global_log "Passed all ${#try[*]} tests"
 			echo "Passed all ${#try[*]} tests" >>$tmp.summary
 		fi
-		echo "" >>$tmp.summary
 		if $do_report; then
 			_make_section_report "$section" "${#try[*]}" \
 					     "${#bad[*]}" "${#notrun[*]}" \
 					     "$((sect_stop - sect_start))"
+			local out_fn="$REPORT_DIR/result.xml"
+			echo "Xunit report: $out_fn" >> $tmp.summary
+			_global_log "Xunit report: $out_fn"
 		fi
+		echo "" >>$tmp.summary
 
 		# Generate code coverage report
 		if [ -n "$REPORT_GCOV" ]; then
@@ -655,18 +685,19 @@ _stash_test_status() {
 
 		if ((loop_on_fail && ${#loop_status[*]} > loop_on_fail)) || \
 		   ((loop_unconditional && ${#loop_status[*]} > loop_unconditional)); then
-			printf "%s aggregate results across %d runs: " \
-				"$test_seq" "${#loop_status[*]}"
-			awk "BEGIN {
-				n=split(\"${loop_status[*]}\", arr);"'
-				for (i = 1; i <= n; i++)
-					stats[arr[i]]++;
-				for (x in stats)
-					printf("%s=%d (%.1f%%)",
-					       (i-- > n ? x : ", " x),
-					       stats[x], 100 * stats[x] / n);
-				}'
-			echo
+			local test_stats=$(printf "%s aggregate results across %d runs: " \
+				"$test_seq" "${#loop_status[*]}")
+			test_stats+=$(awk "BEGIN {
+					n=split(\"${loop_status[*]}\", arr);"'
+					for (i = 1; i <= n; i++)
+						stats[arr[i]]++;
+					for (x in stats)
+						printf("%s=%d (%.1f%%)",
+							   (i-- > n ? x : ", " x),
+							   stats[x], 100 * stats[x] / n);
+					}')
+			echo "$test_stats"
+			loop_test_stats_per_section+=("$test_stats")
 			loop_status=()
 			is_bad_test=false
 		fi
@@ -834,14 +865,7 @@ function run_section()
 	rm -f $check.full
 
 	[ -f $check.time ] || touch $check.time
-
-	# print out our test configuration
-	echo "FSTYP         -- `_full_fstyp_details`"
-	echo "PLATFORM      -- `_full_platform_details`"
-	if [ ! -z "$SCRATCH_DEV" ]; then
-	  echo "MKFS_OPTIONS  -- `_scratch_mkfs_options`"
-	  echo "MOUNT_OPTIONS -- `_scratch_mount_options`"
-	fi
+	_display_test_configuration
 	echo
 	test -n "$REPORT_GCOV" && _gcov_reset
 	needwrap=true
@@ -879,6 +903,7 @@ function run_section()
 
 	is_bad_test=false
 	loop_status=()	# track loop rerun state
+	loop_test_stats_per_section=() # store loop test statistics per section
 	local tc_status ix
 	local -a _list=( $list )
 	for ((ix = 0; ix < ${#_list[*]}; !${#loop_status[*]} && ix++)); do
-- 
2.34.1

Re: [PATCH v2 0/3] Add support for -q <n> unconditional loop

[Nirjhar Roy (IBM)]

On 4/3/25 14:28, Nirjhar Roy (IBM) wrote:
> This series introduces "-q <n>" option to support unconditional looping.
> Along with this, it also brings some improvements to output data both, in stdout
> and in result.log.
>
> -q <n> option can be useful for fast unconditional looping (compared to
> -I <n>) to test a certain flakey test and get it's pass/fail metrics.
>
> This is similar to -L <n>, although with -L the test only loops if it
> fails the 1st time.
> The individual patches can provide more details about other changes and
> improvements.

Can I please get some reviews on this patch series?

--NR

>
> [v1] -> [v2]
>   1. This patch series removes the central config fs feature from [v1] (patch 4
>      patch 5) so that this can be reviewed separately. No functional changes.
>   2. Added R.B tag of Darrick is patch  1/5.
>
> [v1] https://lore.kernel.org/all/cover.1736496620.git.nirjhar.roy.lists@gmail.com/
>
> Nirjhar Roy (IBM) (3):
>    tests/selftest: Add a new pseudo flaky test.
>    check: Add -q <n> option to support unconditional looping.
>    check: Improve pass/fail metrics and section config output
>
>   check                  | 113 ++++++++++++++++++++++++++++++-----------
>   tests/selftest/007     |  21 ++++++++
>   tests/selftest/007.out |   2 +
>   3 files changed, 105 insertions(+), 31 deletions(-)
>   create mode 100755 tests/selftest/007
>   create mode 100644 tests/selftest/007.out
>
> --
> 2.34.1
>
-- 
Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore

Re: [PATCH v2 2/3] check: Add -q <n> option to support unconditional looping.

[Theodore Ts'o]

On Thu, Apr 03, 2025 at 08:58:19AM +0000, Nirjhar Roy (IBM) wrote:
> This patch adds -q <n> option through which one can run a given test <n>
> times unconditionally. It also prints pass/fail metrics at the end.
> 
> The advantage of this over -L <n> and -i/-I <n> is that:
>     a. -L <n> will not re-run a flakey test if the test passes for the first time.
>     b. -I/-i <n> sets up devices during each iteration and hence slower.
> Note -q <n> will override -L <n>.

I'm wondering if we need to keep the current behavior of -I/-i.  The
primary difference between them and how your proposed -q works is that
instead of iterating over the section, your proposed option iterates
over each test.  So for example, if a section contains generic/001 and
generic/002, iterating using -i 3 will do this:

generic/001
generic/002
generic/001
generic/002
generic/001
generic/002

While generic -q 3 would do this instead:

generic/001
generic/001
generic/001
generic/002
generic/002
generic/002


At least for all of the use cases that I can think of where I might
use -i 3, -q 3 is strictly better.  So instead of adding more options
which change how we might do iterations, could we perhaps just replace
-i with your new -q?  And change -I so that it also works like -q,
except if any test fails, that we stop?

					- Ted

Re: [PATCH v2 2/3] check: Add -q <n> option to support unconditional looping.

[Nirjhar Roy (IBM)]

On 4/14/25 03:18, Theodore Ts'o wrote:
> On Thu, Apr 03, 2025 at 08:58:19AM +0000, Nirjhar Roy (IBM) wrote:
>> This patch adds -q <n> option through which one can run a given test <n>
>> times unconditionally. It also prints pass/fail metrics at the end.
>>
>> The advantage of this over -L <n> and -i/-I <n> is that:
>>      a. -L <n> will not re-run a flakey test if the test passes for the first time.
>>      b. -I/-i <n> sets up devices during each iteration and hence slower.
>> Note -q <n> will override -L <n>.
> I'm wondering if we need to keep the current behavior of -I/-i.  The
> primary difference between them and how your proposed -q works is that
> instead of iterating over the section, your proposed option iterates
> over each test.  So for example, if a section contains generic/001 and
> generic/002, iterating using -i 3 will do this:

Yes, the motivation to introduce -q was to:

1. Make the re-run faster and not re-format the device. -i re-formats 
the device and hence is slightly slower.

2. To unconditionally loop a test - useful for scenarios when a flaky 
test doesn't fail for the first time (something that -L) does.

So, are saying that re-formatting a disk on every run, something that -i 
does, doesn't have much value and can be removed?

>
> generic/001
> generic/002
> generic/001
> generic/002
> generic/001
> generic/002
>
> While generic -q 3 would do this instead:
>
> generic/001
> generic/001
> generic/001
> generic/002
> generic/002
> generic/002
>
>
> At least for all of the use cases that I can think of where I might
> use -i 3, -q 3 is strictly better.  So instead of adding more options
> which change how we might do iterations, could we perhaps just replace
> -i with your new -q?  And change -I so that it also works like -q,
> except if any test fails, that we stop?

So -I won't re-format the devices during the loop? is that what your 
suggestion is?

--NR

>
> 					- Ted

-- 
Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore

Re: [PATCH v2 2/3] check: Add -q <n> option to support unconditional looping.

[Ritesh Harjani]

"Theodore Ts'o" <tytso@mit.edu> writes:

> On Thu, Apr 03, 2025 at 08:58:19AM +0000, Nirjhar Roy (IBM) wrote:
>> This patch adds -q <n> option through which one can run a given test <n>
>> times unconditionally. It also prints pass/fail metrics at the end.
>> 
>> The advantage of this over -L <n> and -i/-I <n> is that:
>>     a. -L <n> will not re-run a flakey test if the test passes for the first time.
>>     b. -I/-i <n> sets up devices during each iteration and hence slower.
>> Note -q <n> will override -L <n>.

First things first -q is similar to -L except that it loops
unconditionaly even when there is no failure. (Bad choice of -q naming,
since -l was in use for something else)

>
> I'm wondering if we need to keep the current behavior of -I/-i.  The
> primary difference between them and how your proposed -q works is that
> instead of iterating over the section, your proposed option iterates
> over each test.  So for example, if a section contains generic/001 and
> generic/002, iterating using -i 3 will do this:
>
> generic/001
> generic/002
> generic/001
> generic/002
> generic/001
> generic/002
>
> While generic -q 3 would do this instead:
>
> generic/001
> generic/001
> generic/001
> generic/002
> generic/002
> generic/002
>

Yes, that's correct. Since it iterates at the top level, it also sources
some common configs, re-formats and does a mount re-cycle of the scratch
device during each iteration.

This mostly should not matter, since each test can anyways re-format the
scratch device when it needs to mount/test it.

>
> At least for all of the use cases that I can think of where I might
> use -i 3, -q 3 is strictly better.  So instead of adding more options
> which change how we might do iterations, could we perhaps just replace
> -i with your new -q?  And change -I so that it also works like -q,
> except if any test fails, that we stop?

I agree that in this case it make more sense to replace the underlying
functionality of -i/-I with what -q <n> is doing here. But maybe others
can comment, if anyone has any objection on this.

-ritesh

Re: [PATCH v2 2/3] check: Add -q <n> option to support unconditional looping.

[Dave Chinner]

On Tue, Apr 15, 2025 at 01:02:49PM +0530, Nirjhar Roy (IBM) wrote:
> 
> On 4/14/25 03:18, Theodore Ts'o wrote:
> > On Thu, Apr 03, 2025 at 08:58:19AM +0000, Nirjhar Roy (IBM) wrote:
> > > This patch adds -q <n> option through which one can run a given test <n>
> > > times unconditionally. It also prints pass/fail metrics at the end.
> > > 
> > > The advantage of this over -L <n> and -i/-I <n> is that:
> > >      a. -L <n> will not re-run a flakey test if the test passes for the first time.
> > >      b. -I/-i <n> sets up devices during each iteration and hence slower.
> > > Note -q <n> will override -L <n>.
> > I'm wondering if we need to keep the current behavior of -I/-i.  The
> > primary difference between them and how your proposed -q works is that
> > instead of iterating over the section, your proposed option iterates
> > over each test.  So for example, if a section contains generic/001 and
> > generic/002, iterating using -i 3 will do this:
> 
> Yes, the motivation to introduce -q was to:
> 
> 1. Make the re-run faster and not re-format the device. -i re-formats the
> device and hence is slightly slower.

Why does -i reformat the test device on every run in your setup?
i.e. if the FSTYP is not changing from iteration to iteration, then
each iteration should not reformat the test device at all. Unless, of
course, you have told it to do so via the RECREATE_TEST_DEV env
variable....

Hence it seems to me like this is working around some other setup or
section iteration problem here...

> 2. To unconditionally loop a test - useful for scenarios when a flaky test
> doesn't fail for the first time (something that -L) does.

That's what -i does. it will unconditionally loop over the specified
tests N times regardless of success or failure.

OTOH, -I will abort on first failure. i.e. to enable flakey tests
to be run until it eventually fails and leave the corpse behind for
debugging.

> So, are saying that re-formatting a disk on every run, something that -i
> does, doesn't have much value and can be removed?

-i does not imply that the test device should be reformatted on
every loop. If that is happening, that is likely a result of test
config or environment conditions.

Can you tell us why the test device is getting reformatted on every
iteration in your setup?

> > generic/001
> > generic/002
> > generic/001
> > generic/002
> > generic/001
> > generic/002
> > 
> > While generic -q 3 would do this instead:
> > 
> > generic/001
> > generic/001
> > generic/001
> > generic/002
> > generic/002
> > generic/002

There are arguments both for and against the different iteration
orders. However, if there is no overriding reason to change the
existing order of test execution, then we should not change the
order or test execution....

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v2 2/3] check: Add -q <n> option to support unconditional looping.

[Nirjhar Roy (IBM)]

On 4/16/25 04:58, Dave Chinner wrote:
> On Tue, Apr 15, 2025 at 01:02:49PM +0530, Nirjhar Roy (IBM) wrote:
>> On 4/14/25 03:18, Theodore Ts'o wrote:
>>> On Thu, Apr 03, 2025 at 08:58:19AM +0000, Nirjhar Roy (IBM) wrote:
>>>> This patch adds -q <n> option through which one can run a given test <n>
>>>> times unconditionally. It also prints pass/fail metrics at the end.
>>>>
>>>> The advantage of this over -L <n> and -i/-I <n> is that:
>>>>       a. -L <n> will not re-run a flakey test if the test passes for the first time.
>>>>       b. -I/-i <n> sets up devices during each iteration and hence slower.
>>>> Note -q <n> will override -L <n>.
>>> I'm wondering if we need to keep the current behavior of -I/-i.  The
>>> primary difference between them and how your proposed -q works is that
>>> instead of iterating over the section, your proposed option iterates
>>> over each test.  So for example, if a section contains generic/001 and
>>> generic/002, iterating using -i 3 will do this:
>> Yes, the motivation to introduce -q was to:
>>
>> 1. Make the re-run faster and not re-format the device. -i re-formats the
>> device and hence is slightly slower.
> Why does -i reformat the test device on every run in your setup?
> i.e. if the FSTYP is not changing from iteration to iteration, then
> each iteration should not reformat the test device at all. Unless, of
> course, you have told it to do so via the RECREATE_TEST_DEV env
> variable....

No, it doesn't re-format the test device. It re-formats the scratch 
device. With -q, there will be no re-formatting of the scratch device too.

>
> Hence it seems to me like this is working around some other setup or
> section iteration problem here...
>
>> 2. To unconditionally loop a test - useful for scenarios when a flaky test
>> doesn't fail for the first time (something that -L) does.
> That's what -i does. it will unconditionally loop over the specified
> tests N times regardless of success or failure.
>
> OTOH, -I will abort on first failure. i.e. to enable flakey tests
> to be run until it eventually fails and leave the corpse behind for
> debugging.
>
>> So, are saying that re-formatting a disk on every run, something that -i
>> does, doesn't have much value and can be removed?
> -i does not imply that the test device should be reformatted on
> every loop. If that is happening, that is likely a result of test
> config or environment conditions.
>
> Can you tell us why the test device is getting reformatted on every
> iteration in your setup?

As mentioned above, -i isn't reformatting our test device. It is 
re-formatting scratch device and we introduced -q to unconditionally 
loop without even reformatting the scratch device, and hence making the 
re-runs faster.

--NR

>
>>> generic/001
>>> generic/002
>>> generic/001
>>> generic/002
>>> generic/001
>>> generic/002
>>>
>>> While generic -q 3 would do this instead:
>>>
>>> generic/001
>>> generic/001
>>> generic/001
>>> generic/002
>>> generic/002
>>> generic/002
> There are arguments both for and against the different iteration
> orders. However, if there is no overriding reason to change the
> existing order of test execution, then we should not change the
> order or test execution....
>
> -Dave.

-- 
Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore