[PATCH] check: fix excluded tests are only expunged in the first iteration

[PATCH] check: fix excluded tests are only expunged in the first iteration

[yuezhang.mo]

From: Yuezhang Mo <yuezhang.mo@foxmail.com>

If iterating more than once and excluding some tests, the
excluded tests are expunged in the first iteration, but run in
subsequent iterations. This is not expected.

The problem was caused by the temporary file saving the excluded
tests being deleted at the end of the first iteration.

This commit saves the excluded tests into a variable instead of a
temp file.

Signed-off-by: Yuezhang Mo <yuezhang.mo@foxmail.com>
---
 check | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/check b/check
index e36978c1..a8071121 100755
--- a/check
+++ b/check
@@ -27,6 +27,7 @@ DUMP_OUTPUT=false
 iterations=1
 istop=false
 loop_on_fail=0
+exclude_tests=()
 
 # This is a global variable used to pass test failure text to reporting gunk
 _err_msg=""
@@ -46,7 +47,7 @@ export DIFF_LENGTH=${DIFF_LENGTH:=10}
 # by default don't output timestamps
 timestamp=${TIMESTAMP:=false}
 
-rm -f $tmp.list $tmp.tmp $tmp.grep $here/$iam.out $tmp.xlist $tmp.report.* $tmp.arglist
+rm -f $tmp.list $tmp.tmp $tmp.grep $here/$iam.out $tmp.report.* $tmp.arglist
 
 SRC_GROUPS="generic shared"
 export SRC_DIR="tests"
@@ -302,13 +303,13 @@ while [ $# -gt 0 ]; do
 		;;
 	-e)
 		xfile=$2; shift ;
-		echo "$xfile" | tr ', ' '\n\n' >> $tmp.xlist
+		exclude_tests+=($(echo "$xfile" | tr ', ' '\n\n'))
 		;;
 
 	-E)	xfile=$2; shift ;
 		if [ -f $xfile ]; then
-			sed "s/#.*$//" "$xfile" >> $tmp.xlist
-	        fi
+			exclude_tests+=($(sed "s/#.*$//" "$xfile"))
+		fi
 		;;
 	-s)	RUN_SECTION="$RUN_SECTION $2"; shift ;;
 	-S)	EXCLUDE_SECTION="$EXCLUDE_SECTION $2"; shift ;;
@@ -383,7 +384,7 @@ if [ -n "$subdir_xfile" ]; then
 	for d in $SRC_GROUPS $FSTYP; do
 		[ -f $SRC_DIR/$d/$subdir_xfile ] || continue
 		for f in `sed "s/#.*$//" $SRC_DIR/$d/$subdir_xfile`; do
-			echo $d/$f >> $tmp.xlist
+			exclude_tests+=($d/$f)
 		done
 	done
 fi
@@ -570,11 +571,10 @@ _check_filesystems()
 _expunge_test()
 {
 	local TEST_ID="$1"
-	if [ -s $tmp.xlist ]; then
-		if grep -q $TEST_ID $tmp.xlist; then
-			echo "       [expunged]"
-			return 1
-		fi
+
+	if echo ${exclude_tests[*]} | grep -q $TEST_ID; then
+		echo "       [expunged]"
+		return 1
 	fi
 	return 0
 }
-- 
2.25.1

Re: [PATCH] check: fix excluded tests are only expunged in the first iteration

[Bill O'Donnell]

On Fri, Jun 30, 2023 at 08:28:29PM +0800, yuezhang.mo@foxmail.com wrote:
> From: Yuezhang Mo <yuezhang.mo@foxmail.com>
> 
> If iterating more than once and excluding some tests, the
> excluded tests are expunged in the first iteration, but run in
> subsequent iterations. This is not expected.
> 
> The problem was caused by the temporary file saving the excluded
> tests being deleted at the end of the first iteration.
> 
> This commit saves the excluded tests into a variable instead of a
> temp file.
> 
> Signed-off-by: Yuezhang Mo <yuezhang.mo@foxmail.com>

Makes sense.
Reviewed-by: Bill O'Donnell <bodonnel@redhat.com>

> ---
>  check | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/check b/check
> index e36978c1..a8071121 100755
> --- a/check
> +++ b/check
> @@ -27,6 +27,7 @@ DUMP_OUTPUT=false
>  iterations=1
>  istop=false
>  loop_on_fail=0
> +exclude_tests=()
>  
>  # This is a global variable used to pass test failure text to reporting gunk
>  _err_msg=""
> @@ -46,7 +47,7 @@ export DIFF_LENGTH=${DIFF_LENGTH:=10}
>  # by default don't output timestamps
>  timestamp=${TIMESTAMP:=false}
>  
> -rm -f $tmp.list $tmp.tmp $tmp.grep $here/$iam.out $tmp.xlist $tmp.report.* $tmp.arglist
> +rm -f $tmp.list $tmp.tmp $tmp.grep $here/$iam.out $tmp.report.* $tmp.arglist
>  
>  SRC_GROUPS="generic shared"
>  export SRC_DIR="tests"
> @@ -302,13 +303,13 @@ while [ $# -gt 0 ]; do
>  		;;
>  	-e)
>  		xfile=$2; shift ;
> -		echo "$xfile" | tr ', ' '\n\n' >> $tmp.xlist
> +		exclude_tests+=($(echo "$xfile" | tr ', ' '\n\n'))
>  		;;
>  
>  	-E)	xfile=$2; shift ;
>  		if [ -f $xfile ]; then
> -			sed "s/#.*$//" "$xfile" >> $tmp.xlist
> -	        fi
> +			exclude_tests+=($(sed "s/#.*$//" "$xfile"))
> +		fi
>  		;;
>  	-s)	RUN_SECTION="$RUN_SECTION $2"; shift ;;
>  	-S)	EXCLUDE_SECTION="$EXCLUDE_SECTION $2"; shift ;;
> @@ -383,7 +384,7 @@ if [ -n "$subdir_xfile" ]; then
>  	for d in $SRC_GROUPS $FSTYP; do
>  		[ -f $SRC_DIR/$d/$subdir_xfile ] || continue
>  		for f in `sed "s/#.*$//" $SRC_DIR/$d/$subdir_xfile`; do
> -			echo $d/$f >> $tmp.xlist
> +			exclude_tests+=($d/$f)
>  		done
>  	done
>  fi
> @@ -570,11 +571,10 @@ _check_filesystems()
>  _expunge_test()
>  {
>  	local TEST_ID="$1"
> -	if [ -s $tmp.xlist ]; then
> -		if grep -q $TEST_ID $tmp.xlist; then
> -			echo "       [expunged]"
> -			return 1
> -		fi
> +
> +	if echo ${exclude_tests[*]} | grep -q $TEST_ID; then
> +		echo "       [expunged]"
> +		return 1
>  	fi
>  	return 0
>  }
> -- 
> 2.25.1
> 

Re: [PATCH] check: fix excluded tests are only expunged in the first iteration

[Darrick J. Wong]

On Fri, Jun 30, 2023 at 08:28:29PM +0800, yuezhang.mo@foxmail.com wrote:
> From: Yuezhang Mo <yuezhang.mo@foxmail.com>
> 
> If iterating more than once and excluding some tests, the
> excluded tests are expunged in the first iteration, but run in
> subsequent iterations. This is not expected.

I don't get it.  ./check only deletes $tmp.xlist at startup time, not
between iterations.  Is there somewhere that ./check exec's itself when
you run ./check -i 5 ?

> The problem was caused by the temporary file saving the excluded
> tests being deleted at the end of the first iteration.
> 
> This commit saves the excluded tests into a variable instead of a
> temp file.
> 
> Signed-off-by: Yuezhang Mo <yuezhang.mo@foxmail.com>
> ---
>  check | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/check b/check
> index e36978c1..a8071121 100755
> --- a/check
> +++ b/check
> @@ -27,6 +27,7 @@ DUMP_OUTPUT=false
>  iterations=1
>  istop=false
>  loop_on_fail=0
> +exclude_tests=()
>  
>  # This is a global variable used to pass test failure text to reporting gunk
>  _err_msg=""
> @@ -46,7 +47,7 @@ export DIFF_LENGTH=${DIFF_LENGTH:=10}
>  # by default don't output timestamps
>  timestamp=${TIMESTAMP:=false}
>  
> -rm -f $tmp.list $tmp.tmp $tmp.grep $here/$iam.out $tmp.xlist $tmp.report.* $tmp.arglist
> +rm -f $tmp.list $tmp.tmp $tmp.grep $here/$iam.out $tmp.report.* $tmp.arglist
>  
>  SRC_GROUPS="generic shared"
>  export SRC_DIR="tests"
> @@ -302,13 +303,13 @@ while [ $# -gt 0 ]; do
>  		;;
>  	-e)
>  		xfile=$2; shift ;
> -		echo "$xfile" | tr ', ' '\n\n' >> $tmp.xlist
> +		exclude_tests+=($(echo "$xfile" | tr ', ' '\n\n'))

Please use readarray/mapfile for storing to a bash array, please.

--D

>  		;;
>  
>  	-E)	xfile=$2; shift ;
>  		if [ -f $xfile ]; then
> -			sed "s/#.*$//" "$xfile" >> $tmp.xlist
> -	        fi
> +			exclude_tests+=($(sed "s/#.*$//" "$xfile"))
> +		fi
>  		;;
>  	-s)	RUN_SECTION="$RUN_SECTION $2"; shift ;;
>  	-S)	EXCLUDE_SECTION="$EXCLUDE_SECTION $2"; shift ;;
> @@ -383,7 +384,7 @@ if [ -n "$subdir_xfile" ]; then
>  	for d in $SRC_GROUPS $FSTYP; do
>  		[ -f $SRC_DIR/$d/$subdir_xfile ] || continue
>  		for f in `sed "s/#.*$//" $SRC_DIR/$d/$subdir_xfile`; do
> -			echo $d/$f >> $tmp.xlist
> +			exclude_tests+=($d/$f)
>  		done
>  	done
>  fi
> @@ -570,11 +571,10 @@ _check_filesystems()
>  _expunge_test()
>  {
>  	local TEST_ID="$1"
> -	if [ -s $tmp.xlist ]; then
> -		if grep -q $TEST_ID $tmp.xlist; then
> -			echo "       [expunged]"
> -			return 1
> -		fi
> +
> +	if echo ${exclude_tests[*]} | grep -q $TEST_ID; then
> +		echo "       [expunged]"
> +		return 1
>  	fi
>  	return 0
>  }
> -- 
> 2.25.1
> 

Re: [PATCH] check: fix excluded tests are only expunged in the first iteration

[Zorro Lang]

On Fri, Jun 30, 2023 at 08:28:29PM +0800, yuezhang.mo@foxmail.com wrote:
> From: Yuezhang Mo <yuezhang.mo@foxmail.com>
> 
> If iterating more than once and excluding some tests, the
> excluded tests are expunged in the first iteration, but run in
> subsequent iterations. This is not expected.

Can you provide an example about how to reproduce the issue you metioned above.

> 
> The problem was caused by the temporary file saving the excluded
> tests being deleted at the end of the first iteration.
> 
> This commit saves the excluded tests into a variable instead of a
> temp file.
> 
> Signed-off-by: Yuezhang Mo <yuezhang.mo@foxmail.com>
> ---
>  check | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/check b/check
> index e36978c1..a8071121 100755
> --- a/check
> +++ b/check
> @@ -27,6 +27,7 @@ DUMP_OUTPUT=false
>  iterations=1
>  istop=false
>  loop_on_fail=0
> +exclude_tests=()
>  
>  # This is a global variable used to pass test failure text to reporting gunk
>  _err_msg=""
> @@ -46,7 +47,7 @@ export DIFF_LENGTH=${DIFF_LENGTH:=10}
>  # by default don't output timestamps
>  timestamp=${TIMESTAMP:=false}
>  
> -rm -f $tmp.list $tmp.tmp $tmp.grep $here/$iam.out $tmp.xlist $tmp.report.* $tmp.arglist
> +rm -f $tmp.list $tmp.tmp $tmp.grep $here/$iam.out $tmp.report.* $tmp.arglist
>  
>  SRC_GROUPS="generic shared"
>  export SRC_DIR="tests"
> @@ -302,13 +303,13 @@ while [ $# -gt 0 ]; do
>  		;;
>  	-e)
>  		xfile=$2; shift ;
> -		echo "$xfile" | tr ', ' '\n\n' >> $tmp.xlist
> +		exclude_tests+=($(echo "$xfile" | tr ', ' '\n\n'))

If print to a file, I don't mind above way. But if to an array, how about:

  read -a tmpxlist < <(echo "$xfile" | tr ',' ' ')
  exclude_tests+=(${tmpxlist[@]})

>  		;;
>  
>  	-E)	xfile=$2; shift ;
>  		if [ -f $xfile ]; then
> -			sed "s/#.*$//" "$xfile" >> $tmp.xlist
> -	        fi
> +			exclude_tests+=($(sed "s/#.*$//" "$xfile"))
> +		fi

Similar as above, how about:

  readarray tmpxlist < <(sed "s/#.*$//" $xfile)
  exclude_tests+=(${tmpxlist[@]})

Feel free to provide better methods if anyone has.

>  		;;
>  	-s)	RUN_SECTION="$RUN_SECTION $2"; shift ;;
>  	-S)	EXCLUDE_SECTION="$EXCLUDE_SECTION $2"; shift ;;
> @@ -383,7 +384,7 @@ if [ -n "$subdir_xfile" ]; then
>  	for d in $SRC_GROUPS $FSTYP; do
>  		[ -f $SRC_DIR/$d/$subdir_xfile ] || continue
>  		for f in `sed "s/#.*$//" $SRC_DIR/$d/$subdir_xfile`; do
> -			echo $d/$f >> $tmp.xlist
> +			exclude_tests+=($d/$f)
>  		done
>  	done
>  fi
> @@ -570,11 +571,10 @@ _check_filesystems()
>  _expunge_test()
>  {
>  	local TEST_ID="$1"
> -	if [ -s $tmp.xlist ]; then
> -		if grep -q $TEST_ID $tmp.xlist; then
> -			echo "       [expunged]"
> -			return 1
> -		fi
> +
> +	if echo ${exclude_tests[*]} | grep -q $TEST_ID; then

How about using the "-w" option to grep. Due to the number of fstests' cases
will out of 999 (I'm thinking about how to deal with it), we might have
generic/1000 in one day. If so "grep generic/100" will match generic/1000 and
generic/100 both.

Thanks,
Zorro

> +		echo "       [expunged]"
> +		return 1
>  	fi
>  	return 0
>  }
> -- 
> 2.25.1
> 

Re: [PATCH] check: fix excluded tests are only expunged in the first iteration

[Darrick J. Wong]

On Mon, Jul 03, 2023 at 10:51:20PM +0800, Zorro Lang wrote:
> On Fri, Jun 30, 2023 at 08:28:29PM +0800, yuezhang.mo@foxmail.com wrote:
> > From: Yuezhang Mo <yuezhang.mo@foxmail.com>
> > 
> > If iterating more than once and excluding some tests, the
> > excluded tests are expunged in the first iteration, but run in
> > subsequent iterations. This is not expected.
> 
> Can you provide an example about how to reproduce the issue you metioned above.
> 
> > 
> > The problem was caused by the temporary file saving the excluded
> > tests being deleted at the end of the first iteration.
> > 
> > This commit saves the excluded tests into a variable instead of a
> > temp file.
> > 
> > Signed-off-by: Yuezhang Mo <yuezhang.mo@foxmail.com>
> > ---
> >  check | 20 ++++++++++----------
> >  1 file changed, 10 insertions(+), 10 deletions(-)
> > 
> > diff --git a/check b/check
> > index e36978c1..a8071121 100755
> > --- a/check
> > +++ b/check
> > @@ -27,6 +27,7 @@ DUMP_OUTPUT=false
> >  iterations=1
> >  istop=false
> >  loop_on_fail=0
> > +exclude_tests=()
> >  
> >  # This is a global variable used to pass test failure text to reporting gunk
> >  _err_msg=""
> > @@ -46,7 +47,7 @@ export DIFF_LENGTH=${DIFF_LENGTH:=10}
> >  # by default don't output timestamps
> >  timestamp=${TIMESTAMP:=false}
> >  
> > -rm -f $tmp.list $tmp.tmp $tmp.grep $here/$iam.out $tmp.xlist $tmp.report.* $tmp.arglist
> > +rm -f $tmp.list $tmp.tmp $tmp.grep $here/$iam.out $tmp.report.* $tmp.arglist
> >  
> >  SRC_GROUPS="generic shared"
> >  export SRC_DIR="tests"
> > @@ -302,13 +303,13 @@ while [ $# -gt 0 ]; do
> >  		;;
> >  	-e)
> >  		xfile=$2; shift ;
> > -		echo "$xfile" | tr ', ' '\n\n' >> $tmp.xlist
> > +		exclude_tests+=($(echo "$xfile" | tr ', ' '\n\n'))
> 
> If print to a file, I don't mind above way. But if to an array, how about:
> 
>   read -a tmpxlist < <(echo "$xfile" | tr ',' ' ')
>   exclude_tests+=(${tmpxlist[@]})

readarray -t -O "${#exclude_tests[@]}" exclude_tests < <(echo "$xfile" | tr ',' ' '))

> >  		;;
> >  
> >  	-E)	xfile=$2; shift ;
> >  		if [ -f $xfile ]; then
> > -			sed "s/#.*$//" "$xfile" >> $tmp.xlist
> > -	        fi
> > +			exclude_tests+=($(sed "s/#.*$//" "$xfile"))
> > +		fi
> 
> Similar as above, how about:
> 
>   readarray tmpxlist < <(sed "s/#.*$//" $xfile)
>   exclude_tests+=(${tmpxlist[@]})
> 
> Feel free to provide better methods if anyone has.
> 
> >  		;;
> >  	-s)	RUN_SECTION="$RUN_SECTION $2"; shift ;;
> >  	-S)	EXCLUDE_SECTION="$EXCLUDE_SECTION $2"; shift ;;
> > @@ -383,7 +384,7 @@ if [ -n "$subdir_xfile" ]; then
> >  	for d in $SRC_GROUPS $FSTYP; do
> >  		[ -f $SRC_DIR/$d/$subdir_xfile ] || continue
> >  		for f in `sed "s/#.*$//" $SRC_DIR/$d/$subdir_xfile`; do
> > -			echo $d/$f >> $tmp.xlist
> > +			exclude_tests+=($d/$f)
> >  		done
> >  	done
> >  fi
> > @@ -570,11 +571,10 @@ _check_filesystems()
> >  _expunge_test()
> >  {
> >  	local TEST_ID="$1"
> > -	if [ -s $tmp.xlist ]; then
> > -		if grep -q $TEST_ID $tmp.xlist; then
> > -			echo "       [expunged]"
> > -			return 1
> > -		fi
> > +
> > +	if echo ${exclude_tests[*]} | grep -q $TEST_ID; then
> 
> How about using the "-w" option to grep. Due to the number of fstests' cases
> will out of 999 (I'm thinking about how to deal with it), we might have
> generic/1000 in one day.

Four digit ids work fine; I've had xfs/12xx and xfs/15xx in djwong-dev
for years now with no problems.

> If so "grep generic/100" will match generic/1000 and
> generic/100 both.

Yeah, that grep string ought to be grep -q "^$TEST_ID$" or something.

Or

is_excluded() {
	for f in "${exclude_tests[@]}"; do
		[[ "$f" == "$1" ]] && return 0
	done
	return 1
}

(Though saving the overhead of one fork/exec is probably minimal.)

	if is_excluded "$TEST_ID"; then ...

--D

> Thanks,
> Zorro
> 
> > +		echo "       [expunged]"
> > +		return 1
> >  	fi
> >  	return 0
> >  }
> > -- 
> > 2.25.1
> > 
> 

Re: [PATCH] check: fix excluded tests are only expunged in the first iteration

[Yuezhang Mo]

On Fri, Jun 30, 2023 at 08:28:29PM +0800, yuezhang.mo@foxmail.com wrote:



>> From: Yuezhang Mo <yuezhang.mo@foxmail.com>
>>
>> If iterating more than once and excluding some tests, the
>> excluded tests are expunged in the first iteration, but run in
>> subsequent iterations. This is not expected.

>
> Can you provide an example about how to reproduce the issue you metioned above.



If run `./check -i 5 -e generic/013`.

- generic/013 is expunged in the iteration 1.
   generic/013       [expunged]

- But generic/013 is not expunged in the iteration 2/3/4/5.
  generic/013 76s ... 78s


>>  -e)
>>  xfile=$2; shift ;
>> - echo "$xfile" | tr ', ' '\n\n' >> $tmp.xlist
>> + exclude_tests+=($(echo "$xfile" | tr ', ' '\n\n'))
>
> If print to a file, I don't mind above way. But if to an array, how about:

>
> read -a tmpxlist < <(echo "$xfile" | tr ',' ' ')
> exclude_tests+=(${tmpxlist[@]})
>> + exclude_tests+=($(sed "s/#.*$//" "$xfile"))

>> + fi

>
> Similar as above, how about:

>
> readarray tmpxlist < <(sed "s/#.*$//" $xfile)
> exclude_tests+=(${tmpxlist[@]})

>
> Feel free to provide better methods if anyone has. 
>> + if echo ${exclude_tests[*]} | grep -q $TEST_ID; then

>
> How about using the "-w" option to grep. Due to the number of fstests' cases
> will out of 999 (I'm thinking about how to deal with it), we might have
> generic/1000 in one day. If so "grep generic/100" will match generic/1000 and

> generic/100 both.



OK, I will update the patch.