Reduce the number of globals in tests (#137)

There are many globals in the unit tests for handling event lists.

Since reactors are now returned from install, these globals can instead
be members of the reactors.

It also removes the anonymous namespaces since each of the tests are
built separately and they no longer collide.

src/dsl/word/Pool.hpp

@@ -49,7 +49,7 @@ namespace dsl {
             }
 
             template <typename, typename... A>
-            static constexpr bool check(A&&... /*unused*/) {
+            static constexpr bool check(const A&... /*unused*/) {
                 return true;
             }
 

tests/CMakeLists.txt

@@ -27,6 +27,7 @@ FetchContent_Declare(
   SYSTEM DOWNLOAD_EXTRACT_TIMESTAMP TRUE
   URL https://github.com/catchorg/Catch2/archive/refs/tags/v3.6.0.tar.gz
 )
+set(CATCH_CONFIG_CONSOLE_WIDTH 120)
 FetchContent_MakeAvailable(Catch2)
 
 # Silence clang-tidy warnings from catch2

tests/networktest.cpp

@@ -26,7 +26,6 @@
 #include <iostream>
 #include <nuclear>
 
-namespace {
 struct PerformEmits {};
 
 using NUClear::message::CommandLineArguments;
@@ -119,8 +118,6 @@ class TestReactor : public NUClear::Reactor {
         });
     }
 };
-}  // namespace
-
 
 // NOLINTNEXTLINE(bugprone-exception-escape)
 int main(int argc, const char* argv[]) {

tests/tests/api/ReactionHandle.cpp

@@ -25,23 +25,18 @@
 
 #include "test_util/TestBase.hpp"
 
-// Anonymous namespace to keep everything file local
-namespace {
-
-/// Events that occur during the test
-std::vector<std::string> events;  // NOLINT(cppcoreguidelines-avoid-non-const-global-variables)
-
-struct Message {
-    Message(int i) : i(i) {}
-    int i;
-};
 
 class TestReactor : public test_util::TestBase<TestReactor> {
 public:
+    struct Message {
+        Message(int i) : i(i) {}
+        int i;
+    };
+
     TestReactor(std::unique_ptr<NUClear::Environment> environment) : TestBase(std::move(environment)) {
 
         // Make an always disabled reaction
-        a = on<Trigger<Message>, Priority::HIGH>().then([](const Message& msg) {  //
+        a = on<Trigger<Message>, Priority::HIGH>().then([this](const Message& msg) {  //
             events.push_back("Executed disabled reaction " + std::to_string(msg.i));
         });
         a.disable();
@@ -53,7 +48,7 @@ class TestReactor : public test_util::TestBase<TestReactor> {
             emit(std::make_unique<Message>(1));
         });
 
-        on<Trigger<Message>>().then([](const Message& msg) {  //
+        on<Trigger<Message>>().then([this](const Message& msg) {  //
             events.push_back("Executed enabled reaction " + std::to_string(msg.i));
         });
 
@@ -65,14 +60,17 @@ class TestReactor : public test_util::TestBase<TestReactor> {
 
     ReactionHandle a;
     ReactionHandle b;
+
+    /// Events that occur during the test
+    std::vector<std::string> events;
 };
-}  // namespace
+
 
 TEST_CASE("Testing reaction handle functionality", "[api][reactionhandle]") {
     NUClear::Configuration config;
     config.thread_count = 1;
     NUClear::PowerPlant plant(config);
-    plant.install<TestReactor>();
+    const auto& reactor = plant.install<TestReactor>();
     plant.start();
 
     const std::vector<std::string> expected = {
@@ -82,8 +80,8 @@ TEST_CASE("Testing reaction handle functionality", "[api][reactionhandle]") {
     };
 
     // Make an info print the diff in an easy to read way if we fail
-    INFO(test_util::diff_string(expected, events));
+    INFO(test_util::diff_string(expected, reactor.events));
 
     // Check the events fired in order and only those events
-    REQUIRE(events == expected);
+    REQUIRE(reactor.events == expected);
 }

tests/tests/api/ReactionStatistics.cpp

@@ -26,19 +26,15 @@
 #include "test_util/TestBase.hpp"
 
 // This namespace is named to make things consistent with the reaction statistics test
-namespace stats_test {
-
-/// Events that occur during the test
-std::vector<std::string> events;  // NOLINT(cppcoreguidelines-avoid-non-const-global-variables)
-
-template <int id>
-struct Message {};
-struct LoopMessage {};
 
 using NUClear::message::ReactionEvent;
 
 class TestReactor : public test_util::TestBase<TestReactor> {
 public:
+    template <int id>
+    struct Message {};
+    struct LoopMessage {};
+
     TestReactor(std::unique_ptr<NUClear::Environment> environment) : TestBase(std::move(environment)) {
 
         // This reaction is here to emit something from a ReactionStatistics trigger
@@ -77,7 +73,7 @@ class TestReactor : public test_util::TestBase<TestReactor> {
             }
         });
 
-        on<Trigger<Message<1>>>().then("Exception Handler", [] {
+        on<Trigger<Message<1>>>().then("Exception Handler", [this] {
             events.push_back("Running Exception Handler");
             throw std::runtime_error("Text in an exception");
         });
@@ -92,33 +88,36 @@ class TestReactor : public test_util::TestBase<TestReactor> {
             emit(std::make_unique<Message<0>>());
         });
     }
+
+    /// Events that occur during the test
+    std::vector<std::string> events;
 };
-}  // namespace stats_test
+
 
 TEST_CASE("Testing reaction statistics functionality", "[api][reactionstatistics]") {
 
     NUClear::Configuration config;
     config.thread_count = 1;
     NUClear::PowerPlant plant(config);
-    plant.install<stats_test::TestReactor>();
+    const auto& reactor = plant.install<TestReactor>();
     plant.start();
 
     const std::vector<std::string> expected = {
         "Running Startup Handler",
-        "Stats for Startup Handler from stats_test::TestReactor",
+        "Stats for Startup Handler from TestReactor",
         "NUClear::Reactor::on<NUClear::dsl::word::Startup>",
         "Running Message Handler",
-        "Stats for Message Handler from stats_test::TestReactor",
-        "NUClear::Reactor::on<NUClear::dsl::word::Trigger<stats_test::Message<0>>>",
+        "Stats for Message Handler from TestReactor",
+        "NUClear::Reactor::on<NUClear::dsl::word::Trigger<TestReactor::Message<0>>>",
         "Running Exception Handler",
-        "Stats for Exception Handler from stats_test::TestReactor",
-        "NUClear::Reactor::on<NUClear::dsl::word::Trigger<stats_test::Message<1>>>",
+        "Stats for Exception Handler from TestReactor",
+        "NUClear::Reactor::on<NUClear::dsl::word::Trigger<TestReactor::Message<1>>>",
         "Exception received: \"Text in an exception\"",
     };
 
     // Make an info print the diff in an easy to read way if we fail
-    INFO(test_util::diff_string(expected, stats_test::events));
+    INFO(test_util::diff_string(expected, reactor.events));
 
     // Check the events fired in order and only those events
-    REQUIRE(stats_test::events == expected);
+    REQUIRE(reactor.events == expected);
 }

tests/tests/api/ReactionStatisticsTiming.cpp

@@ -26,9 +26,6 @@
 #include "test_util/TestBase.hpp"
 #include "test_util/TimeUnit.hpp"
 
-// Anonymous namespace to keep everything file local
-namespace {
-
 using TimeUnit = test_util::TimeUnit;
 
 /// Events that occur during the test and the time they occur
@@ -118,7 +115,7 @@ class TestReactor : public test_util::TestBase<TestReactor> {
         });
     }
 };
-}  // namespace
+
 
 TEST_CASE("Testing reaction statistics timing", "[api][reactionstatistics][timing]") {
 

tests/tests/api/ReactorArgs.cpp

@@ -23,9 +23,6 @@
 #include <catch2/catch_test_macros.hpp>
 #include <nuclear>
 
-// Anonymous namespace to keep everything file local
-namespace {
-
 class TestReactorNoArgs : public NUClear::Reactor {
 public:
     TestReactorNoArgs(std::unique_ptr<NUClear::Environment> environment) : NUClear::Reactor(std::move(environment)) {}
@@ -44,7 +41,6 @@ class TestReactorArgs : public NUClear::Reactor {
     uint32_t i{0};
 };
 
-}  // namespace
 
 TEST_CASE("Testing Reactor installation arguments", "[api][reactorargs]") {
     NUClear::Configuration config;

tests/tests/api/TimeTravel.cpp

@@ -5,8 +5,6 @@
 #include "test_util/TestBase.hpp"
 #include "test_util/TimeUnit.hpp"
 
-namespace {
-
 using TimeUnit = test_util::TimeUnit;
 
 constexpr int64_t EVENT_1_TIME = 4;
@@ -73,7 +71,6 @@ class TestReactor : public test_util::TestBase<TestReactor> {
     Results results;
 };
 
-}  // anonymous namespace
 
 TEST_CASE("Test time travel correctly changes the time for non zero rtf", "[time_travel][chrono_controller]") {
 

tests/tests/api/TimeTravelFrozen.cpp

@@ -5,8 +5,6 @@
 
 #include "test_util/TestBase.hpp"
 
-namespace {
-
 constexpr std::chrono::milliseconds EVENT_1_TIME  = std::chrono::milliseconds(4);
 constexpr std::chrono::milliseconds EVENT_2_TIME  = std::chrono::milliseconds(8);
 constexpr std::chrono::milliseconds SHUTDOWN_TIME = std::chrono::milliseconds(12);
@@ -67,7 +65,6 @@ class TestReactor : public test_util::TestBase<TestReactor> {
     std::vector<std::string> events;
 };
 
-}  // anonymous namespace
 
 TEST_CASE("Test time travel correctly changes the time for non zero rtf", "[time_travel][chrono_controller]") {
 
@@ -112,5 +109,5 @@ TEST_CASE("Test time travel correctly changes the time for non zero rtf", "[time
     }
 
     INFO(test_util::diff_string(expected, reactor.events));
-    CHECK(expected == reactor.events);
+    CHECK(reactor.events == expected);
 }

tests/tests/dsl/Always.cpp

@@ -25,15 +25,9 @@
 
 #include "test_util/TestBase.hpp"
 
-namespace {
-
-/// Events that occur during the test
-std::vector<std::string> events;  // NOLINT(cppcoreguidelines-avoid-non-const-global-variables)
-
-struct SimpleMessage {};
-
 class TestReactor : public test_util::TestBase<TestReactor> {
 public:
+    struct SimpleMessage {};
     TestReactor(std::unique_ptr<NUClear::Environment> environment) : TestBase(std::move(environment), false) {
 
         on<Always>().then([this] {
@@ -56,15 +50,18 @@ class TestReactor : public test_util::TestBase<TestReactor> {
 
     /// Counter for the number of times we have run
     int i = 0;
+
+    /// Events that occur during the test
+    std::vector<std::string> events;
 };
-}  // namespace
+
 
 TEST_CASE("The Always DSL keyword runs continuously when it can", "[api][always]") {
 
     NUClear::Configuration config;
     config.thread_count = 1;
     NUClear::PowerPlant plant(config);
-    plant.install<TestReactor>();
+    auto& reactor = plant.install<TestReactor>();
     plant.start();
 
     const std::vector<std::string> expected = {
@@ -82,8 +79,8 @@ TEST_CASE("The Always DSL keyword runs continuously when it can", "[api][always]
     };
 
     // Make an info print the diff in an easy to read way if we fail
-    INFO(test_util::diff_string(expected, events));
+    INFO(test_util::diff_string(expected, reactor.events));
 
     // Check the events fired in order and only those events
-    REQUIRE(events == expected);
+    REQUIRE(reactor.events == expected);
 }

tests/tests/dsl/ArgumentFission.cpp

@@ -26,8 +26,6 @@
 
 #include "test_util/TestBase.hpp"
 
-namespace {
-
 /// Events that occur during the test
 std::vector<std::string> events;  // NOLINT(cppcoreguidelines-avoid-non-const-global-variables)
 
@@ -84,7 +82,7 @@ class TestReactor : public test_util::TestBase<TestReactor> {
         events.push_back("Bind3 returned " + c);
     }
 };
-}  // namespace
+
 
 TEST_CASE("Testing distributing arguments to multiple bind functions (NUClear Fission)", "[api][dsl][fission]") {
 

tests/tests/dsl/BlockNoData.cpp

@@ -25,16 +25,11 @@
 
 #include "test_util/TestBase.hpp"
 
-namespace {
-
-/// Events that occur during the test
-std::vector<std::string> events;  // NOLINT(cppcoreguidelines-avoid-non-const-global-variables)
-
-struct MessageA {};
-struct MessageB {};
-
 class TestReactor : public test_util::TestBase<TestReactor> {
 public:
+    struct MessageA {};
+    struct MessageB {};
+
     TestReactor(std::unique_ptr<NUClear::Environment> environment) : TestBase(std::move(environment)) {
 
         on<Trigger<MessageA>>().then([this] {
@@ -43,11 +38,11 @@ class TestReactor : public test_util::TestBase<TestReactor> {
             emit(std::make_unique<MessageB>());
         });
 
-        on<Trigger<MessageA>, With<MessageB>>().then([] {  //
+        on<Trigger<MessageA>, With<MessageB>>().then([this] {  //
             events.push_back("MessageA with MessageB triggered");
         });
 
-        on<Trigger<MessageB>, With<MessageA>>().then([] {  //
+        on<Trigger<MessageB>, With<MessageA>>().then([this] {  //
             events.push_back("MessageB with MessageA triggered");
         });
 
@@ -61,16 +56,18 @@ class TestReactor : public test_util::TestBase<TestReactor> {
             emit(std::make_unique<Step<1>>());
         });
     }
+
+    /// Events that occur during the test
+    std::vector<std::string> events;
 };
-}  // namespace
 
 
 TEST_CASE("Testing that when an on statement does not have it's data satisfied it does not run", "[api][nodata]") {
 
     NUClear::Configuration config;
     config.thread_count = 1;
     NUClear::PowerPlant plant(config);
-    plant.install<TestReactor>();
+    const auto& reactor = plant.install<TestReactor>();
     plant.start();
 
     const std::vector<std::string> expected = {
@@ -81,8 +78,8 @@ TEST_CASE("Testing that when an on statement does not have it's data satisfied i
     };
 
     // Make an info print the diff in an easy to read way if we fail
-    INFO(test_util::diff_string(expected, events));
+    INFO(test_util::diff_string(expected, reactor.events));
 
     // Check the events fired in order and only those events
-    REQUIRE(events == expected);
+    REQUIRE(reactor.events == expected);
 }