Add tests for user-defined signal handler #215
Conversation
|
investigating windows failures and putting this back in progress in the meantime |
|
Should ticket it connects to be placed in progress as well ? |
|
So, this testing method won't work for windows. Quick fix is to disable the test on windows, but then we aren't testing on any platforms where Here are some options for getting the test coverage for the code path where
Though, this code path wasn't tested before either, so it might not be worth the complexity any of these will introduce. I'd appreciate some input from others on this. |
launch_testing will terminate the process instead of sending SIGINT, so the tests can't check the response to interrupt
|
I've skipped the tests for windows so that this PR can move ahead. If appropriate, code coverage for when sigaction isn't used can just be considered as an incremental improvement for a later date.
|
test_rclcpp/CMakeLists.txt
Outdated
| RMW_IMPLEMENTATION=${rmw_implementation} | ||
| ) | ||
| set_tests_properties(${test_name}${target_suffix} | ||
| PROPERTIES DEPENDS "${executable1}${target_suffix}" |
There was a problem hiding this comment.
The variable executable1 doesn't seem to exist.
| @@ -0,0 +1,51 @@ | |||
| # generated from test_rclcpp/test/test_two_executables.py.in | |||
| std::this_thread::sleep_for(5s); | ||
|
|
||
| printf("Exiting.\n"); | ||
| return 0; |
There was a problem hiding this comment.
Couldn't this test be done without any output comparison? The signal handler could set a global variable which could be checked in the main function afterwards. I think that would simplify the test.
There was a problem hiding this comment.
Just checking the global variable you wouldn't know if the user-defined signal handler has been called "directly" or from within the rclcpp signal handler.
Checking for the presence/absence of the signal_handler(2) output from rclcpp we can infer this without any changes to rclcpp. If we think it's useful in other contexts, we can expose a flag from rclcpp and switch the test to check both variables. Otherwise, for the purpose of this test the console checking can provide that extra info.
test_rclcpp/CMakeLists.txt
Outdated
| if(_ARG_SKIP_TEST) | ||
| set(_ARG_SKIP_TEST "SKIP_TEST") | ||
| endif() | ||
|
|
There was a problem hiding this comment.
Since this macro is only used for internal test I don't think this argument parsing is necessary.
There was a problem hiding this comment.
you're right, I thought it was necessary but it was only because I tried to pass ARGN as the last argument to ament_add_nose_test. Removed in 186356c
connects to ros2/rclcpp#353
This adds two tests.
rclcpp::inithas been called (we call it manually in the rclcpp signal handler).rclcpp::shutdownhas been called, and furthermore that the rclcpp signal handler is not called (notice that the output of that test expects nosignal_handler(2)print from rclcpp). The latter is only true with Restore old signal handler after shutdown rclcpp#353