From 6922c754cdad29b20d7dd6789004fa46182a0c3b Mon Sep 17 00:00:00 2001 From: Victor Moene Date: Thu, 24 Sep 2026 10:20:06 +0200 Subject: [PATCH] Fixed stoppable_thread_test error and hide watcher_test error msg 1. stoppable_thread_test.c: There was a race condition in test_stop_running_thread, which expected the thread to run at least one iteration before being stopped. ThreadedQueuePush was used to signal that the thread had started, but it was called before the loop, so the test could stop the thread before its first iteration. 2. watcher_test.c, stoppable_thread_test.c: Code intended to fail was logging error instead of capturing the output. Signed-off-by: Victor Moene --- tests/unit/stoppable_thread_test.c | 49 +++++++++++++++++++++++++++--- tests/unit/watcher_test.c | 10 +++++- 2 files changed, 54 insertions(+), 5 deletions(-) diff --git a/tests/unit/stoppable_thread_test.c b/tests/unit/stoppable_thread_test.c index c935003b52c..08aaa4f2c7c 100644 --- a/tests/unit/stoppable_thread_test.c +++ b/tests/unit/stoppable_thread_test.c @@ -4,6 +4,10 @@ #include #include #include /* ThreadLock(), ThreadUnlock() */ +#include +#include /* LoggingPrivContext, LoggingPrivSetContext() */ + +#include /* strstr() */ /* How long the thread routines below wait for the test to do something * before failing, so that a broken StoppableThread can't hang the test. */ @@ -38,16 +42,21 @@ static void WaitForStart(RoutineData *data) } /* Loops until stopped, sleeping for a long time on each iteration, so it only - * returns promptly if StoppableThreadStop() wakes it up. */ + * returns promptly if StoppableThreadStop() wakes it up. Signals the start + * only once inside the loop, so a stop requested after WaitForStart() can't + * arrive before the first iteration. */ static void LoopUntilStopped(StoppableThread *thread, void *arg) { RoutineData *data = arg; data->should_stop_at_start = StoppableThreadShouldStop(thread); - ThreadedQueuePush(data->started, data); while (!StoppableThreadShouldStop(thread)) { data->iterations++; + if (data->iterations == 1) + { + ThreadedQueuePush(data->started, data); + } StoppableThreadSleep(thread, 60); } data->saw_stop = true; @@ -195,6 +204,19 @@ static void IgnoreStop(ARG_UNUSED StoppableThread *thread, ARG_UNUSED void *arg) } } +static int captured_err_count = 0; +static char captured_err_message[256]; + +static char *CaptureErrorLogHook(ARG_UNUSED LoggingPrivContext *pctx, LogLevel level, const char *message) +{ + if (level == LOG_LEVEL_ERR) + { + captured_err_count++; + strlcpy(captured_err_message, message, sizeof(captured_err_message)); + } + return (char *) message; +} + static void test_stop_timeout(void) { ignore_stop_started = ThreadedQueueNew(1, NULL); @@ -205,12 +227,31 @@ static void test_stop_timeout(void) assert_true(ThreadedQueuePop(ignore_stop_started, &item, TEST_TIMEOUT_SECS)); /* The routine never checks for a stop, so this must give up after the - * timeout, not wait for the routine to return. */ + * timeout, not wait for the routine to return, and report it. The error + * is captured instead of printed, since it's expected here. Logging is + * restored before asserting, so a failure can't leave it silenced. */ + captured_err_count = 0; + captured_err_message[0] = '\0'; + LoggingPrivContext log_ctx = { + .log_hook = CaptureErrorLogHook, + .force_hook_level = LOG_LEVEL_ERR, + }; + LoggingPrivSetContext(&log_ctx); + const LogLevel old_level = LogGetGlobalLevel(); + LogSetGlobalLevel(LOG_LEVEL_CRIT); + const time_t start = time(NULL); - assert_false(StoppableThreadStop(abandoned_thread, 1)); + const bool stopped = StoppableThreadStop(abandoned_thread, 1); const time_t elapsed = time(NULL) - start; + + LogSetGlobalLevel(old_level); + LoggingPrivSetContext(NULL); + + assert_false(stopped); assert_true(elapsed >= 1); assert_true(elapsed < TEST_TIMEOUT_SECS); + assert_int_equal(captured_err_count, 1); + assert_true(strstr(captured_err_message, "did not exit") != NULL); /* Let the routine return in case it wasn't cancelled (no pthread_cancel() * on this platform). ignore_stop_started is leaked on purpose, since the diff --git a/tests/unit/watcher_test.c b/tests/unit/watcher_test.c index 8991d7d9f53..83e6a3ff805 100644 --- a/tests/unit/watcher_test.c +++ b/tests/unit/watcher_test.c @@ -50,14 +50,22 @@ static void test_watcher_register_duplicate_key_ignored(void) captured_err_count = 0; captured_err_message[0] = '\0'; - LoggingPrivContext log_ctx = { .log_hook = CaptureErrorLogHook }; + /* force_hook_level makes the hook still see the error while the console + * level is lowered, so the expected error isn't printed. */ + LoggingPrivContext log_ctx = { + .log_hook = CaptureErrorLogHook, + .force_hook_level = LOG_LEVEL_ERR, + }; LoggingPrivSetContext(&log_ctx); + const LogLevel old_level = LogGetGlobalLevel(); + LogSetGlobalLevel(LOG_LEVEL_CRIT); /* Registering the same key again must be rejected: an error is logged * (not silently swallowed) and the first registration is kept, not * replaced. */ WatcherRegister("dup-event", EVENT_FILE_DELETED, NULL, fake_bundle_b, 5); + LogSetGlobalLevel(old_level); LoggingPrivSetContext(NULL); assert_int_equal(captured_err_count, 1);