-
Notifications
You must be signed in to change notification settings - Fork 51
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
Test parameter behavior for rclpy nodes #293
Conversation
@@ -40,6 +40,7 @@ def __init__(self, command): | |||
|
|||
# Execute python files using same python used to start this test | |||
if command[0][-3:] == '.py': | |||
self._command = list(self._command) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was needed because the tuples that get passed as command
s are immutable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI failures look like they are due to missing commits from master branch on rclpy. LGTM assuming next CI is green.
test_cli/CMakeLists.txt
Outdated
@@ -37,6 +37,7 @@ if(BUILD_TESTING) | |||
PYTHON_EXECUTABLE "${_PYTHON_EXECUTABLE}" | |||
ENV | |||
INITIAL_PARAMS_RCLCPP=$<TARGET_FILE:initial_params_rclcpp> | |||
INITIAL_PARAMS_RCLPY=test/initial_params.py |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what I would have expected the working directory to be when the test is run. If this passes I guess that means it is the top level source directory containing CMakeLists.txt
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll see how it fares in CI. At the very least it #worksonmymachine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I think it's also reasonable to set the source directory either via CMAKE_CURRENT_LIST_DIR
or some other preferred variable for source paths.
Adds an rclpy node to the test fixtures so that our tests cover both client libraries.
For these tests to succeed they must be run along with ros2/rclpy#225
Connects to ros2/rclpy#202