mw/com : Add the integration tests for moving skeletonMethod. by Tejveerpratap2803 · Pull Request #659 · eclipse-score/communication

Tejveerpratap2803 · 2026-07-07T07:38:53Z

Adding integration tests that verify a Skeleton (and its SkeletonMethod) can be move-constructed or move-assigned without disrupting method handler registration or invocation.

Issue : #483

bemerybmw · 2026-07-16T11:46:05Z

+
+void RunConsumer(const SkeletonMoveScenario& scenario, const score::cpp::stop_token& stop_token)
+{
+    auto proxy_done_sync = ProcessSynchronizer::Create(kProxyDoneShmPath);


We generally avoid abbreviations: proxy_done_process_synchronizer

Removed now we are using concurrency::Notification.

bemerybmw · 2026-08-10T13:25:42Z

+      "instances": [
+        {
+          "instanceId": 1,
+          "asil-level": "QM",


I think in general, we should use asil b by default unless there's a good reason not to. The most important thing is that our code works for asil b applications. Also, an asil b provider sets up infrastructure for both asil b and qm, so we're technically testing more code paths.

bemerybmw · 2026-08-10T13:29:22Z

+
+enum class SkeletonMoveScenario : std::uint8_t
+{
+    kMoveConstructBeforeOffered = 0,


No need to manually specify the numbers unless they're actually important e.g. if the counting starts from a non zero value.

bemerybmw · 2026-08-10T13:47:19Z

+
+    // Step 3. Move construct: Skeleton B = std::move(Skeleton A)  [before OfferService]
+    std::cout << "\nProvider: Step 3 - Move construct Skeleton B = std::move(Skeleton A)" << std::endl;
+    auto skeleton_b = std::move(skeleton_a);


What about moving once before RegisterHandlerOrFail and again here?

Refined the code

bemerybmw · 2026-08-10T13:52:35Z

+    }
+
+    // Step 3. For after-offered scenarios: signal provider that proxy/proxies are created
+    if (IsAfterOffered(scenario))


Having a single function and using these flags makes the test harder to read IMO. I think it's clearer to have separate functions for the different tests to make the sequences clear.

bemerybmw · 2026-08-10T14:03:02Z

+{
+    std::random_device rd{};
+    std::mt19937 gen{rd()};
+    std::uniform_int_distribution<int> dist{0, 500};


We shouldn't have a magic number here of 500. This value is very important for the test so should be a constant (probably defined in test_parameters).

bemerybmw · 2026-08-10T14:19:40Z

+    if (IsAfterOffered(scenario))
+    {
+        std::cout << "\nConsumer: Step 3 - Notify provider: proxy/proxies created" << std::endl;
+        proxy_done_sync->Notify();


After discussing with @limdor, we agreed that we shouldn't go with the approach of calling move in a loop which is more of a stress test. This minimises load in the ci and more importantly keeps the test sequence clearer i.e. that it is just testing one thing, i.e. what happens when we move the handler once. Since we have the nightly job which runs the test many times, we should get a lot of runs with different timings. Based on this and our discussion on Friday, I think that we should have 4 tests:

move construct before offer

move construct after offer (fuzzy)

move assign before offer

move assign after offer (fuzzy)

Before offer
The before offer tests should move the skeleton twice: before and after registering the handler (it then offers the service). After it detects that the first method handler was called (using a concurrency::Notification: it should notify a concurrency::Notification to indicate that the handler was called. the main thread of the test should wait on this notification), it should then register a new handler an once that handler is called, it can finish the test which destroys the skeleton / stop offers the service.

In the consumer, it should calling the first method. It should then keep calling the method until it gets a return value uniquely identifying the second handler and then can finish. If I haven't missed something, I think this sequence would not even require a ProcessSynchronizer for indicating to the skeleton when the consumer is done (since it gets this when the last registered handler is called).

After offer

The provider should offer the service and then sleep a random time and then move the skeleton. After this, it should register a new handler and repeat (similar to the before offer test). The consumer should behave the same as in the before offer test.

The "fuzzy" behaviour is basically part of the after offer tests.

bemerybmw · 2026-08-24T10:11:13Z

+
+/// \brief Returns the expected return value of moved_method_(kTestArgA, kTestArgB) for the first,
+///        deterministic call (always resolves to Handler A: a + b = 15).
+std::int32_t GetFirstHandlerExpectedResult(SkeletonMoveScenario scenario);


Why is this a function which takes SkeletonMoveScenario? It doesn't depend on the scenario so why not just make it a constant?

bemerybmw · 2026-08-24T10:13:16Z

+    std::int32_t actual = 0;
+    do
+    {
+        if (std::chrono::steady_clock::now() >= deadline)


This timeout is handled by the test infrastructure. I don't think we should also do a timeout check within the test itself (since it's redundant and also complicates the test sequence).

bemerybmw · 2026-08-24T10:19:05Z

+    const std::int32_t second_expected = GetSecondHandlerExpectedResult(scenario);
+    const auto deadline = std::chrono::steady_clock::now() + kDetectionWindow;
+    std::size_t call_count = 0U;
+    std::int32_t actual = 0;


We should not use 0 to represent an uninitialized value. We should instead use std::optionalstd::int32_t. You can then also change the do while loop to a while loop with the condition being while (!actual.has_value() || actual.value() != second_expected)

See my comment below, it would also simplify the condition.

bemerybmw · 2026-08-24T10:25:27Z

+            FailTest("Consumer: timed out waiting to observe the second handler's result");
+        }
+        actual = CallMethodOrFail(proxy_moved_to);
+        VerifyResultIsOneOf(actual, first_expected, second_expected, call_count);


I think that this function makes the test logic harder to understand. I would rather something like this to make the sequence clearer:

// If the first handler is still registered, then we continue calling the method if (actual == first_expected) { // sleep continue; } // If the second handler has been registered, then we can finish if (actual == second_expected) { break; } FailTest(true, ...);

What do you think?

bemerybmw · 2026-08-24T10:30:37Z

+    std::cout << "\nProvider: Step 5 - Move construct Skeleton B = std::move(Skeleton A)" << std::endl;
+    auto skeleton_b = std::move(skeleton_a);
+
+    // Step 6. Register Handler B on Skeleton B. Its body notifies notification_b when called.


We should wait for the first handler to be called at least once before registering the new one.

bemerybmw · 2026-08-24T10:50:31Z

+    // Step 5. Move-assign: Skeleton B = std::move(Skeleton A)  [already offered, random timing]
+    std::cout << "\nProvider: Step 5 - Move-assign Skeleton B = std::move(Skeleton A)" << std::endl;
+    skeleton_b = std::move(skeleton_a);
+


Wait for first handler to be called.

github-project-automation Bot added this to COM - Communication FT Jul 7, 2026

github-project-automation Bot moved this to Backlog in COM - Communication FT Jul 7, 2026

Tejveerpratap2803 force-pushed the tepr_skeleton_method_move_tests branch from 612b2cc to a5181ea Compare July 7, 2026 20:34

Tejveerpratap2803 changed the title ~~Tepr skeleton method move tests~~ mw/com : Add the integration tests for moving skeletonMethod. Jul 8, 2026

Tejveerpratap2803 marked this pull request as ready for review July 8, 2026 08:32

Tejveerpratap2803 requested review from LittleHuba, bemerybmw, castler, crimson11, hoe-jo and limdor as code owners July 8, 2026 08:32

anmittag assigned anmittag and Tejveerpratap2803 and unassigned anmittag Jul 21, 2026

anmittag moved this from Backlog to In Progress in COM - Communication FT Jul 21, 2026

Tejveerpratap2803 marked this pull request as draft July 27, 2026 11:13

Tejveerpratap2803 force-pushed the tepr_skeleton_method_move_tests branch 3 times, most recently from 27dc852 to d59adc0 Compare July 31, 2026 21:26

Tejveerpratap2803 marked this pull request as ready for review July 31, 2026 21:34

bemerybmw requested changes Aug 10, 2026

View reviewed changes

Tejveerpratap2803 force-pushed the tepr_skeleton_method_move_tests branch 2 times, most recently from 2f4d6d4 to 22287bc Compare August 16, 2026 16:19

Tejveerpratap2803 added 3 commits August 16, 2026 21:51

test(mw/com): Add interface, parameters and build config

93b85b6

test(mw/com): Add provider and consumer implementation

c1a7e61

test(mw/com): Add integration test scenarios

6c66546

Tejveerpratap2803 force-pushed the tepr_skeleton_method_move_tests branch from 22287bc to 6c66546 Compare August 16, 2026 16:22

Tejveerpratap2803 requested a review from bemerybmw August 16, 2026 16:46

bemerybmw requested changes Aug 24, 2026

View reviewed changes

Uh oh!

Conversation

Tejveerpratap2803 commented Jul 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tejveerpratap2803 Aug 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Tejveerpratap2803 commented Jul 7, 2026 •

edited

Loading

Tejveerpratap2803 Aug 16, 2026 •

edited

Loading