Fix flaky tests (#840)

* Seems better

* Converting all tests

* Adding new API

* Splitting test_helper

* formatting

* Adding unit tests & moving to use CVs

Author: MaciejKaszynski

score/launch_manager/src/lm_control/BUILD

@@ -34,6 +34,7 @@ cc_library(
     deps = [
         "//score/launch_manager:error",
         "//score/launch_manager/src/daemon/src/common:log",
+        "@score_baselibs//score/concurrency:condition_variable",
         "@score_baselibs//score/language/futurecpp",
         "@score_baselibs//score/language/safecpp/scoped_function:copyable_scoped_function",
         "@score_baselibs//score/language/safecpp/scoped_function:scope",

score/launch_manager/src/lm_control/src/details/lm_control_impl.hpp

@@ -13,6 +13,8 @@
 #ifndef SCORE_MW_LIFECYCLE_LM_CONTROL_IMPL_HPP
 #define SCORE_MW_LIFECYCLE_LM_CONTROL_IMPL_HPP
 
+#include <chrono>
+#include <condition_variable>
 #include <cstddef>
 #include <exception>
 #include <memory>
@@ -22,6 +24,7 @@
 #include <string_view>
 #include <utility>
 
+#include "score/concurrency/condition_variable.h"
 #include "score/language/safecpp/scoped_function/copyable_scoped_function.h"
 #include "score/language/safecpp/scoped_function/scope.h"
 #include "score/mw/launch_manager/common/log.hpp"
@@ -245,6 +248,20 @@ class BasicLmControlImpl final : public ILmControl
         return response.run_target;
     }
 
+    score::Result<void> wait_for_connection(std::chrono::milliseconds timeout, const score::cpp::stop_token& stop_token)
+        override
+    {
+        std::unique_lock<std::mutex> lock{mutex_};
+        const bool connected = connection_cv_.wait_for(lock, stop_token, timeout, [this] {
+            return proxy_.has_value();
+        });
+        if (!connected)
+        {
+            return score::MakeUnexpected(ExecErrc::kCommunicationError);
+        }
+        return {};
+    }
+
   private:
     using ProxyType = typename Traits::Proxy;
     using HandleType = typename Traits::HandleType;
@@ -282,9 +299,9 @@ class BasicLmControlImpl final : public ILmControl
             return;
         }
 
-        if (createProxy(handles.front()))
+        if (createProxy(handles.front()) && subscribeToActivationResults())
         {
-            static_cast<void>(subscribeToActivationResults());
+            connection_cv_.notify_all();
         }
     }
 
@@ -430,6 +447,8 @@ class BasicLmControlImpl final : public ILmControl
 
     // Guards proxy_
     std::mutex mutex_;
+    // Signalled (under mutex_) once proxy_ is set up, wakes wait_for_connection()
+    concurrency::InterruptibleConditionalVariableBasic<std::mutex, std::condition_variable> connection_cv_;
     std::optional<ProxyType> proxy_;
 
     // Handle of the running discovery search, stopped by the destructor. Written once by init()

score/launch_manager/src/lm_control/src/details/lm_control_impl_UT.cpp

@@ -15,6 +15,7 @@
 #include <gtest/gtest.h>
 
 #include <algorithm>
+#include <chrono>
 #include <cstddef>
 #include <functional>
 #include <memory>
@@ -391,6 +392,42 @@ TEST_F(LmControlUT, DiscoveryCreatesProxySubscribesAndSetsHandler)
     EXPECT_TRUE(receive_handler_);
 }
 
+TEST_F(LmControlUT, WaitForConnectionTimesOut)
+{
+    RecordProperty(
+        "Description",
+        "wait_for_connection returns kCommunicationError on timeout and when stopped via the stop token, and "
+        "succeeds once the service has been discovered.");
+
+    auto impl = MakeLmControl();
+    ILmControl* const sut = impl.get();
+
+    auto timed_out = sut->wait_for_connection(std::chrono::milliseconds(1));
+    ASSERT_FALSE(timed_out.has_value());
+    EXPECT_EQ(timed_out.error(), ExecErrc::kCommunicationError);
+}
+
+TEST_F(LmControlUT, WaitForConnectionStops)
+{
+    auto impl = MakeLmControl();
+    ILmControl* const sut = impl.get();
+
+    score::cpp::stop_source stop_source;
+    stop_source.request_stop();
+    auto stopped = sut->wait_for_connection(std::chrono::hours(1), stop_source.get_token());
+    ASSERT_FALSE(stopped.has_value());
+    EXPECT_EQ(stopped.error(), ExecErrc::kCommunicationError);
+}
+
+TEST_F(LmControlUT, WaitForConnectionWorks)
+{
+    auto impl = MakeLmControl();
+    ILmControl* const sut = impl.get();
+
+    ConnectService();
+    EXPECT_TRUE(sut->wait_for_connection(std::chrono::milliseconds(1)).has_value());
+}
+
 TEST_F(LmControlUT, EmptyHandlesAreIgnored)
 {
     RecordProperty(

score/launch_manager/src/lm_control/src/ilm_control.hpp

@@ -13,6 +13,7 @@
 #ifndef SCORE_MW_LIFECYCLE_ILM_CONTROL_H_
 #define SCORE_MW_LIFECYCLE_ILM_CONTROL_H_
 
+#include <chrono>
 #include <functional>
 #include <memory>
 
@@ -21,6 +22,8 @@
 #include "score/mw/lifecycle/run_target_activation_source.hpp"
 #include "score/result/result.h"
 
+#include <score/stop_token.hpp>
+
 namespace score::mw::lifecycle
 {
 
@@ -95,6 +98,23 @@ class ILmControl
     /// @error kCommunicationError The service discovery could not be started.
     static score::Result<std::unique_ptr<ILmControl>> Create(std::string_view instance_specifier);
 
+    /// @brief Blocks until the client is connected to the Launch Manager.
+    ///
+    /// Sleeps on a condition variable until service discovery has produced a connected
+    /// proxy, the timeout expires, or a stop is requested via the stop token.
+    ///
+    /// @param[in] timeout     Maximum time to wait for the connection.
+    /// @param[in] stop_token  Allows another thread to abort the wait early. A default
+    ///                        constructed token can never be stopped.
+    ///
+    /// @returns void once connected.
+    ///
+    /// @error kCommunicationError The connection was not established within the timeout,
+    ///                            or the wait was stopped via the stop token.
+    virtual score::Result<void> wait_for_connection(
+        std::chrono::milliseconds timeout = std::chrono::milliseconds(500),
+        const score::cpp::stop_token& stop_token = score::cpp::stop_token{}) = 0;
+
     /// @brief Virtual destructor for safe deletion through this interface.
     virtual ~ILmControl() noexcept = default;
 

tests/integration/complex_monitoring/component_complex_monitoring.cpp

@@ -15,7 +15,7 @@
 
 #include "score/mw/lifecycle/report_running.h"
 #include "score/mw/log/rust/stdout_logger_init.h"
-#include "tests/utils/test_helper/test_helper.hpp"
+#include "tests/utils/test_helper/test_runner.hpp"
 #include <score/mw/health/common.h>
 #include <score/mw/health/health_monitor.h>
 
@@ -59,14 +59,15 @@ TEST(ComplexMonitoring, ComponentComplexMonitoring)
             std::this_thread::sleep_for(75ms);
             heartbeat_monitor.Heartbeat();
         }
-        EXPECT_FALSE(TestRunner::exitRequested) << "Process should not be terminated yet";
+        EXPECT_FALSE(TestRunner::g_exit_requested) << "Process should not be terminated yet";
     }
     // When heartbeats are no longer sent...
 }
 
 int main()
 {
-    TestRunner(__FILE__, TerminationBehavior::kContinue).RunTests();
+    TestRunner::Init();
+    TestRunner::RunTests();
     // Then expect kill due to recovery action (verified by state manager)
     while (true)  // Stop reporting, wait for sigkill
     {

tests/integration/complex_monitoring/control_client_test_driver.cpp

@@ -14,6 +14,7 @@
 #include <unistd.h>
 
 #include "tests/utils/test_helper/test_helper.hpp"
+#include "tests/utils/test_helper/test_runner.hpp"
 #include <score/mw/lifecycle/ilm_control.hpp>
 #include <score/mw/lifecycle/report_running.h>
 
@@ -79,15 +80,13 @@ TEST(ComplexMonitoring, ControlClientTestDriver)
         // did not just send an event without taking the action.
         EXPECT_TRUE(std::filesystem::exists(fallback_file)) << "Fallback Run Target was not activated";
     }
-
-    TEST_STEP("Activate Off Run Target")
-    {
-        const auto result = client->activate_run_target("Off", true);
-        EXPECT_TRUE(result.has_value()) << result.error().Message();
-    }
 }
 
 int main()
 {
-    return TestRunner(__FILE__, TerminationBehavior::kWait, TerminationNotification::kTestEnd).RunTests();
+    TestRunner::Init();
+    const auto result = TestRunner::RunTests();
+    TestRunner::EndTest();
+    TestRunner::WaitForTermination();
+    return result;
 }

tests/integration/crash_ignores_dependents/process_crashing_once.cpp

@@ -13,6 +13,7 @@
 #include <gtest/gtest.h>
 
 #include "tests/utils/test_helper/test_helper.hpp"
+#include "tests/utils/test_helper/test_runner.hpp"
 #include <score/mw/lifecycle/report_running.h>
 
 TEST(CrashIgnoresDependents, CrashingProcess)
@@ -46,5 +47,6 @@ TEST(CrashIgnoresDependents, CrashingProcess)
 
 int main()
 {
-    TestRunner(__FILE__, TerminationBehavior::kContinue).RunTests();
+    TestRunner::Init();
+    TestRunner::RunTests();
 }

tests/integration/crash_ignores_dependents/test_process.cpp

@@ -13,6 +13,7 @@
 #include <gtest/gtest.h>
 
 #include "tests/utils/test_helper/test_helper.hpp"
+#include "tests/utils/test_helper/test_runner.hpp"
 #include <fcntl.h>
 #include <score/mw/lifecycle/report_running.h>
 #include <chrono>
@@ -52,5 +53,8 @@ TEST(CrashIgnoresDependents, TestProcess)
 
 int main()
 {
-    return TestRunner(__FILE__, TerminationBehavior::kContinue, TerminationNotification::kTestEnd).RunTests();
+    TestRunner::Init();
+    const auto result = TestRunner::RunTests();
+    TestRunner::EndTest();
+    return result;
 }

tests/integration/crash_on_startup/control_client_test_driver.cpp

@@ -14,6 +14,7 @@
 #include <filesystem>
 
 #include "tests/utils/test_helper/test_helper.hpp"
+#include "tests/utils/test_helper/test_runner.hpp"
 #include <score/mw/lifecycle/ilm_control.hpp>
 #include <score/mw/lifecycle/report_running.h>
 
@@ -100,15 +101,13 @@ TEST(CrashOnStartup, ControlClientTestDriver)
         // did not just send an event without taking the action.
         EXPECT_TRUE(std::filesystem::exists(fallback_file)) << "Fallback Run Target should have been activated";
     }
-
-    TEST_STEP("Activate Run Target Off")
-    {
-        const auto result = client->activate_run_target("Off", true);
-        EXPECT_TRUE(result.has_value()) << result.error().Message();
-    }
 }
 
 int main()
 {
-    return TestRunner(__FILE__, TerminationBehavior::kWait, TerminationNotification::kTestEnd).RunTests();
+    TestRunner::Init();
+    const auto result = TestRunner::RunTests();
+    TestRunner::EndTest();
+    TestRunner::WaitForTermination();
+    return result;
 }

tests/integration/explicit_fallback_request/control_client_test_driver.cpp

@@ -13,6 +13,7 @@
 #include <gtest/gtest.h>
 
 #include "tests/utils/test_helper/test_helper.hpp"
+#include "tests/utils/test_helper/test_runner.hpp"
 #include <score/mw/lifecycle/ilm_control.hpp>
 #include <score/mw/lifecycle/report_running.h>
 
@@ -62,15 +63,13 @@ TEST(ExplicitFallbackRequest, ControlClientTestDriver)
             EXPECT_EQ(target, "fallback_run_target");
         }
     });
-
-    TEST_STEP("Activate Run Target Off")
-    {
-        const auto result = client->activate_run_target("Off", true);
-        EXPECT_TRUE(result.has_value()) << result.error().Message();
-    }
 }
 
 int main()
 {
-    return TestRunner(__FILE__, TerminationBehavior::kWait, TerminationNotification::kTestEnd).RunTests();
+    TestRunner::Init();
+    const auto result = TestRunner::RunTests();
+    TestRunner::EndTest();
+    TestRunner::WaitForTermination();
+    return result;
 }