[LTP] [PATCH 1/2] tst_test.sh: Fix calling not yet loaded cleanup function

[LTP] [PATCH 1/2] tst_test.sh: Fix calling not yet loaded cleanup function

[Petr Vorel]

e7dc14caa introduced a regression for new API network tests (these using
tst_net.sh), which use network namespaces and have cleanup function:

$ PATH="/opt/ltp/testcases/bin:$PATH" tcp_ipsec.sh -6 -A rfc4543_256 \
  -p esp_aead -m tunnel -s 100:1000:65535:R65535
tcp_ipsec 1 TCONF: Must be super/root for this test!
/opt/ltp/testcases/bin/tst_test.sh: line 32: tst_ipsec_cleanup: command not found

This is caused by tst_brk called in tst_net.sh test preparation (happen
just after loading tst_net.sh, that's why cleanup function haven't been
defined yet. This would require to load tst_net.sh just before tst_run.

But because tst_net.sh doesn't have it's own cleanup function
(tst_cleanup_rhost is always called in _tst_do_exit in tst_test.sh
regardless of setup/cleanup functions), we can assume that only after
starting the actual test code (i.e. running either setup or test
function) it's meaningful to run cleanup function.

This is effectively a revert of e7dc14caa + adding $TST_SETUP_STARTED.

Fixes: e7dc14caa ("tst_test.sh: Run cleanup even setup is not defined")
Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 testcases/lib/tst_test.sh | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh
index 8d24b4abf..aa74ad761 100644
--- a/testcases/lib/tst_test.sh
+++ b/testcases/lib/tst_test.sh
@@ -28,7 +28,8 @@ _tst_do_exit()
 	local ret=0
 	TST_DO_EXIT=1
 
-	if [ -n "$TST_CLEANUP" -a -z "$TST_NO_CLEANUP" ]; then
+	if [ -n "$TST_SETUP_STARTED" -a -n "$TST_RUN_STARTED" -a \
+		-n "$TST_CLEANUP" -a -z "$TST_NO_CLEANUP" ]; then
 		$TST_CLEANUP
 	fi
 
@@ -582,6 +583,7 @@ tst_run()
 	[ -n "$TST_NEEDS_MODULE" ] && tst_require_module "$TST_NEEDS_MODULE"
 
 	if [ -n "$TST_SETUP" ]; then
+		TST_SETUP_STARTED=1
 		$TST_SETUP
 	fi
 
@@ -592,9 +594,11 @@ tst_run()
 			_tst_max=$(( $(echo $TST_TEST_DATA | tr -cd "$TST_TEST_DATA_IFS" | wc -c) +1))
 			for _tst_i in $(seq $_tst_max); do
 				_tst_data="$(echo "$TST_TEST_DATA" | cut -d"$TST_TEST_DATA_IFS" -f$_tst_i)"
+				TST_RUN_STARTED=1
 				_tst_run_tests "$_tst_data"
 			done
 		else
+			TST_RUN_STARTED=1
 			_tst_run_tests
 		fi
 		TST_ITERATIONS=$((TST_ITERATIONS-1))
-- 
2.26.2

[LTP] [PATCH 2/2] tst_test.sh: Warn about setup/cleanup function not loaded

[Petr Vorel]

Inspired by regression fixed by previous commit.

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
 testcases/lib/tst_test.sh | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh
index aa74ad761..cd5bdeae7 100644
--- a/testcases/lib/tst_test.sh
+++ b/testcases/lib/tst_test.sh
@@ -30,7 +30,11 @@ _tst_do_exit()
 
 	if [ -n "$TST_SETUP_STARTED" -a -n "$TST_RUN_STARTED" -a \
 		-n "$TST_CLEANUP" -a -z "$TST_NO_CLEANUP" ]; then
-		$TST_CLEANUP
+		if type "$TST_CLEANUP" >/dev/null 2>&1; then
+			$TST_CLEANUP
+		else
+			tst_res TWARN "cleanup function set (TST_SETUP='$TST_CLEANUP'), but not found (test bug)"
+		fi
 	fi
 
 	if [ "$TST_NEEDS_DEVICE" = 1 -a "$TST_DEVICE_FLAG" = 1 ]; then
@@ -585,6 +589,11 @@ tst_run()
 	if [ -n "$TST_SETUP" ]; then
 		TST_SETUP_STARTED=1
 		$TST_SETUP
+		if type "$TST_SETUP" >/dev/null 2>&1; then
+			$TST_SETUP
+		else
+			tst_res TWARN "setup function set (TST_SETUP='$TST_SETUP'), but not found (test bug)"
+		fi
 	fi
 
 	#TODO check that test reports some results for each test function call
-- 
2.26.2

[LTP] [PATCH 1/2] tst_test.sh: Fix calling not yet loaded cleanup function

[Joerg Vehlow]

Hi Petr,

>   	TST_DO_EXIT=1
>   
> -	if [ -n "$TST_CLEANUP" -a -z "$TST_NO_CLEANUP" ]; then
> +	if [ -n "$TST_SETUP_STARTED" -a -n "$TST_RUN_STARTED" -a \
> +		-n "$TST_CLEANUP" -a -z "$TST_NO_CLEANUP" ]; then
In the description you write "[after] running either setup or test 
function". But this implementation
is "after running setup and test function". Cleanup should also be 
executed, if only setup was run right?
>   
> @@ -592,9 +594,11 @@ tst_run()
>   			_tst_max=$(( $(echo $TST_TEST_DATA | tr -cd "$TST_TEST_DATA_IFS" | wc -c) +1))
>   			for _tst_i in $(seq $_tst_max); do
>   				_tst_data="$(echo "$TST_TEST_DATA" | cut -d"$TST_TEST_DATA_IFS" -f$_tst_i)"
> +				TST_RUN_STARTED=1
>   				_tst_run_tests "$_tst_data"
>   			done
>   		else
> +			TST_RUN_STARTED=1
>   			_tst_run_tests
>   		fi
Is it really important, that test is started? Shouldn't it be enough if 
we got to the point, where the test
could be started. Moving TST_RUN_STARTED out of the condition would 
reduce repetition.

J?rg

[LTP] [PATCH 2/2] tst_test.sh: Warn about setup/cleanup function not loaded

[Joerg Vehlow]

Hi Petr,

this looks good, just one small typo. And I guess I would raise this to 
TBROK, but that
is just my personal preference :)

Am 18.05.2020 um 15:01 schrieb Petr Vorel:
> +		if type "$TST_CLEANUP" >/dev/null 2>&1; then
> +			$TST_CLEANUP
> +		else
> +			tst_res TWARN "cleanup function set (TST_SETUP='$TST_CLEANUP'), but not found (test bug)"
This should probably be TST_CLEANUP=....


J?rg

[LTP] [PATCH 1/2] tst_test.sh: Fix calling not yet loaded cleanup function

[Petr Vorel]

Hi J?rg,

> >   	TST_DO_EXIT=1
> > -	if [ -n "$TST_CLEANUP" -a -z "$TST_NO_CLEANUP" ]; then
> > +	if [ -n "$TST_SETUP_STARTED" -a -n "$TST_RUN_STARTED" -a \
> > +		-n "$TST_CLEANUP" -a -z "$TST_NO_CLEANUP" ]; then
> In the description you write "[after] running either setup or test
> function". But this implementation
> is "after running setup and test function". Cleanup should also be executed,
> if only setup was run right?
Thanks! Yes, I meant -o (or), but typed -a (and).

> > @@ -592,9 +594,11 @@ tst_run()
> >   			_tst_max=$(( $(echo $TST_TEST_DATA | tr -cd "$TST_TEST_DATA_IFS" | wc -c) +1))
> >   			for _tst_i in $(seq $_tst_max); do
> >   				_tst_data="$(echo "$TST_TEST_DATA" | cut -d"$TST_TEST_DATA_IFS" -f$_tst_i)"
> > +				TST_RUN_STARTED=1
> >   				_tst_run_tests "$_tst_data"
> >   			done
> >   		else
> > +			TST_RUN_STARTED=1
> >   			_tst_run_tests
> >   		fi
> Is it really important, that test is started? Shouldn't it be enough if we
> got to the point, where the test
> could be started. Moving TST_RUN_STARTED out of the condition would reduce
> repetition.

Well, if you look into the code, there is tst_require_cmds call in if clause,
which should pass:

	#TODO check that test reports some results for each test function call
	while [ $TST_ITERATIONS -gt 0 ]; do
		if [ -n "$TST_TEST_DATA" ]; then
			tst_require_cmds cut tr wc
			_tst_max=$(( $(echo $TST_TEST_DATA | tr -cd "$TST_TEST_DATA_IFS" | wc -c) +1))
			for _tst_i in $(seq $_tst_max); do
				_tst_data="$(echo "$TST_TEST_DATA" | cut -d"$TST_TEST_DATA_IFS" -f$_tst_i)"
				TST_RUN_STARTED=1
				_tst_run_tests "$_tst_data"
			done
		else
			TST_RUN_STARTED=1
			_tst_run_tests
		fi
		TST_ITERATIONS=$((TST_ITERATIONS-1))
	done

Kind regards,
Petr

[LTP] [PATCH 2/2] tst_test.sh: Warn about setup/cleanup function not loaded

[Petr Vorel]

Hi J?rg,

> this looks good, just one small typo. And I guess I would raise this to
> TBROK, but that
> is just my personal preference :)
Yes, it could be. I thought TBROK would be for setup (tests results are probably
invalid without setup) and TWARN for cleanup (well, cleanup is important, but
results are valid, see other TWARN in tst_test.sh). But I want to be consistent
=> use TBROK.

> Am 18.05.2020 um 15:01 schrieb Petr Vorel:
> > +		if type "$TST_CLEANUP" >/dev/null 2>&1; then
> > +			$TST_CLEANUP
> > +		else
> > +			tst_res TWARN "cleanup function set (TST_SETUP='$TST_CLEANUP'), but not found (test bug)"
> This should probably be TST_CLEANUP=....
Thanks!

Kind regards,
Petr

[LTP] [PATCH 1/2] tst_test.sh: Fix calling not yet loaded cleanup function

[Joerg Vehlow]

Hi,

>> Is it really important, that test is started? Shouldn't it be enough if we
>> got to the point, where the test
>> could be started. Moving TST_RUN_STARTED out of the condition would reduce
>> repetition.
> Well, if you look into the code, there is tst_require_cmds call in if clause,
> which should pass:
>
> 	#TODO check that test reports some results for each test function call
> 	while [ $TST_ITERATIONS -gt 0 ]; do
> 		if [ -n "$TST_TEST_DATA" ]; then
> 			tst_require_cmds cut tr wc
> 			_tst_max=$(( $(echo $TST_TEST_DATA | tr -cd "$TST_TEST_DATA_IFS" | wc -c) +1))
> 			for _tst_i in $(seq $_tst_max); do
> 				_tst_data="$(echo "$TST_TEST_DATA" | cut -d"$TST_TEST_DATA_IFS" -f$_tst_i)"
> 				TST_RUN_STARTED=1
> 				_tst_run_tests "$_tst_data"
> 			done
> 		else
> 			TST_RUN_STARTED=1
> 			_tst_run_tests
> 		fi
> 		TST_ITERATIONS=$((TST_ITERATIONS-1))
> 	done

At this point setup was executed right? So if tst_require_cmds fails, it 
should execute cleanup, to
revert whatever setup did, no?
Oh I guess changing the condition to "setup run _or_ test run" will make 
this work.
But is there really a reason for having both variables? I guess the 
reason they exist at all is to ensure,
that the cleanup function is already defined. But that should be true, 
as soon as tst_run is executed.
So maybe one single variable that is set, when TST_RUN is executed is 
sufficient.
In my opinion the decision to execute of cleanup shouldn't be too 
complicated. The cleanup function
should be able to handle an abort anytime during test execution anyway. 
So it should be callable, as soon
as it is defined. At least when tst_run is executed, it should be ok to 
call cleanup.

J?rg