[16362] Fix Windows StatistisQosTests.cpp linkage and Failed test #3122

Mario-DL · 2022-11-30T06:56:17Z

Description

This PR intends to fix the StatisticsQosTests windows linkage issue. In addition, it fixes a test that tried to read a second xml profiles file with some profiles names shared with the first one, consequently, it was being ignored because of the bool flag default_xml_profiles_loaded in DomainParticipantFactory.

Proposed solution: Change the usage of ParticipantImpl to Participant (user api) to prevent linkage problems and unify all xml profiles from both files into a single one.

Contributor Checklist

Commit messages follow the project guidelines.
The code follows the style guidelines of this project.
N/A Tests that thoroughly check the new feature have been added/Regression tests checking the bug and its fix have been added.
N/A Any new/modified methods have been properly documented using Doxygen.
Fast DDS test suite has been run locally. linux x86_64 Ubuntu 22.04
Changes are ABI compatible.
Changes are API compatible.
N/A Documentation builds and tests pass locally.
N/A New feature has been added to the versions.md file (if applicable).
N/A New feature has been documented/Current behavior is correctly described in the documentation.

Reviewer Checklist

Check contributor checklist is correct.
Check CI results: changes do not issue any warning.
Check CI results: failing tests are unrelated with the changes.

JLBuenoLopez · 2022-12-01T09:09:46Z

@richiprosima please test this

JLBuenoLopez · 2022-12-01T11:30:32Z

test/unittest/statistics/dds/StatisticsQosTests.cpp

    {
+        const char* xml =
+                "                                                                                                               \


Sometimes uncrustify resolves rules in a very weird manner. Nevertheless, I suggest keeping aligned the \.

JLBuenoLopez · 2022-12-01T11:32:55Z

test/unittest/statistics/dds/StatisticsQosTests.cpp

+            <dds xmlns=\"http://www.eprosima.com/XMLSchemas/fastRTPS_Profiles\">                                            \
+            <profiles>                                                                                                      \
+                    <participant profile_name=\"statistics_participant\" is_default_profile=\"true\">                       \
+                    <rtps>                                                                                                  \
+                            <propertiesPolicy>                                                                              \
+                            <properties>                                                                                    \
+                                    <property>                                                                              \
+                                    <name>fastdds.statistics</name>                                                         \
+                                    <value>HISTORY_LATENCY_TOPIC;PUBLICATION_THROUGHPUT_TOPIC;DATA_COUNT_TOPIC;DISCOVERY_TOPIC</value>\
+                                    </property>                                                                             \
+                            </properties>                                                                                   \
+                            </propertiesPolicy>                                                                             \
+                    </rtps>                                                                                                 \
+                    </participant>                                                                                          \
+                    <data_writer profile_name=\"HISTORY_LATENCY_TOPIC\">                                                    \
+                            <qos>                                                                                           \
+                                    <reliability>                                                                           \
+                                            <kind>BEST_EFFORT</kind>                                                        \
+                                    </reliability>                                                                          \
+                                    <durability>                                                                            \
+                                            <kind>VOLATILE</kind>                                                           \
+                                    </durability>                                                                           \
+                                    <publishMode>                                                                           \
+                                            <kind>SYNCHRONOUS</kind>                                                        \
+                                    </publishMode>                                                                          \
+                            </qos>                                                                                          \
+                    </data_writer>                                                                                          \
+                    <data_writer profile_name=\"NETWORK_LATENCY_TOPIC\">                                                    \
+                    </data_writer>                                                                                          \
+                    <data_writer profile_name=\"SUBSCRIPTION_THROUGHPUT_TOPIC\">                                            \
+                            <qos>                                                                                           \
+                                    <reliability>                                                                           \
+                                            <kind>BEST_EFFORT</kind>                                                        \
+                                    </reliability>                                                                          \
+                                    <partition>                                                                             \
+                                            <names>                                                                         \
+                                                    <name>part1</name>                                                      \
+                                                    <name>part2</name>                                                      \
+                                            </names>                                                                        \
+                                    </partition>                                                                            \
+                                    <deadline>                                                                              \
+                                            <period>                                                                        \
+                                                    <sec>3</sec>                                                            \
+                                            </period>                                                                       \
+                                    </deadline>                                                                             \
+                                    <latencyBudget>                                                                         \
+                                            <duration>                                                                      \
+                                                    <sec>2</sec>                                                            \
+                                            </duration>                                                                     \
+                                    </latencyBudget>                                                                        \
+                                    <disable_heartbeat_piggyback>true</disable_heartbeat_piggyback>                         \
+                            </qos>                                                                                          \
+                    </data_writer>                                                                                          \
+                    <data_writer profile_name=\"DATA_COUNT_TOPIC\">                                                         \
+                            <qos>                                                                                           \
+                                    <liveliness>                                                                            \
+                                            <kind>AUTOMATIC</kind>                                                          \
+                                            <lease_duration>                                                                \
+                                                    <sec>1</sec>                                                            \
+                                                    <nanosec>856000</nanosec>                                               \
+                                            </lease_duration>                                                               \
+                                            <announcement_period>                                                           \
+                                                    <sec>1</sec>                                                            \
+                                                    <nanosec>856000</nanosec>                                               \
+                                            </announcement_period>                                                          \
+                                    </liveliness>                                                                           \
+                            </qos>                                                                                          \
+                    </data_writer>                                                                                          \
+                    <data_writer profile_name=\"HEARTBEAT_COUNT_TOPIC\">                                                    \
+                            <qos>                                                                                           \
+                                    <liveliness>                                                                            \
+                                            <kind>AUTOMATIC</kind>                                                          \
+                                            <lease_duration>                                                                \
+                                                    <sec>1</sec>                                                            \
+                                                    <nanosec>856000</nanosec>                                               \
+                                            </lease_duration>                                                               \
+                                            <announcement_period>                                                           \
+                                                    <sec>1</sec>                                                            \
+                                                    <nanosec>856000</nanosec>                                               \
+                                            </announcement_period>                                                          \
+                                    </liveliness>                                                                           \
+                            </qos>                                                                                          \
+                    </data_writer>                                                                                          \
+                    <data_writer profile_name=\"OTHER_NAME_FOR_PROFILE\">                                                   \
+                            <qos>                                                                                           \
+                                    <reliability>                                                                           \
+                                            <kind>BEST_EFFORT</kind>                                                        \
+                                    </reliability>                                                                          \
+                            </qos>                                                                                          \
+                    </data_writer>                                                                                          \
+                    <data_writer profile_name=\"DISCOVERY_TOPIC\">                                                          \
+                            <qos>                                                                                           \
+                                    <reliability>                                                                           \
+                                            <kind>RELIABLE</kind>                                                           \
+                                    </reliability>                                                                          \
+                            </qos>                                                                                          \
+                    </data_writer>                                                                                          \
+                </profiles>                                                                                                 \
+            </dds>                                                                                                          \
+        ";


Is the alignment forced by uncrustify? I do not think so. I suggest modifying and using our tabulations rules (only 4 spaces between levels instead of the 8 being applied now.

The alignment was the one the file had before. However, I will apply a 4 indent for uniformity.

Thanks! I know it is legacy code, but in this way we strive to always be improving the code. 😄

JLBuenoLopez · 2022-12-01T11:33:35Z

test/unittest/statistics/dds/StatisticsQosTests.cpp

+                "                                                                                                               \
+            <?xml version=\"1.0\" encoding=\"utf-8\"  ?>                                                                    \
+            <dds xmlns=\"http://www.eprosima.com/XMLSchemas/fastRTPS_Profiles\">                                            \
+            <profiles>                                                                                                      \


<profiles> tag should be indented with respect to <dds> tag

JLBuenoLopez · 2022-12-01T11:34:38Z

test/unittest/statistics/dds/StatisticsQosTests.cpp

    {
+        const char* xml =
+                "                                                                                                               \
+            <?xml version=\"1.0\" encoding=\"utf-8\"  ?>                                                                    \


I suggest aligning this first line with the " in the line before. I do not think uncrustify will complain, though I am not completely sure.

JLBuenoLopez · 2022-12-01T11:35:09Z

test/unittest/statistics/dds/StatisticsQosTests.cpp

+            <dds xmlns=\"http://www.eprosima.com/XMLSchemas/fastRTPS_Profiles\">                                            \
+            <profiles>                                                                                                      \
+                    <participant profile_name=\"statistics_participant\" is_default_profile=\"true\">                       \
+                    <rtps>                                                                                                  \


This tag should be indented

JLBuenoLopez · 2022-12-01T11:36:49Z

test/unittest/statistics/dds/StatisticsQosTests.cpp

+                            <properties>                                                                                    \
+                                    <property>                                                                              \
+                                    <name>fastdds.statistics</name>                                                         \
+                                    <value>HISTORY_LATENCY_TOPIC;PUBLICATION_THROUGHPUT_TOPIC;DATA_COUNT_TOPIC;DISCOVERY_TOPIC</value>\


I think this line should be split to keep the alignment

JLBuenoLopez · 2022-12-01T11:42:05Z

test/unittest/statistics/dds/StatisticsQosTests.cpp

@@ -341,7 +348,7 @@ TEST_F(StatisticsFromXMLProfileTests, XMLConfigurationForStatisticsDataWritersQo
    // it is just defined as data_writer profile. Thus, should not be created
    std::string subscription_througput_name = SUBSCRIPTION_THROUGHPUT_TOPIC;
    eprosima::fastdds::dds::DataWriter* subscription_througput_writer =
-            statistics_publisher_impl->lookup_datawriter(subscription_througput_name);
+            statistics_publisher->lookup_datawriter(subscription_througput_name);


I know it is not related to these changes, but I suggest fixing this typo:

Suggested change

statistics_publisher->lookup_datawriter(subscription_througput_name);

statistics_publisher->lookup_datawriter(subscription_throughput_name);

I think this previous suggestion was overlooked 😅

JLBuenoLopez · 2022-12-01T11:45:29Z

test/unittest/statistics/dds/StatisticsQosTests.cpp

+                            <properties>                                                                                    \
+                                    <property>                                                                              \
+                                    <name>fastdds.statistics</name>                                                         \
+                                    <value>HISTORY_LATENCY_TOPIC;PUBLICATION_THROUGHPUT_TOPIC;DATA_COUNT_TOPIC;DISCOVERY_TOPIC</value>\


I am missing some cases that were tested with the second XML file:

Check that it worked without using the alias and using directly the topic name (both setting the property as the profile name being used in that specific topic).

The GENERIC_STATISTICS_PROFILE is not being tested applying these changes.

On the next commit, I have rewritten the tests in a (i think) more elegant way. Instead of creating two files and point them from the environment, I use directly the load_XML_profiles_string() so it is clearer.

Probably when the original code was implemented, the option of loading the XML profiles directly from a string was not implemented. As you say, now that this feature is included with Fast DDS it makes more sense. Thank you!

JLBuenoLopez · 2022-12-01T11:49:36Z

test/unittest/statistics/dds/StatisticsQosTests.cpp

-                test_statistics_domain_participant_impl->get_publisher_impl();
-        ASSERT_NE(_statistics_publisher_impl, nullptr);
+        // Get Publisher
+        _statistics_publisher =


I know it is not related to the changes but I think we should unify the code style according to Fast DDS guidestyle. Private class member should have a trailing underscore, but there is no rule to add underscore before the variable name.

test/unittest/statistics/dds/StatisticsQosTests.cpp

JLBuenoLopez · 2022-12-01T11:55:21Z

Related to the checklist:

The second commit is not following the project guidelines (the reference to the corresponding task is missing).
The third point I think it is N/A because no new test have been added, either for regression or check a new feature.

JLBuenoLopez · 2022-12-01T13:07:46Z

I have build in Windows and the linking issue is solved (CI does not test with FASTDDS_STATISTICS option enabled).

…tests so that not file creation needed. Applied requested changes. Signed-off-by: Mario Dominguez <[email protected]>

Mario-DL · 2022-12-01T14:31:44Z

Note: StatisticsQosTests have to be run individually (as in the ci). Re-written both tests under StatisticsFromXMLProfileTests so that no file creation is needed.

Mario-DL · 2022-12-01T14:34:17Z

@richiprosima please test this

JLBuenoLopez

Two small nitpicks

JLBuenoLopez · 2022-12-01T14:47:43Z

test/unittest/statistics/dds/StatisticsQosTests.cpp

@@ -341,7 +348,7 @@ TEST_F(StatisticsFromXMLProfileTests, XMLConfigurationForStatisticsDataWritersQo
    // it is just defined as data_writer profile. Thus, should not be created
    std::string subscription_througput_name = SUBSCRIPTION_THROUGHPUT_TOPIC;
    eprosima::fastdds::dds::DataWriter* subscription_througput_writer =
-            statistics_publisher_impl->lookup_datawriter(subscription_througput_name);
+            statistics_publisher->lookup_datawriter(subscription_througput_name);


I think this previous suggestion was overlooked 😅

JLBuenoLopez · 2022-12-01T14:49:51Z

test/unittest/statistics/dds/StatisticsQosTests.cpp

+                                    <properties>                                                                              \
+                                        <property>                                                                            \
+                                            <name>fastdds.statistics</name>                                                   \
+                                            <value>HISTORY_LATENCY_TOPIC;PUBLICATION_THROUGHPUT_TOPIC;DISCOVERY_TOPIC;</value>\


Instead of the alias it would be nice to have one check with the topic name directly, as it was done before: instead of DISCOVERY_TOPIC, use _fastdds_statistics_discovered_entity. This apply also in the corresponding profile.

Signed-off-by: Mario Dominguez <[email protected]>

JLBuenoLopez

LGTM!

EduPonz · 2022-12-02T06:47:05Z

@Mergifyio backport 2.8.x

mergify · 2022-12-02T06:47:47Z

backport 2.8.x

✅ Backports have been created

#3127 [16362] Fix Windows StatistisQosTests.cpp linkage and Failed test (backport #3122) has been created for branch 2.8.x

* Refs #16362: Fix Windows StatistisQosTests.cpp linkage and rewritten tests so that not file creation needed. Applied requested changes. Signed-off-by: Mario Dominguez <[email protected]> * Refs #16362: Applied requested changes Signed-off-by: Mario Dominguez <[email protected]> Signed-off-by: Mario Dominguez <[email protected]> (cherry picked from commit 5926ee8)

Mario-DL added the needs-review PR that is ready to be reviewed label Nov 30, 2022

JLBuenoLopez suggested changes Dec 1, 2022

View reviewed changes

Refs #16362: Fix Windows StatistisQosTests.cpp linkage and rewritten …

4165326

…tests so that not file creation needed. Applied requested changes. Signed-off-by: Mario Dominguez <[email protected]>

Mario-DL force-pushed the fix/16362 branch from 146716e to 4165326 Compare December 1, 2022 14:27

JLBuenoLopez reviewed Dec 1, 2022

View reviewed changes

Refs #16362: Applied requested changes

3e8adae

Signed-off-by: Mario Dominguez <[email protected]>

JLBuenoLopez approved these changes Dec 1, 2022

View reviewed changes

MiguelCompany merged commit 5926ee8 into master Dec 1, 2022

MiguelCompany deleted the fix/16362 branch December 1, 2022 15:36

mergify bot mentioned this pull request Dec 2, 2022

[16362] Fix Windows StatistisQosTests.cpp linkage and Failed test (backport #3122) #3127

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[16362] Fix Windows StatistisQosTests.cpp linkage and Failed test #3122

[16362] Fix Windows StatistisQosTests.cpp linkage and Failed test #3122

Mario-DL commented Nov 30, 2022 •

edited

Loading

JLBuenoLopez commented Dec 1, 2022

JLBuenoLopez Dec 1, 2022

JLBuenoLopez Dec 1, 2022

Mario-DL Dec 1, 2022

JLBuenoLopez Dec 1, 2022

JLBuenoLopez Dec 1, 2022

JLBuenoLopez Dec 1, 2022

JLBuenoLopez Dec 1, 2022

JLBuenoLopez Dec 1, 2022

JLBuenoLopez Dec 1, 2022

JLBuenoLopez Dec 1, 2022

JLBuenoLopez Dec 1, 2022

Mario-DL Dec 1, 2022

JLBuenoLopez Dec 1, 2022

JLBuenoLopez Dec 1, 2022 •

edited

Loading

JLBuenoLopez commented Dec 1, 2022

JLBuenoLopez commented Dec 1, 2022

Mario-DL commented Dec 1, 2022

Mario-DL commented Dec 1, 2022

JLBuenoLopez left a comment

JLBuenoLopez Dec 1, 2022

JLBuenoLopez Dec 1, 2022

JLBuenoLopez left a comment

EduPonz commented Dec 2, 2022

mergify bot commented Dec 2, 2022

	statistics_publisher->lookup_datawriter(subscription_througput_name);
	statistics_publisher->lookup_datawriter(subscription_throughput_name);

[16362] Fix Windows StatistisQosTests.cpp linkage and Failed test #3122

[16362] Fix Windows StatistisQosTests.cpp linkage and Failed test #3122

Conversation

Mario-DL commented Nov 30, 2022 • edited Loading

Description

Contributor Checklist

Reviewer Checklist

JLBuenoLopez commented Dec 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JLBuenoLopez Dec 1, 2022 • edited Loading

Choose a reason for hiding this comment

JLBuenoLopez commented Dec 1, 2022

JLBuenoLopez commented Dec 1, 2022

Mario-DL commented Dec 1, 2022

Mario-DL commented Dec 1, 2022

JLBuenoLopez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JLBuenoLopez left a comment

Choose a reason for hiding this comment

EduPonz commented Dec 2, 2022

mergify bot commented Dec 2, 2022

✅ Backports have been created

Mario-DL commented Nov 30, 2022 •

edited

Loading

JLBuenoLopez Dec 1, 2022 •

edited

Loading