* [PATCH v3 1/5] report: use array for REPORT_ENV_LIST
2022-07-06 11:23 [PATCH v3 0/5] check: add option to rerun failed tests David Disseldorp
@ 2022-07-06 11:23 ` David Disseldorp
2022-07-06 11:23 ` [PATCH v3 2/5] report: pass through most details as function parameters David Disseldorp
` (3 subsequent siblings)
4 siblings, 0 replies; 11+ messages in thread
From: David Disseldorp @ 2022-07-06 11:23 UTC (permalink / raw)
To: fstests, tytso; +Cc: David Disseldorp, Darrick J . Wong
There's no need for multiple assignments.
Signed-off-by: David Disseldorp <ddiss@suse.de>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
---
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v3 2/5] report: pass through most details as function parameters
2022-07-06 11:23 [PATCH v3 0/5] check: add option to rerun failed tests David Disseldorp
2022-07-06 11:23 ` [PATCH v3 1/5] report: use array for REPORT_ENV_LIST David Disseldorp
@ 2022-07-06 11:23 ` David Disseldorp
2022-07-06 11:23 ` [PATCH v3 3/5] check: make a few variables local David Disseldorp
` (2 subsequent siblings)
4 siblings, 0 replies; 11+ messages in thread
From: David Disseldorp @ 2022-07-06 11:23 UTC (permalink / raw)
To: fstests, tytso; +Cc: David Disseldorp, Darrick J . Wong
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>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
---
check | 14 +++++++----
common/report | 64 +++++++++++++++++++++++++++++----------------------
2 files changed, 47 insertions(+), 31 deletions(-)
diff --git a/check b/check
index 4b0ebad6..dbad6dde 100755
--- a/check
+++ b/check
@@ -432,7 +432,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
@@ -493,7 +495,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
@@ -736,7 +740,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"
@@ -940,7 +945,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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v3 3/5] check: make a few variables local
2022-07-06 11:23 [PATCH v3 0/5] check: add option to rerun failed tests David Disseldorp
2022-07-06 11:23 ` [PATCH v3 1/5] report: use array for REPORT_ENV_LIST David Disseldorp
2022-07-06 11:23 ` [PATCH v3 2/5] report: pass through most details as function parameters David Disseldorp
@ 2022-07-06 11:23 ` David Disseldorp
2022-07-06 11:23 ` [PATCH v3 4/5] check: append bad / notrun arrays in helper function David Disseldorp
2022-07-06 11:23 ` [PATCH v3 5/5] check: add -L <n> parameter to rerun failed tests David Disseldorp
4 siblings, 0 replies; 11+ messages in thread
From: David Disseldorp @ 2022-07-06 11:23 UTC (permalink / raw)
To: fstests, tytso; +Cc: David Disseldorp, Darrick J . Wong
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>
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
---
check | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/check b/check
index dbad6dde..08857f7e 100755
--- a/check
+++ b/check
@@ -179,10 +179,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
@@ -207,7 +207,7 @@ _wallclock()
_timestamp()
{
- now=`date "+%T"`
+ local now=`date "+%T"`
echo -n " [$now]"
}
@@ -603,7 +603,7 @@ fi
function run_section()
{
- local section=$1
+ local section=$1 skip
OLD_FSTYP=$FSTYP
OLD_TEST_FS_MOUNT_OPTS=$TEST_FS_MOUNT_OPTS
@@ -820,7 +820,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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v3 4/5] check: append bad / notrun arrays in helper function
2022-07-06 11:23 [PATCH v3 0/5] check: add option to rerun failed tests David Disseldorp
` (2 preceding siblings ...)
2022-07-06 11:23 ` [PATCH v3 3/5] check: make a few variables local David Disseldorp
@ 2022-07-06 11:23 ` David Disseldorp
2022-07-06 18:33 ` Darrick J. Wong
2022-07-06 11:23 ` [PATCH v3 5/5] check: add -L <n> parameter to rerun failed tests David Disseldorp
4 siblings, 1 reply; 11+ messages in thread
From: David Disseldorp @ 2022-07-06 11:23 UTC (permalink / raw)
To: fstests, tytso; +Cc: 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 which is called at the end of each loop
iteration.
Signed-off-by: David Disseldorp <ddiss@suse.de>
---
check | 68 ++++++++++++++++++++++++++++++++++-------------------------
1 file changed, 39 insertions(+), 29 deletions(-)
diff --git a/check b/check
index 08857f7e..6dbdb2a8 100755
--- a/check
+++ b/check
@@ -553,6 +553,32 @@ _expunge_test()
return 0
}
+# Retain in @bad / @notrun the result of the just-run @test_seq. @try array
+# entries are added prior to execution.
+_stash_test_status() {
+ local test_seq="$1"
+ local test_status="$2"
+
+ if $do_report && [[ $test_status != "expunge" ]]; then
+ _make_testcase_report "$section" "$test_seq" \
+ "$test_status" "$((stop - start))"
+ fi
+
+ case "$test_status" in
+ fail)
+ bad+=("$test_seq")
+ ;;
+ list|notrun)
+ notrun+=("$test_seq")
+ ;;
+ pass|expunge)
+ ;;
+ *)
+ echo "Unexpected test $test_seq status: $test_status"
+ ;;
+ esac
+}
+
# Can we run systemd scopes?
HAVE_SYSTEMD_SCOPES=
systemctl reset-failed "fstests-check" &>/dev/null
@@ -732,19 +758,8 @@ function run_section()
seqres="$check"
_check_test_fs
- local tc_status="init"
- prev_seq=""
+ local tc_status
for seq in $list ; do
- # Run report for previous test!
- if [ "$tc_status" == "fail" ]; then
- bad+=("$seqnum")
- fi
- if $do_report && [[ ! $tc_status =~ ^(init|expunge)$ ]]; then
- _make_testcase_report "$section" "$seqnum" \
- "$tc_status" "$((stop - start))"
- fi
-
- prev_seq="$seq"
if [ ! -f $seq ]; then
# Try to get full name in case the user supplied only
# seq id and the test has a name. A bit of hassle to
@@ -784,20 +799,21 @@ function run_section()
if $showme; then
_expunge_test $seqnum
if [ $? -eq 1 ]; then
- tc_status="expunge"
- continue
+ tc_status="expunge"
+ else
+ echo
+ start=0
+ stop=0
+ tc_status="list"
fi
- echo
- start=0
- stop=0
- tc_status="list"
- notrun+=("$seqnum")
+ _stash_test_status "$seqnum" "$tc_status"
continue
fi
tc_status="pass"
if [ ! -f $seq ]; then
echo " - no such test?"
+ _stash_test_status "$seqnum" "$tc_status"
continue
fi
@@ -808,6 +824,7 @@ function run_section()
_expunge_test $seqnum
if [ $? -eq 1 ]; then
tc_status="expunge"
+ _stash_test_status "$seqnum" "$tc_status"
continue
fi
@@ -857,8 +874,8 @@ function run_section()
$timestamp && echo " [not run]" && \
echo -n " $seqnum -- "
cat $seqres.notrun
- notrun+=("$seqnum")
tc_status="notrun"
+ _stash_test_status "$seqnum" "$tc_status"
# Unmount the scratch fs so that we can wipe the scratch
# dev state prior to the next test run.
@@ -903,6 +920,7 @@ function run_section()
if [ ! -f $seq.out ]; then
_dump_err "no qualified output"
tc_status="fail"
+ _stash_test_status "$seqnum" "$tc_status"
continue;
fi
@@ -938,17 +956,9 @@ function run_section()
rm -f $seqres.hints
fi
fi
+ _stash_test_status "$seqnum" "$tc_status"
done
- # make sure we record the status of the last test we ran.
- if [ "$tc_status" == "fail" ]; then
- bad+=("$seqnum")
- fi
- if $do_report && [[ ! $tc_status =~ ^(init|expunge)$ ]]; then
- _make_testcase_report "$section" "$seqnum" "$tc_status" \
- "$((stop - start))"
- fi
-
sect_stop=`_wallclock`
interrupt=false
_wrapup
--
2.35.3
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v3 4/5] check: append bad / notrun arrays in helper function
2022-07-06 11:23 ` [PATCH v3 4/5] check: append bad / notrun arrays in helper function David Disseldorp
@ 2022-07-06 18:33 ` Darrick J. Wong
0 siblings, 0 replies; 11+ messages in thread
From: Darrick J. Wong @ 2022-07-06 18:33 UTC (permalink / raw)
To: David Disseldorp; +Cc: fstests, tytso
On Wed, Jul 06, 2022 at 01:23:11PM +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 which is called at the end of each loop
> iteration.
>
> Signed-off-by: David Disseldorp <ddiss@suse.de>
I like this better, since this reduces the distance between setting
tc_status and feeding it to _stash_test_status.
Another cleanup would be to eliminate $tc_status entirely, but that's
for another day. ;)
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
--D
> ---
> check | 68 ++++++++++++++++++++++++++++++++++-------------------------
> 1 file changed, 39 insertions(+), 29 deletions(-)
>
> diff --git a/check b/check
> index 08857f7e..6dbdb2a8 100755
> --- a/check
> +++ b/check
> @@ -553,6 +553,32 @@ _expunge_test()
> return 0
> }
>
> +# Retain in @bad / @notrun the result of the just-run @test_seq. @try array
> +# entries are added prior to execution.
> +_stash_test_status() {
> + local test_seq="$1"
> + local test_status="$2"
> +
> + if $do_report && [[ $test_status != "expunge" ]]; then
> + _make_testcase_report "$section" "$test_seq" \
> + "$test_status" "$((stop - start))"
> + fi
> +
> + case "$test_status" in
> + fail)
> + bad+=("$test_seq")
> + ;;
> + list|notrun)
> + notrun+=("$test_seq")
> + ;;
> + pass|expunge)
> + ;;
> + *)
> + echo "Unexpected test $test_seq status: $test_status"
> + ;;
> + esac
> +}
> +
> # Can we run systemd scopes?
> HAVE_SYSTEMD_SCOPES=
> systemctl reset-failed "fstests-check" &>/dev/null
> @@ -732,19 +758,8 @@ function run_section()
> seqres="$check"
> _check_test_fs
>
> - local tc_status="init"
> - prev_seq=""
> + local tc_status
> for seq in $list ; do
> - # Run report for previous test!
> - if [ "$tc_status" == "fail" ]; then
> - bad+=("$seqnum")
> - fi
> - if $do_report && [[ ! $tc_status =~ ^(init|expunge)$ ]]; then
> - _make_testcase_report "$section" "$seqnum" \
> - "$tc_status" "$((stop - start))"
> - fi
> -
> - prev_seq="$seq"
> if [ ! -f $seq ]; then
> # Try to get full name in case the user supplied only
> # seq id and the test has a name. A bit of hassle to
> @@ -784,20 +799,21 @@ function run_section()
> if $showme; then
> _expunge_test $seqnum
> if [ $? -eq 1 ]; then
> - tc_status="expunge"
> - continue
> + tc_status="expunge"
> + else
> + echo
> + start=0
> + stop=0
> + tc_status="list"
> fi
> - echo
> - start=0
> - stop=0
> - tc_status="list"
> - notrun+=("$seqnum")
> + _stash_test_status "$seqnum" "$tc_status"
> continue
> fi
>
> tc_status="pass"
> if [ ! -f $seq ]; then
> echo " - no such test?"
> + _stash_test_status "$seqnum" "$tc_status"
> continue
> fi
>
> @@ -808,6 +824,7 @@ function run_section()
> _expunge_test $seqnum
> if [ $? -eq 1 ]; then
> tc_status="expunge"
> + _stash_test_status "$seqnum" "$tc_status"
> continue
> fi
>
> @@ -857,8 +874,8 @@ function run_section()
> $timestamp && echo " [not run]" && \
> echo -n " $seqnum -- "
> cat $seqres.notrun
> - notrun+=("$seqnum")
> tc_status="notrun"
> + _stash_test_status "$seqnum" "$tc_status"
>
> # Unmount the scratch fs so that we can wipe the scratch
> # dev state prior to the next test run.
> @@ -903,6 +920,7 @@ function run_section()
> if [ ! -f $seq.out ]; then
> _dump_err "no qualified output"
> tc_status="fail"
> + _stash_test_status "$seqnum" "$tc_status"
> continue;
> fi
>
> @@ -938,17 +956,9 @@ function run_section()
> rm -f $seqres.hints
> fi
> fi
> + _stash_test_status "$seqnum" "$tc_status"
> done
>
> - # make sure we record the status of the last test we ran.
> - if [ "$tc_status" == "fail" ]; then
> - bad+=("$seqnum")
> - fi
> - if $do_report && [[ ! $tc_status =~ ^(init|expunge)$ ]]; then
> - _make_testcase_report "$section" "$seqnum" "$tc_status" \
> - "$((stop - start))"
> - fi
> -
> sect_stop=`_wallclock`
> interrupt=false
> _wrapup
> --
> 2.35.3
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3 5/5] check: add -L <n> parameter to rerun failed tests
2022-07-06 11:23 [PATCH v3 0/5] check: add option to rerun failed tests David Disseldorp
` (3 preceding siblings ...)
2022-07-06 11:23 ` [PATCH v3 4/5] check: append bad / notrun arrays in helper function David Disseldorp
@ 2022-07-06 11:23 ` David Disseldorp
2022-07-06 19:00 ` Darrick J. Wong
4 siblings, 1 reply; 11+ messages in thread
From: David Disseldorp @ 2022-07-06 11:23 UTC (permalink / raw)
To: fstests, tytso; +Cc: 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 | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 53 insertions(+), 3 deletions(-)
diff --git a/check b/check
index 6dbdb2a8..46fca6e6 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=""
@@ -78,6 +79,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
@@ -336,6 +338,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.
@@ -553,6 +558,18 @@ _expunge_test()
return 0
}
+# retain files which would be overwritten in subsequent reruns of the same test
+_stash_fail_loop_files() {
+ local test_seq="$1"
+ local suffix="$2"
+
+ for i in "${REPORT_DIR}/${test_seq}.full" \
+ "${REPORT_DIR}/${test_seq}.dmesg" \
+ "${REPORT_DIR}/${test_seq}.out.bad"; do
+ [ -f "$i" ] && cp "$i" "${i}${suffix}"
+ done
+}
+
# Retain in @bad / @notrun the result of the just-run @test_seq. @try array
# entries are added prior to execution.
_stash_test_status() {
@@ -564,8 +581,35 @@ _stash_test_status() {
"$test_status" "$((stop - start))"
fi
+ if ((${#loop_status[*]} > 0)); then
+ # continuing or completing rerun-on-failure loop
+ _stash_fail_loop_files "$test_seq" ".rerun${#loop_status[*]}"
+ loop_status+=("$test_status")
+ if ((${#loop_status[*]} > loop_on_fail)); 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
+ loop_status=()
+ fi
+ return # only stash @bad result for initial failure in loop
+ 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"
+ loop_status+=("$test_status")
+ fi
bad+=("$test_seq")
;;
list|notrun)
@@ -758,8 +802,12 @@ function run_section()
seqres="$check"
_check_test_fs
- local tc_status
- for seq in $list ; do
+ loop_status=() # track rerun-on-failure state
+ local tc_status ix
+ local -a _list=( $list )
+ for ((ix = 0; ix < ${#_list[*]}; !${#loop_status[*]} && ix++)); do
+ seq="${_list[$ix]}"
+
if [ ! -f $seq ]; then
# Try to get full name in case the user supplied only
# seq id and the test has a name. A bit of hassle to
@@ -829,7 +877,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} \
--
2.35.3
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v3 5/5] check: add -L <n> parameter to rerun failed tests
2022-07-06 11:23 ` [PATCH v3 5/5] check: add -L <n> parameter to rerun failed tests David Disseldorp
@ 2022-07-06 19:00 ` Darrick J. Wong
2022-07-06 21:54 ` David Disseldorp
0 siblings, 1 reply; 11+ messages in thread
From: Darrick J. Wong @ 2022-07-06 19:00 UTC (permalink / raw)
To: David Disseldorp; +Cc: fstests, tytso
On Wed, Jul 06, 2022 at 01:23:12PM +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 | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 53 insertions(+), 3 deletions(-)
>
> diff --git a/check b/check
> index 6dbdb2a8..46fca6e6 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=""
> @@ -78,6 +79,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
> @@ -336,6 +338,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.
> @@ -553,6 +558,18 @@ _expunge_test()
> return 0
> }
>
> +# retain files which would be overwritten in subsequent reruns of the same test
> +_stash_fail_loop_files() {
> + local test_seq="$1"
> + local suffix="$2"
> +
> + for i in "${REPORT_DIR}/${test_seq}.full" \
> + "${REPORT_DIR}/${test_seq}.dmesg" \
> + "${REPORT_DIR}/${test_seq}.out.bad"; do
> + [ -f "$i" ] && cp "$i" "${i}${suffix}"
I wonder, is there any particular reason to copy the output file and let
it get overwritten instead of simply mv'ing it?
> + done
> +}
> +
> # Retain in @bad / @notrun the result of the just-run @test_seq. @try array
> # entries are added prior to execution.
> _stash_test_status() {
> @@ -564,8 +581,35 @@ _stash_test_status() {
> "$test_status" "$((stop - start))"
> fi
>
> + if ((${#loop_status[*]} > 0)); then
> + # continuing or completing rerun-on-failure loop
> + _stash_fail_loop_files "$test_seq" ".rerun${#loop_status[*]}"
> + loop_status+=("$test_status")
> + if ((${#loop_status[*]} > loop_on_fail)); 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%%)",
Hmm, if I parse this correctly, do you end up with something like:
"xfs/555 aggregate results across 15 runs: pass=5 (33.3%) fail=10 (66.7%)" ?
> + (i-- > n ? x : ", " x),
> + stats[x], 100 * stats[x] / n);
> + }'
> + echo
> + loop_status=()
> + fi
> + return # only stash @bad result for initial failure in loop
> + 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"
> + loop_status+=("$test_status")
So if I'm reading this right, the length of the $loop_status array is
what gates us moving on or retrying, right? If the length is zero, then
we move on to the next test; otherwise, that loopy logic in
_stash_test_result above will keep the same test running until the
length exceeds loop_on_fail, at which point we print the aggregation
report, empty out $loop_status, and then ix increments and we move on to
the next test?
If the answers to the last two questions are both "yes", then I think
this looks ok.
--D
> + fi
> bad+=("$test_seq")
> ;;
> list|notrun)
> @@ -758,8 +802,12 @@ function run_section()
> seqres="$check"
> _check_test_fs
>
> - local tc_status
> - for seq in $list ; do
> + loop_status=() # track rerun-on-failure state
> + local tc_status ix
> + local -a _list=( $list )
> + for ((ix = 0; ix < ${#_list[*]}; !${#loop_status[*]} && ix++)); do
> + seq="${_list[$ix]}"
> +
> if [ ! -f $seq ]; then
> # Try to get full name in case the user supplied only
> # seq id and the test has a name. A bit of hassle to
> @@ -829,7 +877,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} \
> --
> 2.35.3
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 5/5] check: add -L <n> parameter to rerun failed tests
2022-07-06 19:00 ` Darrick J. Wong
@ 2022-07-06 21:54 ` David Disseldorp
2022-07-07 18:09 ` David Disseldorp
0 siblings, 1 reply; 11+ messages in thread
From: David Disseldorp @ 2022-07-06 21:54 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: fstests, tytso
Thanks for the follow-up feedback, Darrick...
On Wed, 6 Jul 2022 12:00:07 -0700, Darrick J. Wong wrote:
> On Wed, Jul 06, 2022 at 01:23:12PM +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 | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++++---
> > 1 file changed, 53 insertions(+), 3 deletions(-)
> >
> > diff --git a/check b/check
> > index 6dbdb2a8..46fca6e6 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=""
> > @@ -78,6 +79,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
> > @@ -336,6 +338,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.
> > @@ -553,6 +558,18 @@ _expunge_test()
> > return 0
> > }
> >
> > +# retain files which would be overwritten in subsequent reruns of the same test
> > +_stash_fail_loop_files() {
> > + local test_seq="$1"
> > + local suffix="$2"
> > +
> > + for i in "${REPORT_DIR}/${test_seq}.full" \
> > + "${REPORT_DIR}/${test_seq}.dmesg" \
> > + "${REPORT_DIR}/${test_seq}.out.bad"; do
> > + [ -f "$i" ] && cp "$i" "${i}${suffix}"
>
> I wonder, is there any particular reason to copy the output file and let
> it get overwritten instead of simply mv'ing it?
The copy is left over from an earlier version I had where xunit report
generation was done after the copy. Looking closer:
- .full is removed in _begin_fstest()
- _check_dmesg() overwrites .dmesg and retains on failure or KEEP_DMESG
- out.bad is removed in the main check loop prior to seq invocation
- .notrun, .core and .hints are also removed in the check loop at
various places before seq (.hints again in _begin_fstest())
One concern I have in changing this to a move is that external scripts
may check for presence / parse these files after check invocation. I'd
considered moving and then copying / symlinking back the .rerun0 files
on rerun-on-failure loop completion but that's also pretty ugly. IMO
leaving this as a copy, with the non-suffix file state left to reflect
the results of the last rerun-on-failure loop, would make the most
sense for now.
> > + done
> > +}
> > +
> > # Retain in @bad / @notrun the result of the just-run @test_seq. @try array
> > # entries are added prior to execution.
> > _stash_test_status() {
> > @@ -564,8 +581,35 @@ _stash_test_status() {
> > "$test_status" "$((stop - start))"
> > fi
> >
> > + if ((${#loop_status[*]} > 0)); then
> > + # continuing or completing rerun-on-failure loop
> > + _stash_fail_loop_files "$test_seq" ".rerun${#loop_status[*]}"
> > + loop_status+=("$test_status")
> > + if ((${#loop_status[*]} > loop_on_fail)); 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%%)",
>
> Hmm, if I parse this correctly, do you end up with something like:
>
> "xfs/555 aggregate results across 15 runs: pass=5 (33.3%) fail=10 (66.7%)" ?
Yes, with a comma in between "... (33.3%), fail=10 ...".
> > + (i-- > n ? x : ", " x),
> > + stats[x], 100 * stats[x] / n);
> > + }'
> > + echo
> > + loop_status=()
> > + fi
> > + return # only stash @bad result for initial failure in loop
> > + 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"
> > + loop_status+=("$test_status")
>
> So if I'm reading this right, the length of the $loop_status array is
> what gates us moving on or retrying, right? If the length is zero, then
> we move on to the next test; otherwise, that loopy logic in
> _stash_test_result above will keep the same test running until the
> length exceeds loop_on_fail, at which point we print the aggregation
> report, empty out $loop_status, and then ix increments and we move on to
> the next test?
Yes, exactly.
Cheers, David
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 5/5] check: add -L <n> parameter to rerun failed tests
2022-07-06 21:54 ` David Disseldorp
@ 2022-07-07 18:09 ` David Disseldorp
2022-07-08 0:34 ` Zorro Lang
0 siblings, 1 reply; 11+ messages in thread
From: David Disseldorp @ 2022-07-07 18:09 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: fstests, tytso
On Wed, 6 Jul 2022 23:54:52 +0200, David Disseldorp wrote:
> Thanks for the follow-up feedback, Darrick...
>
> On Wed, 6 Jul 2022 12:00:07 -0700, Darrick J. Wong wrote:
...
> > >
> > > +# retain files which would be overwritten in subsequent reruns of the same test
> > > +_stash_fail_loop_files() {
> > > + local test_seq="$1"
> > > + local suffix="$2"
> > > +
> > > + for i in "${REPORT_DIR}/${test_seq}.full" \
> > > + "${REPORT_DIR}/${test_seq}.dmesg" \
> > > + "${REPORT_DIR}/${test_seq}.out.bad"; do
> > > + [ -f "$i" ] && cp "$i" "${i}${suffix}"
> >
> > I wonder, is there any particular reason to copy the output file and let
> > it get overwritten instead of simply mv'ing it?
>
> The copy is left over from an earlier version I had where xunit report
> generation was done after the copy. Looking closer:
> - .full is removed in _begin_fstest()
> - _check_dmesg() overwrites .dmesg and retains on failure or KEEP_DMESG
> - out.bad is removed in the main check loop prior to seq invocation
> - .notrun, .core and .hints are also removed in the check loop at
> various places before seq (.hints again in _begin_fstest())
>
> One concern I have in changing this to a move is that external scripts
> may check for presence / parse these files after check invocation. I'd
> considered moving and then copying / symlinking back the .rerun0 files
> on rerun-on-failure loop completion but that's also pretty ugly. IMO
> leaving this as a copy, with the non-suffix file state left to reflect
> the results of the last rerun-on-failure loop, would make the most
> sense for now.
As a follow up here, I plan on squashing in the following change to
cover the extra notrun, core and hints files, and also avoid stale
.rerun# state:
--- a/check
+++ b/check
@@ -560,13 +560,14 @@ _expunge_test()
# retain files which would be overwritten in subsequent reruns of the same test
_stash_fail_loop_files() {
- local test_seq="$1"
- local suffix="$2"
+ local seq_prefix="${REPORT_DIR}/${1}"
+ local cp_suffix="$2"
- for i in "${REPORT_DIR}/${test_seq}.full" \
- "${REPORT_DIR}/${test_seq}.dmesg" \
- "${REPORT_DIR}/${test_seq}.out.bad"; do
- [ -f "$i" ] && cp "$i" "${i}${suffix}"
+ for i in ".full" ".dmesg" ".out.bad" ".notrun" ".core" ".hints"; do
+ rm -f "${seq_prefix}${i}${cp_suffix}"
+ if [ -f "${seq_prefix}${i}" ]; then
+ cp "${seq_prefix}${i}" "${seq_prefix}${i}${cp_suffix}"
+ fi
done
}
Cheers, David
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 5/5] check: add -L <n> parameter to rerun failed tests
2022-07-07 18:09 ` David Disseldorp
@ 2022-07-08 0:34 ` Zorro Lang
0 siblings, 0 replies; 11+ messages in thread
From: Zorro Lang @ 2022-07-08 0:34 UTC (permalink / raw)
To: David Disseldorp; +Cc: Darrick J. Wong, fstests, tytso
On Thu, Jul 07, 2022 at 08:09:36PM +0200, David Disseldorp wrote:
> On Wed, 6 Jul 2022 23:54:52 +0200, David Disseldorp wrote:
>
> > Thanks for the follow-up feedback, Darrick...
> >
> > On Wed, 6 Jul 2022 12:00:07 -0700, Darrick J. Wong wrote:
> ...
> > > >
> > > > +# retain files which would be overwritten in subsequent reruns of the same test
> > > > +_stash_fail_loop_files() {
> > > > + local test_seq="$1"
> > > > + local suffix="$2"
> > > > +
> > > > + for i in "${REPORT_DIR}/${test_seq}.full" \
> > > > + "${REPORT_DIR}/${test_seq}.dmesg" \
> > > > + "${REPORT_DIR}/${test_seq}.out.bad"; do
> > > > + [ -f "$i" ] && cp "$i" "${i}${suffix}"
> > >
> > > I wonder, is there any particular reason to copy the output file and let
> > > it get overwritten instead of simply mv'ing it?
> >
> > The copy is left over from an earlier version I had where xunit report
> > generation was done after the copy. Looking closer:
> > - .full is removed in _begin_fstest()
> > - _check_dmesg() overwrites .dmesg and retains on failure or KEEP_DMESG
> > - out.bad is removed in the main check loop prior to seq invocation
> > - .notrun, .core and .hints are also removed in the check loop at
> > various places before seq (.hints again in _begin_fstest())
> >
> > One concern I have in changing this to a move is that external scripts
> > may check for presence / parse these files after check invocation. I'd
> > considered moving and then copying / symlinking back the .rerun0 files
> > on rerun-on-failure loop completion but that's also pretty ugly. IMO
> > leaving this as a copy, with the non-suffix file state left to reflect
> > the results of the last rerun-on-failure loop, would make the most
> > sense for now.
>
> As a follow up here, I plan on squashing in the following change to
> cover the extra notrun, core and hints files, and also avoid stale
> .rerun# state:
OK, will wait your v4 of this patch, hope to catch the release of this week.
Thanks,
Zorro
>
> --- a/check
> +++ b/check
> @@ -560,13 +560,14 @@ _expunge_test()
>
> # retain files which would be overwritten in subsequent reruns of the same test
> _stash_fail_loop_files() {
> - local test_seq="$1"
> - local suffix="$2"
> + local seq_prefix="${REPORT_DIR}/${1}"
> + local cp_suffix="$2"
>
> - for i in "${REPORT_DIR}/${test_seq}.full" \
> - "${REPORT_DIR}/${test_seq}.dmesg" \
> - "${REPORT_DIR}/${test_seq}.out.bad"; do
> - [ -f "$i" ] && cp "$i" "${i}${suffix}"
> + for i in ".full" ".dmesg" ".out.bad" ".notrun" ".core" ".hints"; do
> + rm -f "${seq_prefix}${i}${cp_suffix}"
> + if [ -f "${seq_prefix}${i}" ]; then
> + cp "${seq_prefix}${i}" "${seq_prefix}${i}${cp_suffix}"
> + fi
> done
> }
>
> Cheers, David
>
^ permalink raw reply [flat|nested] 11+ messages in thread