Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Verify rmw implementation of rclcpp examples at runtime #106

Merged
merged 2 commits into from
May 25, 2016

Conversation

dhood
Copy link
Member

@dhood dhood commented Apr 26, 2016

Modifies existing rclcpp_examples tests to act as a regression test for fda5938 (in comparison to #105 which adds new tests to explicitly check the linking)

Currently just for talker - once approved I can extend to the other executables

Note that I had to modify the exit handler used in the tests in order to get them to fail with the non-zero return code from talker (when fda5938 is reverted). This does not seem to have any negative side-effects (no regressions in CI). As a result, I am not entirely clear on the reason it was ignore_exit_handler before, given that it suppresses errors and seems to have been unnecessary. So, please be extra skeptical of this change in this PR.

http://ci.ros2.org/job/ci_windows/1265/
http://ci.ros2.org/job/ci_linux/1218/
http://ci.ros2.org/job/ci_osx/983/

Connects to #96

@wjwwood wjwwood added the in progress Actively being worked on (Kanban column) label Apr 26, 2016
rmw_identifier << "'" << std::endl;
return 1;
}
#endif
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the kind of thing that I didn't want to have in the examples. (I think it detracts from the otherwise simple example code)

I was thinking we could instead put the ability to check this into the rmw library itself. I never prototyped this, but I'm still interested in doing it.

If we merge this as-is, I'd want a TODO to remind us that there is probably a way to do it which doesn't affect the user code here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks - I'll integrate it

@dirk-thomas
Copy link
Member

Before our executable did not exit cleanly when they received a SIGINT. Therefore the tests needed to ignore their exit code using the ignore_exit_handler. Maybe that has changed in the mean time.

@tfoote
Copy link
Contributor

tfoote commented May 3, 2016

@dhood "should we have a macro that checks this for all tests which call it?": no, just opt in where we think it is needed.

@tfoote tfoote mentioned this pull request May 3, 2016
1 task
@dhood dhood force-pushed the test-linking-runtime branch 2 times, most recently from 8476fa1 to d31cee6 Compare May 17, 2016 19:01
@dhood dhood merged commit e798ff1 into ros2:master May 25, 2016
@dhood dhood deleted the test-linking-runtime branch May 25, 2016 01:46
@dhood dhood removed the in progress Actively being worked on (Kanban column) label May 25, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants