[12529] Fix file watch initialization #2280
Conversation
…e file does not exist Signed-off-by: JLBuenoLopez-eProsima <joseluisbueno@eprosima.com>
Signed-off-by: JLBuenoLopez-eProsima <joseluisbueno@eprosima.com>
Signed-off-by: JLBuenoLopez-eProsima <joseluisbueno@eprosima.com>
Signed-off-by: JLBuenoLopez-eProsima <joseluisbueno@eprosima.com>
|
File watch is a feature not implemented yet for MacOS. I have fixed the new test so it also works in MacOS not running the checks related with the addition of dynamic servers. |
| @@ -36,16 +44,23 @@ | |||
| #include <fastdds/dds/subscriber/Subscriber.hpp> | |||
| #include <fastdds/dds/topic/qos/TopicQos.hpp> | |||
| #include <fastdds/rtps/attributes/RTPSParticipantAttributes.h> | |||
| #include <fastdds/rtps/attributes/ServerAttributes.h> | |||
| #include <fastdds/rtps/common/Locator.h> | |||
| #include <fastdds/rtps/participant/RTPSParticipant.h> | |||
There was a problem hiding this comment.
Is this include really needed?
There was a problem hiding this comment.
It is not being used. Good catch!
There was a problem hiding this comment.
It is necessary because RTPSParticipant::getRTPSParticipantAttributes is defined within that header file. Without this include, the project does not build.
| * FASTDDS_ENVIRONMENT_FILE_ENV_VAR should be defined from the start because it is only loaded once (when the XML | ||
| * profiles are loaded). The file is not going to exist until a later case. However, the filewatch is initialized | ||
| * even though the file does not exist () |
There was a problem hiding this comment.
According to comments later in this test and this check:
Fast-DDS/src/cpp/rtps/RTPSDomain.cpp
Line 120 in 4f6ca2a
It looks like the filewatch is in fact NOT initialized if the file does not exist. If that is so please correct this comment.
There was a problem hiding this comment.
This comment does not apply any longer as the test has been split into smaller tests
Signed-off-by: JLBuenoLopez-eProsima <joseluisbueno@eprosima.com>
Signed-off-by: JLBuenoLopez-eProsima <joseluisbueno@eprosima.com>
| #ifndef __APPLE__ | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(1100)); | ||
| file.open(filename); | ||
| file << "{\"ROS_DISCOVERY_SERVER\": \"84.22.253.128:8888;192.168.1.133:64863;localhost:1234\"}"; | ||
| file.close(); | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(1100)); | ||
| get_rtps_attributes(participant, attributes); | ||
|
|
There was a problem hiding this comment.
Maybe add an EXPECT_NE here so it is tested that modifying the file does in fact not modify the RTPS attributes, hence the next lines.
There was a problem hiding this comment.
The attributes have indeed been modified. The variable output used to validate the expected attributes output is the one we need to update so the check is correct.
| * SERVER Participant without initial remote server list set by QoS. | ||
| * The environment variable applies and adds those remote servers to the list. |
There was a problem hiding this comment.
You are not calling set_environment_variable() and you are checking truth against the vector being empty. Either the test is wrong or else the explanation is. Please check it.
| @@ -799,30 +894,44 @@ TEST(ParticipantTests, RemoteServersListConfiguration) | |||
| file << "{\"ROS_DISCOVERY_SERVER\": \"172.17.0.5:4321;192.168.1.133:64863\"}"; | |||
There was a problem hiding this comment.
Shouldn't this fail since we already defined a Discovery Server via environment variable earlier and it is not included here?
There was a problem hiding this comment.
That is just the issue this test intends to check: what happens when two servers with the same guidPrefix are being added, one through the environment variable and another through QoS. It seems that both are included though only one of them is pinged (as the test comment explains). Also, the attributes only holds the one set by QoS (which is not the one being pinged as far as I know checking with a wireshark capture) and it is against this server that the feature checks that no server has been removed. That is the reason why this test is called inconsistent. The main goal was checking that Fast DDS does not have a core and that any mishandle of the API is safe (even though the behavior can be undefined). That is the reason why in our documentation we have included a note stating that it is advised to use either the API or the environment variable but not both at the same time.
Signed-off-by: JLBuenoLopez-eProsima <joseluisbueno@eprosima.com>
|
Just one more CI run. |
|
@richiprosima Please test this |
If the
FASTDDS_ENVIRONMENT_FILEenvironment variable is set but the file does not exist, Fast DDS breaks. This PR fixes this issue and adds also a test to check different Discovery Server configurations.