Enable parameters tests #44

esteve · 2015-09-04T20:38:24Z

Make sure that parameters tests are built and run.

dirk-thomas · 2015-09-04T20:39:55Z

test_rclcpp/test/test_local_parameters.cpp

-
-int main(int argc, char ** argv)
-{
-  rclcpp::init(argc, argv);


Without this custom main rclcpp::init is not called anywhere.

Fixed, added rclcpp::init to the tests.

esteve · 2015-09-04T20:40:00Z

In faea4a5#diff-a9fbdd47e2e789e3da282a7c222d166dR158 it looks to me that the test will never be built, because the target is not set anywhere. Should that be changed?

dirk-thomas · 2015-09-04T20:42:41Z

ament_add_gtest(target ..) creates a target with the given name if gtest is available. If gtest is not available it should be mentioned as a warning during configuration but since we always have gtest_vendor as a fallback now that should never be the case.

esteve · 2015-09-04T20:45:09Z

@dirk-thomas is that a reply to my question or to something in the code of the pull request? If it's the former, I can't seem to find where the target test_remote_parameters_cpp__${middleware_impl}) is defined.

dirk-thomas · 2015-09-04T20:47:42Z

Sorry, I looked at the wrong target gtest_local_parameters__....

Yes, checking for the test_remote_parameters_cpp__... TARGET first and then creating it with add_executable seems wrong. I guess the check should be removed altogether.

esteve · 2015-09-04T20:50:54Z

@dirk-thomas no problem :-) I removed the check so that the executables are always built.

esteve · 2015-09-04T21:06:22Z

I've added a custom main to the timer tests, updated the PR description to reflect the changes and squashed the commits. Tests pass locally on Linux, these are the CI jobs:

http://ci.ros2.org/job/ros2_batch_ci_windows/378/ (pending, Windows slave is offline)
http://ci.ros2.org/job/ros2_batch_ci_osx/252/
http://ci.ros2.org/job/ros2_batch_ci_linux/322/

tests may on Windows, since services don't work on that platform (I'm working on that on another branch).

dirk-thomas · 2015-09-04T21:09:59Z

I have separated the SKIP_LINKING_MAIN_LIBRARIES option from the list of source files for better readability in 0f79015d6623a1847181c7ca0ebae346e16c151b.

esteve · 2015-09-04T21:10:47Z

@dirk-thomas thanks!

esteve · 2015-09-04T21:11:40Z

@dirk-thomas I just squashed the branch to include your change.

dirk-thomas · 2015-09-04T21:15:23Z

test_rclcpp/CMakeLists.txt

+    ament_target_dependencies(test_parameters_server_cpp__${middleware_impl}
+      "${middleware_impl}"
+      "rclcpp")
+    set(TEST_PARAMETERS_SERVER_EXECUTABLE "${CMAKE_CURRENT_BINARY_DIR}${test_executable_subfolder}/test_parameters_server_cpp__${middleware_impl}${test_executable_extension}")


The variable test_executable_extension seems not to be defined. In order to work on Windows I think something like ament/ament_cmake#28 will be necessary to point to the correct location of the .exe file.

There's this snippet in https://github.com/ros2/system_tests/blob/master/test_communication/CMakeLists.txt#L44 to set test_executable_extension for the tests in test_communication, but seems cumbersome to add it to every CMakeLists.txt

How would the approach from ament/ament_cmake#28 work? Using get_target_property(executable "${target}" LOCATION) ?

test_executable_extension is not sufficient. I don't think it works for the test_communication package.

Yes LOCATION, and replacing the configuration variable with a generator expression.

I changed CMakeLists.txt to use get_target_property(...), tests pass. If the changes look fine, I'll update test_communication to use the same approach too.

I ended up updating test_communication anyway :-)

dirk-thomas · 2015-09-08T20:39:12Z

Current CI not passing yet. Moving back to in-progress.

esteve · 2015-09-10T20:47:17Z

There's not much work left to do in this PR, the CI just shows that the parameters tests are not actually added to CMake on master.

esteve · 2015-09-11T00:16:37Z

I've pushed this branch as fix-windows-crash together with the other branches from the same set and the CI job only fails for one test, but I suspect it'll be fixed when Jackie merges the QoS fix:

http://ci.ros2.org/job/ros2_batch_ci_linux/339/

I couldn't get the tests to fail locally, though, despite multiple runs.

The Windows CI job fails because it can't find the GTest headers, I'm working on that:

http://ci.ros2.org/job/ros2_batch_ci_windows/389/

esteve · 2015-09-11T17:25:02Z

GTest include dirs fixed, moving back to review.

jacquelinekay · 2015-09-14T18:31:41Z

I see a few local test failures (Linux):

The following tests FAILED:
      6 - gtest_local_parameters__rmw_connext_cpp (Failed)
      7 - parameter_server_cpp__rmw_connext_cpp (Failed)
     13 - gtest_local_parameters__rmw_connext_dynamic_cpp (Timeout)
     14 - parameter_server_cpp__rmw_connext_dynamic_cpp (Failed)
     20 - gtest_local_parameters__rmw_opensplice_cpp (Failed)
     21 - parameter_server_cpp__rmw_opensplice_cpp (Timeout)

I built it with the services QoS fix but they still fail. I'm not sure if it's a problem with my local setup. Also, I wonder if it has to do with merging the fix_future changes, though the jenkins build for that branch was okay.

esteve · 2015-09-14T18:35:05Z

The Jenkins build passed becase these tests didn't run on master, the CMake targets were not added properly.

esteve · 2015-09-14T20:14:53Z

I've run this PR along with the fix-crash-windows branches rebased on top of the latest master and all tests pass locally on Linux. I'm going to check they run on Windows as well.

jacquelinekay · 2015-09-14T20:23:33Z

Are the fix-crash-windows branches in a PR somewhere? If they're needed for these tests to pass, they should get merged first. I think I'm seeing the same segfault on my Linux machine in set_parameters_atomically that is referenced in ros2/ros2#109.

jacquelinekay · 2015-09-16T17:44:55Z

On another note: the remote_parameters tests and parameters_server tests hang on my machine, but pass on Jenkins. Is there a step I'm missing here?

esteve · 2015-09-16T18:34:20Z

I've removed the get_target_property patch and these are new CI jobs:

http://ci.ros2.org/job/ros2_batch_ci_windows/409/
http://ci.ros2.org/job/ros2_batch_ci_osx/273/
http://ci.ros2.org/job/ros2_batch_ci_linux/364/

the timer test fails on OSX (which seems unrelated to these changes) and parameter_server_cpp__rmw_connext_dynamic_cpp on Linux. The latter seems to be the what @jacquelinekay has seen locally, my guess is that it could be related to the QoS fix for ros2/rmw_connext#96

dirk-thomas · 2015-09-16T21:48:08Z

Since the CI job links themself are not very informative I compared them to current jobs run against master:

The new parameter_server test on Linux (http://ci.ros2.org/job/ros2_batch_ci_linux/364/testReport/(root)/test_rclcpp/parameter_server_cpp__rmw_connext_dynamic_cpp_xunit_missing_result/) seems to timeout. Since it doesn't print anything I don't know what could cause that.

The failing test on OS X (http://ci.ros2.org/job/ros2_batch_ci_osx/273/testReport/(root)/test_time__rmw_connext_cpp/timer_during_wait/) seems to be the flaky one ticketed here: #36

On Windows it adds 14 new tests and fails 16 tests more than plain master. I can't see what the difference is due to the amount of failing tests.

jacquelinekay · 2015-09-16T21:51:32Z

It appears to me that on Windows all of the new parameters test
(local_synchronous, local_asynchronous, remote_paramters,
parameters_server) fail. So the new tests make up the majority of the new
failing tests.

http://ci.ros2.org/job/ros2_batch_ci_windows/409/testReport/%28root%29/parameters/local_synchronous/
http://ci.ros2.org/job/ros2_batch_ci_windows/409/testReport/%28root%29/parameters/local_asynchronous/
http://ci.ros2.org/job/ros2_batch_ci_windows/409/testReport/%28root%29/test_parameters_server_cpp__rmw_connext_cpp/test_remote_parameters/
http://ci.ros2.org/job/ros2_batch_ci_windows/409/testReport/projectrootC_.Jenkins.workspace.ros2_batch_ci_windows.workspace.src.ros2.system_tests/test_rclcpp/parameter_server_cpp__rmw_opensplice_cpp/

On Wed, Sep 16, 2015 at 2:48 PM, Dirk Thomas notifications@github.com
wrote:

Since the CI job links themself are not very informative I compared them
to current jobs run against master:

The new parameter_server test on Linux (
http://ci.ros2.org/job/ros2_batch_ci_linux/364/testReport/(root)/test_rclcpp/parameter_server_cpp__rmw_connext_dynamic_cpp_xunit_missing_result/)
seems to timeout. Since it doesn't print anything I don't know what could
cause that.

The failing test on OS X (
http://ci.ros2.org/job/ros2_batch_ci_osx/273/testReport/(root)/test_time__rmw_connext_cpp/timer_during_wait/)
seems to be the flaky one tickets here: #36
#36

On Windows it adds 14 new tests and fails 16 tests more than plain master.
I can't see what the difference is due to the amount of failing tests.

—
Reply to this email directly or view it on GitHub
#44 (comment).

dirk-thomas · 2015-09-16T22:11:25Z

Especially the tests failing with The system cannot find the file specified should be straight forward to fix.

esteve · 2015-09-16T22:20:15Z

@dirk-thomas when the parameter_server_cpp__rmw_connext_dynamic_cpp test is run manually, the client gets stuck waiting for a response from the server, but never occurs. The branches for ros2/rmw_connext#96 seem to fix it.

@jacquelinekay Windows is currently a bit of a mess, many other tests fail too. I'll fix the ones that can't find the executables, but the rest will require more work.

esteve · 2015-09-17T16:39:04Z

I've replaced get_target_property logic with setting the path to the binaries manually and the test now can find the executables, though some still fail on Windows.

esteve · 2015-10-13T21:31:14Z

CI jobs:

http://ci.ros2.org/job/ros2_batch_ci_linux/430/
http://ci.ros2.org/job/ros2_batch_ci_osx/340/
http://ci.ros2.org/job/ros2_batch_ci_windows/482/

esteve · 2015-10-22T00:43:15Z

This now passes on Windows locally, let's see if it does on the farm:

http://ci.ros2.org/job/ros2_batch_ci_linux/453/
http://ci.ros2.org/job/ros2_batch_ci_osx/365/
http://ci.ros2.org/job/ros2_batch_ci_windows/510/

esteve · 2015-10-22T17:53:57Z

CI after rebasing against master:

http://ci.ros2.org/job/ros2_batch_ci_linux/457/
http://ci.ros2.org/job/ros2_batch_ci_osx/369/
http://ci.ros2.org/job/ros2_batch_ci_windows/513/

previous run failed on Windows, but I can't reproduce it locally after multiple runs.

esteve · 2015-10-22T18:34:56Z

So it seems the failing tests on Windows are only for RTI Connext, this might be related to ros2/rmw_connext#31

The OpenSplice versions of the tests pass on Windows:

http://ci.ros2.org/job/ros2_batch_ci_windows/513/testReport/%28root%29/test_local_parameters__rmw_opensplice_cpp/
http://ci.ros2.org/job/ros2_batch_ci_windows/513/testReport/%28root%29/test_parameters_server_cpp__rmw_opensplice_cpp/

jacquelinekay · 2015-10-23T17:47:00Z

+1 with squash. I am also convinced that the failures on Windows are due to the requester/replier issue.

esteve · 2015-10-23T17:53:20Z

Squashed and merging now.

Enable parameters tests

esteve added the in progress Actively being worked on (Kanban column) label Sep 4, 2015

dirk-thomas reviewed Sep 4, 2015
View reviewed changes

esteve added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Sep 4, 2015

esteve self-assigned this Sep 4, 2015

esteve changed the title ~~Adapt to use the same GTest structure~~ tests cleanups Sep 4, 2015

esteve force-pushed the gtest-parameters branch from c5a55b6 to 2c4d814 Compare September 4, 2015 21:02

esteve force-pushed the gtest-parameters branch from 0f79015 to 7d512b8 Compare September 4, 2015 21:11

dirk-thomas reviewed Sep 4, 2015
View reviewed changes

dirk-thomas added in progress Actively being worked on (Kanban column) and removed in review Waiting for review (Kanban column) labels Sep 8, 2015

esteve added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Sep 11, 2015

esteve force-pushed the gtest-parameters branch from a2548d2 to 84dc22f Compare October 13, 2015 21:29

esteve added in progress Actively being worked on (Kanban column) and removed in review Waiting for review (Kanban column) labels Oct 14, 2015

esteve force-pushed the gtest-parameters branch 3 times, most recently from 233cf91 to 5024493 Compare October 22, 2015 00:41

esteve added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Oct 22, 2015

esteve mentioned this pull request Oct 22, 2015

Pass weak pointer to callback ros2/rclcpp#136

Merged

esteve force-pushed the gtest-parameters branch from 5024493 to 115a6e0 Compare October 22, 2015 16:17

esteve mentioned this pull request Oct 22, 2015

Alpha 2 release (2015-10-30) ros2/ros2#131

Closed

21 tasks

Enable parameters tests

dde3a31

esteve force-pushed the gtest-parameters branch from 78e4bf0 to dde3a31 Compare October 23, 2015 17:53

esteve added a commit that referenced this pull request Oct 23, 2015

Merge pull request #44 from ros2/gtest-parameters

cacc2e4

Enable parameters tests

esteve merged commit cacc2e4 into master Oct 23, 2015

esteve removed the in review Waiting for review (Kanban column) label Oct 23, 2015

esteve deleted the gtest-parameters branch October 23, 2015 17:53

Enable parameters tests #44

Enable parameters tests #44

Conversation

esteve commented Sep 4, 2015

dirk-thomas Sep 4, 2015

Choose a reason for hiding this comment

esteve Sep 4, 2015

Choose a reason for hiding this comment

esteve commented Sep 4, 2015

dirk-thomas commented Sep 4, 2015

esteve commented Sep 4, 2015

dirk-thomas commented Sep 4, 2015

esteve commented Sep 4, 2015

esteve commented Sep 4, 2015

dirk-thomas commented Sep 4, 2015

esteve commented Sep 4, 2015

esteve commented Sep 4, 2015

dirk-thomas Sep 4, 2015

Choose a reason for hiding this comment

esteve Sep 4, 2015

Choose a reason for hiding this comment

dirk-thomas Sep 4, 2015

Choose a reason for hiding this comment

esteve Sep 4, 2015

Choose a reason for hiding this comment

esteve Sep 4, 2015

Choose a reason for hiding this comment

dirk-thomas commented Sep 8, 2015

esteve commented Sep 10, 2015

esteve commented Sep 11, 2015

esteve commented Sep 11, 2015

jacquelinekay commented Sep 14, 2015

esteve commented Sep 14, 2015

esteve commented Sep 14, 2015

jacquelinekay commented Sep 14, 2015

jacquelinekay commented Sep 16, 2015

esteve commented Sep 16, 2015

dirk-thomas commented Sep 16, 2015

jacquelinekay commented Sep 16, 2015

dirk-thomas commented Sep 16, 2015

esteve commented Sep 16, 2015

esteve commented Sep 17, 2015

esteve commented Oct 13, 2015

esteve commented Oct 22, 2015

esteve commented Oct 22, 2015

esteve commented Oct 22, 2015

jacquelinekay commented Oct 23, 2015

esteve commented Oct 23, 2015