From 8871185d22df7919dfab10d0f937c62a4e2b480f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marius=20B=C3=B6rschig?= Date: Thu, 30 Jul 2026 09:20:24 +0200 Subject: [PATCH 1/2] system and monitor tests, failing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Marius Börschig --- .../services/orchestration/CMakeLists.txt | 5 + .../orchestration/Test_SystemMonitor.cpp | 148 ++++++++ .../orchestration/Test_SystemStateTracker.cpp | 336 ++++++++++++++++++ 3 files changed, 489 insertions(+) create mode 100644 SilKit/source/services/orchestration/Test_SystemStateTracker.cpp diff --git a/SilKit/source/services/orchestration/CMakeLists.txt b/SilKit/source/services/orchestration/CMakeLists.txt index 722890b9f..d91747fc6 100644 --- a/SilKit/source/services/orchestration/CMakeLists.txt +++ b/SilKit/source/services/orchestration/CMakeLists.txt @@ -33,6 +33,7 @@ add_library(O_SilKit_Services_Orchestration OBJECT TimeConfiguration.hpp TimeConfiguration.cpp + SystemStateTracker.hpp SystemStateTracker.cpp ) @@ -52,6 +53,10 @@ add_silkit_test_to_executable(SilKitUnitTests SOURCES Test_SystemMonitor.cpp LIBS S_SilKitImpl I_SilKit ) +add_silkit_test_to_executable(SilKitUnitTests + SOURCES Test_SystemStateTracker.cpp + LIBS S_SilKitImpl I_SilKit +) add_silkit_test_to_executable(SilKitUnitTests SOURCES Test_WatchDog.cpp LIBS S_SilKitImpl diff --git a/SilKit/source/services/orchestration/Test_SystemMonitor.cpp b/SilKit/source/services/orchestration/Test_SystemMonitor.cpp index 00773a030..00fca5a8b 100644 --- a/SilKit/source/services/orchestration/Test_SystemMonitor.cpp +++ b/SilKit/source/services/orchestration/Test_SystemMonitor.cpp @@ -7,6 +7,7 @@ #include #include #include +#include #include "gtest/gtest.h" #include "gmock/gmock.h" @@ -767,4 +768,151 @@ TEST_F(Test_SystemMonitor, add_and_remove_system_state_and_participant_status_ha EXPECT_EQ(monitor.InvalidTransitionCount(), 0u); } +// ================================================================================================ +// Participants running at different speeds +// +// The tests above always advance P1, P2 and P3 by one state at a time, in ascending order. Without +// virtual time synchronization there is no back pressure between participants, so under load one +// participant can run several states ahead of another. The tests below cover that regime. +// +// All of them are single-threaded and deterministic - no threads, no sleeps, no timeouts and no +// wall-clock reads - so they behave identically on a heavily loaded CI machine. +// ================================================================================================ + +/*! Intermediate system states must not be skipped. + * + * The SystemState handler is fed from an aggregate that is only sampled when a ParticipantStatus + * message arrives, but it is consumed as a sequence of transitions. If P1 races ahead to Stopping + * before P2 reports Running at all, the aggregate jumps straight from ReadyToRun to Stopping and + * SystemState::Running is never emitted. + * + * This is not only an observability problem: LifecycleService::NewSystemState uses + * SystemState::Stopping as the trigger that makes a coordinated participant stop. Unlike the + * startup transitions - which are backed by the participant-replies and pending-subscription + * barriers - the stop path has no barrier behind it, so a skipped state is a missed trigger. + */ +TEST_F(Test_SystemMonitor, intermediate_system_states_are_not_skipped) +{ + monitor.UpdateRequiredParticipantNames({"P1", "P2"}); + + std::vector observedStates; + monitor.AddSystemStateHandler([&observedStates](SystemState systemState) { + observedStates.push_back(systemState); + }); + + // Both participants reach ReadyToRun in lock-step. + for (const auto state : {ParticipantState::ServicesCreated, ParticipantState::CommunicationInitializing, + ParticipantState::CommunicationInitialized, ParticipantState::ReadyToRun}) + { + SetParticipantStatus(1, state); + SetParticipantStatus(2, state); + } + ASSERT_EQ(monitor.SystemState(), SystemState::ReadyToRun); + + // P1 runs ahead: it starts, finishes its work and stops before P2 reports Running. + SetParticipantStatus(1, ParticipantState::Running); + SetParticipantStatus(1, ParticipantState::Stopping); + SetParticipantStatus(2, ParticipantState::Running); + + EXPECT_THAT(observedStates, Contains(SystemState::Running)) + << "both participants were running, but SystemState::Running was never reported"; +} + +/*! A system state change that happens while a handler is being registered must not be lost. + * + * SystemMonitor::AddSystemStateHandler first invokes the handler with the current system state and + * only then adds it to the handler list. A change delivered in between is dispatched to a list that + * does not yet contain the new handler, and is lost for good: the aggregate only changes when a + * participant status changes, so nothing will re-deliver it. + * + * In production the two statements are separated by the IO worker thread; here the change is + * injected from inside the initial invocation, which exercises the very same code path and the very + * same lost notification without depending on thread timing. There is no deadlock risk: + * AddSystemStateHandler invokes the handler outside the SynchronizedHandlers lock, and the + * reentrant InvokeAll takes a recursive_mutex over an empty handler list. + * + * LifecycleService::StartLifecycle registers its handlers from the user thread, so the lost + * notification can be a coordinated participant's own startup or stop trigger. + */ +TEST_F(Test_SystemMonitor, system_state_change_during_handler_registration_is_not_lost) +{ + monitor.UpdateRequiredParticipantNames({"P1", "P2"}); + + for (const auto state : {ParticipantState::ServicesCreated, ParticipantState::CommunicationInitializing, + ParticipantState::CommunicationInitialized, ParticipantState::ReadyToRun}) + { + SetParticipantStatus(1, state); + SetParticipantStatus(2, state); + } + SetParticipantStatus(1, ParticipantState::Running); + ASSERT_EQ(monitor.SystemState(), SystemState::ReadyToRun); + + std::vector observedStates; + bool isFirstInvocation{true}; + + monitor.AddSystemStateHandler([this, &observedStates, &isFirstInvocation](SystemState systemState) { + observedStates.push_back(systemState); + + if (isFirstInvocation) + { + isFirstInvocation = false; + // The system state changes to Running while AddSystemStateHandler sits between invoking + // this handler and registering it. + SetParticipantStatus(2, ParticipantState::Running); + } + }); + + ASSERT_EQ(monitor.SystemState(), SystemState::Running); + EXPECT_THAT(observedStates, Contains(SystemState::Running)); + ASSERT_FALSE(observedStates.empty()); + EXPECT_EQ(observedStates.back(), monitor.SystemState()) + << "the handler's last observed system state must not lag behind the monitor"; +} + +/*! ParticipantStatus() must not hand out a reference to storage that keeps changing. + * + * SystemMonitor::ParticipantStatus returns the pointer produced by + * SystemStateTracker::GetParticipantStatus, which is taken after the tracker's mutex has already + * been released. The returned reference therefore aliases the live map value. + * + * This test only observes the benign half of the problem - the value changes underneath the caller + * while the map node is still alive. The real hazard is worse: SetParticipantStatus assigns the + * std::string members while a caller may be reading them, and RemoveParticipant erases the node + * outright when a participant disconnects, leaving a dangling reference. + * + * The fix pattern already exists in this directory: LifecycleService::Status() copies under lock + * into a 'mutable ParticipantStatus _returnValueForStatus', and SystemStateTracker already offers + * the copying overload GetParticipantStatus(name, ParticipantStatus&). + */ +TEST_F(Test_SystemMonitor, participant_status_must_not_alias_mutable_storage) +{ + SetParticipantStatus(1, ParticipantState::ServicesCreated); + + const auto& participantStatus = monitor.ParticipantStatus("P1"); + ASSERT_EQ(participantStatus.state, ParticipantState::ServicesCreated); + + SetParticipantStatus(1, ParticipantState::CommunicationInitializing); + + EXPECT_EQ(participantStatus.state, ParticipantState::ServicesCreated) + << "the previously returned ParticipantStatus changed when an unrelated update arrived"; +} + +/*! Invalid participant state transitions must be counted. + * + * SystemMonitor::InvalidTransitionCount() reads _invalidTransitionCount, which is never written + * anywhere. The detection moved into SystemStateTracker::ValidateParticipantStateUpdate, which only + * logs and deliberately lets the transition through, and the count was never wired back up. + * + * As a result every 'EXPECT_EQ(monitor.InvalidTransitionCount(), 0u)' in this file - there are 27 - + * cannot fail. Do not read them as evidence that no invalid transition was detected. + */ +TEST_F(Test_SystemMonitor, invalid_participant_transition_is_counted) +{ + // Invalid -> Running: only ServicesCreated is a valid successor of Invalid. + SetParticipantStatus(1, ParticipantState::Running); + ASSERT_EQ(monitor.ParticipantStatus("P1").state, ParticipantState::Running); + + EXPECT_EQ(monitor.InvalidTransitionCount(), 1u); +} + } // anonymous namespace diff --git a/SilKit/source/services/orchestration/Test_SystemStateTracker.cpp b/SilKit/source/services/orchestration/Test_SystemStateTracker.cpp new file mode 100644 index 000000000..6e1f7683c --- /dev/null +++ b/SilKit/source/services/orchestration/Test_SystemStateTracker.cpp @@ -0,0 +1,336 @@ +// SPDX-FileCopyrightText: 2026 Vector Informatik GmbH +// +// SPDX-License-Identifier: MIT + +#include "services/orchestration/SystemStateTracker.hpp" + +#include +#include +#include +#include +#include +#include + +#include "gtest/gtest.h" +#include "gmock/gmock.h" + +#include "silkit/services/orchestration/string_utils.hpp" + +namespace { + +using namespace testing; + +using SilKit::Services::Orchestration::ParticipantState; +using SilKit::Services::Orchestration::ParticipantStatus; +using SilKit::Services::Orchestration::SystemState; + +using VSilKit::SystemStateTracker; + +// The states a participant passes through on its way to Running. Used as a common prelude so that +// the interesting part of each test starts from a well-defined SystemState. +const std::vector startupStates{ + ParticipantState::ServicesCreated, ParticipantState::CommunicationInitializing, + ParticipantState::CommunicationInitialized, ParticipantState::ReadyToRun, ParticipantState::Running}; + +//! A single participant status update, i.e. one received ParticipantStatus message. +struct Update +{ + std::string participantName; + ParticipantState state; +}; + +//! Thin, timing-free wrapper around the tracker. No clock is read: enterTime and refreshTime are +//! left default-constructed, so every test below is fully deterministic. +class Tracker +{ +public: + void Require(const std::vector& participantNames) + { + _tracker.UpdateRequiredParticipants(participantNames); + } + + void Send(const std::string& participantName, ParticipantState state) + { + ParticipantStatus status{}; + status.participantName = participantName; + status.state = state; + + _tracker.UpdateParticipantStatus(status); + } + + void Send(const Update& update) + { + Send(update.participantName, update.state); + } + + void Send(const std::vector& updates) + { + for (const auto& update : updates) + { + Send(update); + } + } + + //! Drive a participant through ServicesCreated .. Running. + void SendStartup(const std::string& participantName) + { + for (const auto state : startupStates) + { + Send(participantName, state); + } + } + + void Remove(const std::string& participantName) + { + _tracker.RemoveParticipant(participantName); + } + + auto State() const -> SystemState + { + return _tracker.GetSystemState(); + } + +private: + SystemStateTracker _tracker; +}; + +auto FormatUpdates(const std::vector& updates) -> std::string +{ + std::stringstream ss; + bool isFirst{true}; + for (const auto& update : updates) + { + ss << (isFirst ? "" : ", ") << update.participantName << "->" << update.state; + isFirst = false; + } + return ss.str(); +} + +/*! Enumerate every interleaving of two per-participant update sequences that preserves the order + * within each sequence. + * + * This models what participants actually observe: SIL Kit guarantees ordering per peer, but there + * is no global order across peers. Two participants therefore legitimately receive the very same + * set of status messages in different orders. + */ +auto MakeInterleavings(const std::vector& first, const std::vector& second) + -> std::vector> +{ + // 'selection' marks, for each slot of the merged sequence, whether it is taken from 'first'. + std::vector selection(first.size() + second.size(), false); + std::fill(selection.begin(), selection.begin() + static_cast(first.size()), true); + std::sort(selection.begin(), selection.end()); + + std::vector> interleavings; + + do + { + std::vector interleaving; + interleaving.reserve(selection.size()); + + size_t firstIndex{0}; + size_t secondIndex{0}; + + for (const bool takeFromFirst : selection) + { + interleaving.emplace_back(takeFromFirst ? first.at(firstIndex++) : second.at(secondIndex++)); + } + + interleavings.emplace_back(std::move(interleaving)); + } while (std::next_permutation(selection.begin(), selection.end())); + + return interleavings; +} + +// ================================================================================================ +// The SystemState must be a function of the participant states, not of their arrival order. +// ================================================================================================ + +/*! The resulting SystemState must not depend on the order in which the ParticipantStatus messages + * arrive. + * + * SystemStateTracker::ComputeSystemState switches on *which* participant moved and *to what*, and + * keeps the previous SystemState whenever no rule matches. The aggregate is therefore a function + * of the arrival order, not of the participant states - so two SystemMonitors observing the same + * participants can end up reporting different SystemStates, permanently. + * + * Without virtual time synchronization there is no back pressure between participants, so a fast + * participant genuinely does run several states ahead of a slow one under load. + * + * This test deliberately asserts *agreement* between all interleavings rather than one particular + * SystemState: it pins down the defect without prescribing a specific aggregation rule. + */ +TEST(Test_SystemStateTracker, system_state_must_not_depend_on_arrival_order) +{ + // 'A' runs ahead and has already stopped by the time 'B' reports Running. + std::vector updatesOfA; + for (const auto state : startupStates) + { + updatesOfA.push_back({"A", state}); + } + updatesOfA.push_back({"A", ParticipantState::Stopping}); + updatesOfA.push_back({"A", ParticipantState::Stopped}); + + std::vector updatesOfB; + for (const auto state : startupStates) + { + updatesOfB.push_back({"B", state}); + } + + const auto interleavings = MakeInterleavings(updatesOfA, updatesOfB); + ASSERT_FALSE(interleavings.empty()); + + // Every interleaving ends in the same participant states, so every interleaving must end in the + // same SystemState. Collect one witnessing interleaving per distinct outcome. + std::map> witnessByState; + + for (const auto& interleaving : interleavings) + { + Tracker tracker; + tracker.Require({"A", "B"}); + tracker.Send(interleaving); + + witnessByState.emplace(tracker.State(), interleaving); + } + + std::stringstream outcomes; + for (const auto& kv : witnessByState) + { + outcomes << "\n SystemState::" << kv.first << " e.g. via [" << FormatUpdates(kv.second) << "]"; + } + + EXPECT_EQ(witnessByState.size(), 1u) + << interleavings.size() << " interleavings of the same " << (updatesOfA.size() + updatesOfB.size()) + << " participant status updates produced " << witnessByState.size() << " different system states:" + << outcomes.str(); +} + +/*! Minimal, readable witness for the interleaving property above. + * + * Both trackers end up with the exact same participant states - A is Stopped, B is Running - but + * report different SystemStates: + * + * | arrival order | trace | result today | + * | ------------------------------------------------ | ----------------------------------------------------------------------------------------------------------- | -------------- | + * | A->Running, A->Stopping, A->Stopped, B->Running | no rule matches while B lags behind at ReadyToRun; B->Running then matches PS::Running's {Running, Stopped} | SS::Stopped | + * | B->Running, A->Running, A->Stopping, A->Stopped | SS::Running, then SS::Stopping; PS::Stopped requires all of {Stopped, ShuttingDown, Shutdown}, so B=Running | SS::Stopping | + * | | blocks the last step and the stale SS::Stopping is retained | | + */ +TEST(Test_SystemStateTracker, same_participant_states_yield_same_system_state) +{ + Tracker aheadFirst; + { + aheadFirst.Require({"A", "B"}); + + // Bring both participants to ReadyToRun in lock-step. + for (const auto state : startupStates) + { + if (state == ParticipantState::Running) + { + break; + } + aheadFirst.Send("A", state); + aheadFirst.Send("B", state); + } + ASSERT_EQ(aheadFirst.State(), SystemState::ReadyToRun); + + aheadFirst.Send({{"A", ParticipantState::Running}, + {"A", ParticipantState::Stopping}, + {"A", ParticipantState::Stopped}, + {"B", ParticipantState::Running}}); + } + + Tracker laggingFirst; + { + laggingFirst.Require({"A", "B"}); + + for (const auto state : startupStates) + { + if (state == ParticipantState::Running) + { + break; + } + laggingFirst.Send("A", state); + laggingFirst.Send("B", state); + } + ASSERT_EQ(laggingFirst.State(), SystemState::ReadyToRun); + + laggingFirst.Send({{"B", ParticipantState::Running}, + {"A", ParticipantState::Running}, + {"A", ParticipantState::Stopping}, + {"A", ParticipantState::Stopped}}); + } + + EXPECT_EQ(aheadFirst.State(), laggingFirst.State()) + << "both trackers saw A=Stopped and B=Running, but disagree on the system state"; +} + +// ================================================================================================ +// Required participants with unknown or removed status +// ================================================================================================ + +/*! A required participant whose state is unknown must not leave the system reported as Running. + * + * SystemStateTracker::GetAnyRequiredParticipantState() picks *_requiredParticipants.begin(), i.e. + * an arbitrary element of an unordered_set, and uses it as the "who moved" input of + * ComputeSystemState. The choice is unrelated to what actually happened. + * + * This fails for either possible pick, so the test does not depend on hash order: + * - picking "A" (Running): every ChangeToIfAllIn rule fails because B has no status, so the + * previous SystemState is retained; + * - picking "B" (no status, hence Invalid): 'case PS::Invalid' breaks out without recomputing. + */ +TEST(Test_SystemStateTracker, unknown_required_participant_must_not_report_running) +{ + Tracker tracker; + tracker.Require({"A"}); + tracker.SendStartup("A"); + ASSERT_EQ(tracker.State(), SystemState::Running); + + // 'B' joins the set of required participants but has never reported a status. + tracker.Require({"A", "B"}); + + EXPECT_NE(tracker.State(), SystemState::Running) + << "the state of required participant 'B' is unknown, so the system cannot be running"; +} + +/*! The SystemState must keep progressing after a participant has been removed. + * + * RemoveParticipant erases the participant from the status cache, but the const + * GetParticipantStatus does not re-insert a default. ChangeToIfAllIn therefore returns false on + * the removed participant for every subsequent update, and the SystemState is pinned to whatever + * it happened to be at the time of removal. The IsEmpty() escape hatch never triggers, because + * the remaining participant is still cached. + * + * Here the tracker freezes at SS::Stopping - the value it took when A reported Stopping while B + * was still Running - and never reaches SS::Shutdown, even though both required participants have + * reported Shutdown. + */ +TEST(Test_SystemStateTracker, system_state_must_still_progress_after_participant_removal) +{ + Tracker tracker; + tracker.Require({"A", "B"}); + + tracker.SendStartup("A"); + tracker.SendStartup("B"); + ASSERT_EQ(tracker.State(), SystemState::Running); + + tracker.Send({{"A", ParticipantState::Stopping}, + {"A", ParticipantState::Stopped}, + {"A", ParticipantState::ShuttingDown}, + {"A", ParticipantState::Shutdown}}); + + // 'A' shut down gracefully and disconnected. + tracker.Remove("A"); + + tracker.Send({{"B", ParticipantState::Stopping}, + {"B", ParticipantState::Stopped}, + {"B", ParticipantState::ShuttingDown}, + {"B", ParticipantState::Shutdown}}); + + EXPECT_EQ(tracker.State(), SystemState::Shutdown) + << "both required participants reported Shutdown, but the system state froze at the value it " + "had when 'A' was removed"; +} + +} // anonymous namespace From 5b64a26ea399f44105dc4b8c23c70d7ccb1fdd61 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marius=20B=C3=B6rschig?= Date: Thu, 30 Jul 2026 10:48:58 +0200 Subject: [PATCH 2/2] fix disabled tests in systemmonitor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Marius Börschig --- .../orchestration/Test_SystemMonitor.cpp | 116 ++++++++---------- 1 file changed, 53 insertions(+), 63 deletions(-) diff --git a/SilKit/source/services/orchestration/Test_SystemMonitor.cpp b/SilKit/source/services/orchestration/Test_SystemMonitor.cpp index 00fca5a8b..fcd289f00 100644 --- a/SilKit/source/services/orchestration/Test_SystemMonitor.cpp +++ b/SilKit/source/services/orchestration/Test_SystemMonitor.cpp @@ -297,24 +297,6 @@ TEST_F(Test_SystemMonitor, detect_multiple_paused_clients) EXPECT_EQ(monitor.InvalidTransitionCount(), 0u); } -TEST_F(Test_SystemMonitor, DISABLED_detect_system_stopping) -{ - SetAllParticipantStates(ParticipantState::ServicesCreated); - SetAllParticipantStates(ParticipantState::CommunicationInitializing); - SetAllParticipantStates(ParticipantState::CommunicationInitialized); - SetAllParticipantStates(ParticipantState::ReadyToRun); - SetAllParticipantStates(ParticipantState::Running); - EXPECT_EQ(monitor.SystemState(), SystemState::Running); - - AddSystemStateHandler(); - EXPECT_CALL(callbacks, SystemStateHandler(SystemState::Stopping)).Times(1); - - SetParticipantStatus(1, ParticipantState::Stopping); - EXPECT_EQ(monitor.ParticipantStatus("P1").state, ParticipantState::Stopping); - EXPECT_EQ(monitor.SystemState(), SystemState::Stopping); - EXPECT_EQ(monitor.InvalidTransitionCount(), 0u); -} - TEST_F(Test_SystemMonitor, detect_system_stopped) { SetAllParticipantStates(ParticipantState::ServicesCreated); @@ -354,20 +336,6 @@ TEST_F(Test_SystemMonitor, detect_system_stopped) EXPECT_EQ(monitor.InvalidTransitionCount(), 0u); } -TEST_F(Test_SystemMonitor, DISABLED_detect_reinitializing_after_stopped) -{ - SetAllParticipantStates(ParticipantState::ServicesCreated); - SetAllParticipantStates(ParticipantState::CommunicationInitializing); - SetAllParticipantStates(ParticipantState::CommunicationInitialized); - SetAllParticipantStates(ParticipantState::ReadyToRun); - SetAllParticipantStates(ParticipantState::Running); - SetAllParticipantStates(ParticipantState::Stopped); - EXPECT_EQ(monitor.SystemState(), SystemState::Stopped); - - AddSystemStateHandler(); - EXPECT_EQ(monitor.InvalidTransitionCount(), 0u); -} - TEST_F(Test_SystemMonitor, detect_controllers_com_initialized_after_stopped) { SetAllParticipantStates(ParticipantState::ServicesCreated); @@ -620,24 +588,6 @@ TEST_F(Test_SystemMonitor, detect_error_from_shuttingdown) EXPECT_EQ(monitor.InvalidTransitionCount(), 0u); } -TEST_F(Test_SystemMonitor, DISABLED_detect_initializing_after_error) -{ - SetAllParticipantStates(ParticipantState::ServicesCreated); - EXPECT_EQ(monitor.SystemState(), SystemState::ServicesCreated); - - SetParticipantStatus(1, ParticipantState::Error); - EXPECT_EQ(monitor.ParticipantStatus("P1").state, ParticipantState::Error); - EXPECT_EQ(monitor.SystemState(), SystemState::Error); - - AddSystemStateHandler(); - - SetParticipantStatus(1, ParticipantState::ServicesCreated); - EXPECT_EQ(monitor.ParticipantStatus("P1").state, ParticipantState::ServicesCreated); - EXPECT_EQ(monitor.SystemState(), SystemState::ServicesCreated); - - EXPECT_EQ(monitor.InvalidTransitionCount(), 0u); -} - TEST_F(Test_SystemMonitor, detect_shuttingdown_after_error) { SetAllParticipantStates(ParticipantState::ServicesCreated); @@ -657,34 +607,69 @@ TEST_F(Test_SystemMonitor, detect_shuttingdown_after_error) EXPECT_EQ(monitor.InvalidTransitionCount(), 0u); } -TEST_F(Test_SystemMonitor, DISABLED_detect_initializing_after_invalid) +/*! The system state must stay Invalid until every required participant has reported, and must then + * follow the least advanced one. + * + * Test that the monitor recovers from seemingly erroneous state transitions. + * + * Due to the distributed nature, it can occur that some participants + * have not matched yet, while others are already fully connected. This can lead + * to one participant already starting initalization while the other not having yet + * connected to the (local) participant, which is seemingly a wrong state transition + * as the whole system is not idle yet. The SystemMonitor must be able to recover + * from such erroneous state transitions. + * + * Was DISABLED_detect_initializing_after_invalid: the VIB-807 state machine rework (2022) disabled + * it with 'TODO why would this be an error? (CommunicationReady used to be initializing)', and that + * TODO was later dropped by "fix remove todos (#382)". The TODO was right - the old + * CommunicationReady had been mapped onto CommunicationInitializing here but onto + * CommunicationInitialized everywhere else, which left the test expecting the system state to jump + * to the *most advanced* participant. The scenario is worth covering; only the expectations were + * wrong, and they have been corrected below. + */ +TEST_F(Test_SystemMonitor, detect_system_state_once_all_participants_reported) { - // Test that the monitor recovers from seemingly erroneous state transitions. - // - // Due to the distributed nature, it can occur that some participants - // have not matched yet, while others are already fully connected. This can lead - // to one participant already starting initalization while the other not having yet - // connected to the (local) participant, which is seemingly a wrong state transition - // as the whole system is not idle yet. The SystemMonitor must be able to recover - // from such erroneous state transitions. - + // P1 is already initializing while P2 and P3 have not reported at all yet. SetParticipantStatus(1, ParticipantState::ServicesCreated); SetParticipantStatus(1, ParticipantState::CommunicationInitializing); + // As long as a required participant is unaccounted for, there is no system state. EXPECT_EQ(monitor.SystemState(), SystemState::Invalid); AddSystemStateHandler(); + EXPECT_CALL(callbacks, SystemStateHandler(SystemState::ServicesCreated)).Times(1); EXPECT_CALL(callbacks, SystemStateHandler(SystemState::CommunicationInitializing)).Times(1); SetParticipantStatus(2, ParticipantState::ServicesCreated); + EXPECT_EQ(monitor.SystemState(), SystemState::Invalid); + + // With P3 the picture is complete. P2 and P3 are the laggards, so the system is ServicesCreated - + // it does not jump ahead to where P1 already is. SetParticipantStatus(3, ParticipantState::ServicesCreated); + EXPECT_EQ(monitor.SystemState(), SystemState::ServicesCreated); + + // Once the laggards catch up, the system state follows. + SetParticipantStatus(2, ParticipantState::CommunicationInitializing); + EXPECT_EQ(monitor.SystemState(), SystemState::ServicesCreated); + + SetParticipantStatus(3, ParticipantState::CommunicationInitializing); EXPECT_EQ(monitor.SystemState(), SystemState::CommunicationInitializing); } -TEST_F(Test_SystemMonitor, DISABLED_detect_initialized_after_invalid) +/*! The system state must track the single lagging participant all the way up the startup ladder. + * + * Same distributed-startup situation as above, but taken to the extreme: two participants run all + * the way to ReadyToRun while the third has not reported at all. Every step the laggard takes must + * move the system state with it, and the system state must never run ahead of it. + * + * Was DISABLED_detect_initialized_after_invalid, disabled by the VIB-807 state machine rework (2022) + * with 'TODO clarify the purpose of this test' (later dropped by "fix remove todos (#382)"). Its + * EXPECT_EQ assertions were in fact correct; what made it fail was a missing expectation for the + * ServicesCreated notification, which gmock reported as an unexpected call. The walk is now carried + * through to ReadyToRun so that the whole ladder is covered. + */ +TEST_F(Test_SystemMonitor, detect_system_state_follows_lagging_participant) { - // Test that the monitor recovers from seemingly erroneous state transitions. - SetParticipantStatus(1, ParticipantState::ServicesCreated); SetParticipantStatus(1, ParticipantState::CommunicationInitializing); SetParticipantStatus(1, ParticipantState::CommunicationInitialized); @@ -698,8 +683,10 @@ TEST_F(Test_SystemMonitor, DISABLED_detect_initialized_after_invalid) EXPECT_EQ(monitor.SystemState(), SystemState::Invalid); AddSystemStateHandler(); + EXPECT_CALL(callbacks, SystemStateHandler(SystemState::ServicesCreated)).Times(1); EXPECT_CALL(callbacks, SystemStateHandler(SystemState::CommunicationInitializing)).Times(1); EXPECT_CALL(callbacks, SystemStateHandler(SystemState::CommunicationInitialized)).Times(1); + EXPECT_CALL(callbacks, SystemStateHandler(SystemState::ReadyToRun)).Times(1); SetParticipantStatus(3, ParticipantState::ServicesCreated); EXPECT_EQ(monitor.SystemState(), SystemState::ServicesCreated); @@ -709,6 +696,9 @@ TEST_F(Test_SystemMonitor, DISABLED_detect_initialized_after_invalid) SetParticipantStatus(3, ParticipantState::CommunicationInitialized); EXPECT_EQ(monitor.SystemState(), SystemState::CommunicationInitialized); + + SetParticipantStatus(3, ParticipantState::ReadyToRun); + EXPECT_EQ(monitor.SystemState(), SystemState::ReadyToRun); } TEST_F(Test_SystemMonitor, check_on_partitipant_connected_triggers_callback)