From ba3208fc2b5ff9069c71630287c25d49daa27cdf Mon Sep 17 00:00:00 2001 From: Trent Houliston Date: Wed, 19 Jun 2024 10:54:54 +1000 Subject: [PATCH 1/4] Add a test which reveals a bug when combining groups and pools --- tests/CMakeLists.txt | 6 +-- tests/dsl/GroupPool.cpp | 87 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 90 insertions(+), 3 deletions(-) create mode 100644 tests/dsl/GroupPool.cpp diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 2a1158efc..8f6a10e65 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -58,7 +58,7 @@ if(CATCH_FOUND) get_filename_component(test_name ${test_src} NAME_WE) add_executable(${test_name} ${test_src}) - target_link_libraries(${test_name} NUClear::nuclear test_util) + target_link_libraries(${test_name} test_util) set_target_properties(${test_name} PROPERTIES RUNTIME_OUTPUT_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}/individual") target_include_directories(${test_name} PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}) # Enable warnings, and all warnings are errors @@ -69,11 +69,11 @@ if(CATCH_FOUND) add_executable(test_nuclear ${test_src}) target_include_directories(test_nuclear PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}) - target_link_libraries(test_nuclear NUClear::nuclear test_util) + target_link_libraries(test_nuclear test_util) add_test(test_nuclear test_nuclear) add_executable(test_network networktest.cpp) - target_link_libraries(test_network NUClear::nuclear test_util) + target_link_libraries(test_network test_util) target_include_directories(test_network PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}) endif(BUILD_TESTS) diff --git a/tests/dsl/GroupPool.cpp b/tests/dsl/GroupPool.cpp new file mode 100644 index 000000000..0a4fd63ce --- /dev/null +++ b/tests/dsl/GroupPool.cpp @@ -0,0 +1,87 @@ +/* + * MIT License + * + * Copyright (c) 2024 NUClear Contributors + * + * This file is part of the NUClear codebase. + * See https://github.com/Fastcode/NUClear for further info. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy of this software and associated + * documentation files (the "Software"), to deal in the Software without restriction, including without limitation the + * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies of the Software, and to + * permit persons to whom the Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in all copies or substantial portions of the + * Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE + * WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR + * COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR + * OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + */ + +#include +#include + +#include "test_util/TestBase.hpp" + +namespace { + +/// @brief A vector of events that have happened +std::vector events; // NOLINT(cppcoreguidelines-avoid-non-const-global-variables) + +struct MessageA {}; +struct MessageB {}; + +class TestReactor : public test_util::TestBase { +public: + static constexpr int thread_count = 1; + + TestReactor(std::unique_ptr environment) : TestBase(std::move(environment), false) { + + on().then([this] { + events.push_back("Startup"); + emit(std::make_unique()); + }); + + // Emit the first half of the pair + on, Sync>().then([this] { + events.push_back("First Half"); + emit(std::make_unique()); + // Wait for a bit to ensure that the second half checks this message/group + std::this_thread::sleep_for(std::chrono::milliseconds(100)); + }); + + // Won't run until the emit from First Half due to the lack of message b. + // However since First Half is sync with this one, this one can't run until it finishes so all threads will + // sleep + on, Trigger, Pool, Sync>().then([this] { + events.push_back("Second Half"); + powerplant.shutdown(); + }); + } +}; + +} // namespace + + +TEST_CASE("Test that if a pool has nothing to do because of a sync group it will recover", "[api][pool][group]") { + + NUClear::Configuration config; + config.thread_count = 1; + NUClear::PowerPlant plant(config); + plant.install(); + plant.start(); + + const std::vector expected = { + "Startup", + "First Half", + "Second Half", + }; + + // Make an info print the diff in an easy to read way if we fail + INFO(test_util::diff_string(expected, events)); + + // Check the events fired in order and only those events + REQUIRE(events == expected); +} From ed28f89795ac3fb4b721a4c75f321381716d5ea7 Mon Sep 17 00:00:00 2001 From: Trent Houliston Date: Thu, 20 Jun 2024 10:26:36 +1000 Subject: [PATCH 2/4] Improve the test to not rely on sleeping --- tests/dsl/GroupPool.cpp | 58 +++++++++++++++++++++++++---------------- 1 file changed, 36 insertions(+), 22 deletions(-) diff --git a/tests/dsl/GroupPool.cpp b/tests/dsl/GroupPool.cpp index 0a4fd63ce..7d54e27eb 100644 --- a/tests/dsl/GroupPool.cpp +++ b/tests/dsl/GroupPool.cpp @@ -30,35 +30,49 @@ namespace { /// @brief A vector of events that have happened std::vector events; // NOLINT(cppcoreguidelines-avoid-non-const-global-variables) -struct MessageA {}; -struct MessageB {}; +struct StartTest {}; +struct Synced {}; +template +struct PoolFinished {}; + +static constexpr int POOL_COUNT = 10; class TestReactor : public test_util::TestBase { public: - static constexpr int thread_count = 1; + template + struct TestPool { + static constexpr int thread_count = 1; + }; + + template + void register_callbacks(NUClear::util::Sequence) { + + NUClear::util::unpack(on, Pool>, Sync>().then([this] { + events.push_back("Pool Message"); + emit(std::make_unique>()); + })...); + + on>...>().then([this] { + events.push_back("Finished"); + powerplant.shutdown(); + }); + } TestReactor(std::unique_ptr environment) : TestBase(std::move(environment), false) { on().then([this] { events.push_back("Startup"); - emit(std::make_unique()); + emit(std::make_unique()); }); - // Emit the first half of the pair - on, Sync>().then([this] { - events.push_back("First Half"); - emit(std::make_unique()); - // Wait for a bit to ensure that the second half checks this message/group - std::this_thread::sleep_for(std::chrono::milliseconds(100)); + // Trigger on the start message and emit a message that will be synced + on, Sync>().then([this] { + events.push_back("Send Synced Message"); + emit(std::make_unique()); }); - // Won't run until the emit from First Half due to the lack of message b. - // However since First Half is sync with this one, this one can't run until it finishes so all threads will - // sleep - on, Trigger, Pool, Sync>().then([this] { - events.push_back("Second Half"); - powerplant.shutdown(); - }); + // Register all the callbacks and a final callback to gather the results + register_callbacks(NUClear::util::GenerateSequence<0, POOL_COUNT>()); } }; @@ -73,11 +87,11 @@ TEST_CASE("Test that if a pool has nothing to do because of a sync group it will plant.install(); plant.start(); - const std::vector expected = { - "Startup", - "First Half", - "Second Half", - }; + std::vector expected = {"Send Synced Message"}; + for (int i = 0; i < POOL_COUNT; ++i) { + expected.push_back("Pool Message"); + } + expected.push_back("Finished"); // Make an info print the diff in an easy to read way if we fail INFO(test_util::diff_string(expected, events)); From 27d78524dc5214176bfceaba427ed2f8f7ef74ef Mon Sep 17 00:00:00 2001 From: Trent Houliston Date: Thu, 20 Jun 2024 10:33:37 +1000 Subject: [PATCH 3/4] Fix test --- tests/dsl/GroupPool.cpp | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/tests/dsl/GroupPool.cpp b/tests/dsl/GroupPool.cpp index 7d54e27eb..f8e017bdd 100644 --- a/tests/dsl/GroupPool.cpp +++ b/tests/dsl/GroupPool.cpp @@ -30,6 +30,13 @@ namespace { /// @brief A vector of events that have happened std::vector events; // NOLINT(cppcoreguidelines-avoid-non-const-global-variables) +/// @brief Add an event to the event list +void add_event(const std::string& event) { + static std::mutex events_mutex; + std::lock_guard lock(events_mutex); + events.push_back(event); +} + struct StartTest {}; struct Synced {}; template @@ -48,12 +55,12 @@ class TestReactor : public test_util::TestBase { void register_callbacks(NUClear::util::Sequence) { NUClear::util::unpack(on, Pool>, Sync>().then([this] { - events.push_back("Pool Message"); + add_event("Pool Message"); emit(std::make_unique>()); })...); on>...>().then([this] { - events.push_back("Finished"); + add_event("Finished"); powerplant.shutdown(); }); } @@ -61,13 +68,13 @@ class TestReactor : public test_util::TestBase { TestReactor(std::unique_ptr environment) : TestBase(std::move(environment), false) { on().then([this] { - events.push_back("Startup"); + add_event("Startup"); emit(std::make_unique()); }); // Trigger on the start message and emit a message that will be synced on, Sync>().then([this] { - events.push_back("Send Synced Message"); + add_event("Send Synced Message"); emit(std::make_unique()); }); @@ -87,7 +94,10 @@ TEST_CASE("Test that if a pool has nothing to do because of a sync group it will plant.install(); plant.start(); - std::vector expected = {"Send Synced Message"}; + std::vector expected = { + "Startup", + "Send Synced Message", + }; for (int i = 0; i < POOL_COUNT; ++i) { expected.push_back("Pool Message"); } From 14f689196476b83ed9b8c9cff38a1df4fb78d83c Mon Sep 17 00:00:00 2001 From: Trent Houliston Date: Wed, 21 Aug 2024 14:05:40 +1000 Subject: [PATCH 4/4] Fix --- tests/tests/dsl/GroupPool.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/tests/dsl/GroupPool.cpp b/tests/tests/dsl/GroupPool.cpp index f8e017bdd..09f18cb63 100644 --- a/tests/tests/dsl/GroupPool.cpp +++ b/tests/tests/dsl/GroupPool.cpp @@ -20,7 +20,7 @@ * OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. */ -#include +#include #include #include "test_util/TestBase.hpp" @@ -44,7 +44,7 @@ struct PoolFinished {}; static constexpr int POOL_COUNT = 10; -class TestReactor : public test_util::TestBase { +class TestReactor : public test_util::TestBase { public: template struct TestPool {