From 1f6ec75f6e0e65c5a2fcee5cde9c3dcc0f40568d Mon Sep 17 00:00:00 2001 From: Trent Houliston Date: Sun, 18 Aug 2024 13:49:03 +1000 Subject: [PATCH 1/5] Make tests pass in their timeout, and don't use watchdog as for tests like TimeTravel where we manipulate the clock it won't work. --- tests/test_util/TestBase.hpp | 42 ++++++++++++++++++++++++---- tests/tests/api/TimeTravelFrozen.cpp | 2 +- tests/tests/dsl/Every.cpp | 5 ++-- tests/tests/dsl/Idle.cpp | 2 +- tests/tests/dsl/IdleSync.cpp | 2 +- tests/tests/dsl/TCP.cpp | 5 ++-- tests/tests/dsl/Watchdog.cpp | 4 +-- tests/tests/dsl/emit/Delay.cpp | 5 ++-- 8 files changed, 51 insertions(+), 16 deletions(-) diff --git a/tests/test_util/TestBase.hpp b/tests/test_util/TestBase.hpp index 58382579..bcb4a09a 100644 --- a/tests/test_util/TestBase.hpp +++ b/tests/test_util/TestBase.hpp @@ -32,7 +32,7 @@ namespace test_util { -template +template class TestBase : public NUClear::Reactor { public: /** @@ -46,7 +46,17 @@ class TestBase : public NUClear::Reactor { template struct Step {}; - explicit TestBase(std::unique_ptr environment, const bool& shutdown_on_idle = true) + /** + * Emit this struct to fail the test + */ + struct Fail { + explicit Fail(std::string message) : message(std::move(message)) {} + std::string message; + }; + + explicit TestBase(std::unique_ptr environment, + const bool& shutdown_on_idle = true, + const std::chrono::milliseconds& timeout = std::chrono::milliseconds(1000)) : Reactor(std::move(environment)) { // Shutdown if the system is idle @@ -54,13 +64,35 @@ class TestBase : public NUClear::Reactor { on>().then([this] { powerplant.shutdown(); }); } + on().then([this] { + std::lock_guard lock(timeout_mutex); + clean_shutdown = true; + timeout_cv.notify_all(); + }); + // Timeout if the test doesn't complete in time - on, MainThread>().then([this] { - powerplant.shutdown(); - INFO("Test timed out"); + // Typically we would use a watchdog, however it is subject to Time Travel + // So instead spawn a thread that will wait for the timeout and then fail the test and shut down + on().then([this, timeout] { + std::unique_lock lock(timeout_mutex); + timeout_cv.wait_for(lock, timeout); + if (!clean_shutdown) { + powerplant.shutdown(); + emit(std::make_unique("Test timed out")); + } + }); + + on, MainThread>().then([this](const Fail& f) { + INFO(f.message); CHECK(false); + powerplant.shutdown(); }); } + +private: + std::mutex timeout_mutex; + std::condition_variable timeout_cv; + bool clean_shutdown = false; }; } // namespace test_util diff --git a/tests/tests/api/TimeTravelFrozen.cpp b/tests/tests/api/TimeTravelFrozen.cpp index 9ab5ae09..162b6023 100644 --- a/tests/tests/api/TimeTravelFrozen.cpp +++ b/tests/tests/api/TimeTravelFrozen.cpp @@ -13,7 +13,7 @@ constexpr std::chrono::milliseconds SHUTDOWN_TIME = std::chrono::milliseconds(12 struct WaitForShutdown {}; -class TestReactor : public test_util::TestBase { +class TestReactor : public test_util::TestBase { public: TestReactor(std::unique_ptr environment) : TestBase(std::move(environment), false) { diff --git a/tests/tests/dsl/Every.cpp b/tests/tests/dsl/Every.cpp index 63b0dbce..445b9650 100644 --- a/tests/tests/dsl/Every.cpp +++ b/tests/tests/dsl/Every.cpp @@ -33,10 +33,11 @@ std::vector every_times; // NOLINT(cppcoreguideli std::vector per_times; // NOLINT(cppcoreguidelines-avoid-non-const-global-variables) std::vector dynamic_times; // NOLINT(cppcoreguidelines-avoid-non-const-global-variables) -class TestReactor : public test_util::TestBase { +class TestReactor : public test_util::TestBase { public: - TestReactor(std::unique_ptr environment) : TestBase(std::move(environment), false) { + TestReactor(std::unique_ptr environment) + : TestBase(std::move(environment), false, std::chrono::milliseconds(10000)) { // Trigger on 3 different types of every on>>().then([]() { every_times.push_back(NUClear::clock::now()); }); diff --git a/tests/tests/dsl/Idle.cpp b/tests/tests/dsl/Idle.cpp index e2a86a2b..b2a043ff 100644 --- a/tests/tests/dsl/Idle.cpp +++ b/tests/tests/dsl/Idle.cpp @@ -37,7 +37,7 @@ struct SimpleMessage { constexpr int time_step = 50; -class TestReactor : public test_util::TestBase { +class TestReactor : public test_util::TestBase { public: template struct CustomPool { diff --git a/tests/tests/dsl/IdleSync.cpp b/tests/tests/dsl/IdleSync.cpp index 4a5c860a..eccd9624 100644 --- a/tests/tests/dsl/IdleSync.cpp +++ b/tests/tests/dsl/IdleSync.cpp @@ -30,7 +30,7 @@ namespace { /// A vector of events that have happened std::vector events; // NOLINT(cppcoreguidelines-avoid-non-const-global-variables) -class TestReactor : public test_util::TestBase { +class TestReactor : public test_util::TestBase { public: TestReactor(std::unique_ptr environment) : TestBase(std::move(environment), false) { diff --git a/tests/tests/dsl/TCP.cpp b/tests/tests/dsl/TCP.cpp index 70c1175f..56a346de 100644 --- a/tests/tests/dsl/TCP.cpp +++ b/tests/tests/dsl/TCP.cpp @@ -57,7 +57,7 @@ struct TestConnection { struct Finished {}; -class TestReactor : public test_util::TestBase { +class TestReactor : public test_util::TestBase { public: void handle_data(const std::string& name, const IO::Event& event) { // We have data to read @@ -78,7 +78,8 @@ class TestReactor : public test_util::TestBase { } } - TestReactor(std::unique_ptr environment) : TestBase(std::move(environment), false) { + TestReactor(std::unique_ptr environment) + : TestBase(std::move(environment), false, std::chrono::milliseconds(2000)) { for (const auto& t : active_tests) { switch (t) { diff --git a/tests/tests/dsl/Watchdog.cpp b/tests/tests/dsl/Watchdog.cpp index 6669bf8a..407c014b 100644 --- a/tests/tests/dsl/Watchdog.cpp +++ b/tests/tests/dsl/Watchdog.cpp @@ -35,10 +35,10 @@ std::vector events; // NOLINT(cppcoreguidelines-avoid-non-const-gl template struct Flag {}; -class TestReactor : public test_util::TestBase { +class TestReactor : public test_util::TestBase { public: TestReactor(std::unique_ptr environment) - : TestBase(std::move(environment), false), start(NUClear::clock::now()) { + : TestBase(std::move(environment), false, std::chrono::milliseconds(10000)), start(NUClear::clock::now()) { on, 50, std::chrono::milliseconds>>().then([this] { events.push_back("Watchdog 1 triggered @ " + floored_time()); diff --git a/tests/tests/dsl/emit/Delay.cpp b/tests/tests/dsl/emit/Delay.cpp index a20e1aad..573f243a 100644 --- a/tests/tests/dsl/emit/Delay.cpp +++ b/tests/tests/dsl/emit/Delay.cpp @@ -51,9 +51,10 @@ struct TargetTimeMessage { struct FinishTest {}; -class TestReactor : public test_util::TestBase { +class TestReactor : public test_util::TestBase { public: - TestReactor(std::unique_ptr environment) : TestBase(std::move(environment), false) { + TestReactor(std::unique_ptr environment) + : TestBase(std::move(environment), false, std::chrono::milliseconds(2000)) { // Measure when messages were sent and received and print those values on>().then([](const DelayedMessage& m) { From 9ae1d2aee18bc0395ea7d131aa22e921e77c9aa3 Mon Sep 17 00:00:00 2001 From: Trent Houliston Date: Sun, 18 Aug 2024 14:21:09 +1000 Subject: [PATCH 2/5] Add a force option to shutdown for tests which blow out tasks --- src/PowerPlant.cpp | 4 ++-- src/PowerPlant.hpp | 4 +++- src/threading/TaskScheduler.cpp | 5 ++++- src/threading/TaskScheduler.hpp | 6 ++++-- tests/test_util/TestBase.hpp | 13 ++++++++----- tests/tests/api/TimeTravel.cpp | 2 +- 6 files changed, 22 insertions(+), 12 deletions(-) diff --git a/src/PowerPlant.cpp b/src/PowerPlant.cpp index c805334f..dc938839 100644 --- a/src/PowerPlant.cpp +++ b/src/PowerPlant.cpp @@ -143,13 +143,13 @@ void PowerPlant::log(const LogLevel& level, std::stringstream& message) { log(level, message.str()); } -void PowerPlant::shutdown() { +void PowerPlant::shutdown(bool force) { // Emit our shutdown event emit(std::make_unique()); // Shutdown the scheduler - scheduler.shutdown(); + scheduler.shutdown(force); } } // namespace NUClear diff --git a/src/PowerPlant.hpp b/src/PowerPlant.hpp index 4a8582d5..92475152 100644 --- a/src/PowerPlant.hpp +++ b/src/PowerPlant.hpp @@ -118,8 +118,10 @@ class PowerPlant { /** * Shuts down the PowerPlant, tells all component threads to terminate and waits for them to finish. + * + * @param force If true, the PowerPlant will shutdown immediately without waiting for tasks to finish */ - void shutdown(); + void shutdown(bool force = false); /** * Installs a reactor of a particular type to the system. diff --git a/src/threading/TaskScheduler.cpp b/src/threading/TaskScheduler.cpp index 35f1c65f..ed5d5846 100644 --- a/src/threading/TaskScheduler.cpp +++ b/src/threading/TaskScheduler.cpp @@ -167,11 +167,14 @@ namespace threading { } } - void TaskScheduler::shutdown() { + void TaskScheduler::shutdown(bool force) { started.store(false, std::memory_order_release); running.store(false, std::memory_order_release); for (auto& pool : pool_queues) { const std::lock_guard lock(pool.second->mutex); + if (force) { + pool.second->queue.clear(); + } pool.second->condition.notify_all(); } } diff --git a/src/threading/TaskScheduler.hpp b/src/threading/TaskScheduler.hpp index d5c96958..96eeb9e3 100644 --- a/src/threading/TaskScheduler.hpp +++ b/src/threading/TaskScheduler.hpp @@ -163,9 +163,11 @@ namespace threading { /** * * Shuts down the scheduler, all waiting threads are woken, and any attempt to get a task results in an - * exception + * exception. + * + * @param force If true, the scheduler will be shutdown immediately, and all tasks will be dropped */ - void shutdown(); + void shutdown(bool force = false); /** * Submit a new task to be executed to the Scheduler. diff --git a/tests/test_util/TestBase.hpp b/tests/test_util/TestBase.hpp index bcb4a09a..e6f15d89 100644 --- a/tests/test_util/TestBase.hpp +++ b/tests/test_util/TestBase.hpp @@ -20,8 +20,8 @@ * OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. */ -#ifndef TEST_UTIL_TESTBASE_HPP -#define TEST_UTIL_TESTBASE_HPP +#ifndef TEST_UTIL_TEST_BASE_HPP +#define TEST_UTIL_TEST_BASE_HPP #include #include @@ -74,10 +74,14 @@ class TestBase : public NUClear::Reactor { // Typically we would use a watchdog, however it is subject to Time Travel // So instead spawn a thread that will wait for the timeout and then fail the test and shut down on().then([this, timeout] { + if (clean_shutdown) { + return; + } std::unique_lock lock(timeout_mutex); timeout_cv.wait_for(lock, timeout); + if (!clean_shutdown) { - powerplant.shutdown(); + powerplant.shutdown(true); emit(std::make_unique("Test timed out")); } }); @@ -85,7 +89,6 @@ class TestBase : public NUClear::Reactor { on, MainThread>().then([this](const Fail& f) { INFO(f.message); CHECK(false); - powerplant.shutdown(); }); } @@ -97,4 +100,4 @@ class TestBase : public NUClear::Reactor { } // namespace test_util -#endif // TEST_UTIL_TESTBASE_HPP +#endif // TEST_UTIL_TEST_BASE_HPP diff --git a/tests/tests/api/TimeTravel.cpp b/tests/tests/api/TimeTravel.cpp index b44bbc8c..36aad917 100644 --- a/tests/tests/api/TimeTravel.cpp +++ b/tests/tests/api/TimeTravel.cpp @@ -23,7 +23,7 @@ struct Results { std::array events; }; -class TestReactor : public test_util::TestBase { +class TestReactor : public test_util::TestBase { public: TestReactor(std::unique_ptr environment) : TestBase(std::move(environment), false) { From 8cb7a4a9b03cee719a59ab8ab9cd41306bd1d6ed Mon Sep 17 00:00:00 2001 From: Trent Houliston Date: Sun, 18 Aug 2024 18:30:15 +1000 Subject: [PATCH 3/5] clang-tidy --- tests/test_util/TestBase.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_util/TestBase.hpp b/tests/test_util/TestBase.hpp index e6f15d89..da8e33fc 100644 --- a/tests/test_util/TestBase.hpp +++ b/tests/test_util/TestBase.hpp @@ -65,7 +65,7 @@ class TestBase : public NUClear::Reactor { } on().then([this] { - std::lock_guard lock(timeout_mutex); + const std::lock_guard lock(timeout_mutex); clean_shutdown = true; timeout_cv.notify_all(); }); From dd2a1e4c8367a8dac63eb9a706a0a3026519910e Mon Sep 17 00:00:00 2001 From: Trent Houliston Date: Mon, 19 Aug 2024 09:59:09 +1000 Subject: [PATCH 4/5] Longer idle timeout --- tests/tests/dsl/Idle.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/tests/dsl/Idle.cpp b/tests/tests/dsl/Idle.cpp index b2a043ff..7ca7ce74 100644 --- a/tests/tests/dsl/Idle.cpp +++ b/tests/tests/dsl/Idle.cpp @@ -51,7 +51,8 @@ class TestReactor : public test_util::TestBase { emit(std::make_unique>()); } - TestReactor(std::unique_ptr environment) : TestBase(std::move(environment), false) { + TestReactor(std::unique_ptr environment) + : TestBase(std::move(environment), false, std::chrono::seconds(2)) { start_time = NUClear::clock::now(); From a3bf9eacf0a6646d6e93eae93971b76f17743fba Mon Sep 17 00:00:00 2001 From: Trent Houliston Date: Mon, 19 Aug 2024 10:02:23 +1000 Subject: [PATCH 5/5] Seconds are easier to read --- tests/tests/dsl/Every.cpp | 2 +- tests/tests/dsl/TCP.cpp | 2 +- tests/tests/dsl/Watchdog.cpp | 2 +- tests/tests/dsl/emit/Delay.cpp | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/tests/dsl/Every.cpp b/tests/tests/dsl/Every.cpp index 445b9650..cfacd231 100644 --- a/tests/tests/dsl/Every.cpp +++ b/tests/tests/dsl/Every.cpp @@ -37,7 +37,7 @@ class TestReactor : public test_util::TestBase { public: TestReactor(std::unique_ptr environment) - : TestBase(std::move(environment), false, std::chrono::milliseconds(10000)) { + : TestBase(std::move(environment), false, std::chrono::seconds(10)) { // Trigger on 3 different types of every on>>().then([]() { every_times.push_back(NUClear::clock::now()); }); diff --git a/tests/tests/dsl/TCP.cpp b/tests/tests/dsl/TCP.cpp index 56a346de..6b2f4236 100644 --- a/tests/tests/dsl/TCP.cpp +++ b/tests/tests/dsl/TCP.cpp @@ -79,7 +79,7 @@ class TestReactor : public test_util::TestBase { } TestReactor(std::unique_ptr environment) - : TestBase(std::move(environment), false, std::chrono::milliseconds(2000)) { + : TestBase(std::move(environment), false, std::chrono::seconds(2)) { for (const auto& t : active_tests) { switch (t) { diff --git a/tests/tests/dsl/Watchdog.cpp b/tests/tests/dsl/Watchdog.cpp index 407c014b..03ce6658 100644 --- a/tests/tests/dsl/Watchdog.cpp +++ b/tests/tests/dsl/Watchdog.cpp @@ -38,7 +38,7 @@ struct Flag {}; class TestReactor : public test_util::TestBase { public: TestReactor(std::unique_ptr environment) - : TestBase(std::move(environment), false, std::chrono::milliseconds(10000)), start(NUClear::clock::now()) { + : TestBase(std::move(environment), false), start(NUClear::clock::now()) { on, 50, std::chrono::milliseconds>>().then([this] { events.push_back("Watchdog 1 triggered @ " + floored_time()); diff --git a/tests/tests/dsl/emit/Delay.cpp b/tests/tests/dsl/emit/Delay.cpp index 573f243a..cefc17e7 100644 --- a/tests/tests/dsl/emit/Delay.cpp +++ b/tests/tests/dsl/emit/Delay.cpp @@ -54,7 +54,7 @@ struct FinishTest {}; class TestReactor : public test_util::TestBase { public: TestReactor(std::unique_ptr environment) - : TestBase(std::move(environment), false, std::chrono::milliseconds(2000)) { + : TestBase(std::move(environment), false, std::chrono::seconds(2)) { // Measure when messages were sent and received and print those values on>().then([](const DelayedMessage& m) {