| Message ID | 20180423091706.24154-1-pvorel@suse.cz |
|---|---|
| State | Changes Requested |
| Delegated to: | Cyril Hrubis |
| Headers | show |
| Series | [v2,1/1] tst_test.sh: Add test cmd helper tst_test_cmds() | expand |
Hi, ping, please. Kind regards, Petr > + tst_cmd_available() > tst_test_cmds() is meant to be a check just for a particular test. > Works like tst_check_cmds(), but instead of tst_brk() calls tst_res(). > tst_cmd_available() helper can handle cases when command shell builtin > is not available (e.g. Busybox). > Signed-off-by: Petr Vorel <pvorel@suse.cz> > --- > Hi, > if you don't like using which or testing with 127 exit code in > tst_cmd_available() (or if you don't like tst_cmd_available()), I > can remove it. > Kind regards, > Petr > --- > doc/test-writing-guidelines.txt | 17 +++++++++++++++++ > testcases/lib/tst_test.sh | 29 +++++++++++++++++++++++++++-- > 2 files changed, 44 insertions(+), 2 deletions(-) > diff --git a/doc/test-writing-guidelines.txt b/doc/test-writing-guidelines.txt > index cbbfe6c0f..320b42bf8 100644 > --- a/doc/test-writing-guidelines.txt > +++ b/doc/test-writing-guidelines.txt > @@ -1519,6 +1519,23 @@ existence each of them and exits the test with 'TCONF' on first misssing. > Alternatively the 'tst_check_cmds()' function can be used to do the same on > runtime, since sometimes we need to the check at runtime too. > +'tst_test_cmds()' can be used for requirements just for a particular test > +as it doesn't exit (it issues 'tst_res TCONF'). Expected usage is: > +... > + > +TST_TESTFUNC=do_test > +. tst_test.sh > + > +do_test() > +{ > + tst_test_cmds cmd || return > + cmd --foo > + ... > +} > + > +tst_run > +... > + > Locating kernel modules > +++++++++++++++++++++++ > diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh > index 8d49d34b6..b3e803e05 100644 > --- a/testcases/lib/tst_test.sh > +++ b/testcases/lib/tst_test.sh > @@ -201,12 +201,37 @@ tst_mkfs() > ROD_SILENT mkfs.$fs_type $fs_opts $device > } > +tst_cmd_available() > +{ > + if type command > /dev/null 2>&1; then > + command -v $1 > /dev/null 2>&1 || return 1 > + else > + which $1 > /dev/null 2>&1 > + if [ $? -eq 0 ]; then > + return 0 > + elif [ $? -eq 127 ]; then > + tst_brk TCONF "missing which command" > + else > + return 1 > + fi > + fi > +} > + > tst_check_cmds() > { > local cmd > for cmd in $*; do > - if ! command -v $cmd > /dev/null 2>&1; then > - tst_brk TCONF "'$cmd' not found" > + tst_cmd_available $cmd || tst_brk TCONF "'$cmd' not found" > + done > +} > + > +tst_test_cmds() > +{ > + local cmd > + for cmd in $*; do > + if ! tst_cmd_available $cmd; then > + tst_res TCONF "'$cmd' not found" > + return 1 > fi > done > }
Hi! > + tst_cmd_available() > > tst_test_cmds() is meant to be a check just for a particular test. > Works like tst_check_cmds(), but instead of tst_brk() calls tst_res(). Hmm looking at this after a while I would expect the tst_test_cmds() to exit the test while tst_check_cmds() to return a value, the question is if this is worth of the work of renaming the current uses... > tst_cmd_available() helper can handle cases when command shell builtin > is not available (e.g. Busybox). > > Signed-off-by: Petr Vorel <pvorel@suse.cz> > --- > Hi, > > if you don't like using which or testing with 127 exit code in > tst_cmd_available() (or if you don't like tst_cmd_available()), I > can remove it. This looks fine to me. > Locating kernel modules > +++++++++++++++++++++++ > > diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh > index 8d49d34b6..b3e803e05 100644 > --- a/testcases/lib/tst_test.sh > +++ b/testcases/lib/tst_test.sh > @@ -201,12 +201,37 @@ tst_mkfs() > ROD_SILENT mkfs.$fs_type $fs_opts $device > } > > +tst_cmd_available() > +{ > + if type command > /dev/null 2>&1; then > + command -v $1 > /dev/null 2>&1 || return 1 > + else > + which $1 > /dev/null 2>&1 > + if [ $? -eq 0 ]; then > + return 0 > + elif [ $? -eq 127 ]; then > + tst_brk TCONF "missing which command" > + else > + return 1 > + fi > + fi > +} > + > tst_check_cmds() > { > local cmd > for cmd in $*; do BTW you can just write 'for cmd; do' here since the default array to loop over are the parameters passed to a function. > - if ! command -v $cmd > /dev/null 2>&1; then > - tst_brk TCONF "'$cmd' not found" > + tst_cmd_available $cmd || tst_brk TCONF "'$cmd' not found" > + done > +} > + > +tst_test_cmds() > +{ > + local cmd > + for cmd in $*; do > + if ! tst_cmd_available $cmd; then > + tst_res TCONF "'$cmd' not found" > + return 1 > fi > done Can we add explicit return 0 here? Other than that it's fine. > } > -- > 2.16.3 >
Hi Cyril, > Hi! > > + tst_cmd_available() > > tst_test_cmds() is meant to be a check just for a particular test. > > Works like tst_check_cmds(), but instead of tst_brk() calls tst_res(). > Hmm looking at this after a while I would expect the tst_test_cmds() to > exit the test while tst_check_cmds() to return a value, the question is > if this is worth of the work of renaming the current uses... Indeed, names would be better to be reversed. It's up to you. If you want, I'll first rename tst_check_cmds() to tst_test_cmds() and then add this new one as tst_check_cmds(). > > tst_cmd_available() helper can handle cases when command shell builtin > > is not available (e.g. Busybox). > > Signed-off-by: Petr Vorel <pvorel@suse.cz> > > --- > > Hi, > > if you don't like using which or testing with 127 exit code in > > tst_cmd_available() (or if you don't like tst_cmd_available()), I > > can remove it. > This looks fine to me. Thx! > > Locating kernel modules > > +++++++++++++++++++++++ > > diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh ... > > tst_check_cmds() > > { > > local cmd > > for cmd in $*; do > BTW you can just write 'for cmd; do' here since the default > array to loop over are the parameters passed to a function. Thanks, I'll simplify it. This POSIX shell feature was hidden to me. ... > > +tst_test_cmds() > > +{ > > + local cmd > > + for cmd in $*; do > > + if ! tst_cmd_available $cmd; then > > + tst_res TCONF "'$cmd' not found" > > + return 1 > > fi > > done > Can we add explicit return 0 here? Sure. > Other than that it's fine. Kind regards, Petr
Hi! > > Hmm looking at this after a while I would expect the tst_test_cmds() to > > exit the test while tst_check_cmds() to return a value, the question is > > if this is worth of the work of renaming the current uses... > Indeed, names would be better to be reversed. It's up to you. > If you want, I'll first rename tst_check_cmds() to tst_test_cmds() > and then add this new one as tst_check_cmds(). If it's easy enough, let's do that, otherwise I would keep it as it is.
Hi Cyril, > Hi! > > > Hmm looking at this after a while I would expect the tst_test_cmds() to > > > exit the test while tst_check_cmds() to return a value, the question is > > > if this is worth of the work of renaming the current uses... > > Indeed, names would be better to be reversed. It's up to you. > > If you want, I'll first rename tst_check_cmds() to tst_test_cmds() > > and then add this new one as tst_check_cmds(). > If it's easy enough, let's do that, otherwise I would keep it as it is. merged rename, updated version of now called tst_check_cmds() and it's initial usage in IMA. + Updated wiki (+ updating changes to wiki from commit e11b7c036 tst_timer: Add tst_timer_expired_ms() Thanks for your time to review! Kind regards, Petr
diff --git a/doc/test-writing-guidelines.txt b/doc/test-writing-guidelines.txt index cbbfe6c0f..320b42bf8 100644 --- a/doc/test-writing-guidelines.txt +++ b/doc/test-writing-guidelines.txt @@ -1519,6 +1519,23 @@ existence each of them and exits the test with 'TCONF' on first misssing. Alternatively the 'tst_check_cmds()' function can be used to do the same on runtime, since sometimes we need to the check at runtime too. +'tst_test_cmds()' can be used for requirements just for a particular test +as it doesn't exit (it issues 'tst_res TCONF'). Expected usage is: +... + +TST_TESTFUNC=do_test +. tst_test.sh + +do_test() +{ + tst_test_cmds cmd || return + cmd --foo + ... +} + +tst_run +... + Locating kernel modules +++++++++++++++++++++++ diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh index 8d49d34b6..b3e803e05 100644 --- a/testcases/lib/tst_test.sh +++ b/testcases/lib/tst_test.sh @@ -201,12 +201,37 @@ tst_mkfs() ROD_SILENT mkfs.$fs_type $fs_opts $device } +tst_cmd_available() +{ + if type command > /dev/null 2>&1; then + command -v $1 > /dev/null 2>&1 || return 1 + else + which $1 > /dev/null 2>&1 + if [ $? -eq 0 ]; then + return 0 + elif [ $? -eq 127 ]; then + tst_brk TCONF "missing which command" + else + return 1 + fi + fi +} + tst_check_cmds() { local cmd for cmd in $*; do - if ! command -v $cmd > /dev/null 2>&1; then - tst_brk TCONF "'$cmd' not found" + tst_cmd_available $cmd || tst_brk TCONF "'$cmd' not found" + done +} + +tst_test_cmds() +{ + local cmd + for cmd in $*; do + if ! tst_cmd_available $cmd; then + tst_res TCONF "'$cmd' not found" + return 1 fi done }
+ tst_cmd_available() tst_test_cmds() is meant to be a check just for a particular test. Works like tst_check_cmds(), but instead of tst_brk() calls tst_res(). tst_cmd_available() helper can handle cases when command shell builtin is not available (e.g. Busybox). Signed-off-by: Petr Vorel <pvorel@suse.cz> --- Hi, if you don't like using which or testing with 127 exit code in tst_cmd_available() (or if you don't like tst_cmd_available()), I can remove it. Kind regards, Petr --- doc/test-writing-guidelines.txt | 17 +++++++++++++++++ testcases/lib/tst_test.sh | 29 +++++++++++++++++++++++++++-- 2 files changed, 44 insertions(+), 2 deletions(-)