iox-#415 Rename variable and test case name in ServiceRegistry_test and fix bug in ServiceDescription c'tor

iceoryx_posh/include/iceoryx_posh/internal/roudi/service_registry.hpp

@@ -55,10 +55,9 @@ class ServiceRegistry
     /// @return ServiceRegistryError, error wrapped in cxx::expected
     cxx::expected<Error> add(const capro::ServiceDescription& serviceDescription) noexcept;
 
-    /// @brief Removes given service description from registry
+    /// @brief Removes given service description from registry if service is found
     /// @param[in] serviceDescription, service to be removed
-    /// @return true, if service description was removed, false otherwise
-    bool remove(const capro::ServiceDescription& serviceDescription) noexcept;
+    void remove(const capro::ServiceDescription& serviceDescription) noexcept;
 
     /// @brief Removes given service description from registry
     /// @param[in] searchResult, reference to the vector which will be filled with the results

iceoryx_posh/source/capro/service_description.cpp

@@ -73,31 +73,26 @@ ServiceDescription::ServiceDescription(const cxx::Serialization& serial) noexcep
 {
     std::underlying_type<Scope>::type scope = 0;
     std::underlying_type<Interfaces>::type interfaceSource = 0;
-    serial.extract(m_serviceString,
-                   m_instanceString,
-                   m_eventString,
-                   m_classHash[0u],
-                   m_classHash[1u],
-                   m_classHash[2u],
-                   m_classHash[3u],
-                   scope,
-                   interfaceSource);
-    if (scope > static_cast<std::underlying_type<Scope>::type>(Scope::INVALID))
+    auto deserializationSuccessful = serial.extract(m_serviceString,
+                                                    m_instanceString,
+                                                    m_eventString,
+                                                    m_classHash[0u],
+                                                    m_classHash[1u],
+                                                    m_classHash[2u],
+                                                    m_classHash[3u],
+                                                    scope,
+                                                    interfaceSource);
+    if (!deserializationSuccessful || scope >= static_cast<std::underlying_type<Scope>::type>(Scope::INVALID)
+        || interfaceSource >= static_cast<std::underlying_type<Interfaces>::type>(Interfaces::INTERFACE_END))
     {
-        m_scope = Scope::INVALID;
-    }
-    else
-    {
-        m_scope = static_cast<Scope>(scope);
-    }
-    if (interfaceSource > static_cast<std::underlying_type<Interfaces>::type>(Interfaces::INTERFACE_END))
-    {
-        m_interfaceSource = Interfaces::INTERFACE_END;
-    }
-    else
-    {
-        m_interfaceSource = static_cast<Interfaces>(interfaceSource);
+        m_serviceString = iox::capro::InvalidIdString;
+        m_instanceString = iox::capro::InvalidIdString;
+        m_eventString = iox::capro::InvalidIdString;
+        return;
     }
+
+    m_scope = static_cast<Scope>(scope);
+    m_interfaceSource = static_cast<Interfaces>(interfaceSource);
 }
 
 ServiceDescription::ServiceDescription() noexcept

iceoryx_posh/source/popo/client_options.cpp

@@ -46,29 +46,15 @@ ClientOptions::deserialize(const cxx::Serialization& serialized) noexcept
                                                         responseQueueFullPolicy,
                                                         serverTooSlowPolicy);
 
-    if (!deserializationSuccessful)
+    if (!deserializationSuccessful
+        || responseQueueFullPolicy > static_cast<QueueFullPolicyUT>(QueueFullPolicy2::DISCARD_OLDEST_DATA)
+        || serverTooSlowPolicy > static_cast<ConsumerTooSlowPolicyUT>(ConsumerTooSlowPolicy::DISCARD_OLDEST_DATA))
     {
         return cxx::error<cxx::Serialization::Error>(cxx::Serialization::Error::DESERIALIZATION_FAILED);
     }
 
-    if (responseQueueFullPolicy > static_cast<QueueFullPolicyUT>(QueueFullPolicy2::DISCARD_OLDEST_DATA))
-    {
-        return cxx::error<cxx::Serialization::Error>(cxx::Serialization::Error::DESERIALIZATION_FAILED);
-    }
-    else
-    {
-        clientOptions.responseQueueFullPolicy = static_cast<QueueFullPolicy2>(responseQueueFullPolicy);
-    }
-
-    if (serverTooSlowPolicy > static_cast<ConsumerTooSlowPolicyUT>(ConsumerTooSlowPolicy::DISCARD_OLDEST_DATA))
-    {
-        return cxx::error<cxx::Serialization::Error>(cxx::Serialization::Error::DESERIALIZATION_FAILED);
-    }
-    else
-    {
-        clientOptions.serverTooSlowPolicy = static_cast<ConsumerTooSlowPolicy>(serverTooSlowPolicy);
-    }
-
+    clientOptions.responseQueueFullPolicy = static_cast<QueueFullPolicy2>(responseQueueFullPolicy);
+    clientOptions.serverTooSlowPolicy = static_cast<ConsumerTooSlowPolicy>(serverTooSlowPolicy);
     return cxx::success<ClientOptions>(clientOptions);
 }
 } // namespace popo

iceoryx_posh/source/roudi/service_registry.cpp

@@ -47,7 +47,7 @@ cxx::expected<ServiceRegistry::Error> ServiceRegistry::add(const capro::ServiceD
     return cxx::success<>();
 }
 
-bool ServiceRegistry::remove(const capro::ServiceDescription& serviceDescription) noexcept
+void ServiceRegistry::remove(const capro::ServiceDescription& serviceDescription) noexcept
 {
     bool removedElement{false};
     uint64_t index = 0U;
@@ -65,7 +65,7 @@ bool ServiceRegistry::remove(const capro::ServiceDescription& serviceDescription
             }
             else
             {
-                return true;
+                return;
             }
             // There can't be more than one element
             break;
@@ -96,8 +96,6 @@ bool ServiceRegistry::remove(const capro::ServiceDescription& serviceDescription
         removeIndexFromMap(m_serviceMap, index);
         removeIndexFromMap(m_instanceMap, index);
     }
-
-    return removedElement;
 }
 
 void ServiceRegistry::find(ServiceDescriptionVector_t& searchResult,

iceoryx_posh/test/moduletests/test_capro_service.cpp

@@ -164,7 +164,8 @@ TEST_F(ServiceDescription_test, ServiceDescriptionSerializationCreatesServiceDes
 /// @attention The purpose of the Serialization is not to be an alternative Constructor. It is intended to send/receive
 /// the ServiceDescription over communication protocols which transfers strings like the MessageQueue. The testcase is
 /// only intended to check the functionality by injecting the valus directly.
-TEST_F(ServiceDescription_test, ServiceDescriptionObjectInitialisationWithOutOfBoundaryScopeSetsTheScopeToInvalid)
+TEST_F(ServiceDescription_test,
+       ServiceDescriptionObjectInitialisationWithOutOfBoundaryScopeLeadsToInvalidServiceDescription)
 {
     ServiceDescription::ClassHash testHash = {14U, 28U, 42U, 56U};
     testService = "Service";
@@ -182,14 +183,14 @@ TEST_F(ServiceDescription_test, ServiceDescriptionObjectInitialisationWithOutOfB
 
     ServiceDescription serviceDescription1 = ServiceDescription(serialObj);
 
-    EXPECT_THAT(serviceDescription1.getScope(), Eq(Scope::INVALID));
+    EXPECT_FALSE(serviceDescription1.isValid());
 }
 
 /// @attention The purpose of the Serialization is not to be an alternative Constructor. It is intended to send/receive
 /// the ServiceDescription over communication protocols which transfers strings like the MessageQueue. The testcase is
 /// only intended to check the functionality by injecting the valus directly.
 TEST_F(ServiceDescription_test,
-       ServiceDescriptionObjectInitialisationWithOutOfBoundaryInterfaceSourceSetsTheInterfaceSourceToInterfaceEnd)
+       ServiceDescriptionObjectInitialisationWithOutOfBoundaryInterfaceSourceLeadsToInvalidServiceDescription)
 {
     ServiceDescription::ClassHash testHash = {17U, 34U, 51U, 68U};
     testService = "Service";
@@ -209,10 +210,19 @@ TEST_F(ServiceDescription_test,
 
     ServiceDescription serviceDescription1 = ServiceDescription(serialObj);
 
-    EXPECT_THAT(serviceDescription1.getSourceInterface(), Eq(Interfaces::INTERFACE_END));
+    EXPECT_FALSE(serviceDescription1.isValid());
+}
+
+TEST_F(ServiceDescription_test, ServiceDescriptionObjectInitialisationWithEmptyStringLeadsToInvalidServiceDescription)
+{
+    std::string emptyString;
+    iox::cxx::Serialization invalidSerialObj{emptyString};
+
+    ServiceDescription serviceDescription1 = ServiceDescription(invalidSerialObj);
+
+    EXPECT_FALSE(serviceDescription1.isValid());
 }
 
-/// @todo remove
 TEST_F(ServiceDescription_test, ServiceDescriptionDefaultCtorInitializesStringsToInvalidString)
 {
     ServiceDescription serviceDescription1 = ServiceDescription();
@@ -433,7 +443,6 @@ TEST_F(ServiceDescription_test, LessThanOperatorReturnsFalseIfEventStringOfFirst
     EXPECT_FALSE(serviceDescription1 < serviceDescription2);
 }
 
-/// @todo add new tests for service description?
 
 /// END SERVICEDESCRIPTION TESTS