[LTP] [PATCH] lsmod01.sh: retry test couple times to lower false positives

[LTP] [PATCH] lsmod01.sh: retry test couple times to lower false positives

[Jan Stancek]

Test sporadically fails with:
  lsmod01 1 TFAIL: lsmod output different from /proc/modules.
  36c36
  < loop 42057 2
  ---
  > loop 42057 1

commands runtest file runs mkswap01 before this test. That test is
using loop device, and udev is presumably still holding a reference
by the time lsmod01 test starts.

Repeat the test couple times to avoid racing with rest of the system.

Signed-off-by: Jan Stancek <jstancek@redhat.com>
---
 testcases/commands/lsmod/lsmod01.sh | 28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/testcases/commands/lsmod/lsmod01.sh b/testcases/commands/lsmod/lsmod01.sh
index ad170dcd41b8..38ba8e0c94ad 100755
--- a/testcases/commands/lsmod/lsmod01.sh
+++ b/testcases/commands/lsmod/lsmod01.sh
@@ -10,31 +10,41 @@ TST_NEEDS_TMPDIR=1
 TST_NEEDS_CMDS="lsmod"
 . tst_test.sh
 
-lsmod_test()
+lsmod_matches_proc_modules()
 {
 	lsmod_output=$(lsmod | awk '!/Module/{print $1, $2, $3}' | sort)
 	if [ -z "$lsmod_output" ]; then
-		tst_res TFAIL "Failed to parse the output from lsmod"
-		return
+		tst_brk TBROK "Failed to parse the output from lsmod"
 	fi
 
-	modules_output=$(awk '{print $1, $2, $3}' /proc/modules | sort)
+	modules_output=$(awk '{print $1, $2, $3} 1' /proc/modules | sort)
 	if [ -z "$modules_output" ]; then
-		tst_res TFAIL "Failed to parse /proc/modules"
-		return
+		tst_brk TBROK "Failed to parse /proc/modules"
 	fi
 
 	if [ "$lsmod_output" != "$modules_output" ]; then
-		tst_res TFAIL "lsmod output different from /proc/modules."
+		tst_res TINFO "lsmod output different from /proc/modules."
 
 		echo "$lsmod_output" > temp1
 		echo "$modules_output" > temp2
 		diff temp1 temp2
 
-		return
+		return 1
 	fi
+	return 0
+}
 
-	tst_res TPASS "'lsmod' passed."
+lsmod_test()
+{
+	for i in $(seq 1 5); do
+		if lsmod_matches_proc_modules; then
+			tst_res TPASS "'lsmod' passed."
+			return
+		fi
+		tst_res TINFO "Trying again"
+		sleep 1
+	done
+	tst_res TFAIL "'lsmod' doesn't match /proc/modules output"
 }
 
 tst_run
-- 
1.8.3.1

[LTP] [PATCH] lsmod01.sh: retry test couple times to lower false positives

[Petr Vorel]

Hi Jan,

> Test sporadically fails with:
>   lsmod01 1 TFAIL: lsmod output different from /proc/modules.
>   36c36
>   < loop 42057 2
>   ---
>   > loop 42057 1

> commands runtest file runs mkswap01 before this test. That test is
> using loop device, and udev is presumably still holding a reference
> by the time lsmod01 test starts.

> Repeat the test couple times to avoid racing with rest of the system.

> Signed-off-by: Jan Stancek <jstancek@redhat.com>
Reviewed-by: Petr Vorel <pvorel@suse.cz>

...
> +++ b/testcases/commands/lsmod/lsmod01.sh
> @@ -10,31 +10,41 @@ TST_NEEDS_TMPDIR=1
>  TST_NEEDS_CMDS="lsmod"
>  . tst_test.sh

> -lsmod_test()
> +lsmod_matches_proc_modules()
>  {
>  	lsmod_output=$(lsmod | awk '!/Module/{print $1, $2, $3}' | sort)
>  	if [ -z "$lsmod_output" ]; then
> -		tst_res TFAIL "Failed to parse the output from lsmod"
> -		return
> +		tst_brk TBROK "Failed to parse the output from lsmod"
>  	fi

> -	modules_output=$(awk '{print $1, $2, $3}' /proc/modules | sort)
> +	modules_output=$(awk '{print $1, $2, $3} 1' /proc/modules | sort)
This is a regression. Please keep the old version (without 1).


-	modules_output=$(awk '{print $1, $2, $3} 1' /proc/modules | sort)
+	modules_output=$(awk '{print $1, $2, $3}' /proc/modules | sort)

>  	if [ -z "$modules_output" ]; then
> -		tst_res TFAIL "Failed to parse /proc/modules"
> -		return
> +		tst_brk TBROK "Failed to parse /proc/modules"
>  	fi

>  	if [ "$lsmod_output" != "$modules_output" ]; then
> -		tst_res TFAIL "lsmod output different from /proc/modules."
> +		tst_res TINFO "lsmod output different from /proc/modules."
nit: please remove dots at the end of messages, when you're touching that file.

The rest looks good to me.

Kind regards,
Petr

[LTP] [PATCH] lsmod01.sh: retry test couple times to lower false positives

[Jan Stancek]

----- Original Message -----
> Hi Jan,
> 
> > Test sporadically fails with:
> >   lsmod01 1 TFAIL: lsmod output different from /proc/modules.
> >   36c36
> >   < loop 42057 2
> >   ---
> >   > loop 42057 1
> 
> > commands runtest file runs mkswap01 before this test. That test is
> > using loop device, and udev is presumably still holding a reference
> > by the time lsmod01 test starts.
> 
> > Repeat the test couple times to avoid racing with rest of the system.
> 
> > Signed-off-by: Jan Stancek <jstancek@redhat.com>
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
> 
> ...
> > +++ b/testcases/commands/lsmod/lsmod01.sh
> > @@ -10,31 +10,41 @@ TST_NEEDS_TMPDIR=1
> >  TST_NEEDS_CMDS="lsmod"
> >  . tst_test.sh
> 
> > -lsmod_test()
> > +lsmod_matches_proc_modules()
> >  {
> >  	lsmod_output=$(lsmod | awk '!/Module/{print $1, $2, $3}' | sort)
> >  	if [ -z "$lsmod_output" ]; then
> > -		tst_res TFAIL "Failed to parse the output from lsmod"
> > -		return
> > +		tst_brk TBROK "Failed to parse the output from lsmod"
> >  	fi
> 
> > -	modules_output=$(awk '{print $1, $2, $3}' /proc/modules | sort)
> > +	modules_output=$(awk '{print $1, $2, $3} 1' /proc/modules | sort)
> This is a regression. Please keep the old version (without 1).

My bad, I left it in by accident after testing.

> 
> 
> -	modules_output=$(awk '{print $1, $2, $3} 1' /proc/modules | sort)
> +	modules_output=$(awk '{print $1, $2, $3}' /proc/modules | sort)
> 
> >  	if [ -z "$modules_output" ]; then
> > -		tst_res TFAIL "Failed to parse /proc/modules"
> > -		return
> > +		tst_brk TBROK "Failed to parse /proc/modules"
> >  	fi
> 
> >  	if [ "$lsmod_output" != "$modules_output" ]; then
> > -		tst_res TFAIL "lsmod output different from /proc/modules."
> > +		tst_res TINFO "lsmod output different from /proc/modules."
> nit: please remove dots at the end of messages, when you're touching that
> file.

will do

> 
> The rest looks good to me.
> 
> Kind regards,
> Petr
> 

[LTP] [PATCH] lsmod01.sh: retry test couple times to lower false positives

[Petr Vorel]

Hi Jan,

...
>  	if [ "$lsmod_output" != "$modules_output" ]; then
> -		tst_res TFAIL "lsmod output different from /proc/modules."
> +		tst_res TINFO "lsmod output different from /proc/modules."

>  		echo "$lsmod_output" > temp1
>  		echo "$modules_output" > temp2
>  		diff temp1 temp2
Also this code could be wrapped with
if tst_cmd_available diff; then

(I wouldn't add it to TST_NEEDS_CMDS)

Kind regards,
Petr

[LTP] [PATCH] lsmod01.sh: retry test couple times to lower false positives

[Jan Stancek]

----- Original Message -----
> Hi Jan,
> 
> ...
> >  	if [ "$lsmod_output" != "$modules_output" ]; then
> > -		tst_res TFAIL "lsmod output different from /proc/modules."
> > +		tst_res TINFO "lsmod output different from /proc/modules."
> 
> >  		echo "$lsmod_output" > temp1
> >  		echo "$modules_output" > temp2
> >  		diff temp1 temp2
> Also this code could be wrapped with
> if tst_cmd_available diff; then

Added this as well and pushed.

[LTP] [PATCH] lsmod01.sh: retry test couple times to lower false positives

[Petr Vorel]

Hi Jan,

...
> -lsmod_test()
> +lsmod_matches_proc_modules()
>  {
>  	lsmod_output=$(lsmod | awk '!/Module/{print $1, $2, $3}' | sort)
>  	if [ -z "$lsmod_output" ]; then
> -		tst_res TFAIL "Failed to parse the output from lsmod"
> -		return
> +		tst_brk TBROK "Failed to parse the output from lsmod"
>  	fi

> -	modules_output=$(awk '{print $1, $2, $3}' /proc/modules | sort)
> +	modules_output=$(awk '{print $1, $2, $3} 1' /proc/modules | sort)
>  	if [ -z "$modules_output" ]; then
> -		tst_res TFAIL "Failed to parse /proc/modules"
> -		return
> +		tst_brk TBROK "Failed to parse /proc/modules"
>  	fi

>  	if [ "$lsmod_output" != "$modules_output" ]; then
> -		tst_res TFAIL "lsmod output different from /proc/modules."
> +		tst_res TINFO "lsmod output different from /proc/modules."

>  		echo "$lsmod_output" > temp1
>  		echo "$modules_output" > temp2
>  		diff temp1 temp2

> -		return
> +		return 1
>  	fi
> +	return 0
> +}

> -	tst_res TPASS "'lsmod' passed."
> +lsmod_test()
> +{
> +	for i in $(seq 1 5); do
> +		if lsmod_matches_proc_modules; then
> +			tst_res TPASS "'lsmod' passed."
> +			return
> +		fi
> +		tst_res TINFO "Trying again"
> +		sleep 1
> +	done
This is similar pattern to TST_RETRY_FUNC()/TST_RETRY_FN_EXP_BACKOFF()
(for both shell and C). I wonder if we also have use for TPASS/TFAIL
instead of just TBROK and specifying number of tries instead of time to be
setup.
C and shell usage is a bit different, so maybe
TST_RETRY_FUNC()/TST_RETRY_FN_EXP_BACKOFF() doesn't make much sense for shell
(actually nothing uses them in shell) and I don't see much usage for my proposal
in C.

> +	tst_res TFAIL "'lsmod' doesn't match /proc/modules output"

+ we forget to use local (for lsmod_outputa and i), but that's not that
important.

Kind regards,
Petr

[LTP] [PATCH] lsmod01.sh: retry test couple times to lower false positives

[Jan Stancek]

----- Original Message -----
> 
> > -	tst_res TPASS "'lsmod' passed."
> > +lsmod_test()
> > +{
> > +	for i in $(seq 1 5); do
> > +		if lsmod_matches_proc_modules; then
> > +			tst_res TPASS "'lsmod' passed."
> > +			return
> > +		fi
> > +		tst_res TINFO "Trying again"
> > +		sleep 1
> > +	done
> This is similar pattern to TST_RETRY_FUNC()/TST_RETRY_FN_EXP_BACKOFF()
> (for both shell and C). I wonder if we also have use for TPASS/TFAIL
> instead of just TBROK and specifying number of tries instead of time to be
> setup.

I think TFAIL fits more here, it's outcome of what we are testing.
TBROK in my mind is failure unrelated to subject of test.

But functionally TST_RETRY_FUNC should work too. 

> C and shell usage is a bit different, so maybe
> TST_RETRY_FUNC()/TST_RETRY_FN_EXP_BACKOFF() doesn't make much sense for shell

I see it used in mkswap01.sh and numa01.sh.

I wonder if we need to TBROK in TST_RETRY_FUNC(). We could just return
what the FUNC returns and let the test decide.
TST_RETRY_FUNC_BRK() could be a wrapper that TBROKs on timeout.

[LTP] [PATCH] lsmod01.sh: retry test couple times to lower false positives

[Petr Vorel]

Hi Jan,

> > > -	tst_res TPASS "'lsmod' passed."
> > > +lsmod_test()
> > > +{
> > > +	for i in $(seq 1 5); do
> > > +		if lsmod_matches_proc_modules; then
> > > +			tst_res TPASS "'lsmod' passed."
> > > +			return
> > > +		fi
> > > +		tst_res TINFO "Trying again"
> > > +		sleep 1
> > > +	done
> > This is similar pattern to TST_RETRY_FUNC()/TST_RETRY_FN_EXP_BACKOFF()
> > (for both shell and C). I wonder if we also have use for TPASS/TFAIL
> > instead of just TBROK and specifying number of tries instead of time to be
> > setup.

> I think TFAIL fits more here, it's outcome of what we are testing.
> TBROK in my mind is failure unrelated to subject of test.
I express myself wrong. Sure, I meant to have TPASS/TFAIL,
just to use some helper function instead of writing the wrapper in the test.

> But functionally TST_RETRY_FUNC should work too. 

> > C and shell usage is a bit different, so maybe
> > TST_RETRY_FUNC()/TST_RETRY_FN_EXP_BACKOFF() doesn't make much sense for shell

> I see it used in mkswap01.sh and numa01.sh.
Sorry, I searched just TST_RETRY_FN_EXP_BACKOFF.
Correct, TST_RETRY_FUNC is used there.

> I wonder if we need to TBROK in TST_RETRY_FUNC(). We could just return
> what the FUNC returns and let the test decide.
> TST_RETRY_FUNC_BRK() could be a wrapper that TBROKs on timeout.
That could work (apart from the fact it diverges the functionality from C).
+ there could be the third one, which TPASS/TFAIL (instead of nothing/TBROK).

But this should be based on TST_RETRY_FN_EXP_BACKOFF (TST_RETRY_FUNC is reusing
TST_RETRY_FN_EXP_BACKOFF) + add also TST_RETRY_FUNC wrappers.

Do we need similar functionality in C?

Kind regards,
Petr

[LTP] [PATCH] lsmod01.sh: retry test couple times to lower false positives

[Li Wang]

On Thu, Oct 24, 2019 at 2:28 AM Petr Vorel <pvorel@suse.cz> wrote:

> Hi Jan,
>
> > > > - tst_res TPASS "'lsmod' passed."
> > > > +lsmod_test()
> > > > +{
> > > > + for i in $(seq 1 5); do
> > > > +         if lsmod_matches_proc_modules; then
> > > > +                 tst_res TPASS "'lsmod' passed."
> > > > +                 return
> > > > +         fi
> > > > +         tst_res TINFO "Trying again"
> > > > +         sleep 1
> > > > + done
> > > This is similar pattern to TST_RETRY_FUNC()/TST_RETRY_FN_EXP_BACKOFF()
> > > (for both shell and C). I wonder if we also have use for TPASS/TFAIL
> > > instead of just TBROK and specifying number of tries instead of time
> to be
> > > setup.
>
> > I think TFAIL fits more here, it's outcome of what we are testing.
> > TBROK in my mind is failure unrelated to subject of test.
> I express myself wrong. Sure, I meant to have TPASS/TFAIL,
> just to use some helper function instead of writing the wrapper in the
> test.
>
> > But functionally TST_RETRY_FUNC should work too.
>
> > > C and shell usage is a bit different, so maybe
> > > TST_RETRY_FUNC()/TST_RETRY_FN_EXP_BACKOFF() doesn't make much sense
> for shell
>
> > I see it used in mkswap01.sh and numa01.sh.
> Sorry, I searched just TST_RETRY_FN_EXP_BACKOFF.
> Correct, TST_RETRY_FUNC is used there.
>
> > I wonder if we need to TBROK in TST_RETRY_FUNC(). We could just return
> > what the FUNC returns and let the test decide.
> > TST_RETRY_FUNC_BRK() could be a wrapper that TBROKs on timeout.
> That could work (apart from the fact it diverges the functionality from C).
> + there could be the third one, which TPASS/TFAIL (instead of
> nothing/TBROK).
>
> But this should be based on TST_RETRY_FN_EXP_BACKOFF (TST_RETRY_FUNC is
> reusing
> TST_RETRY_FN_EXP_BACKOFF) + add also TST_RETRY_FUNC wrappers.
>
> Do we need similar functionality in C?
>

Not sure, but we could collect the requirement for the EXP_BACKOFF series
macro. I'm also thinking about to extend the functionality for more
situations.

e.g http://lists.linux.it/pipermail/ltp/2019-October/013896.html

-- 
Regards,
Li Wang
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.linux.it/pipermail/ltp/attachments/20191024/e640bf64/attachment.htm>

[LTP] [PATCH] lsmod01.sh: retry test couple times to lower false positives

[Petr Vorel]

Hi Li, Jan,

...
> > > I wonder if we need to TBROK in TST_RETRY_FUNC(). We could just return
> > > what the FUNC returns and let the test decide.
> > > TST_RETRY_FUNC_BRK() could be a wrapper that TBROKs on timeout.
> > That could work (apart from the fact it diverges the functionality from C).
> > + there could be the third one, which TPASS/TFAIL (instead of
> > nothing/TBROK).

> > But this should be based on TST_RETRY_FN_EXP_BACKOFF (TST_RETRY_FUNC is
> > reusing
> > TST_RETRY_FN_EXP_BACKOFF) + add also TST_RETRY_FUNC wrappers.

> > Do we need similar functionality in C?


> Not sure, but we could collect the requirement for the EXP_BACKOFF series
> macro. I'm also thinking about to extend the functionality for more
> situations.

> e.g http://lists.linux.it/pipermail/ltp/2019-October/013896.html
+1 for @INFI - 1: retry infinitely, 0: retry in limit times

Combining with TPASS/TFAIL vs. nothing/TBROK it might make sense to either use
enum flags for C implementation, which would save one parameter and allow
further extension. Shell could have getopts instead of more parameters.
And it's a question whether cover all variants with wrappers like
TST_RETRY_FUNC* or not.

Kind regards,
Petr

[LTP] [PATCH] lsmod01.sh: retry test couple times to lower false positives

[Jan Stancek]

----- Original Message -----
> Hi Jan,
> 
> > > > -	tst_res TPASS "'lsmod' passed."
> > > > +lsmod_test()
> > > > +{
> > > > +	for i in $(seq 1 5); do
> > > > +		if lsmod_matches_proc_modules; then
> > > > +			tst_res TPASS "'lsmod' passed."
> > > > +			return
> > > > +		fi
> > > > +		tst_res TINFO "Trying again"
> > > > +		sleep 1
> > > > +	done
> > > This is similar pattern to TST_RETRY_FUNC()/TST_RETRY_FN_EXP_BACKOFF()
> > > (for both shell and C). I wonder if we also have use for TPASS/TFAIL
> > > instead of just TBROK and specifying number of tries instead of time to
> > > be
> > > setup.
> 
> > I think TFAIL fits more here, it's outcome of what we are testing.
> > TBROK in my mind is failure unrelated to subject of test.
> I express myself wrong. Sure, I meant to have TPASS/TFAIL,
> just to use some helper function instead of writing the wrapper in the test.
> 
> > But functionally TST_RETRY_FUNC should work too.
> 
> > > C and shell usage is a bit different, so maybe
> > > TST_RETRY_FUNC()/TST_RETRY_FN_EXP_BACKOFF() doesn't make much sense for
> > > shell
> 
> > I see it used in mkswap01.sh and numa01.sh.
> Sorry, I searched just TST_RETRY_FN_EXP_BACKOFF.
> Correct, TST_RETRY_FUNC is used there.
> 
> > I wonder if we need to TBROK in TST_RETRY_FUNC(). We could just return
> > what the FUNC returns and let the test decide.
> > TST_RETRY_FUNC_BRK() could be a wrapper that TBROKs on timeout.
> That could work (apart from the fact it diverges the functionality from C).
> + there could be the third one, which TPASS/TFAIL (instead of nothing/TBROK).
> 
> But this should be based on TST_RETRY_FN_EXP_BACKOFF (TST_RETRY_FUNC is
> reusing
> TST_RETRY_FN_EXP_BACKOFF) + add also TST_RETRY_FUNC wrappers.
> 
> Do we need similar functionality in C?

If we make modifications we should keep it consistent with C.