* [LTP] [PATCH v4 0/3] shell API setup/cleanup checks
@ 2020-12-17 14:44 Petr Vorel
2020-12-17 14:44 ` [LTP] [PATCH v4 1/3] tst_test.sh: Call cleanup function after test start Petr Vorel
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Petr Vorel @ 2020-12-17 14:44 UTC (permalink / raw)
To: ltp
Hi,
previous version posted in
https://patchwork.ozlabs.org/project/ltp/list/?series=178107&state=*
But this version properly run cleanup function after test start
(IMHO most important part of the patchset).
Kind regards,
Petr
Petr Vorel (3):
tst_test.sh: Call cleanup function after test start
tst_test.sh: Warn on missing cleanup function
tst_test.sh: Exit when setup declared but not defined
doc/test-writing-guidelines.txt | 10 +++++++---
testcases/lib/tst_test.sh | 16 ++++++++++++----
2 files changed, 19 insertions(+), 7 deletions(-)
--
2.29.2
^ permalink raw reply [flat|nested] 9+ messages in thread
* [LTP] [PATCH v4 1/3] tst_test.sh: Call cleanup function after test start
2020-12-17 14:44 [LTP] [PATCH v4 0/3] shell API setup/cleanup checks Petr Vorel
@ 2020-12-17 14:44 ` Petr Vorel
2021-01-07 10:05 ` Cyril Hrubis
2020-12-17 14:44 ` [LTP] [PATCH v4 2/3] tst_test.sh: Warn on missing cleanup function Petr Vorel
2020-12-17 14:44 ` [LTP] [PATCH v4 3/3] tst_test.sh: Exit when setup declared but not defined Petr Vorel
2 siblings, 1 reply; 9+ messages in thread
From: Petr Vorel @ 2020-12-17 14:44 UTC (permalink / raw)
To: ltp
It does not make sense to run the test cleanup function when the setup
function has been run.
And at least some network tests expect setup has been run before running
cleanup (e.g. tcp_fastopen_run.sh).
When shell API was introduced, cleanup function was run only if 1) setup
function was defined 2) and also run. That was inconsistent from C API,
thus e7dc14caa run it always.
But shell API is different from C API: tst_brk can be called from
tst_test.sh (or other library which is run before tst_run, e.g.
tst_net.sh). That was probably the reason, why detection via
$TST_SETUP_STARTED was introduced in initial shell API.
NOTE: using type is better than grep $TST_TEST_PATH, because cleanup
function can be in other library sourced by the test.
Fixes: e7dc14caa ("tst_test.sh: Run cleanup even setup is not defined")
Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
doc/test-writing-guidelines.txt | 10 +++++++---
testcases/lib/tst_test.sh | 4 ++--
2 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/doc/test-writing-guidelines.txt b/doc/test-writing-guidelines.txt
index 99fb34628..f3a55cf26 100644
--- a/doc/test-writing-guidelines.txt
+++ b/doc/test-writing-guidelines.txt
@@ -2251,11 +2251,15 @@ able to clean up correctly even in this situation. The easiest solution for
this is to keep track of what was initialized and act accordingly in the
cleanup.
-WARNING: Similar to the C library, calling tst_brk() in the $TST_CLEANUP does
+WARNING: Similar to the C library, calling 'tst_brk' in the $TST_CLEANUP does
not exit the test and 'TBROK' is converted to 'TWARN'.
-Notice also the 'tst_run' function called at the end of the test that actually
-starts the test.
+Notice also the 'tst_run' shell API function called at the end of the test that
+actually starts the test.
+
+WARNING: cleanup function is called only after 'tst_run' has been started.
+Calling 'tst_brk' in shell libraries, e.g. 'tst_test.sh' or 'tst_net.sh' does
+not trigger calling it.
[source,sh]
-------------------------------------------------------------------------------
diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh
index 2417da140..c205bc91b 100644
--- a/testcases/lib/tst_test.sh
+++ b/testcases/lib/tst_test.sh
@@ -28,7 +28,7 @@ _tst_do_exit()
local ret=0
TST_DO_EXIT=1
- if [ -n "$TST_CLEANUP" -a -z "$TST_NO_CLEANUP" ]; then
+ if [ -n "$TST_RUN_STARTED" -a -n "$TST_CLEANUP" -a -z "$TST_NO_CLEANUP" ]; then
$TST_CLEANUP
fi
@@ -599,7 +599,6 @@ tst_run()
fi
TST_ITERATIONS=$((TST_ITERATIONS-1))
done
-
_tst_do_exit
}
@@ -608,6 +607,7 @@ _tst_run_tests()
local _tst_data="$1"
local _tst_i
+ TST_RUN_STARTED=1
for _tst_i in $(seq ${TST_CNT:-1}); do
if type ${TST_TESTFUNC}1 > /dev/null 2>&1; then
_tst_run_test "$TST_TESTFUNC$_tst_i" $_tst_i "$_tst_data"
--
2.29.2
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [LTP] [PATCH v4 2/3] tst_test.sh: Warn on missing cleanup function
2020-12-17 14:44 [LTP] [PATCH v4 0/3] shell API setup/cleanup checks Petr Vorel
2020-12-17 14:44 ` [LTP] [PATCH v4 1/3] tst_test.sh: Call cleanup function after test start Petr Vorel
@ 2020-12-17 14:44 ` Petr Vorel
2021-01-07 10:06 ` Cyril Hrubis
2020-12-17 14:44 ` [LTP] [PATCH v4 3/3] tst_test.sh: Exit when setup declared but not defined Petr Vorel
2 siblings, 1 reply; 9+ messages in thread
From: Petr Vorel @ 2020-12-17 14:44 UTC (permalink / raw)
To: ltp
.i.e. be more informative than:
/opt/ltp/testcases/bin/tst_test.sh: line 33: cleanup: command not found
tst_brk does not make sense here, thus only warning.
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 c205bc91b..dbf1d2a97 100644
--- a/testcases/lib/tst_test.sh
+++ b/testcases/lib/tst_test.sh
@@ -29,7 +29,11 @@ _tst_do_exit()
TST_DO_EXIT=1
if [ -n "$TST_RUN_STARTED" -a -n "$TST_CLEANUP" -a -z "$TST_NO_CLEANUP" ]; then
- $TST_CLEANUP
+ if type $TST_CLEANUP >/dev/null 2>/dev/null; then
+ $TST_CLEANUP
+ else
+ tst_res TWARN "TST_CLEANUP=$TST_CLEANUP declared, but function not defined (or cmd not found)"
+ fi
fi
if [ "$TST_NEEDS_DEVICE" = 1 -a "$TST_DEVICE_FLAG" = 1 ]; then
--
2.29.2
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [LTP] [PATCH v4 3/3] tst_test.sh: Exit when setup declared but not defined
2020-12-17 14:44 [LTP] [PATCH v4 0/3] shell API setup/cleanup checks Petr Vorel
2020-12-17 14:44 ` [LTP] [PATCH v4 1/3] tst_test.sh: Call cleanup function after test start Petr Vorel
2020-12-17 14:44 ` [LTP] [PATCH v4 2/3] tst_test.sh: Warn on missing cleanup function Petr Vorel
@ 2020-12-17 14:44 ` Petr Vorel
2021-01-07 10:07 ` Cyril Hrubis
2 siblings, 1 reply; 9+ messages in thread
From: Petr Vorel @ 2020-12-17 14:44 UTC (permalink / raw)
To: ltp
It can lead to fail positive when just warn about
TST_SETUP declared but not defined (or cmd not found).
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 dbf1d2a97..1b05c1036 100644
--- a/testcases/lib/tst_test.sh
+++ b/testcases/lib/tst_test.sh
@@ -586,7 +586,11 @@ tst_run()
[ -n "$TST_NEEDS_MODULE" ] && tst_require_module "$TST_NEEDS_MODULE"
if [ -n "$TST_SETUP" ]; then
- $TST_SETUP
+ if type $TST_SETUP >/dev/null 2>/dev/null; then
+ $TST_SETUP
+ else
+ tst_brk TBROK "TST_SETUP=$TST_SETUP declared, but function not defined (or cmd not found)"
+ fi
fi
#TODO check that test reports some results for each test function call
--
2.29.2
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [LTP] [PATCH v4 1/3] tst_test.sh: Call cleanup function after test start
2020-12-17 14:44 ` [LTP] [PATCH v4 1/3] tst_test.sh: Call cleanup function after test start Petr Vorel
@ 2021-01-07 10:05 ` Cyril Hrubis
2021-01-07 11:02 ` Petr Vorel
2021-01-07 11:32 ` Petr Vorel
0 siblings, 2 replies; 9+ messages in thread
From: Cyril Hrubis @ 2021-01-07 10:05 UTC (permalink / raw)
To: ltp
Hi!
> It does not make sense to run the test cleanup function when the setup
> function has been run.
Did you mean "setup function hasn't been run." here?
As it is this sentence does not make any sense to me.
> And at least some network tests expect setup has been run before running
> cleanup (e.g. tcp_fastopen_run.sh).
>
> When shell API was introduced, cleanup function was run only if 1) setup
> function was defined 2) and also run. That was inconsistent from C API,
> thus e7dc14caa run it always.
>
> But shell API is different from C API: tst_brk can be called from
> tst_test.sh (or other library which is run before tst_run, e.g.
> tst_net.sh). That was probably the reason, why detection via
> $TST_SETUP_STARTED was introduced in initial shell API.
>
> NOTE: using type is better than grep $TST_TEST_PATH, because cleanup
> function can be in other library sourced by the test.
>
> Fixes: e7dc14caa ("tst_test.sh: Run cleanup even setup is not defined")
This still possibly break tests that would call tst_brk() in the middle
of setup and expect the cleanup() to be executed, right?
I guess that we would need TST_DO_CLEANUP that would be set in both
cases i.e. before we run setup and also before we execute the tests.
What about this:
diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh
index 2417da140..94d95df6f 100644
--- a/testcases/lib/tst_test.sh
+++ b/testcases/lib/tst_test.sh
@@ -28,7 +28,7 @@ _tst_do_exit()
local ret=0
TST_DO_EXIT=1
- if [ -n "$TST_CLEANUP" -a -z "$TST_NO_CLEANUP" ]; then
+ if [ -n $TST_DO_CLEANUP -a -n "$TST_CLEANUP" -a -z "$TST_NO_CLEANUP" ]; then
$TST_CLEANUP
fi
@@ -582,6 +582,7 @@ tst_run()
[ -n "$TST_NEEDS_MODULE" ] && tst_require_module "$TST_NEEDS_MODULE"
if [ -n "$TST_SETUP" ]; then
+ TST_DO_CLEANUP=1
$TST_SETUP
fi
@@ -608,6 +609,7 @@ _tst_run_tests()
local _tst_data="$1"
local _tst_i
+ TST_DO_CLEANUP=1
for _tst_i in $(seq ${TST_CNT:-1}); do
if type ${TST_TESTFUNC}1 > /dev/null 2>&1; then
_tst_run_test "$TST_TESTFUNC$_tst_i" $_tst_i "$_tst_data"
--
Cyril Hrubis
chrubis@suse.cz
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [LTP] [PATCH v4 2/3] tst_test.sh: Warn on missing cleanup function
2020-12-17 14:44 ` [LTP] [PATCH v4 2/3] tst_test.sh: Warn on missing cleanup function Petr Vorel
@ 2021-01-07 10:06 ` Cyril Hrubis
0 siblings, 0 replies; 9+ messages in thread
From: Cyril Hrubis @ 2021-01-07 10:06 UTC (permalink / raw)
To: ltp
Hi!
Looks good, Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
--
Cyril Hrubis
chrubis@suse.cz
^ permalink raw reply [flat|nested] 9+ messages in thread
* [LTP] [PATCH v4 3/3] tst_test.sh: Exit when setup declared but not defined
2020-12-17 14:44 ` [LTP] [PATCH v4 3/3] tst_test.sh: Exit when setup declared but not defined Petr Vorel
@ 2021-01-07 10:07 ` Cyril Hrubis
0 siblings, 0 replies; 9+ messages in thread
From: Cyril Hrubis @ 2021-01-07 10:07 UTC (permalink / raw)
To: ltp
Hi!
Looks good, Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
--
Cyril Hrubis
chrubis@suse.cz
^ permalink raw reply [flat|nested] 9+ messages in thread
* [LTP] [PATCH v4 1/3] tst_test.sh: Call cleanup function after test start
2021-01-07 10:05 ` Cyril Hrubis
@ 2021-01-07 11:02 ` Petr Vorel
2021-01-07 11:32 ` Petr Vorel
1 sibling, 0 replies; 9+ messages in thread
From: Petr Vorel @ 2021-01-07 11:02 UTC (permalink / raw)
To: ltp
Hi Cyril,
> Hi!
> > It does not make sense to run the test cleanup function when the setup
> > function has been run.
> Did you mean "setup function hasn't been run." here?
> As it is this sentence does not make any sense to me.
Yes, thanks!
> > And at least some network tests expect setup has been run before running
> > cleanup (e.g. tcp_fastopen_run.sh).
> > When shell API was introduced, cleanup function was run only if 1) setup
> > function was defined 2) and also run. That was inconsistent from C API,
> > thus e7dc14caa run it always.
> > But shell API is different from C API: tst_brk can be called from
> > tst_test.sh (or other library which is run before tst_run, e.g.
> > tst_net.sh). That was probably the reason, why detection via
> > $TST_SETUP_STARTED was introduced in initial shell API.
> > NOTE: using type is better than grep $TST_TEST_PATH, because cleanup
> > function can be in other library sourced by the test.
> > Fixes: e7dc14caa ("tst_test.sh: Run cleanup even setup is not defined")
> This still possibly break tests that would call tst_brk() in the middle
> of setup and expect the cleanup() to be executed, right?
> I guess that we would need TST_DO_CLEANUP that would be set in both
> cases i.e. before we run setup and also before we execute the tests.
> What about this:
Good catch, agree.
I'm going to merge with one runtime fix:
> - if [ -n $TST_DO_CLEANUP -a -n "$TST_CLEANUP" -a -z "$TST_NO_CLEANUP" ]; then
> + if [ -n "$TST_DO_CLEANUP" -a -n "$TST_CLEANUP" -a -z "$TST_NO_CLEANUP" ]; then
Quotes are required otherwise we can get error:
/opt/ltp/testcases/bin/tst_test.sh: line 31: [: too many arguments
Kind regards,
Petr
> diff --git a/testcases/lib/tst_test.sh b/testcases/lib/tst_test.sh
> index 2417da140..94d95df6f 100644
> --- a/testcases/lib/tst_test.sh
> +++ b/testcases/lib/tst_test.sh
> @@ -28,7 +28,7 @@ _tst_do_exit()
> local ret=0
> TST_DO_EXIT=1
> - if [ -n "$TST_CLEANUP" -a -z "$TST_NO_CLEANUP" ]; then
> + if [ -n $TST_DO_CLEANUP -a -n "$TST_CLEANUP" -a -z "$TST_NO_CLEANUP" ]; then
> $TST_CLEANUP
> fi
> @@ -582,6 +582,7 @@ tst_run()
> [ -n "$TST_NEEDS_MODULE" ] && tst_require_module "$TST_NEEDS_MODULE"
> if [ -n "$TST_SETUP" ]; then
> + TST_DO_CLEANUP=1
> $TST_SETUP
> fi
> @@ -608,6 +609,7 @@ _tst_run_tests()
> local _tst_data="$1"
> local _tst_i
> + TST_DO_CLEANUP=1
> for _tst_i in $(seq ${TST_CNT:-1}); do
> if type ${TST_TESTFUNC}1 > /dev/null 2>&1; then
> _tst_run_test "$TST_TESTFUNC$_tst_i" $_tst_i "$_tst_data"
^ permalink raw reply [flat|nested] 9+ messages in thread
* [LTP] [PATCH v4 1/3] tst_test.sh: Call cleanup function after test start
2021-01-07 10:05 ` Cyril Hrubis
2021-01-07 11:02 ` Petr Vorel
@ 2021-01-07 11:32 ` Petr Vorel
1 sibling, 0 replies; 9+ messages in thread
From: Petr Vorel @ 2021-01-07 11:32 UTC (permalink / raw)
To: ltp
Hi Cyril,
...
> This still possibly break tests that would call tst_brk() in the middle
> of setup and expect the cleanup() to be executed, right?
> I guess that we would need TST_DO_CLEANUP that would be set in both
> cases i.e. before we run setup and also before we execute the tests.
> What about this:
Whole patchset merged with your credit.
Thanks!
Kind regards,
Petr
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-01-07 11:32 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-17 14:44 [LTP] [PATCH v4 0/3] shell API setup/cleanup checks Petr Vorel
2020-12-17 14:44 ` [LTP] [PATCH v4 1/3] tst_test.sh: Call cleanup function after test start Petr Vorel
2021-01-07 10:05 ` Cyril Hrubis
2021-01-07 11:02 ` Petr Vorel
2021-01-07 11:32 ` Petr Vorel
2020-12-17 14:44 ` [LTP] [PATCH v4 2/3] tst_test.sh: Warn on missing cleanup function Petr Vorel
2021-01-07 10:06 ` Cyril Hrubis
2020-12-17 14:44 ` [LTP] [PATCH v4 3/3] tst_test.sh: Exit when setup declared but not defined Petr Vorel
2021-01-07 10:07 ` Cyril Hrubis
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.