Refactor some asserts to conditional expressions (#238)

rsanchez15 · JesusPoderoso · web-flow · commit 0596f614d916 · 2024-09-16T12:06:44.000+02:00
* Refactor some asserts to conditional expressions

Signed-off-by: Raul Sanchez-Mateos &lt;raul@eprosima.com&gt;

* Fix build issue

Signed-off-by: JesusPoderoso &lt;jesuspoderoso@eprosima.com&gt;

* Refactor substraction operation

Signed-off-by: JesusPoderoso &lt;jesuspoderoso@eprosima.com&gt;

* Refs #21681: Remove death asserts in tests

Signed-off-by: JesusPoderoso &lt;jesuspoderoso@eprosima.com&gt;

---------

Signed-off-by: Raul Sanchez-Mateos &lt;raul@eprosima.com&gt;
Signed-off-by: JesusPoderoso &lt;jesuspoderoso@eprosima.com&gt;
Co-authored-by: JesusPoderoso &lt;jesuspoderoso@eprosima.com&gt;
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
diff --git a/test/unittest/StatisticsBackend/CallingUserListenersTests.cpp b/test/unittest/StatisticsBackend/CallingUserListenersTests.cpp
diff --git a/test/unittest/StatisticsBackend/StatisticsBackendTests.cpp b/test/unittest/StatisticsBackend/StatisticsBackendTests.cpp

Original file line number	Diff line number	Diff line change
`@@ -75,9 +75,11 @@ class FASTDDS_STATISTICS_BACKEND_DllAPI DomainListener`
`75`	`75`
`76`	`76`	`void on_instance_undiscovered()`
`77`	`77`	`{`
`78`		`- assert (current_count > 0);`
`79`		`- --current_count;`
`80`		`- --current_count_change;`
	`78`	`+ if (current_count > 0)`
	`79`	`+ {`
	`80`	`+ --current_count;`
	`81`	`+ --current_count_change;`
	`82`	`+ }`
`81`	`83`	`}`
`82`	`84`
`83`	`85`	`void on_status_read()`
Original file line number	Diff line number	Diff line change
`@@ -25,6 +25,7 @@`
`25`	`25`	`#include <fastdds/dds/domain/DomainParticipantFactory.hpp>`
`26`	`26`	`#include <fastdds/dds/domain/qos/DomainParticipantFactoryQos.hpp>`
`27`	`27`	`#include <fastdds/dds/domain/DomainParticipantListener.hpp>`
	`28`	`+#include <fastdds/dds/log/Log.hpp>`
`28`	`29`	`#include <fastdds/dds/subscriber/DataReader.hpp>`
`29`	`30`	`#include <fastdds/dds/subscriber/Subscriber.hpp>`
`30`	`31`	`#include <fastdds/dds/topic/Topic.hpp>`
`@@ -122,7 +123,11 @@ void StatisticsBackendData::on_data_available(`
`122`	`123`	`DataKind data_kind)`
`123`	`124`	`{`
`124`	`125`	`auto monitor = monitors_by_entity_.find(domain_id);`
`125`		`- assert(monitor != monitors_by_entity_.end());`
	`126`	`+ if (monitor == monitors_by_entity_.end())`
	`127`	`+ {`
	`128`	`+ logWarning(STATISTICS_BACKEND_DATA, "Monitor not found for domain " << domain_id);`
	`129`	`+ return;`
	`130`	`+ }`
`126`	`131`
`127`	`132`	`if (should_call_domain_listener(*monitor->second, CallbackKind::ON_DATA_AVAILABLE, data_kind))`
`128`	`133`	`{`
`@@ -140,7 +145,11 @@ void StatisticsBackendData::on_status_reported(`
`140`	`145`	`StatusKind status_kind)`
`141`	`146`	`{`
`142`	`147`	`auto monitor = monitors_by_entity_.find(domain_id);`
`143`		`- assert(monitor != monitors_by_entity_.end());`
	`148`	`+ if (monitor == monitors_by_entity_.end())`
	`149`	`+ {`
	`150`	`+ logWarning(STATISTICS_BACKEND_DATA, "Monitor not found for domain " << domain_id);`
	`151`	`+ return;`
	`152`	`+ }`
`144`	`153`
`145`	`154`	`if (should_call_domain_listener(*monitor->second, CallbackKind::ON_STATUS_REPORTED))`
`146`	`155`	`{`
`@@ -228,7 +237,12 @@ void StatisticsBackendData::on_domain_entity_discovery(`
`228`	`237`	`DiscoveryStatus discovery_status)`
`229`	`238`	`{`
`230`	`239`	`auto monitor = monitors_by_entity_.find(domain_id);`
`231`		`- assert(monitor != monitors_by_entity_.end());`
	`240`	`+ if (monitor == monitors_by_entity_.end())`
	`241`	`+ {`
	`242`	`+ logWarning(STATISTICS_BACKEND_DATA, "Monitor not found for domain " << domain_id);`
	`243`	`+ return;`
	`244`	`+ }`
	`245`	`+`
`232`	`246`	`switch (entity_kind)`
`233`	`247`	`{`
`234`	`248`	`case EntityKind::PARTICIPANT:`
`@@ -306,7 +320,7 @@ void StatisticsBackendData::on_domain_entity_discovery(`
`306`	`320`	`}`
`307`	`321`	`default:`
`308`	`322`	`{`
`309`		`- assert(false && "Invalid domain entity kind");`
	`323`	`+ logWarning(STATISTICS_BACKEND, "Invalid domain entity kind");`
`310`	`324`	`}`
`311`	`325`	`}`
`312`	`326`	`}`
`@@ -316,7 +330,11 @@ void StatisticsBackendData::on_physical_entity_discovery(`
`316`	`330`	`EntityKind entity_kind,`
`317`	`331`	`DiscoveryStatus discovery_status)`
`318`	`332`	`{`
`319`		`- assert(discovery_status != DiscoveryStatus::UPDATE);`
	`333`	`+ if (discovery_status == DiscoveryStatus::UPDATE)`
	`334`	`+ {`
	`335`	`+ logWarning(STATISTICS_BACKEND, "UPDATE discovery status is not supported for physical entities");`
	`336`	`+ return;`
	`337`	`+ }`
`320`	`338`
`321`	`339`	`switch (entity_kind)`
`322`	`340`	`{`
`@@ -370,7 +388,7 @@ void StatisticsBackendData::on_physical_entity_discovery(`
`370`	`388`	`}`
`371`	`389`	`default:`
`372`	`390`	`{`
`373`		`- assert(false && "Invalid physical entity kind");`
	`391`	`+ logWarning(STATISTICS_BACKEND, "Invalid physical entity kind");`
`374`	`392`	`}`
`375`	`393`	`}`
`376`	`394`	`}`
Original file line number	Diff line number	Diff line change
`@@ -187,7 +187,11 @@ EntityId DatabaseEntityQueue::process_datareader(`
`187`	`187`	`try`
`188`	`188`	`{`
`189`	`189`	`participant_id = database_->get_entity_by_guid(EntityKind::PARTICIPANT, to_string(participant_guid));`
`190`		`- assert(participant_id.first == info.domain_id);`
	`190`	`+ if (participant_id.first != info.domain_id)`
	`191`	`+ {`
	`192`	`+ throw BadParameter("Participant " + to_string(participant_guid)`
	`193`	`+ + " found in the database but it is not in the current domain");`
	`194`	`+ }`
`191`	`195`	`}`
`192`	`196`	`catch (const Exception&)`
`193`	`197`	`{`
`@@ -258,7 +262,11 @@ EntityId DatabaseEntityQueue::process_datawriter(`
`258`	`262`	`try`
`259`	`263`	`{`
`260`	`264`	`participant_id = database_->get_entity_by_guid(EntityKind::PARTICIPANT, to_string(participant_guid));`
`261`		`- assert(participant_id.first == info.domain_id);`
	`265`	`+ if (participant_id.first != info.domain_id)`
	`266`	`+ {`
	`267`	`+ throw BadParameter("Participant " + to_string(participant_guid)`
	`268`	`+ + " found in the database but it is not in the current domain");`
	`269`	`+ }`
`262`	`270`	`}`
`263`	`271`	`catch (const Exception&)`
`264`	`272`	`{`
`@@ -317,7 +325,11 @@ EntityId DatabaseEntityQueue::process_endpoint_discovery(`
`317`	`325`	`try`
`318`	`326`	`{`
`319`	`327`	`participant_id = database_->get_entity_by_guid(EntityKind::PARTICIPANT, to_string(participant_guid));`
`320`		`- assert(participant_id.first == info.domain_id);`
	`328`	`+ if (participant_id.first != info.domain_id)`
	`329`	`+ {`
	`330`	`+ throw BadParameter("Participant " + to_string(participant_guid)`
	`331`	`+ + " found in the database but it is not in the current domain");`
	`332`	`+ }`
`321`	`333`	`}`
`322`	`334`	`catch (const Exception&)`
`323`	`335`	`{`