Refactor some asserts to conditional expressions (#238)

* Refactor some asserts to conditional expressions Signed-off-by: Raul Sanchez-Mateos <[email protected]> * Fix build issue Signed-off-by: JesusPoderoso <[email protected]> * Refactor substraction operation Signed-off-by: JesusPoderoso <[email protected]> * Refs #21681: Remove death asserts in tests Signed-off-by: JesusPoderoso <[email protected]> --------- Signed-off-by: Raul Sanchez-Mateos <[email protected]> Signed-off-by: JesusPoderoso <[email protected]> Co-authored-by: JesusPoderoso <[email protected]>
eProsima · Sep 16, 2024 · 0596f61 · 0596f61
1 parent 751e61a
commit 0596f61
Show file tree

Hide file tree

Showing 8 changed files with 59 additions and 408 deletions.
diff --git a/include/fastdds_statistics_backend/listener/DomainListener.hpp b/include/fastdds_statistics_backend/listener/DomainListener.hpp
@@ -75,9 +75,11 @@ class FASTDDS_STATISTICS_BACKEND_DllAPI DomainListener
 
         void on_instance_undiscovered()
         {
-            assert (current_count > 0);
-            --current_count;
-            --current_count_change;
+            if (current_count > 0)
+            {
+                --current_count;
+                --current_count_change;
+            }
         }
 
         void on_status_read()

diff --git a/src/cpp/StatisticsBackendData.cpp b/src/cpp/StatisticsBackendData.cpp
@@ -25,6 +25,7 @@
 #include <fastdds/dds/domain/DomainParticipantFactory.hpp>
 #include <fastdds/dds/domain/qos/DomainParticipantFactoryQos.hpp>
 #include <fastdds/dds/domain/DomainParticipantListener.hpp>
+#include <fastdds/dds/log/Log.hpp>
 #include <fastdds/dds/subscriber/DataReader.hpp>
 #include <fastdds/dds/subscriber/Subscriber.hpp>
 #include <fastdds/dds/topic/Topic.hpp>
@@ -122,7 +123,11 @@ void StatisticsBackendData::on_data_available(
         DataKind data_kind)
 {
     auto monitor = monitors_by_entity_.find(domain_id);
-    assert(monitor != monitors_by_entity_.end());
+    if (monitor == monitors_by_entity_.end())
+    {
+        logWarning(STATISTICS_BACKEND_DATA, "Monitor not found for domain " << domain_id);
+        return;
+    }
 
     if (should_call_domain_listener(*monitor->second, CallbackKind::ON_DATA_AVAILABLE, data_kind))
     {
@@ -140,7 +145,11 @@ void StatisticsBackendData::on_status_reported(
         StatusKind status_kind)
 {
     auto monitor = monitors_by_entity_.find(domain_id);
-    assert(monitor != monitors_by_entity_.end());
+    if (monitor == monitors_by_entity_.end())
+    {
+        logWarning(STATISTICS_BACKEND_DATA, "Monitor not found for domain " << domain_id);
+        return;
+    }
 
     if (should_call_domain_listener(*monitor->second, CallbackKind::ON_STATUS_REPORTED))
     {
@@ -228,7 +237,12 @@ void StatisticsBackendData::on_domain_entity_discovery(
         DiscoveryStatus discovery_status)
 {
     auto monitor = monitors_by_entity_.find(domain_id);
-    assert(monitor != monitors_by_entity_.end());
+    if (monitor == monitors_by_entity_.end())
+    {
+        logWarning(STATISTICS_BACKEND_DATA, "Monitor not found for domain " << domain_id);
+        return;
+    }
+
     switch (entity_kind)
     {
         case EntityKind::PARTICIPANT:
@@ -306,7 +320,7 @@ void StatisticsBackendData::on_domain_entity_discovery(
         }
         default:
         {
-            assert(false && "Invalid domain entity kind");
+            logWarning(STATISTICS_BACKEND, "Invalid domain entity kind");
         }
     }
 }
@@ -316,7 +330,11 @@ void StatisticsBackendData::on_physical_entity_discovery(
         EntityKind entity_kind,
         DiscoveryStatus discovery_status)
 {
-    assert(discovery_status != DiscoveryStatus::UPDATE);
+    if (discovery_status == DiscoveryStatus::UPDATE)
+    {
+        logWarning(STATISTICS_BACKEND, "UPDATE discovery status is not supported for physical entities");
+        return;
+    }
 
     switch (entity_kind)
     {
@@ -370,7 +388,7 @@ void StatisticsBackendData::on_physical_entity_discovery(
         }
         default:
         {
-            assert(false && "Invalid physical entity kind");
+            logWarning(STATISTICS_BACKEND, "Invalid physical entity kind");
         }
     }
 }

diff --git a/src/cpp/database/database_queue.cpp b/src/cpp/database/database_queue.cpp
@@ -187,7 +187,11 @@ EntityId DatabaseEntityQueue::process_datareader(
         try
         {
             participant_id = database_->get_entity_by_guid(EntityKind::PARTICIPANT, to_string(participant_guid));
-            assert(participant_id.first == info.domain_id);
+            if (participant_id.first != info.domain_id)
+            {
+                throw BadParameter("Participant " + to_string(participant_guid)
+                          +  " found in the database but it is not in the current domain");
+            }
         }
         catch (const Exception&)
         {
@@ -258,7 +262,11 @@ EntityId DatabaseEntityQueue::process_datawriter(
         try
         {
             participant_id = database_->get_entity_by_guid(EntityKind::PARTICIPANT, to_string(participant_guid));
-            assert(participant_id.first == info.domain_id);
+            if (participant_id.first != info.domain_id)
+            {
+                throw BadParameter("Participant " + to_string(participant_guid)
+                          +  " found in the database but it is not in the current domain");
+            }
         }
         catch (const Exception&)
         {
@@ -317,7 +325,11 @@ EntityId DatabaseEntityQueue::process_endpoint_discovery(
     try
     {
         participant_id = database_->get_entity_by_guid(EntityKind::PARTICIPANT, to_string(participant_guid));
-        assert(participant_id.first == info.domain_id);
+        if (participant_id.first != info.domain_id)
+        {
+            throw BadParameter("Participant " + to_string(participant_guid)
+                        +  " found in the database but it is not in the current domain");
+        }
     }
     catch (const Exception&)
     {

diff --git a/src/cpp/database/samples.hpp b/src/cpp/database/samples.hpp
@@ -22,6 +22,7 @@
 
 #include <chrono>
 
+#include <fastdds/dds/log/Log.hpp>
 #include <fastdds_statistics_backend/exception/Exception.hpp>
 #include <fastdds_statistics_backend/types/types.hpp>
 
@@ -122,10 +123,17 @@ struct EntityCountSample : StatisticsSample
     inline EntityCountSample operator -(
             const EntityCountSample& other) const noexcept
     {
-        assert(count >= other.count);
         EntityCountSample ret(kind);
         ret.src_ts = src_ts;
-        ret.count = count - other.count;
+        if (count >= other.count)
+        {
+            ret.count = count - other.count;
+        }
+        else
+        {
+            logWarning(STATISTICS_BACKEND, "Subtraction of EntityCountSample resulted in a negative number");
+            ret.count = 0;
+        }
         return ret;
     }
 

diff --git a/test/unittest/Database/SampleTests.cpp b/test/unittest/Database/SampleTests.cpp
@@ -109,6 +109,7 @@ TEST(database, entitycountsample_operator_comparison)
     ASSERT_NE(sample_1, sample_2);
 }
 
+// updated to avoid unnecessary asserts in code, check #21681 Redmine task
 TEST(database, entitycountsample_operator_minus)
 {
     EntityCountSample sample_1;
@@ -125,11 +126,11 @@ TEST(database, entitycountsample_operator_minus)
     ASSERT_EQ(sample_3.count, 1u);
     ASSERT_EQ(sample_3.src_ts, sample_1.src_ts);
 
-#ifndef NDEBUG
-    // Assertion are active here
+    // If negative number, subtraction returns 0
     sample_2.count = 3;
-    ASSERT_DEATH(sample_1 - sample_2, "");
-#endif // ifndef NDEBUG
+    sample_3 = sample_1 - sample_2;
+    ASSERT_EQ(sample_3.count, 0u);
+    ASSERT_EQ(sample_3.src_ts, sample_1.src_ts);
 }
 
 TEST(database, bytecountsample_operator_comparison)

diff --git a/test/unittest/StatisticsBackend/CMakeLists.txt b/test/unittest/StatisticsBackend/CMakeLists.txt
@@ -70,7 +70,6 @@ set(BACKEND_TEST_LIST
     get_type
     get_data_supported_entity_kinds
     set_alias
-    internal_callbacks_negative_cases
     set_listener_non_existent_monitor
     get_domain_view_graph
     get_domain_view_graph_invalid_domain
@@ -178,26 +177,6 @@ foreach(test_name ${USER_LISTENERS_TEST_LIST})
 
 endforeach()
 
-set(USER_LISTENERS_DEATH_TEST_LIST
-    wrong_entity_kind
-    )
-
-foreach(test_name ${USER_LISTENERS_DEATH_TEST_LIST})
-    add_test(NAME calling_user_listeners_DeathTest.${test_name}
-        COMMAND calling_user_listeners_tests
-        --gtest_filter=calling_user_listeners_DeathTest.${test_name}:*/calling_user_listeners_DeathTest.${test_name}/*)
-
-    if(TEST_FRIENDLY_PATH)
-        set_tests_properties(calling_user_listeners_DeathTest.${test_name}
-            PROPERTIES
-                ENVIRONMENT "PATH=${TEST_FRIENDLY_PATH}"
-        )
-    endif(TEST_FRIENDLY_PATH)
-endforeach()
-
-# Add xfail label to failing test
-set_property(TEST calling_user_listeners_DeathTest.wrong_entity_kind PROPERTY LABELS xfail)
-
 set(USER_LISTENERS_TEST_LIST_PHYSICAL_ENTITIES
     entity_discovered
     entity_discovered_not_in_mask