Adapt to '--ros-args ... [--]'-based ROS args extraction #52
Conversation
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
hidmic
force-pushed
the
hidmic/ros-args-extraction
branch
from
1945644
to
31a4b53
Compare
| @@ -82,7 +82,5 @@ def test_push_ros_namespace(config): | |||
| node_namespace=config.node_ns, | |||
| ) | |||
| node._perform_substitutions(lc) | |||
| expected_cmd_len = 2 if config.expected_ns is not None else 1 | |||
| assert expected_cmd_len == len(node.cmd) | |||
There was a problem hiding this comment.
I think this should have been updated to:
expected_cmd_len = 3 if config.expected_ns is not None else 2
assert expected_cmd_len == len(node.cmd)Checking that __ns:=/ is not added when the expanded namespace is '' is important.
There was a problem hiding this comment.
The thing is that the original test was incorrect, as node.cmd substitutions are not resolved by node._perform_substitutions().
There was a problem hiding this comment.
The len of the list is the same before and after performing the substitutions, so it was ok correct.
There was a problem hiding this comment.
The len of the list is the same before and after performing the substitutions
That is a result of the current implementation, and thus not guaranteed nor enforced by the API.
Checking that __ns:=/ is not added when the expanded namespace is '' is important.
I agree with this, but the former test was not adequate for said purpose. We can change either launch_ros.actions.Node implementation or the test itself to query the fully resolved cmd instead of counting substitutions and expecting them to be right.
Precisely what the title says. Connected to ros2/rcl#477.