[bugfix] registration logic to throw away samples fixed so it doesn't…

… throw away things if host_group_name is set. (#1807) Additionally run tests in different configuration combinations. (including host_group_name)
eclipse-ecal · Nov 26, 2024 · aa60793 · aa60793
1 parent 9df6cfc
commit aa60793
Show file tree

Hide file tree

Showing 10 changed files with 171 additions and 103 deletions.
diff --git a/ecal/core/src/config/builder/registration_attribute_builder.cpp b/ecal/core/src/config/builder/registration_attribute_builder.cpp
@@ -30,15 +30,8 @@ namespace eCAL
     attr.refresh           = reg_config_.registration_refresh;
     attr.network_enabled   = reg_config_.network_enabled;
     attr.loopback          = reg_config_.loopback;
-
-    if (reg_config_.host_group_name.empty())
-    {
-      attr.host_group_name = eCAL::Process::GetHostName();
-    }
-    else
-    {
-      attr.host_group_name = reg_config_.host_group_name;
-    }
+    attr.host_name         = eCAL::Process::GetHostName();
+    attr.host_group_name   = reg_config_.host_group_name;
 
     attr.process_id        = process_id_;
 

diff --git a/ecal/core/src/registration/config/attributes/registration_attributes.h b/ecal/core/src/registration/config/attributes/registration_attributes.h
@@ -57,6 +57,7 @@ namespace eCAL
       bool                      shm_enabled;
       bool                      udp_enabled;
       unsigned int              refresh;
+      std::string               host_name;
       std::string               host_group_name;
       int                       process_id;
 

diff --git a/ecal/core/src/registration/config/attributes/sample_applier_attributes.h b/ecal/core/src/registration/config/attributes/sample_applier_attributes.h
@@ -32,6 +32,7 @@ namespace eCAL
         bool        network_enabled;
         bool        loopback;
         std::string host_group_name;
+        std::string host_name;
         int         process_id;
       };
     }

diff --git a/ecal/core/src/registration/config/builder/sample_applier_attribute_builder.cpp b/ecal/core/src/registration/config/builder/sample_applier_attribute_builder.cpp
@@ -31,6 +31,7 @@ namespace eCAL
 
         sample_applier_attr.network_enabled = attr_.network_enabled;
         sample_applier_attr.loopback        = attr_.loopback;
+        sample_applier_attr.host_name       = attr_.host_name;
         sample_applier_attr.host_group_name = attr_.host_group_name;
         sample_applier_attr.process_id      = attr_.process_id;
 

diff --git a/ecal/core/src/registration/ecal_registration_sample_applier.cpp b/ecal/core/src/registration/ecal_registration_sample_applier.cpp
@@ -57,15 +57,42 @@ namespace eCAL
 
     bool CSampleApplier::IsHostGroupMember(const Registration::Sample& sample_) const
     {
-      std::string host_group_name;
-      const std::string host_name = sample_.identifier.host_name;
+      // When are we in the same hostgroup?
+      // Either we are on the same host
+      // Or the hgname attribute of the sample are identical
+
+      if (IsSameHost(sample_))
+        return true;
+
+      if (IsSameHostGroup(sample_))
+        return true;
+
+      return false;
+    }
+
+    bool CSampleApplier::IsSameProcess(const Registration::Sample& sample_) const
+    {
+      // is this actually sufficient? should we also check host_name?
+      const int32_t pid = sample_.identifier.process_id;
+      return pid == m_attributes.process_id;
+    }
+
+    bool CSampleApplier::IsSameHost(const Registration::Sample& sample_) const
+    {
+      const std::string& sample_host_name = sample_.identifier.host_name;
+      return (sample_host_name == m_attributes.host_name);
+    }
+
+    bool CSampleApplier::IsSameHostGroup(const Registration::Sample& sample_) const
+    {
+      std::string sample_host_group_name;
       switch (sample_.cmd_type)
       {
       case bct_reg_publisher:
       case bct_unreg_publisher:
       case bct_reg_subscriber:
       case bct_unreg_subscriber:
-        host_group_name = sample_.topic.hgname;
+        sample_host_group_name = sample_.topic.hgname;
         break;
       case bct_reg_service:
       case bct_unreg_service:
@@ -79,25 +106,14 @@ namespace eCAL
         break;
       }
 
-      const std::string& sample_host_group_name = host_group_name.empty() ? host_name : host_group_name;
-
-      if (sample_host_group_name.empty() || m_attributes.host_group_name.empty())
-        return false;
-      if (sample_host_group_name != m_attributes.host_group_name)
-        return false;
-
-      return true;
-    }
-
-    bool CSampleApplier::IsSameProcess(const Registration::Sample& sample_) const
-    {
-      // is this actually sufficient? should we also check host_name?
-      const int32_t pid = sample_.identifier.process_id;
-      return pid == m_attributes.process_id;
+      return (sample_host_group_name == m_attributes.host_group_name);
     }
 
     bool CSampleApplier::AcceptRegistrationSample(const Registration::Sample& sample_)
     {
+      // Under wich circumstances do we apply samples, so we can filter ahead of time
+      // otherwise we could apply them to
+
       // check if the sample is from the same host group
       if (IsHostGroupMember(sample_))
       {

diff --git a/ecal/core/src/registration/ecal_registration_sample_applier.h b/ecal/core/src/registration/ecal_registration_sample_applier.h
@@ -55,6 +55,9 @@ namespace eCAL
 
     private:
       bool IsSameProcess(const Registration::Sample& sample_) const;
+      bool IsSameHost(const Registration::Sample& sample_) const;
+      bool IsSameHostGroup(const Registration::Sample& sample_) const;
+
       bool IsHostGroupMember(const eCAL::Registration::Sample& sample_) const;
 
       bool AcceptRegistrationSample(const Registration::Sample& sample_);

diff --git a/ecal/tests/cpp/registration_test_public/src/registration_getclients.cpp b/ecal/tests/cpp/registration_test_public/src/registration_getclients.cpp
@@ -25,14 +25,33 @@ enum {
   CMN_MONITORING_TIMEOUT_MS   = (5000 + 100),
   CMN_REGISTRATION_REFRESH_MS = (1000)
 };
-TEST(core_cpp_registration_public, ClientExpiration)
+
+// struct to hold the test parameters
+struct ClientsTestParams
 {
-  // initialize eCAL API
-  eCAL::Initialize(0, nullptr, "core_cpp_registration_public");
+  eCAL::Configuration configuration;
+};
 
-  // enable loop back communication in the same process
-  eCAL::Util::EnableLoopback(true);
+// test class that accepts TestParams as a parameter
+class ClientsTestFixture : public ::testing::TestWithParam<ClientsTestParams>
+{
+protected:
+  void SetUp() override
+  {
+    // set configuration from the test parameters
+    auto params = GetParam();
+    eCAL::Initialize(params.configuration, "core_cpp_registration_public", eCAL::Init::All);
+    eCAL::Util::EnableLoopback(true);
+  }
 
+  void TearDown() override
+  {
+    // clean up
+    eCAL::Finalize();
+  }
+};
+TEST_P(ClientsTestFixture, ClientExpiration)
+{
   std::map<eCAL::Registration::SServiceMethod, eCAL::SServiceMethodInformation> client_info_map;
 
   // create simple client and let it expire
@@ -83,19 +102,10 @@ TEST(core_cpp_registration_public, ClientExpiration)
 
   // check size
   EXPECT_EQ(client_info_map.size(), 0);
-
-  // finalize eCAL API
-  eCAL::Finalize();
 }
 
-TEST(core_cpp_registration_public, ClientEqualQualities)
+TEST_P(ClientsTestFixture, ClientEqualQualities)
 {
-  // initialize eCAL API
-  eCAL::Initialize(0, nullptr, "core_cpp_registration_public");
-
-  // enable loop back communication in the same process
-  eCAL::Util::EnableLoopback(true);
-
   std::map<eCAL::Registration::SServiceMethod, eCAL::SServiceMethodInformation> client_info_map;
 
   // create 2 clients with the same quality of data type information
@@ -181,19 +191,10 @@ TEST(core_cpp_registration_public, ClientEqualQualities)
 
   // check size
   EXPECT_EQ(client_info_map.size(), 0);
-
-  // finalize eCAL API
-  eCAL::Finalize();
 }
 
-TEST(core_cpp_registration_public, ClientDifferentQualities)
+TEST_P(ClientsTestFixture, ClientDifferentQualities)
 {
-  // initialize eCAL API
-  eCAL::Initialize(0, nullptr, "core_cpp_registration_public");
-
-  // enable loop back communication in the same process
-  eCAL::Util::EnableLoopback(true);
-
   std::map<eCAL::Registration::SServiceMethod, eCAL::SServiceMethodInformation> client_info_map;
 
   // create 2 clients with different qualities of data type information
@@ -258,19 +259,10 @@ TEST(core_cpp_registration_public, ClientDifferentQualities)
 
   // check size
   EXPECT_EQ(client_info_map.size(), 0);
-
-  // finalize eCAL API
-  eCAL::Finalize();
 }
 
-TEST(core_cpp_registration_public, GetClientIDs)
+TEST_P(ClientsTestFixture, GetClientIDs)
 {
-  // initialize eCAL API
-  eCAL::Initialize(0, nullptr, "core_cpp_registration_public");
-
-  // enable loop back communication in the same process
-  eCAL::Util::EnableLoopback(true);
-
   // create simple client
   {
     // create client
@@ -296,7 +288,33 @@ TEST(core_cpp_registration_public, GetClientIDs)
       EXPECT_EQ(service_method_info, info.info);
     }
   }
-
-  // finalize eCAL API
-  eCAL::Finalize();
 }
+
+INSTANTIATE_TEST_SUITE_P(
+  core_cpp_registration_public_clients,
+  ClientsTestFixture,
+  ::testing::Values(
+    ClientsTestParams{ []() {
+      // shm
+      eCAL::Configuration config;
+      config.registration.layer.shm.enable = true;
+      config.registration.layer.udp.enable = false;
+      return config;
+    }() },
+    ClientsTestParams{ []() {
+      // shm + host group name
+      eCAL::Configuration config;
+      config.registration.layer.shm.enable = true;
+      config.registration.layer.udp.enable = false;
+      config.registration.host_group_name = "abc";
+      return config;
+    }() },
+      ClientsTestParams{ []() {
+      // udp
+      eCAL::Configuration config;
+      config.registration.layer.shm.enable = false;
+      config.registration.layer.udp.enable = true;
+      return config;
+    }() }
+      )
+);
diff --git a/ecal/tests/cpp/registration_test_public/src/registration_getpublisherids.cpp b/ecal/tests/cpp/registration_test_public/src/registration_getpublisherids.cpp
@@ -161,6 +161,14 @@ INSTANTIATE_TEST_SUITE_P(
       config.registration.layer.udp.enable     = false;
       return config;
     }() },
+    TestParams{ 10, []() {
+      // shm + host group name
+      eCAL::Configuration config;
+      config.registration.layer.shm.enable = true;
+      config.registration.layer.udp.enable = false;
+      config.registration.host_group_name = "abc";
+      return config;
+    }() },
     TestParams{ 10, []() {
       // udp
       eCAL::Configuration config;