Message ID | c8843f2f4a325e820d030d9c7c36d7624f1baa82.1571393044.git.jstancek@redhat.com |
---|---|
State | Accepted, archived |
Headers | show |
Series | lsmod01.sh: retry test couple times to lower false positives | expand |
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
----- 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 >
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
----- 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.
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
----- 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.
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
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
----- 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.
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
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