Add tests for parameters #14

tfoote · 2015-07-07T18:57:43Z

This has some basic tests. However I need some help on the RTI Connext Dynamic implmeentation tests which hang.

How much details do we want to implement before we add support for a testing framework like gtest. Writing detailed unit tests with return values only is not very productive.

This is currently running the tests and checking their values only.

Connects to #10

dirk-thomas · 2015-07-07T21:06:42Z

Invoking the test launch files directly fails with:

NameError: name 'test_parameters' is not defined

Update: fixed in 2cae78f1d14486a282936774ebe4d08f4f7bf3b0

dirk-thomas · 2015-07-07T21:17:16Z

The Python launch file does not follow the style conventions.

dirk-thomas · 2015-07-07T21:25:38Z

test_communication/CMakeLists.txt

+      PROPERTIES DEPENDS "test_parameters_cpp__${middleware_impl1}"
+    )
+
+


Please remove the two empty lines.

dirk-thomas · 2015-07-07T21:30:42Z

Instead of streaming to cout / cerr incrementally this should use printf / fprintf to avoid interleaving.

dirk-thomas · 2015-07-07T21:39:44Z

Connext Dynamic fails with the following for me which indicate some type mismatch:

DDS_DynamicData_set_string:type mismatch for field <no name> (id=1)

Update: rmw_connext_dynamic_cpp does not implement arrays of submessages correctly.

dirk-thomas · 2015-07-08T20:23:23Z

The Connext dynamic test currently hangs forever since the sync API has no timeouts. May be it would be better to cover the async API in a first test which has timeouts instead.

Update: seems to work now.

esteve · 2015-07-28T22:36:12Z

@dirk-thomas I pushed a test that exposes the issue with subscriptions not being fired. The test will hang at the end because we rclcpp::spin_until_future_complete does not support timeouts yet.

dirk-thomas · 2015-07-29T16:30:54Z

The current behavior is that rmw_wait() returns when one or multiple samples are ready to be taken. But the executor only takes one of them and then calls rmw_wait() again. This call won't return until new samples arrive.

So either rmw has to always return as long as not yet taken samples are available or the executor has to handle available sample after wait returns. Since the SyncParametersClient currently embeds its own executor it is not feasible to make the executor stateful at the moment.

esteve · 2015-07-29T16:44:31Z

@dirk-thomas I have a local branch that modifies the executors so that there's a global default one that can be reused instead of instantiating a new one.

dirk-thomas · 2015-07-29T18:51:03Z

#20 contains a test for the spinning problem.

ros2/rmw_connext#62 fixes the behavior of rmw_connext_cpp for subscriptions.

dirk-thomas · 2015-07-29T19:23:37Z

@esteve Can you please implement a similar fix as ros2/rmw_connext#62 (and for opensplice) to change the conditions for services too? I would expect that to fix this broken test.

esteve · 2015-07-29T19:26:32Z

@dirk-thomas I won't be able to do it today or tomorrow, so if you have the fix fresh in your mind, go for it. Thanks!

dirk-thomas · 2015-07-29T19:27:31Z

The patch will look exactly the same way as for the subscription. Nothing new or different, just effort to put it together.

It can wait until you (or @tfoote) have time for it.

tfoote · 2015-07-31T18:09:28Z

I've updated it to use gtest and actually test the results. It's revealed some underlying errors. The returned parameters have different values than the set parameters.

I will next look at the implementation to try to get the unit tests passing. I triggered a CI job to verify the same behavior on the farm: http://ci.ros2.org/job/ros2_batch_ci_linux/122/

$ ./test_parameters_cpp__rmw_connext_cpp 
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from parameters
[ RUN      ] parameters.test_parametesr
Parameter name: bar
Parameter name: foo
Parameter name: foo.first
/home/tfoote/work/ros2/test_param/src/ros2/system_tests/test_communication/test/test_parameters.cpp:57: Failure
Value of: name == "foo" || name == "bar"
  Actual: false
Expected: true
Parameter name: foo.second
/home/tfoote/work/ros2/test_param/src/ros2/system_tests/test_communication/test/test_parameters.cpp:57: Failure
Value of: name == "foo" || name == "bar"
  Actual: false
Expected: true
footruebool
/home/tfoote/work/ros2/test_param/src/ros2/system_tests/test_communication/test/test_parameters.cpp:69: Failure
Value of: parameter.to_string().c_str()
  Actual: "true"
Expected: "2"
/home/tfoote/work/ros2/test_param/src/ros2/system_tests/test_communication/test/test_parameters.cpp:70: Failure
Value of: parameter.get_type_name().c_str()
  Actual: "bool"
Expected: "int"
bar1.450000double
/home/tfoote/work/ros2/test_param/src/ros2/system_tests/test_communication/test/test_parameters.cpp:74: Failure
Value of: parameter.to_string().c_str()
  Actual: "1.450000"
Expected: "hello"
/home/tfoote/work/ros2/test_param/src/ros2/system_tests/test_communication/test/test_parameters.cpp:75: Failure
Value of: parameter.get_type_name().c_str()
  Actual: "double"
Expected: "string"
baz2integer
/home/tfoote/work/ros2/test_param/src/ros2/system_tests/test_communication/test/test_parameters.cpp:79: Failure
Value of: parameter.to_string().c_str()
  Actual: "2"
Expected: "1.45"
/home/tfoote/work/ros2/test_param/src/ros2/system_tests/test_communication/test/test_parameters.cpp:80: Failure
Value of: parameter.get_type_name().c_str()
  Actual: "integer"
Expected: "double"
tested parameters for 0.277822 seconds
[  FAILED  ] parameters.test_parametesr (3384 ms)
[----------] 1 test from parameters (3384 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (3384 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] parameters.test_parametesr

tfoote · 2015-08-03T23:30:36Z

ros2/rmw_connext#66 should fix the services issue

tfoote · 2015-08-19T22:25:46Z

This is blocked by #29 before it can be merged.

dirk-thomas · 2015-08-23T01:17:02Z

test_rclcpp/CMakeLists.txt

@@ -131,6 +133,42 @@ if(AMENT_ENABLE_TESTING)
        "rclcpp")
    endif()

+    ament_add_gtest(local_parameters_cpp__${middleware_impl} "test/local_parameters.cpp")
+    add_dependencies(local_parameters_cpp__${middleware_impl} ${PROJECT_NAME})


Before using the gtest target you must check that it actually exists (e.g. when gtest is not found):

if(TARGET local_parameters_cpp__${middleware_impl})

Same below.

I just added both to CMakeLists.txt, let me know if they look ok and I'll squash and merge this PR.

esteve · 2015-08-28T04:29:50Z

I've removed the tests for the to_json functions since it's not clear if ros2/rclcpp#91 will be merged soon as it is. I've pushed the tests to a separate branch https://github.com/ros2/system_tests/tree/test_json

esteve · 2015-08-28T04:46:37Z

Just triggered the CI jobs:

http://ci.ros2.org/job/ros2_batch_ci_linux/284/
http://ci.ros2.org/job/ros2_batch_ci_windows/284/
http://ci.ros2.org/job/ros2_batch_ci_osx/210/

tfoote · 2015-08-28T06:51:16Z

It looks like we still have intermittent failures in the services.

The OSX build failed with: test_requester_replier_cpp__empty__rmw_opensplice_cpp.test_requester_replier
And the linux build failed with
test_requester_replier_cpp__primitives__rmw_connext_cpp.test_requester_replier

I retriggered the linux and osx jobs:
http://ci.ros2.org/job/ros2_batch_ci_linux/289/
http://ci.ros2.org/job/ros2_batch_ci_osx/214/

Update: the above tests passed

Windows still has 28 failures which is our baseline at the moment.

esteve · 2015-08-28T17:53:52Z

@tfoote should we merge this PR? I don't see a reason why not, the intermittent failures are not introduced by this PR, and these changes will just make it easier to watch for errors in the parameters code.

dirk-thomas · 2015-08-28T19:11:06Z

test_rclcpp/CMakeLists.txt

@@ -135,6 +137,46 @@ if(AMENT_ENABLE_TESTING)
        "rclcpp")
    endif()

+    if(TARGET local_parameters_cpp__${middleware_impl})
+      ament_add_gtest(local_parameters_cpp__${middleware_impl} "test/local_parameters.cpp")


The cpp file contains a main function and should therefore not link against gtest's main function.

What's the solution here? Remove the main function from local_parameters.cpp or not add it with ament_add_gtest ?

There's an argument to SKIP_LINKING_MAIN_LIBRARIES I'll take care of that and clean up the whitespace above before we merge.

tfoote · 2015-08-28T19:39:31Z

I've amended for dirk's comment about the main. And cleared a few whitespace only changes.

CI pending:
http://ci.ros2.org/job/ros2_batch_ci_linux/293/
http://ci.ros2.org/job/ros2_batch_ci_osx/218/
http://ci.ros2.org/job/ros2_batch_ci_windows/292/

tfoote · 2015-08-28T21:04:26Z

All tests pass except the one lower level test, tickted in #37 http://ci.ros2.org/job/ros2_batch_ci_osx/218/testReport/junit/%28root%29/test_requester_replier_cpp__primitives__rmw_opensplice_cpp/test_requester_replier/

Add tests for parameters

tfoote added the in progress Actively being worked on (Kanban column) label Jul 7, 2015

dirk-thomas reviewed Jul 7, 2015
View reviewed changes

This was referenced Jul 8, 2015

add missing checks of the return for rmw publish / send calls ros2/rclcpp#53

Closed

add test to cover messages with an array of sub messages #15

Merged

esteve force-pushed the test_parameters branch 2 times, most recently from 0d91358 to 3094070 Compare July 28, 2015 20:26

tfoote mentioned this pull request Jul 28, 2015

Document how to write and run gtests ros2/ros2#76

Closed

dirk-thomas mentioned this pull request Jul 30, 2015

tests for services #22

Closed

tfoote force-pushed the test_parameters branch 2 times, most recently from db45df1 to 750b571 Compare July 31, 2015 18:01

tfoote force-pushed the test_parameters branch 3 times, most recently from 16b4bad to 29fa865 Compare August 3, 2015 21:24

tfoote mentioned this pull request Aug 3, 2015

fix returning from rmw_wait while any samples have not been taken yet: For Services ros2/rmw_connext#67

Closed

dirk-thomas reopened this Aug 4, 2015

dirk-thomas added in progress Actively being worked on (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Aug 4, 2015

tfoote force-pushed the test_parameters branch from d210d32 to 06e1628 Compare August 4, 2015 18:46

tfoote mentioned this pull request Aug 4, 2015

ParameterVariant constructor with hard coded string gets interpreted as bool ros2/rclcpp#75

Closed

dirk-thomas mentioned this pull request Aug 4, 2015

Add support for test processes ros2/launch#5

Closed

tfoote force-pushed the test_parameters branch from 06e1628 to 31bd174 Compare August 17, 2015 19:26

dirk-thomas mentioned this pull request Aug 17, 2015

This fixes the order of parameters returned ros2/rclcpp#90

Merged

tfoote force-pushed the test_parameters branch from e81a761 to bbe6adf Compare August 18, 2015 06:05

tfoote mentioned this pull request Aug 19, 2015

reiniting a node after destructing a parameter_service in opensplice causes a segfault #29

Closed

tfoote self-assigned this Aug 19, 2015

tfoote mentioned this pull request Aug 20, 2015

a method to dump all elements of a parameter as yaml ros2/rclcpp#91

Merged

tfoote force-pushed the test_parameters branch from 6e5336f to 45be86d Compare August 22, 2015 00:41

dirk-thomas reviewed Aug 23, 2015
View reviewed changes

esteve force-pushed the test_parameters branch from 45be86d to 618d89a Compare August 28, 2015 04:27

esteve force-pushed the test_parameters branch from 618d89a to 3a551ad Compare August 28, 2015 04:44

dirk-thomas reviewed Aug 28, 2015
View reviewed changes

Added tests for parameters

97efbef

tfoote force-pushed the test_parameters branch from 3a551ad to 97efbef Compare August 28, 2015 19:36

tfoote added a commit that referenced this pull request Aug 28, 2015

Merge pull request #14 from ros2/test_parameters

f913262

Add tests for parameters

tfoote merged commit f913262 into master Aug 28, 2015

tfoote removed the in progress Actively being worked on (Kanban column) label Aug 28, 2015

tfoote deleted the test_parameters branch August 28, 2015 21:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests for parameters #14

Add tests for parameters #14

tfoote commented Jul 7, 2015

dirk-thomas commented Jul 7, 2015

dirk-thomas commented Jul 7, 2015

dirk-thomas Jul 7, 2015

dirk-thomas commented Jul 7, 2015

dirk-thomas commented Jul 7, 2015

dirk-thomas commented Jul 8, 2015

esteve commented Jul 28, 2015

dirk-thomas commented Jul 29, 2015

esteve commented Jul 29, 2015

dirk-thomas commented Jul 29, 2015

dirk-thomas commented Jul 29, 2015

esteve commented Jul 29, 2015

dirk-thomas commented Jul 29, 2015

tfoote commented Jul 31, 2015

tfoote commented Aug 3, 2015

tfoote commented Aug 19, 2015

dirk-thomas Aug 23, 2015

esteve Aug 28, 2015

esteve commented Aug 28, 2015

esteve commented Aug 28, 2015

tfoote commented Aug 28, 2015

esteve commented Aug 28, 2015

dirk-thomas Aug 28, 2015

esteve Aug 28, 2015

tfoote Aug 28, 2015

tfoote commented Aug 28, 2015

tfoote commented Aug 28, 2015

		PROPERTIES DEPENDS "test_parameters_cpp__${middleware_impl1}"
		)

Add tests for parameters #14

Add tests for parameters #14

Conversation

tfoote commented Jul 7, 2015

dirk-thomas commented Jul 7, 2015

dirk-thomas commented Jul 7, 2015

dirk-thomas Jul 7, 2015

Choose a reason for hiding this comment

dirk-thomas commented Jul 7, 2015

dirk-thomas commented Jul 7, 2015

dirk-thomas commented Jul 8, 2015

esteve commented Jul 28, 2015

dirk-thomas commented Jul 29, 2015

esteve commented Jul 29, 2015

dirk-thomas commented Jul 29, 2015

dirk-thomas commented Jul 29, 2015

esteve commented Jul 29, 2015

dirk-thomas commented Jul 29, 2015

tfoote commented Jul 31, 2015

tfoote commented Aug 3, 2015

tfoote commented Aug 19, 2015

dirk-thomas Aug 23, 2015

Choose a reason for hiding this comment

esteve Aug 28, 2015

Choose a reason for hiding this comment

esteve commented Aug 28, 2015

esteve commented Aug 28, 2015

tfoote commented Aug 28, 2015

esteve commented Aug 28, 2015

dirk-thomas Aug 28, 2015

Choose a reason for hiding this comment

esteve Aug 28, 2015

Choose a reason for hiding this comment

tfoote Aug 28, 2015

Choose a reason for hiding this comment

tfoote commented Aug 28, 2015

tfoote commented Aug 28, 2015