From 784408358d39346b410df65b9f657007c467a010 Mon Sep 17 00:00:00 2001 From: Feoramund <161657516+Feoramund@users.noreply.github.com> Date: Sat, 15 Jun 2024 10:25:58 -0400 Subject: [PATCH 1/3] Call `cleanups` after test signal --- core/testing/runner.odin | 11 +++++++++-- core/testing/testing.odin | 12 +++++++++++- 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/core/testing/runner.odin b/core/testing/runner.odin index 328186c35..147c6d094 100644 --- a/core/testing/runner.odin +++ b/core/testing/runner.odin @@ -604,10 +604,10 @@ runner :: proc(internal_tests: []Internal_Test) -> bool { }) fmt.assertf(alloc_error == nil, "Error appending to log messages: %v", alloc_error) - find_task_data: for &data in task_data_slots { + find_task_data_for_timeout: for &data in task_data_slots { if data.it.pkg == it.pkg && data.it.name == it.name { end_t(&data.t) - break find_task_data + break find_task_data_for_timeout } } } @@ -655,6 +655,13 @@ runner :: proc(internal_tests: []Internal_Test) -> bool { } + find_task_data_for_stop_signal: for &data in task_data_slots { + if data.it.pkg == it.pkg && data.it.name == it.name { + end_t(&data.t) + break find_task_data_for_stop_signal + } + } + when FANCY_OUTPUT { bypass_progress_overwrite = true signals_were_raised = true diff --git a/core/testing/testing.odin b/core/testing/testing.odin index 92b4d391d..07e2063ca 100644 --- a/core/testing/testing.odin +++ b/core/testing/testing.odin @@ -94,7 +94,17 @@ logf :: proc(t: ^T, format: string, args: ..any, loc := #caller_location) { // cleanup registers a procedure and user_data, which will be called when the test, and all its subtests, complete. // Cleanup procedures will be called in LIFO (last added, first called) order. -// Each procedure will use a copy of the context at the time of registering. +// +// Each procedure will use a copy of the context at the time of registering, +// and if the test failed due to a timeout, failed assertion, panic, bounds-checking error, +// memory access violation, or any other signal-based fault, this procedure will +// run with greater privilege in the test runner's main thread. +// +// That means that any cleanup procedure absolutely must not fail in the same way, +// or it will take down the entire test runner with it. This is for when you +// need something to run no matter what, if a test failed. +// +// For almost every usual case, `defer` should be preferable and sufficient. cleanup :: proc(t: ^T, procedure: proc(rawptr), user_data: rawptr) { append(&t.cleanups, Internal_Cleanup{procedure, user_data, context}) } From bb823d5ba0aa412c40a0e23c9b27f00c734e9866 Mon Sep 17 00:00:00 2001 From: Feoramund <161657516+Feoramund@users.noreply.github.com> Date: Sat, 15 Jun 2024 10:36:05 -0400 Subject: [PATCH 2/3] Make `testing.fail_now` divergent This is in line with the old way it worked on Windows. --- core/testing/runner.odin | 37 +++++++++++++++++++++++-------------- core/testing/testing.odin | 15 ++++++++++----- 2 files changed, 33 insertions(+), 19 deletions(-) diff --git a/core/testing/runner.odin b/core/testing/runner.odin index 147c6d094..6a33436f6 100644 --- a/core/testing/runner.odin +++ b/core/testing/runner.odin @@ -497,6 +497,7 @@ runner :: proc(internal_tests: []Internal_Test) -> bool { data.it = it data.t.seed = shared_random_seed data.t.error_count = 0 + data.t._fail_now_called = false thread.pool_add_task(&pool, task.allocator, run_test_task, data, run_index) } @@ -645,28 +646,36 @@ runner :: proc(internal_tests: []Internal_Test) -> bool { "A signal (%v) was raised to stop test #%i %s.%s, but it was unable to be found.", reason, test_index, it.pkg, it.name) - if test_index not_in failed_test_reason_map { - // We only write a new error message here if there wasn't one - // already, because the message we can provide based only on - // the signal won't be very useful, whereas asserts and panics - // will provide a user-written error message. - failed_test_reason_map[test_index] = fmt.aprintf("Signal caught: %v", reason, allocator = shared_log_allocator) - pkg_log.fatalf("Caught signal to stop test #%i %s.%s for: %v.", test_index, it.pkg, it.name, reason) - - } - + // The order this is handled in is a little particular. + task_data: ^Task_Data find_task_data_for_stop_signal: for &data in task_data_slots { if data.it.pkg == it.pkg && data.it.name == it.name { - end_t(&data.t) + task_data = &data break find_task_data_for_stop_signal } } - when FANCY_OUTPUT { - bypass_progress_overwrite = true - signals_were_raised = true + fmt.assertf(task_data != nil, "A signal (%v) was raised to stop test #%i %s.%s, but its task data is missing.", + reason, test_index, it.pkg, it.name) + + if !task_data.t._fail_now_called { + if test_index not_in failed_test_reason_map { + // We only write a new error message here if there wasn't one + // already, because the message we can provide based only on + // the signal won't be very useful, whereas asserts and panics + // will provide a user-written error message. + failed_test_reason_map[test_index] = fmt.aprintf("Signal caught: %v", reason, allocator = shared_log_allocator) + pkg_log.fatalf("Caught signal to stop test #%i %s.%s for: %v.", test_index, it.pkg, it.name, reason) + } + + when FANCY_OUTPUT { + bypass_progress_overwrite = true + signals_were_raised = true + } } + end_t(&task_data.t) + total_failure_count += 1 total_done_count += 1 } diff --git a/core/testing/testing.odin b/core/testing/testing.odin index 07e2063ca..29fe853ef 100644 --- a/core/testing/testing.odin +++ b/core/testing/testing.odin @@ -48,7 +48,7 @@ T :: struct { // tests during channel transmission. _log_allocator: runtime.Allocator, - _fail_now: proc() -> !, + _fail_now_called: bool, } @@ -66,15 +66,20 @@ fail :: proc(t: ^T, loc := #caller_location) { pkg_log.error("FAIL", location=loc) } -fail_now :: proc(t: ^T, msg := "", loc := #caller_location) { +// fail_now will cause a test to immediately fail and abort, much in the same +// way a failed assertion or panic call will stop a thread. +// +// It is for when you absolutely need a test to fail without calling any of its +// deferred statements. It will be cleaner than a regular assert or panic, +// as the test runner will know to expect the signal this procedure will raise. +fail_now :: proc(t: ^T, msg := "", loc := #caller_location) -> ! { + t._fail_now_called = true if msg != "" { pkg_log.error("FAIL:", msg, location=loc) } else { pkg_log.error("FAIL", location=loc) } - if t._fail_now != nil { - t._fail_now() - } + runtime.trap() } failed :: proc(t: ^T) -> bool { From f353adc7fbb9a185724e36fba1b8bb5ec1059913 Mon Sep 17 00:00:00 2001 From: Feoramund <161657516+Feoramund@users.noreply.github.com> Date: Sat, 15 Jun 2024 10:39:28 -0400 Subject: [PATCH 3/3] Prefer `log.error` over `fail_now` in this case --- tests/core/slice/test_core_slice.odin | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/core/slice/test_core_slice.odin b/tests/core/slice/test_core_slice.odin index 23de1b482..3a15ddaa0 100644 --- a/tests/core/slice/test_core_slice.odin +++ b/tests/core/slice/test_core_slice.odin @@ -3,6 +3,7 @@ package test_core_slice import "core:slice" import "core:testing" import "core:math/rand" +import "core:log" @test test_sort_with_indices :: proc(t: ^testing.T) { @@ -205,7 +206,7 @@ test_permutation_iterator :: proc(t: ^testing.T) { n += item } if n in seen { - testing.fail_now(t, "Permutation iterator made a duplicate permutation.") + log.error("Permutation iterator made a duplicate permutation.") return } seen[n] = true