[v2,1/1] tst_test.sh: Add test cmd helper tst_test_cmds()

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()
Related show

Commit Message

Petr Vorel April 23, 2018, 9:17 a.m.
+ 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(-)

Comments

Petr Vorel June 1, 2018, 9:42 a.m. | #1
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
>  }
Cyril Hrubis July 24, 2018, 9:35 a.m. | #2
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
>
Petr Vorel July 24, 2018, 12:57 p.m. | #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
Cyril Hrubis July 24, 2018, 1:15 p.m. | #4
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.
Petr Vorel July 24, 2018, 3:19 p.m. | #5
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

Patch

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
 }