unit test for rpc client with transfer id map

Author: serges147

include/libcyphal/presentation/client_impl.hpp

@@ -252,7 +252,7 @@ class SharedClient : public common::cavl::Node<SharedClient>,
         if (auto* const transfer_id_map = delegate_.getTransferIdMap())
         {
             const SessionSpec session_spec{response_rx_params_.service_id, response_rx_params_.server_node_id};
-            transfer_id_map->setIdFor(next_transfer_id_, session_spec);
+            transfer_id_map->setIdFor(session_spec, next_transfer_id_);
         }
 
         delegate_.forgetSharedClient(*this);

include/libcyphal/presentation/publisher_impl.hpp

@@ -106,7 +106,7 @@ class PublisherImpl final : public common::cavl::Node<PublisherImpl>, public Sha
             if (const auto local_node_id = delegate_.getLocalNodeId())
             {
                 const SessionSpec session_spec{subject_id_, local_node_id.value()};
-                transfer_id_map->setIdFor(next_transfer_id_, session_spec);
+                transfer_id_map->setIdFor(session_spec, next_transfer_id_);
             }
         }
 

include/libcyphal/transport/transfer_id_map.hpp

@@ -63,7 +63,7 @@ class ITransferIdMap
     /// Depending on the implementation, the previously set transfer ids may be stored in memory
     /// or also persisted to a file (but probably on exit to fulfill the above performance expectations).
     ///
-    virtual void setIdFor(const TransferId transfer_id, const SessionSpec& session_spec) noexcept = 0;
+    virtual void setIdFor(const SessionSpec& session_spec, const TransferId transfer_id) noexcept = 0;
 
 protected:
     ITransferIdMap()  = default;
@@ -108,36 +108,6 @@ class ITransferIdStorage
 
 };  // ITransferIdStorage
 
-/// Default implementation of the transfer ID storage.
-///
-/// It just stores given transfer id in memory.
-/// In use by the Presentation layer entities as a default fallback behavior.
-///
-class DefaultTransferIdStorage final : public ITransferIdStorage
-{
-public:
-    explicit DefaultTransferIdStorage(const TransferId initial_transfer_id = 0) noexcept
-        : transfer_id_{initial_transfer_id}
-    {
-    }
-
-    // ITransferIdMap
-
-    TransferId load() const noexcept override
-    {
-        return transfer_id_;
-    }
-
-    void save(const TransferId transfer_id) noexcept override
-    {
-        transfer_id_ = transfer_id;
-    }
-
-private:
-    TransferId transfer_id_;
-
-};  // DefaultTransferIdStorage
-
 /// @brief Defines a trivial transfer ID generator.
 ///
 /// The generator is trivial in the sense that it simply increments the transfer ID.

test/unittest/presentation/test_client.cpp

@@ -11,6 +11,7 @@
 #include "tracking_memory_resource.hpp"
 #include "transport/scattered_buffer_storage_mock.hpp"
 #include "transport/svc_sessions_mock.hpp"
+#include "transport/transfer_id_map_mock.hpp"
 #include "transport/transport_gtest_helpers.hpp"
 #include "transport/transport_mock.hpp"
 #include "virtual_time_scheduler.hpp"
@@ -23,6 +24,7 @@
 #include <libcyphal/presentation/response_promise.hpp>
 #include <libcyphal/transport/errors.hpp>
 #include <libcyphal/transport/svc_sessions.hpp>
+#include <libcyphal/transport/transfer_id_map.hpp>
 #include <libcyphal/transport/types.hpp>
 #include <libcyphal/types.hpp>
 
@@ -789,7 +791,7 @@ TEST_F(TestClient, raw_request_response_failures)
     State state{mr_, transport_mock_, rx_params};
 
     // Emulate that transport supports only 2 concurrent transfers by having module equal to 2^1.
-    // This will make the client fail to make more than 2 request.
+    // This will make the client fail to make more than 2 requests.
     EXPECT_CALL(transport_mock_, getProtocolParams()).WillRepeatedly(Return(ProtocolParams{2, 0, 0}));
 
     Presentation presentation{mr_, scheduler_, transport_mock_};
@@ -829,6 +831,84 @@ TEST_F(TestClient, raw_request_response_failures)
     scheduler_.spinFor(10s);
 }
 
+TEST_F(TestClient, multiple_clients_with_transfer_id_map)
+{
+    using SvcResPromise = ResponsePromise<void>;
+    using SessionSpec   = ITransferIdMap::SessionSpec;
+
+    StrictMock<TransferIdMapMock> transfer_id_map_mock;
+
+    constexpr ResponseRxParams rx_params_n31{4, 147, 0x31};
+    constexpr ResponseRxParams rx_params_n32{4, 147, 0x32};
+
+    State state_31{mr_, transport_mock_, rx_params_n31};
+    State state_32{mr_, transport_mock_, rx_params_n32};
+
+    Presentation presentation{mr_, scheduler_, transport_mock_};
+    presentation.setTransferIdMap(&transfer_id_map_mock);
+
+    EXPECT_CALL(transfer_id_map_mock, getIdFor(SessionSpec{rx_params_n31.service_id, rx_params_n31.server_node_id}))
+        .WillOnce(Return(0x310));
+    EXPECT_CALL(transfer_id_map_mock, getIdFor(SessionSpec{rx_params_n32.service_id, rx_params_n32.server_node_id}))
+        .WillOnce(Return(0x320));
+
+    auto maybe_client_n31 = presentation.makeClient(  //
+        rx_params_n31.server_node_id,
+        rx_params_n31.service_id,
+        rx_params_n31.extent_bytes);
+    ASSERT_THAT(maybe_client_n31, VariantWith<RawServiceClient>(_));
+    cetl::optional<RawServiceClient> client_n31 = cetl::get<RawServiceClient>(std::move(maybe_client_n31));
+
+    auto maybe_client_n32 = presentation.makeClient(  //
+        rx_params_n32.server_node_id,
+        rx_params_n32.service_id,
+        rx_params_n32.extent_bytes);
+    ASSERT_THAT(maybe_client_n32, VariantWith<RawServiceClient>(_));
+    cetl::optional<RawServiceClient> client_n32 = cetl::get<RawServiceClient>(std::move(maybe_client_n32));
+
+    scheduler_.scheduleAt(1s, [&](const auto&) {
+        //
+        EXPECT_CALL(state_31.req_tx_session_mock_, send(_, _))  //
+            .WillOnce(Invoke([now = now()](const auto& metadata, const auto) {
+                //
+                EXPECT_THAT(metadata.base.transfer_id, 0x310);
+                return cetl::nullopt;
+            }));
+
+        const auto maybe_promise = client_n31->request(now() + 100ms, {}, now() + 2s);
+        EXPECT_THAT(maybe_promise, VariantWith<SvcResPromise>(_));
+    });
+    scheduler_.scheduleAt(2s, [&](const auto&) {
+        //
+        EXPECT_CALL(state_32.req_tx_session_mock_, send(_, _))  //
+            .WillOnce(Invoke([now = now()](const auto& metadata, const auto) {
+                //
+                EXPECT_THAT(metadata.base.transfer_id, 0x320);
+                return cetl::nullopt;
+            }));
+
+        const auto maybe_promise = client_n32->request(now() + 100ms, {}, now() + 2s);
+        EXPECT_THAT(maybe_promise, VariantWith<SvcResPromise>(_));
+    });
+    scheduler_.scheduleAt(8s, [&](const auto&) {
+        //
+        EXPECT_CALL(transfer_id_map_mock,
+                    setIdFor(SessionSpec{rx_params_n32.service_id, rx_params_n32.server_node_id}, 0x320 + 1))
+            .WillOnce(Return());
+
+        client_n32.reset();
+    });
+    scheduler_.scheduleAt(9s, [&](const auto&) {
+        //
+        EXPECT_CALL(transfer_id_map_mock,
+                    setIdFor(SessionSpec{rx_params_n31.service_id, rx_params_n31.server_node_id}, 0x310 + 1))
+            .WillOnce(Return());
+
+        client_n31.reset();
+    });
+    scheduler_.spinFor(10s);
+}
+
 // NOLINTEND(cppcoreguidelines-avoid-magic-numbers, readability-magic-numbers, *-function-cognitive-complexity)
 
 }  // namespace

test/unittest/transport/test_transfer_id_map.cpp

@@ -27,7 +27,34 @@ using testing::StrictMock;
 // NOLINTBEGIN(cppcoreguidelines-avoid-magic-numbers, readability-magic-numbers)
 
 class TestTransferIdMapAndGens : public testing::Test
-{};
+{
+protected:
+    class TransferIdStorage final : public detail::ITransferIdStorage
+    {
+    public:
+        explicit TransferIdStorage(const TransferId initial_transfer_id = 0) noexcept
+            : transfer_id_{initial_transfer_id}
+        {
+        }
+
+        // detail::ITransferIdStorage
+
+        TransferId load() const noexcept override
+        {
+            return transfer_id_;
+        }
+
+        void save(const TransferId transfer_id) noexcept override
+        {
+            transfer_id_ = transfer_id;
+        }
+
+    private:
+        TransferId transfer_id_;
+
+    };  // TransferIdStorage
+
+};  // TestTransferIdMapAndGens
 
 // MARK: - Tests:
 
@@ -77,11 +104,11 @@ TEST_F(TestTransferIdMapAndGens, trivial_max_tf_id)
 
 TEST_F(TestTransferIdMapAndGens, small_range_with_default_map)
 {
-    detail::DefaultTransferIdStorage default_transfer_id_storage{9};
+    TransferIdStorage transfer_id_storage{9};  // on purpose bigger than modulo 4
 
-    detail::SmallRangeTransferIdGenerator<8> tf_id_gen{4, default_transfer_id_storage};
+    detail::SmallRangeTransferIdGenerator<8> tf_id_gen{4, transfer_id_storage};
 
-    EXPECT_THAT(tf_id_gen.nextTransferId(), Optional(1));
+    EXPECT_THAT(tf_id_gen.nextTransferId(), Optional(1));  // 9 % 4 -> 1
     EXPECT_THAT(tf_id_gen.nextTransferId(), Optional(2));
     EXPECT_THAT(tf_id_gen.nextTransferId(), Optional(3));
     EXPECT_THAT(tf_id_gen.nextTransferId(), Optional(0));

test/unittest/transport/transfer_id_map_mock.hpp

@@ -0,0 +1,37 @@
+/// @copyright
+/// Copyright (C) OpenCyphal Development Team  <opencyphal.org>
+/// Copyright Amazon.com Inc. or its affiliates.
+/// SPDX-License-Identifier: MIT
+
+#ifndef LIBCYPHAL_TRANSPORT_TRANSFER_ID_MAP_MOCK_HPP_INCLUDED
+#define LIBCYPHAL_TRANSPORT_TRANSFER_ID_MAP_MOCK_HPP_INCLUDED
+
+#include <libcyphal/transport/transfer_id_map.hpp>
+
+#include <gmock/gmock.h>
+
+namespace libcyphal
+{
+namespace transport
+{
+
+class TransferIdMapMock : public ITransferIdMap
+{
+public:
+    TransferIdMapMock()          = default;
+    virtual ~TransferIdMapMock() = default;
+
+    TransferIdMapMock(const TransferIdMapMock&)                = delete;
+    TransferIdMapMock(TransferIdMapMock&&) noexcept            = delete;
+    TransferIdMapMock& operator=(const TransferIdMapMock&)     = delete;
+    TransferIdMapMock& operator=(TransferIdMapMock&&) noexcept = delete;
+
+    MOCK_METHOD(TransferId, getIdFor, (const SessionSpec& session_spec), (const, noexcept, override));
+    MOCK_METHOD(void, setIdFor, (const SessionSpec& session_spec, const TransferId transfer_id), (noexcept, override));
+
+};  // TransferIdMapMock
+
+}  // namespace transport
+}  // namespace libcyphal
+
+#endif  // LIBCYPHAL_TRANSPORT_TRANSFER_ID_MAP_MOCK_HPP_INCLUDED