ztest teardown function not called if test function is interrupted

[rakons]

Describe the bug
If a test function fails (call to ztest_test_fail) or a ztest_test_pass is called from the function no teardown function is called.

To Reproduce
It is really enough to look into the code, but I may provide some tests if really required.

Expected behavior
Teardown function should be called even if ztest_test_fail) or a ztest_test_pass is called from the test function body.

Impact
annoyance

Environment (please complete the following information):

• Unit test on local machine, it means without a Kernel.

Additional context
The problem is that test teardown is called linearly here:

static void run_test_functions(struct unit_test *test)
{
	phase = TEST_PHASE_SETUP;
	test->setup();
	phase = TEST_PHASE_TEST;
	test->test();
	phase = TEST_PHASE_TEARDOWN;
	test->teardown();
	phase = TEST_PHASE_FRAMEWORK;
}

While calling ztest_test_fail or ztest_test_fail pass would finally lead to longjump into run_test function from witch run_test_functions is called.
As an effect - the line test->teardown(); would be never reached.

To solve this it should be enough to move the teardown call into run_test function just after out: label.

[rakons]

CC @jgke @nashif

[andrewboie]

This is actually pretty bad.

In SMP scenarios, if the 1cpu test variants are used, the setup function spawns threads which block other CPUs. The teardown function kills those threads. If the test is interrupted (ztest_test_fail, ztest_test_pass, ztest_test_skip are examples) then the teardown function is never run.

If a subsequent case tries to also run a 1cpu test, then we have a problem: the 1cpu thread objects have k_thread_create() called on them again, when they are already running! I'm getting a crash like the following, I added a printk when the blocking threads are started/stopped:

diff --git a/subsys/testsuite/ztest/src/ztest.c b/subsys/testsuite/ztest/src/ztest.c
index 2adda688d2..c341a339b0 100644
--- a/subsys/testsuite/ztest/src/ztest.c
+++ b/subsys/testsuite/ztest/src/ztest.c
@@ -108,6 +108,7 @@ void z_impl_z_test_1cpu_start(void)
         * each to signal us that it's locked and spinning.
         */
        for (int i = 0; i < NUM_CPUHOLD; i++)  {
+               printk("start cpuhold thread %d\n", i);
                k_thread_create(&cpuhold_threads[i],
                                cpuhold_stacks[i], CPUHOLD_STACK_SZ,
                                (k_thread_entry_t) cpu_hold, NULL, NULL, NULL,
@@ -121,6 +122,7 @@ void z_impl_z_test_1cpu_stop(void)
        cpuhold_active = 0;
 
        for (int i = 0; i < NUM_CPUHOLD; i++)  {
+               printk("stop cpuhold thread %d\n", 1);
                k_thread_abort(&cpuhold_threads[i]);
        }
 }

*** Booting Zephyr OS build zephyr-v2.1.0-336-g609ff77949f6  ***
Running test suite queue_api
===================================================================
starting test - test_queue_supv_to_user
start cpuhold thread 0
SKIP - test_queue_supv_to_user
===================================================================
starting test - test_queue_alloc_prepend_user
SKIP - test_queue_alloc_prepend_user
===================================================================
starting test - test_queue_alloc_append_user
SKIP - test_queue_alloc_append_user
===================================================================
starting test - test_auto_free
SKIP - test_auto_free
===================================================================
starting test - test_queue_thread2thread
start cpuhold thread 0
E: ***** CPU Page Fault (error code 0x0000000000000010)
E: Supervisor thread executed address 0x0000000000000000
E: PML4E: 0x000000000011b827 Writable, User, Execute Enabled
E: PDPTE: 0x000000000011a827 Writable, User, Execute Enabled
E:   PDE: 0x0000000000119827 Writable, User, Execute Enabled
E:   PTE: Non-present
E: RAX: 0x000000000016845d RBX: 0x00000000000186a0 RCX: 0x00000000000f4240 RDX: 0x0000000000000000
E: RSI: 0x0000000000000006 RDI: 0x00000000000003e8 RBP: 0x0000000000166e4c RSP: 0x000000000012e1b0
E:  R8: 0x0000000000000000  R9: 0x0000000000000000 R10: 0x0000000000000000 R11: 0x0000000000000000
E: R12: 0x0000000000000000 R13: 0x0000000000000000 R14: 0x0000000000000000 R15: 0x0000000000000000
E: RSP: 0x000000000012e1b0 RFLAGS: 0x0000000000000083 CS: 0x0018 CR3: 0x000000000010b000
E: call trace:
E: RIP: 0x0000000000000000
E: unaligned frame ptr
E: >>> ZEPHYR FATAL ERROR 0: CPU exception on CPU 1
E: Current thread: 0x000000000011dae0 (unknown)
E: Halting system

[andyross]

Don't have an exerciser handy, and not an expert on ztest. But looking at it it seems fairly straightforward. This moves the cleanup handling until after the setjmp targets and after the spawned thread is aborted. It doesn't seem to have broken anything obvious:

diff --git a/subsys/testsuite/ztest/src/ztest.c b/subsys/testsuite/ztest/src/ztest.c
index 2adda688d2..161b511bb7 100644
--- a/subsys/testsuite/ztest/src/ztest.c
+++ b/subsys/testsuite/ztest/src/ztest.c
@@ -42,6 +42,10 @@ static int cleanup_test(struct unit_test *test)
        k_thread_abort(&ztest_thread);
 #endif
 
+       phase = TEST_PHASE_TEARDOWN;
+       test->teardown();
+       phase = TEST_PHASE_FRAMEWORK;
+
        if (!ret && mock_status == 1) {
                PRINT("Test %s failed: Unused mock parameter values\n",
                      test->name);
@@ -146,8 +150,6 @@ static void run_test_functions(struct unit_test *test)
        test->setup();
        phase = TEST_PHASE_TEST;
        test->test();
-       phase = TEST_PHASE_TEARDOWN;
-       test->teardown();
        phase = TEST_PHASE_FRAMEWORK;
 }

[andrewboie]

@andyross this approach looks good -- one potential gotcha is that teardown is running in the main thread instead of ztest_thread, and IIRC main has a pretty small stack size as compared to the test runner. But I agree we should move the teardown out of the ztest thread, the gymnastics involved in ensuring it runs if its still in ztest_thread context seems somewhere between very complex to impossible.