From a6eb611676b77da6329de0eb2c3a429ad5f1835b Mon Sep 17 00:00:00 2001 From: Shane Loretz Date: Tue, 14 Feb 2023 17:49:41 +0000 Subject: [PATCH 01/20] First pass discovery tests Signed-off-by: Shane Loretz --- test_discovery/CMakeLists.txt | 39 +++++++ test_discovery/README.md | 0 test_discovery/package.xml | 30 ++++++ test_discovery/scripts/conftest.py | 7 ++ test_discovery/scripts/run_tests.py | 94 ++++++++++++++++ test_discovery/scripts/test_discovery.py | 132 +++++++++++++++++++++++ test_discovery/src/publish_once.cpp | 46 ++++++++ test_discovery/src/subscribe_once.cpp | 44 ++++++++ 8 files changed, 392 insertions(+) create mode 100644 test_discovery/CMakeLists.txt create mode 100644 test_discovery/README.md create mode 100644 test_discovery/package.xml create mode 100644 test_discovery/scripts/conftest.py create mode 100644 test_discovery/scripts/run_tests.py create mode 100644 test_discovery/scripts/test_discovery.py create mode 100644 test_discovery/src/publish_once.cpp create mode 100644 test_discovery/src/subscribe_once.cpp diff --git a/test_discovery/CMakeLists.txt b/test_discovery/CMakeLists.txt new file mode 100644 index 00000000..9e844f97 --- /dev/null +++ b/test_discovery/CMakeLists.txt @@ -0,0 +1,39 @@ +cmake_minimum_required(VERSION 3.5) +project(test_discovery) + +find_package(ament_cmake_ros REQUIRED) +find_package(rclcpp REQUIRED) +find_package(test_msgs REQUIRED) + +add_executable(publish_once src/publish_once.cpp) +target_compile_features(publish_once PUBLIC cxx_std_17) +target_link_libraries(publish_once PRIVATE + rclcpp::rclcpp + ${test_msgs_TARGETS} +) + +add_executable(subscribe_once src/subscribe_once.cpp) +target_compile_features(subscribe_once PUBLIC cxx_std_17) +target_link_libraries(subscribe_once PRIVATE + rclcpp::rclcpp + ${test_msgs_TARGETS} +) + +install(TARGETS publish_once subscribe_once + DESTINATION lib/${PROJECT_NAME}) + +install(FILES + scripts/test_discovery.py + scripts/conftest.py + DESTINATION share/${PROJECT_NAME}/tests/) + +install(PROGRAMS + scripts/run_tests.py + DESTINATION lib/${PROJECT_NAME}/) + +if(BUILD_TESTING) + find_package(ament_lint_auto REQUIRED) + ament_lint_auto_find_test_dependencies() +endif() + +ament_package() \ No newline at end of file diff --git a/test_discovery/README.md b/test_discovery/README.md new file mode 100644 index 00000000..e69de29b diff --git a/test_discovery/package.xml b/test_discovery/package.xml new file mode 100644 index 00000000..a82be9cf --- /dev/null +++ b/test_discovery/package.xml @@ -0,0 +1,30 @@ + + + + test_discovery + 0.14.0 + + Test discovery behaviors in ROS 2 using semiautomated tests. + + + Brandon Ong + + Apache License 2.0 + + Shane Loretz + + ament_cmake_ros + + + rclcpp + test_msgs + pytest + + ament_lint_auto + ament_lint_common + test_msgs + + + ament_cmake + + diff --git a/test_discovery/scripts/conftest.py b/test_discovery/scripts/conftest.py new file mode 100644 index 00000000..edf8e16f --- /dev/null +++ b/test_discovery/scripts/conftest.py @@ -0,0 +1,7 @@ +def pytest_addoption(parser): + parser.addoption("--ros-workspaces", action="store") + parser.addoption("--rmws", action="store") + +def pytest_generate_tests(metafunc): + if 'rmw' in metafunc.fixturenames: + metafunc.parametrize("rmw", metafunc.config.option.rmws.split(':')) \ No newline at end of file diff --git a/test_discovery/scripts/run_tests.py b/test_discovery/scripts/run_tests.py new file mode 100644 index 00000000..2c711853 --- /dev/null +++ b/test_discovery/scripts/run_tests.py @@ -0,0 +1,94 @@ +#!/usr/bin/env python3 + +# Copyright 2015 Open Source Robotics Foundation, Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import argparse +import os +import shlex +import sys + +from ament_index_python import get_package_share_path +from ament_index_python import get_resources + + + +def get_rmw_implementations(): + resources = list(get_resources('rmw_typesupport').keys()) + if 'rmw_implementation' in resources: + resources.remove('rmw_implementation') + return tuple(resources) + + +def get_tests_dir(): + pkg_path = get_package_share_path('test_discovery') + return pkg_path / "tests" + + +def get_workspaces(): + # Get an ordered list of workspaces that are sourced + prefixes = os.environ['AMENT_PREFIX_PATH'] + if not prefixes: + raise ValueError('No ROS/Colcon workspace sourced') + workspaces = set() + for prefix in prefixes.split(':'): + if not prefix: + # env var might have began or ended with a ':' + continue + # If there exists a parent folder containing a setup.bash + # then assume this is an isolated colcon workspace + if os.path.exists(os.path.join(prefix, "../setup.bash")): + workspaces.add(os.path.dirname(prefix)) + else: + # Assume a merged ament/colcon workspace + workspaces.add(prefix) + return tuple(workspaces) + + +def main(): + parser = argparse.ArgumentParser() + parser.add_argument('--rmw', default=None) + parser.add_argument('--select', default=None) + args = parser.parse_args() + + rmw_implementations = get_rmw_implementations() + if args.rmw: + if args.rmw not in rmw_implementations: + raise ValueError(f'{args.rmw} is not an installed rmw: {rmw_implementations}') + rmw_implementations = [args.rmw] + + cmd = [] + cmd.append('sudo') + cmd.append(sys.executable) + cmd.append('-m') + cmd.append('pytest') + cmd.append('-c') + cmd.append(str(get_tests_dir() / "conftest.py")) + if args.select: + cmd.append('-k') + cmd.append(args.select) + cmd.append(f'--rmws={":".join(rmw_implementations)}') + cmd.append(f'--ros-workspaces={":".join(get_workspaces())}') + cmd.append(str(get_tests_dir() / "test_discovery.py")) + + # find path to test_discovery package, use that for location to conftest.py and test_discovery.py + print("Executing the following command:") + print("================================") + print("$", *cmd) + print("================================") + + os.execvp(cmd[0], cmd) + +if __name__ == '__main__': + main() \ No newline at end of file diff --git a/test_discovery/scripts/test_discovery.py b/test_discovery/scripts/test_discovery.py new file mode 100644 index 00000000..b7588211 --- /dev/null +++ b/test_discovery/scripts/test_discovery.py @@ -0,0 +1,132 @@ +# Run discovery tests +# 1. Install dependencies +# sudo apt install iputils-ping iproute2 mininet +# TODO(sloretz) not needed? openvswitch-switch openvswitch-testcontroller +# 2. TODO(sloretz) not needed? Start openvswitch service if not already running +# sudo service openvswitch-switch start +# 3. Run this file as root +# cd path/to/dir/containing/this/file/ +# sudo python3 -m pytest -c conftest.py --ros-workspace ~/ws/ros2/install ./test_all.py +# 3a. Use -k to limit which rmw implementation +# sudo python3 -m pytest -k rmw_fastrtps_cpp -c conftest.py --ros-workspace ~/ws/ros2/install +# 3b. Running one test with all output +# sudo python3 -m pytest -vv -k 'test_samehost[no_peer-LOCALHOST-no_peer-LOCALHOST-rmw_fastrtps_cpp]' -c conftest.py --ros-workspace ~/ws/ros2/install ./test_all.py + +import pytest +from mininet.net import Mininet +from mininet.util import dumpNetConnections +from mininet.topo import MinimalTopo + + +RANGES = [ + "OFF", + "SUBNET", + "LOCALHOST", +] + + +class MininetFixture: + __slots__ = ( + 'net', + 'h1', + 'h2', + ) + + +def h1_ipv4(net: MininetFixture) -> str: + return net.h1.IP() + + +def h2_ipv4(net: MininetFixture) -> str: + return net.h2.IP() + + +def no_peer(net: MininetFixture) -> str: + return "" + + +@pytest.fixture() +def mn(): + f = MininetFixture() + f.net = Mininet(topo=MinimalTopo()) + f.h1 = f.net.getNodeByName('h1') + f.h2 = f.net.getNodeByName('h2') + + f.net.start() + yield f + f.net.stop() + + +# TODO(sloretz) figure out ROS workspace path from environment variables +@pytest.fixture(scope="session") +def ros_ws(pytestconfig): + return pytestconfig.getoption("ros_workspaces").split(':') + + +def make_env_str(ros_ws, rmw, range, peer): + cmd = [] + for ws in ros_ws: + cmd.append('.') + cmd.append(f'"{ws}/setup.bash"') + cmd.append('&&') + cmd.append(f'RMW_IMPLEMENTATION={rmw}') + cmd.append(f'ROS_AUTOMATIC_DISCOVERY_RANGE={range}') + cmd.append(f'ROS_STATIC_PEERS="{peer}"') + cmd.append(' ') + return ' '.join(cmd) + +# TODO samehost doesn't require mininet does it. Could do this with normal testing +@pytest.mark.parametrize("sub_peer", (no_peer, h1_ipv4)) +@pytest.mark.parametrize("sub_range", RANGES) +@pytest.mark.parametrize("pub_peer", (no_peer, h1_ipv4)) +@pytest.mark.parametrize("pub_range", RANGES) +def test_samehost(mn, ros_ws, rmw, pub_range, pub_peer, sub_range, sub_peer): + pub_peer = pub_peer(mn) + sub_peer = sub_peer(mn) + + pub_cmd = make_env_str(ros_ws, rmw, pub_range, pub_peer) + 'ros2 run test_discovery publish_once > /dev/null &' + sub_cmd = make_env_str(ros_ws, rmw, sub_range, sub_peer) + 'ros2 run test_discovery subscribe_once' + print("$", pub_cmd) + print("$", sub_cmd) + + mn.h1.cmd(pub_cmd) + result = mn.h1.cmd(sub_cmd) + message_received = "test_discovery: message was received" in result.strip() + + if pub_peer or sub_peer: + # if either has a static peer set, discovery should succeed + assert message_received, result.strip() + elif "OFF" in (pub_range, sub_range): + # With no static peer, if either has discovery off then it won't succeed + assert not message_received, result.strip() + else: + # All other cases discovery + assert message_received, result.strip() + + +@pytest.mark.parametrize("sub_peer", (no_peer, h1_ipv4)) +@pytest.mark.parametrize("sub_range", RANGES) +@pytest.mark.parametrize("pub_peer", (no_peer, h2_ipv4)) +@pytest.mark.parametrize("pub_range", RANGES) +def test_differenthost(mn, ros_ws, rmw, pub_range, pub_peer, sub_range, sub_peer): + pub_peer = pub_peer(mn) + sub_peer = sub_peer(mn) + + pub_cmd = make_env_str(ros_ws, rmw, pub_range, pub_peer) + 'ros2 run test_discovery publish_once > /dev/null &' + sub_cmd = make_env_str(ros_ws, rmw, sub_range, sub_peer) + 'ros2 run test_discovery subscribe_once' + print("$", pub_cmd) + print("$", sub_cmd) + + mn.h1.cmd(pub_cmd) + result = mn.h2.cmd(sub_cmd) + message_received = "test_discovery: message was received" in result.strip() + + if pub_peer or sub_peer: + # if either has a static peer set, discovery should succeed + assert message_received, result.strip() + elif "SUBNET" == pub_range and "SUBNET" == sub_range: + # With no static peer, succeed only if both are set to SUBNET + assert message_received, result.strip() + else: + # All other cases discovery + assert not message_received, result.strip() diff --git a/test_discovery/src/publish_once.cpp b/test_discovery/src/publish_once.cpp new file mode 100644 index 00000000..6f0c1052 --- /dev/null +++ b/test_discovery/src/publish_once.cpp @@ -0,0 +1,46 @@ +// Copyright 2023 Open Source Robotics Foundation, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include +#include + +#include +#include + + +constexpr double kMaxDiscoveryTime = 5; + + +int main(int argc, char * argv[]) +{ + rclcpp::init(argc, argv); + auto node = rclcpp::Node::make_shared("publish_once"); + auto publisher = node->create_publisher("test_topic", 10); + + auto clock = node->get_clock(); + + auto end_time = clock->now() + rclcpp::Duration::from_seconds(kMaxDiscoveryTime); + while (rclcpp::ok() && publisher->get_subscription_count() == 0) { + if (clock->now() >= end_time) { + return 0; + } + rclcpp::sleep_for(std::chrono::milliseconds(100)); + } + + publisher->publish(test_msgs::msg::Builtins()); + + // Do nothing until killed. + rclcpp::spin(node); + return 0; +} diff --git a/test_discovery/src/subscribe_once.cpp b/test_discovery/src/subscribe_once.cpp new file mode 100644 index 00000000..af8dc62c --- /dev/null +++ b/test_discovery/src/subscribe_once.cpp @@ -0,0 +1,44 @@ +// Copyright 2023 Open Source Robotics Foundation, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include +#include + +#include +#include + +constexpr double kTimeout = 10; + +void topic_callback(const test_msgs::msg::Builtins & ) +{ + std::cout << "test_discovery: message was received\n" << std::flush; + std::exit(0); +} + +int main(int argc, char * argv[]) +{ + rclcpp::init(argc, argv); + auto node = rclcpp::Node::make_shared("subscribe_once"); + auto subscription = + node->create_subscription("test_topic", 10, topic_callback); + + auto clock = node->get_clock(); + auto end_time = clock->now() + rclcpp::Duration::from_seconds(kTimeout); + while (rclcpp::ok() && clock->now() <= end_time ) { + rclcpp::spin_some(node); + rclcpp::sleep_for(std::chrono::milliseconds(100)); + } + + return 0; +} From 01ac3c5984081fdfad4fd4614ddb0dbebde0bfdd Mon Sep 17 00:00:00 2001 From: Shane Loretz Date: Tue, 14 Feb 2023 18:06:06 +0000 Subject: [PATCH 02/20] Rename to rootests Signed-off-by: Shane Loretz --- test_discovery/CMakeLists.txt | 6 +++--- test_discovery/{scripts => roottests}/conftest.py | 0 test_discovery/{scripts => roottests}/test_discovery.py | 0 test_discovery/scripts/run_tests.py | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) rename test_discovery/{scripts => roottests}/conftest.py (100%) rename test_discovery/{scripts => roottests}/test_discovery.py (100%) diff --git a/test_discovery/CMakeLists.txt b/test_discovery/CMakeLists.txt index 9e844f97..eb073702 100644 --- a/test_discovery/CMakeLists.txt +++ b/test_discovery/CMakeLists.txt @@ -23,9 +23,9 @@ install(TARGETS publish_once subscribe_once DESTINATION lib/${PROJECT_NAME}) install(FILES - scripts/test_discovery.py - scripts/conftest.py - DESTINATION share/${PROJECT_NAME}/tests/) + roottests/test_discovery.py + roottests/conftest.py + DESTINATION share/${PROJECT_NAME}/roottests/) install(PROGRAMS scripts/run_tests.py diff --git a/test_discovery/scripts/conftest.py b/test_discovery/roottests/conftest.py similarity index 100% rename from test_discovery/scripts/conftest.py rename to test_discovery/roottests/conftest.py diff --git a/test_discovery/scripts/test_discovery.py b/test_discovery/roottests/test_discovery.py similarity index 100% rename from test_discovery/scripts/test_discovery.py rename to test_discovery/roottests/test_discovery.py diff --git a/test_discovery/scripts/run_tests.py b/test_discovery/scripts/run_tests.py index 2c711853..8ddac01b 100644 --- a/test_discovery/scripts/run_tests.py +++ b/test_discovery/scripts/run_tests.py @@ -33,7 +33,7 @@ def get_rmw_implementations(): def get_tests_dir(): pkg_path = get_package_share_path('test_discovery') - return pkg_path / "tests" + return pkg_path / "roottests" def get_workspaces(): From 7b659b046ffedbb2640c31fea0db168783460843 Mon Sep 17 00:00:00 2001 From: Shane Loretz Date: Tue, 14 Feb 2023 18:07:47 +0000 Subject: [PATCH 03/20] run_tests -> run_root_tests Signed-off-by: Shane Loretz --- test_discovery/CMakeLists.txt | 2 +- test_discovery/scripts/{run_tests.py => run_root_tests.py} | 0 2 files changed, 1 insertion(+), 1 deletion(-) rename test_discovery/scripts/{run_tests.py => run_root_tests.py} (100%) diff --git a/test_discovery/CMakeLists.txt b/test_discovery/CMakeLists.txt index eb073702..0cd5dabd 100644 --- a/test_discovery/CMakeLists.txt +++ b/test_discovery/CMakeLists.txt @@ -28,7 +28,7 @@ install(FILES DESTINATION share/${PROJECT_NAME}/roottests/) install(PROGRAMS - scripts/run_tests.py + scripts/run_root_tests.py DESTINATION lib/${PROJECT_NAME}/) if(BUILD_TESTING) diff --git a/test_discovery/scripts/run_tests.py b/test_discovery/scripts/run_root_tests.py similarity index 100% rename from test_discovery/scripts/run_tests.py rename to test_discovery/scripts/run_root_tests.py From 8b3c56050699841992acd1104e218cbb5e511c44 Mon Sep 17 00:00:00 2001 From: Shane Loretz Date: Tue, 14 Feb 2023 19:22:11 +0000 Subject: [PATCH 04/20] Add thishost tests that don't use mininet Signed-off-by: Shane Loretz --- test_discovery/CMakeLists.txt | 2 + test_discovery/tests/test_discovery.py | 103 +++++++++++++++++++++++++ 2 files changed, 105 insertions(+) create mode 100644 test_discovery/tests/test_discovery.py diff --git a/test_discovery/CMakeLists.txt b/test_discovery/CMakeLists.txt index 0cd5dabd..c35dca89 100644 --- a/test_discovery/CMakeLists.txt +++ b/test_discovery/CMakeLists.txt @@ -34,6 +34,8 @@ install(PROGRAMS if(BUILD_TESTING) find_package(ament_lint_auto REQUIRED) ament_lint_auto_find_test_dependencies() + + ament_add_pytest_test(test_discovery tests/test_discovery.py TIMEOUT 600) endif() ament_package() \ No newline at end of file diff --git a/test_discovery/tests/test_discovery.py b/test_discovery/tests/test_discovery.py new file mode 100644 index 00000000..483392fe --- /dev/null +++ b/test_discovery/tests/test_discovery.py @@ -0,0 +1,103 @@ +# Copyright 2023 Open Source Robotics Foundation, Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import os +import pathlib +import subprocess +import sys + +from ament_index_python import get_package_prefix +from ament_index_python import get_resources +import pytest + + +RANGES = [ + "OFF", + "SUBNET", + "LOCALHOST", +] + + +PEERS = [ + None, + "127.0.0.1", +] + +def get_rmw_implementations(): + resources = list(get_resources('rmw_typesupport').keys()) + if 'rmw_implementation' in resources: + resources.remove('rmw_implementation') + return tuple(resources)[:1] + + +def get_executable(name): + p = pathlib.Path(get_package_prefix('test_discovery')) + return str(p / 'lib' / 'test_discovery' / name) + + +def make_env(rmw, range, peer): + env = dict(os.environ) + env['RMW_IMPLEMENTATION'] = rmw + env['ROS_AUTOMATIC_DISCOVERY_RANGE'] = range + if peer is None: + peer = '' + env['ROS_STATIC_PEERS'] = peer + return env + + +def communicate(name, proc): + try: + stdout, stderr = proc.communicate(timeout=10) + except subprocess.TimeoutExpired: + proc.kill() + stdout, stderr = proc.communicate() + stdout = stdout.decode() + stderr = stderr.decode() + for line in stdout.split('\n'): + sys.stdout.write(f'{name}[stdout]: {line}\n') + for line in stderr.split('\n'): + sys.stderr.write(f'{name}[stderr]: {line}\n') + return stdout, stderr + + +@pytest.mark.parametrize("sub_peer", PEERS) +@pytest.mark.parametrize("sub_range", RANGES) +@pytest.mark.parametrize("pub_peer", PEERS) +@pytest.mark.parametrize("pub_range", RANGES) +@pytest.mark.parametrize("rmw", get_rmw_implementations()) +def test_thishost(rmw, pub_range, pub_peer, sub_range, sub_peer): + pub_env = make_env(rmw, pub_range, pub_peer) + sub_env = make_env(rmw, sub_range, sub_peer) + pub_cmd = [get_executable('publish_once')] + sub_cmd = [get_executable('subscribe_once')] + + pub_proc = subprocess.Popen( + pub_cmd, env=pub_env, stdout=subprocess.PIPE, stderr=subprocess.PIPE) + sub_proc = subprocess.Popen( + sub_cmd, env=sub_env, stdout=subprocess.PIPE, stderr=subprocess.PIPE) + + stdout, _ = communicate('sub', sub_proc) + message_received = "test_discovery: message was received" in stdout + pub_proc.kill() + communicate('pub', pub_proc) + + if pub_peer or sub_peer: + # if either has a static peer set, discovery should succeed + assert message_received + elif "OFF" in (pub_range, sub_range): + # With no static peer, if either has discovery off then it won't succeed + assert not message_received + else: + # All other cases discovery + assert message_received From 2153480aeeb2d8227357051a59f9e086a7c54ef2 Mon Sep 17 00:00:00 2001 From: Shane Loretz Date: Tue, 14 Feb 2023 19:22:25 +0000 Subject: [PATCH 05/20] Minor tweaks Signed-off-by: Shane Loretz --- test_discovery/scripts/run_root_tests.py | 1 - test_discovery/src/publish_once.cpp | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/test_discovery/scripts/run_root_tests.py b/test_discovery/scripts/run_root_tests.py index 8ddac01b..ec59ec51 100644 --- a/test_discovery/scripts/run_root_tests.py +++ b/test_discovery/scripts/run_root_tests.py @@ -23,7 +23,6 @@ from ament_index_python import get_resources - def get_rmw_implementations(): resources = list(get_resources('rmw_typesupport').keys()) if 'rmw_implementation' in resources: diff --git a/test_discovery/src/publish_once.cpp b/test_discovery/src/publish_once.cpp index 6f0c1052..a58fabf0 100644 --- a/test_discovery/src/publish_once.cpp +++ b/test_discovery/src/publish_once.cpp @@ -19,7 +19,7 @@ #include -constexpr double kMaxDiscoveryTime = 5; +constexpr double kMaxDiscoveryTime = 10; int main(int argc, char * argv[]) From 3ad7bf56e31c55d4592233b2d1c7a3257fc87563 Mon Sep 17 00:00:00 2001 From: Shane Loretz Date: Tue, 14 Feb 2023 19:56:16 +0000 Subject: [PATCH 06/20] Linting Signed-off-by: Shane Loretz --- test_discovery/CMakeLists.txt | 2 +- test_discovery/roottests/conftest.py | 21 +++++- test_discovery/roottests/test_discovery.py | 87 ++++++++++++---------- test_discovery/scripts/run_root_tests.py | 23 +++--- test_discovery/src/subscribe_once.cpp | 4 +- test_discovery/tests/test_discovery.py | 29 ++++---- 6 files changed, 95 insertions(+), 71 deletions(-) diff --git a/test_discovery/CMakeLists.txt b/test_discovery/CMakeLists.txt index c35dca89..b1c88145 100644 --- a/test_discovery/CMakeLists.txt +++ b/test_discovery/CMakeLists.txt @@ -35,7 +35,7 @@ if(BUILD_TESTING) find_package(ament_lint_auto REQUIRED) ament_lint_auto_find_test_dependencies() - ament_add_pytest_test(test_discovery tests/test_discovery.py TIMEOUT 600) + ament_add_pytest_test(test_discovery tests/test_discovery.py TIMEOUT 800) endif() ament_package() \ No newline at end of file diff --git a/test_discovery/roottests/conftest.py b/test_discovery/roottests/conftest.py index edf8e16f..fa9ded54 100644 --- a/test_discovery/roottests/conftest.py +++ b/test_discovery/roottests/conftest.py @@ -1,7 +1,22 @@ +# Copyright 2023 Open Source Robotics Foundation, Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + def pytest_addoption(parser): - parser.addoption("--ros-workspaces", action="store") - parser.addoption("--rmws", action="store") + parser.addoption('--ros-workspaces', action='store') + parser.addoption('--rmws', action='store') + def pytest_generate_tests(metafunc): if 'rmw' in metafunc.fixturenames: - metafunc.parametrize("rmw", metafunc.config.option.rmws.split(':')) \ No newline at end of file + metafunc.parametrize('rmw', metafunc.config.option.rmws.split(':')) diff --git a/test_discovery/roottests/test_discovery.py b/test_discovery/roottests/test_discovery.py index b7588211..4e38ba9c 100644 --- a/test_discovery/roottests/test_discovery.py +++ b/test_discovery/roottests/test_discovery.py @@ -1,27 +1,33 @@ +# Copyright 2023 Open Source Robotics Foundation, Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + # Run discovery tests # 1. Install dependencies # sudo apt install iputils-ping iproute2 mininet # TODO(sloretz) not needed? openvswitch-switch openvswitch-testcontroller # 2. TODO(sloretz) not needed? Start openvswitch service if not already running # sudo service openvswitch-switch start -# 3. Run this file as root -# cd path/to/dir/containing/this/file/ -# sudo python3 -m pytest -c conftest.py --ros-workspace ~/ws/ros2/install ./test_all.py -# 3a. Use -k to limit which rmw implementation -# sudo python3 -m pytest -k rmw_fastrtps_cpp -c conftest.py --ros-workspace ~/ws/ros2/install -# 3b. Running one test with all output -# sudo python3 -m pytest -vv -k 'test_samehost[no_peer-LOCALHOST-no_peer-LOCALHOST-rmw_fastrtps_cpp]' -c conftest.py --ros-workspace ~/ws/ros2/install ./test_all.py -import pytest from mininet.net import Mininet -from mininet.util import dumpNetConnections from mininet.topo import MinimalTopo +import pytest RANGES = [ - "OFF", - "SUBNET", - "LOCALHOST", + 'OFF', + 'SUBNET', + 'LOCALHOST', ] @@ -42,7 +48,7 @@ def h2_ipv4(net: MininetFixture) -> str: def no_peer(net: MininetFixture) -> str: - return "" + return '' @pytest.fixture() @@ -58,45 +64,46 @@ def mn(): # TODO(sloretz) figure out ROS workspace path from environment variables -@pytest.fixture(scope="session") +@pytest.fixture(scope='session') def ros_ws(pytestconfig): - return pytestconfig.getoption("ros_workspaces").split(':') + return pytestconfig.getoption('ros_workspaces').split(':') -def make_env_str(ros_ws, rmw, range, peer): +def make_env_str(ros_ws, rmw, discovery_range, peer): cmd = [] for ws in ros_ws: cmd.append('.') cmd.append(f'"{ws}/setup.bash"') cmd.append('&&') cmd.append(f'RMW_IMPLEMENTATION={rmw}') - cmd.append(f'ROS_AUTOMATIC_DISCOVERY_RANGE={range}') + cmd.append(f'ROS_AUTOMATIC_DISCOVERY_RANGE={discovery_range}') cmd.append(f'ROS_STATIC_PEERS="{peer}"') - cmd.append(' ') return ' '.join(cmd) -# TODO samehost doesn't require mininet does it. Could do this with normal testing -@pytest.mark.parametrize("sub_peer", (no_peer, h1_ipv4)) -@pytest.mark.parametrize("sub_range", RANGES) -@pytest.mark.parametrize("pub_peer", (no_peer, h1_ipv4)) -@pytest.mark.parametrize("pub_range", RANGES) + +@pytest.mark.parametrize('sub_peer', (no_peer, h1_ipv4)) +@pytest.mark.parametrize('sub_range', RANGES) +@pytest.mark.parametrize('pub_peer', (no_peer, h1_ipv4)) +@pytest.mark.parametrize('pub_range', RANGES) def test_samehost(mn, ros_ws, rmw, pub_range, pub_peer, sub_range, sub_peer): pub_peer = pub_peer(mn) sub_peer = sub_peer(mn) - pub_cmd = make_env_str(ros_ws, rmw, pub_range, pub_peer) + 'ros2 run test_discovery publish_once > /dev/null &' - sub_cmd = make_env_str(ros_ws, rmw, sub_range, sub_peer) + 'ros2 run test_discovery subscribe_once' - print("$", pub_cmd) - print("$", sub_cmd) + pub_env = make_env_str(ros_ws, rmw, pub_range, pub_peer) + sub_env = make_env_str(ros_ws, rmw, sub_range, sub_peer) + pub_cmd = pub_env + ' ros2 run test_discovery publish_once > /dev/null &' + sub_cmd = sub_env + ' ros2 run test_discovery subscribe_once' + print('$', pub_cmd) + print('$', sub_cmd) mn.h1.cmd(pub_cmd) result = mn.h1.cmd(sub_cmd) - message_received = "test_discovery: message was received" in result.strip() + message_received = 'test_discovery: message was received' in result.strip() if pub_peer or sub_peer: # if either has a static peer set, discovery should succeed assert message_received, result.strip() - elif "OFF" in (pub_range, sub_range): + elif 'OFF' in (pub_range, sub_range): # With no static peer, if either has discovery off then it won't succeed assert not message_received, result.strip() else: @@ -104,27 +111,29 @@ def test_samehost(mn, ros_ws, rmw, pub_range, pub_peer, sub_range, sub_peer): assert message_received, result.strip() -@pytest.mark.parametrize("sub_peer", (no_peer, h1_ipv4)) -@pytest.mark.parametrize("sub_range", RANGES) -@pytest.mark.parametrize("pub_peer", (no_peer, h2_ipv4)) -@pytest.mark.parametrize("pub_range", RANGES) +@pytest.mark.parametrize('sub_peer', (no_peer, h1_ipv4)) +@pytest.mark.parametrize('sub_range', RANGES) +@pytest.mark.parametrize('pub_peer', (no_peer, h2_ipv4)) +@pytest.mark.parametrize('pub_range', RANGES) def test_differenthost(mn, ros_ws, rmw, pub_range, pub_peer, sub_range, sub_peer): pub_peer = pub_peer(mn) sub_peer = sub_peer(mn) - pub_cmd = make_env_str(ros_ws, rmw, pub_range, pub_peer) + 'ros2 run test_discovery publish_once > /dev/null &' - sub_cmd = make_env_str(ros_ws, rmw, sub_range, sub_peer) + 'ros2 run test_discovery subscribe_once' - print("$", pub_cmd) - print("$", sub_cmd) + pub_env = make_env_str(ros_ws, rmw, pub_range, pub_peer) + sub_env = make_env_str(ros_ws, rmw, sub_range, sub_peer) + pub_cmd = pub_env + ' ros2 run test_discovery publish_once > /dev/null &' + sub_cmd = sub_env + ' ros2 run test_discovery subscribe_once' + print('$', pub_cmd) + print('$', sub_cmd) mn.h1.cmd(pub_cmd) result = mn.h2.cmd(sub_cmd) - message_received = "test_discovery: message was received" in result.strip() + message_received = 'test_discovery: message was received' in result.strip() if pub_peer or sub_peer: # if either has a static peer set, discovery should succeed assert message_received, result.strip() - elif "SUBNET" == pub_range and "SUBNET" == sub_range: + elif 'SUBNET' == pub_range and 'SUBNET' == sub_range: # With no static peer, succeed only if both are set to SUBNET assert message_received, result.strip() else: diff --git a/test_discovery/scripts/run_root_tests.py b/test_discovery/scripts/run_root_tests.py index ec59ec51..f5befcd3 100644 --- a/test_discovery/scripts/run_root_tests.py +++ b/test_discovery/scripts/run_root_tests.py @@ -1,6 +1,6 @@ #!/usr/bin/env python3 -# Copyright 2015 Open Source Robotics Foundation, Inc. +# Copyright 2023 Open Source Robotics Foundation, Inc. # # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. @@ -16,7 +16,6 @@ import argparse import os -import shlex import sys from ament_index_python import get_package_share_path @@ -32,7 +31,7 @@ def get_rmw_implementations(): def get_tests_dir(): pkg_path = get_package_share_path('test_discovery') - return pkg_path / "roottests" + return pkg_path / 'roottests' def get_workspaces(): @@ -47,7 +46,7 @@ def get_workspaces(): continue # If there exists a parent folder containing a setup.bash # then assume this is an isolated colcon workspace - if os.path.exists(os.path.join(prefix, "../setup.bash")): + if os.path.exists(os.path.join(prefix, '../setup.bash')): workspaces.add(os.path.dirname(prefix)) else: # Assume a merged ament/colcon workspace @@ -73,21 +72,21 @@ def main(): cmd.append('-m') cmd.append('pytest') cmd.append('-c') - cmd.append(str(get_tests_dir() / "conftest.py")) + cmd.append(str(get_tests_dir() / 'conftest.py')) if args.select: cmd.append('-k') cmd.append(args.select) cmd.append(f'--rmws={":".join(rmw_implementations)}') cmd.append(f'--ros-workspaces={":".join(get_workspaces())}') - cmd.append(str(get_tests_dir() / "test_discovery.py")) + cmd.append(str(get_tests_dir() / 'test_discovery.py')) - # find path to test_discovery package, use that for location to conftest.py and test_discovery.py - print("Executing the following command:") - print("================================") - print("$", *cmd) - print("================================") + print('Executing the following command:') + print('================================') + print('$', *cmd) + print('================================') os.execvp(cmd[0], cmd) + if __name__ == '__main__': - main() \ No newline at end of file + main() diff --git a/test_discovery/src/subscribe_once.cpp b/test_discovery/src/subscribe_once.cpp index af8dc62c..cb5e6395 100644 --- a/test_discovery/src/subscribe_once.cpp +++ b/test_discovery/src/subscribe_once.cpp @@ -20,7 +20,7 @@ constexpr double kTimeout = 10; -void topic_callback(const test_msgs::msg::Builtins & ) +void topic_callback(const test_msgs::msg::Builtins &) { std::cout << "test_discovery: message was received\n" << std::flush; std::exit(0); @@ -35,7 +35,7 @@ int main(int argc, char * argv[]) auto clock = node->get_clock(); auto end_time = clock->now() + rclcpp::Duration::from_seconds(kTimeout); - while (rclcpp::ok() && clock->now() <= end_time ) { + while (rclcpp::ok() && clock->now() <= end_time) { rclcpp::spin_some(node); rclcpp::sleep_for(std::chrono::milliseconds(100)); } diff --git a/test_discovery/tests/test_discovery.py b/test_discovery/tests/test_discovery.py index 483392fe..32465cf1 100644 --- a/test_discovery/tests/test_discovery.py +++ b/test_discovery/tests/test_discovery.py @@ -23,22 +23,23 @@ RANGES = [ - "OFF", - "SUBNET", - "LOCALHOST", + 'OFF', + 'SUBNET', + 'LOCALHOST', ] PEERS = [ None, - "127.0.0.1", + '127.0.0.1', ] + def get_rmw_implementations(): resources = list(get_resources('rmw_typesupport').keys()) if 'rmw_implementation' in resources: resources.remove('rmw_implementation') - return tuple(resources)[:1] + return tuple(resources) def get_executable(name): @@ -46,10 +47,10 @@ def get_executable(name): return str(p / 'lib' / 'test_discovery' / name) -def make_env(rmw, range, peer): +def make_env(rmw, discovery_range, peer): env = dict(os.environ) env['RMW_IMPLEMENTATION'] = rmw - env['ROS_AUTOMATIC_DISCOVERY_RANGE'] = range + env['ROS_AUTOMATIC_DISCOVERY_RANGE'] = discovery_range if peer is None: peer = '' env['ROS_STATIC_PEERS'] = peer @@ -71,11 +72,11 @@ def communicate(name, proc): return stdout, stderr -@pytest.mark.parametrize("sub_peer", PEERS) -@pytest.mark.parametrize("sub_range", RANGES) -@pytest.mark.parametrize("pub_peer", PEERS) -@pytest.mark.parametrize("pub_range", RANGES) -@pytest.mark.parametrize("rmw", get_rmw_implementations()) +@pytest.mark.parametrize('sub_peer', PEERS) +@pytest.mark.parametrize('sub_range', RANGES) +@pytest.mark.parametrize('pub_peer', PEERS) +@pytest.mark.parametrize('pub_range', RANGES) +@pytest.mark.parametrize('rmw', get_rmw_implementations()) def test_thishost(rmw, pub_range, pub_peer, sub_range, sub_peer): pub_env = make_env(rmw, pub_range, pub_peer) sub_env = make_env(rmw, sub_range, sub_peer) @@ -88,14 +89,14 @@ def test_thishost(rmw, pub_range, pub_peer, sub_range, sub_peer): sub_cmd, env=sub_env, stdout=subprocess.PIPE, stderr=subprocess.PIPE) stdout, _ = communicate('sub', sub_proc) - message_received = "test_discovery: message was received" in stdout + message_received = 'test_discovery: message was received' in stdout pub_proc.kill() communicate('pub', pub_proc) if pub_peer or sub_peer: # if either has a static peer set, discovery should succeed assert message_received - elif "OFF" in (pub_range, sub_range): + elif 'OFF' in (pub_range, sub_range): # With no static peer, if either has discovery off then it won't succeed assert not message_received else: From dec1548523bd5545d6d7689bbbb1ccfeddfaa6ab Mon Sep 17 00:00:00 2001 From: Shane Loretz Date: Fri, 24 Mar 2023 23:56:34 +0000 Subject: [PATCH 07/20] Update test expectations for changed matrix Signed-off-by: Shane Loretz --- test_discovery/roottests/test_discovery.py | 16 ++++++++-------- test_discovery/tests/test_discovery.py | 9 +++------ 2 files changed, 11 insertions(+), 14 deletions(-) diff --git a/test_discovery/roottests/test_discovery.py b/test_discovery/roottests/test_discovery.py index 4e38ba9c..7ec23d52 100644 --- a/test_discovery/roottests/test_discovery.py +++ b/test_discovery/roottests/test_discovery.py @@ -100,14 +100,11 @@ def test_samehost(mn, ros_ws, rmw, pub_range, pub_peer, sub_range, sub_peer): result = mn.h1.cmd(sub_cmd) message_received = 'test_discovery: message was received' in result.strip() - if pub_peer or sub_peer: - # if either has a static peer set, discovery should succeed - assert message_received, result.strip() - elif 'OFF' in (pub_range, sub_range): - # With no static peer, if either has discovery off then it won't succeed + if 'OFF' in (pub_range, sub_range): + # If either has discovery off then it won't succeed assert not message_received, result.strip() else: - # All other cases discovery + # All other cases discover each other assert message_received, result.strip() @@ -130,12 +127,15 @@ def test_differenthost(mn, ros_ws, rmw, pub_range, pub_peer, sub_range, sub_peer result = mn.h2.cmd(sub_cmd) message_received = 'test_discovery: message was received' in result.strip() - if pub_peer or sub_peer: + if 'OFF' in (pub_range, sub_range): + # If either has discovery off then it won't succeed + assert not message_received, result.strip() + elif pub_peer or sub_peer: # if either has a static peer set, discovery should succeed assert message_received, result.strip() elif 'SUBNET' == pub_range and 'SUBNET' == sub_range: # With no static peer, succeed only if both are set to SUBNET assert message_received, result.strip() else: - # All other cases discovery + # All other cases don't discover each other assert not message_received, result.strip() diff --git a/test_discovery/tests/test_discovery.py b/test_discovery/tests/test_discovery.py index 32465cf1..328407c7 100644 --- a/test_discovery/tests/test_discovery.py +++ b/test_discovery/tests/test_discovery.py @@ -93,12 +93,9 @@ def test_thishost(rmw, pub_range, pub_peer, sub_range, sub_peer): pub_proc.kill() communicate('pub', pub_proc) - if pub_peer or sub_peer: - # if either has a static peer set, discovery should succeed - assert message_received - elif 'OFF' in (pub_range, sub_range): - # With no static peer, if either has discovery off then it won't succeed + if 'OFF' in (pub_range, sub_range): + # If either has discovery off then it won't succeed assert not message_received else: - # All other cases discovery + # All other cases discover each other assert message_received From c1ec828adbfd444fa61075d1b409d381e156dfcf Mon Sep 17 00:00:00 2001 From: Shane Loretz Date: Tue, 28 Mar 2023 01:29:25 +0000 Subject: [PATCH 08/20] increase test timeout Signed-off-by: Shane Loretz --- test_discovery/CMakeLists.txt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test_discovery/CMakeLists.txt b/test_discovery/CMakeLists.txt index b1c88145..c4a027c3 100644 --- a/test_discovery/CMakeLists.txt +++ b/test_discovery/CMakeLists.txt @@ -35,7 +35,7 @@ if(BUILD_TESTING) find_package(ament_lint_auto REQUIRED) ament_lint_auto_find_test_dependencies() - ament_add_pytest_test(test_discovery tests/test_discovery.py TIMEOUT 800) + ament_add_pytest_test(test_discovery tests/test_discovery.py TIMEOUT 1600) endif() -ament_package() \ No newline at end of file +ament_package() From 4df83da152087e480e9c7ce42d32d3c2fb448e48 Mon Sep 17 00:00:00 2001 From: Shane Loretz Date: Tue, 28 Mar 2023 01:29:57 +0000 Subject: [PATCH 09/20] Improve package.xml Signed-off-by: Shane Loretz --- test_discovery/package.xml | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/test_discovery/package.xml b/test_discovery/package.xml index a82be9cf..fa5a07b1 100644 --- a/test_discovery/package.xml +++ b/test_discovery/package.xml @@ -15,15 +15,19 @@ ament_cmake_ros - + rclcpp test_msgs pytest ament_lint_auto ament_lint_common + ament_cmake_pytest + test_msgs + ros2cli + ament_cmake From 371ec2358f966d9834f49f3f7bbe444b2041c5d4 Mon Sep 17 00:00:00 2001 From: Shane Loretz Date: Tue, 28 Mar 2023 01:30:24 +0000 Subject: [PATCH 10/20] Use extend and lists more than append() Signed-off-by: Shane Loretz --- test_discovery/roottests/test_discovery.py | 16 +++++++----- test_discovery/scripts/run_root_tests.py | 29 +++++++++++++--------- 2 files changed, 27 insertions(+), 18 deletions(-) diff --git a/test_discovery/roottests/test_discovery.py b/test_discovery/roottests/test_discovery.py index 7ec23d52..695bfb4f 100644 --- a/test_discovery/roottests/test_discovery.py +++ b/test_discovery/roottests/test_discovery.py @@ -72,12 +72,16 @@ def ros_ws(pytestconfig): def make_env_str(ros_ws, rmw, discovery_range, peer): cmd = [] for ws in ros_ws: - cmd.append('.') - cmd.append(f'"{ws}/setup.bash"') - cmd.append('&&') - cmd.append(f'RMW_IMPLEMENTATION={rmw}') - cmd.append(f'ROS_AUTOMATIC_DISCOVERY_RANGE={discovery_range}') - cmd.append(f'ROS_STATIC_PEERS="{peer}"') + cmd.extend([ + '.', + f'"{ws}/setup.bash"', + '&&', + ]) + cmd.extend([ + f'RMW_IMPLEMENTATION={rmw}', + f'ROS_AUTOMATIC_DISCOVERY_RANGE={discovery_range}', + f'ROS_STATIC_PEERS="{peer}"', + ]) return ' '.join(cmd) diff --git a/test_discovery/scripts/run_root_tests.py b/test_discovery/scripts/run_root_tests.py index f5befcd3..58a7d0ee 100644 --- a/test_discovery/scripts/run_root_tests.py +++ b/test_discovery/scripts/run_root_tests.py @@ -66,19 +66,24 @@ def main(): raise ValueError(f'{args.rmw} is not an installed rmw: {rmw_implementations}') rmw_implementations = [args.rmw] - cmd = [] - cmd.append('sudo') - cmd.append(sys.executable) - cmd.append('-m') - cmd.append('pytest') - cmd.append('-c') - cmd.append(str(get_tests_dir() / 'conftest.py')) + cmd = [ + 'sudo', + sys.executable, + '-m', + 'pytest', + '-c', + str(get_tests_dir() / 'conftest.py'), + ] if args.select: - cmd.append('-k') - cmd.append(args.select) - cmd.append(f'--rmws={":".join(rmw_implementations)}') - cmd.append(f'--ros-workspaces={":".join(get_workspaces())}') - cmd.append(str(get_tests_dir() / 'test_discovery.py')) + cmd.extend([ + '-k', + args.select, + ]) + cmd.extend([ + f'--rmws={":".join(rmw_implementations)}', + f'--ros-workspaces={":".join(get_workspaces())}', + str(get_tests_dir() / 'test_discovery.py'), + ]) print('Executing the following command:') print('================================') From 1d0cd6ac91646d8681e9b747612f6b849855a7fb Mon Sep 17 00:00:00 2001 From: Shane Loretz Date: Tue, 28 Mar 2023 20:52:44 +0000 Subject: [PATCH 11/20] Commands are required Signed-off-by: Shane Loretz --- test_discovery/roottests/test_discovery.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/test_discovery/roottests/test_discovery.py b/test_discovery/roottests/test_discovery.py index 695bfb4f..c2da7828 100644 --- a/test_discovery/roottests/test_discovery.py +++ b/test_discovery/roottests/test_discovery.py @@ -14,9 +14,8 @@ # Run discovery tests # 1. Install dependencies -# sudo apt install iputils-ping iproute2 mininet -# TODO(sloretz) not needed? openvswitch-switch openvswitch-testcontroller -# 2. TODO(sloretz) not needed? Start openvswitch service if not already running +# sudo apt install iputils-ping iproute2 mininet openvswitch-switch openvswitch-testcontroller +# 2. Start openvswitch service if not already running # sudo service openvswitch-switch start from mininet.net import Mininet From 710592a069017fecf33d742e66e907cf7f6fc387 Mon Sep 17 00:00:00 2001 From: Shane Loretz Date: Tue, 28 Mar 2023 20:53:18 +0000 Subject: [PATCH 12/20] Don't need samehost root tests because non-root tests cover that already Signed-off-by: Shane Loretz --- test_discovery/roottests/test_discovery.py | 27 ---------------------- 1 file changed, 27 deletions(-) diff --git a/test_discovery/roottests/test_discovery.py b/test_discovery/roottests/test_discovery.py index c2da7828..58edb126 100644 --- a/test_discovery/roottests/test_discovery.py +++ b/test_discovery/roottests/test_discovery.py @@ -84,33 +84,6 @@ def make_env_str(ros_ws, rmw, discovery_range, peer): return ' '.join(cmd) -@pytest.mark.parametrize('sub_peer', (no_peer, h1_ipv4)) -@pytest.mark.parametrize('sub_range', RANGES) -@pytest.mark.parametrize('pub_peer', (no_peer, h1_ipv4)) -@pytest.mark.parametrize('pub_range', RANGES) -def test_samehost(mn, ros_ws, rmw, pub_range, pub_peer, sub_range, sub_peer): - pub_peer = pub_peer(mn) - sub_peer = sub_peer(mn) - - pub_env = make_env_str(ros_ws, rmw, pub_range, pub_peer) - sub_env = make_env_str(ros_ws, rmw, sub_range, sub_peer) - pub_cmd = pub_env + ' ros2 run test_discovery publish_once > /dev/null &' - sub_cmd = sub_env + ' ros2 run test_discovery subscribe_once' - print('$', pub_cmd) - print('$', sub_cmd) - - mn.h1.cmd(pub_cmd) - result = mn.h1.cmd(sub_cmd) - message_received = 'test_discovery: message was received' in result.strip() - - if 'OFF' in (pub_range, sub_range): - # If either has discovery off then it won't succeed - assert not message_received, result.strip() - else: - # All other cases discover each other - assert message_received, result.strip() - - @pytest.mark.parametrize('sub_peer', (no_peer, h1_ipv4)) @pytest.mark.parametrize('sub_range', RANGES) @pytest.mark.parametrize('pub_peer', (no_peer, h2_ipv4)) From 1c0f82d4e2b096131773073a33ccfe384d14c7ab Mon Sep 17 00:00:00 2001 From: Shane Loretz Date: Wed, 29 Mar 2023 23:14:59 +0000 Subject: [PATCH 13/20] Test case when RANGE is not set Signed-off-by: Shane Loretz --- test_discovery/roottests/test_discovery.py | 4 +++- test_discovery/tests/test_discovery.py | 6 +++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/test_discovery/roottests/test_discovery.py b/test_discovery/roottests/test_discovery.py index 58edb126..7ba02153 100644 --- a/test_discovery/roottests/test_discovery.py +++ b/test_discovery/roottests/test_discovery.py @@ -24,6 +24,7 @@ RANGES = [ + None, 'OFF', 'SUBNET', 'LOCALHOST', @@ -78,9 +79,10 @@ def make_env_str(ros_ws, rmw, discovery_range, peer): ]) cmd.extend([ f'RMW_IMPLEMENTATION={rmw}', - f'ROS_AUTOMATIC_DISCOVERY_RANGE={discovery_range}', f'ROS_STATIC_PEERS="{peer}"', ]) + if discovery_range is not None: + cmd.append(f'ROS_AUTOMATIC_DISCOVERY_RANGE={discovery_range}') return ' '.join(cmd) diff --git a/test_discovery/tests/test_discovery.py b/test_discovery/tests/test_discovery.py index 328407c7..643390ef 100644 --- a/test_discovery/tests/test_discovery.py +++ b/test_discovery/tests/test_discovery.py @@ -23,6 +23,7 @@ RANGES = [ + None, 'OFF', 'SUBNET', 'LOCALHOST', @@ -50,7 +51,10 @@ def get_executable(name): def make_env(rmw, discovery_range, peer): env = dict(os.environ) env['RMW_IMPLEMENTATION'] = rmw - env['ROS_AUTOMATIC_DISCOVERY_RANGE'] = discovery_range + if discovery_range is None: + del env['ROS_AUTOMATIC_DISCOVERY_RANGE'] + else: + env['ROS_AUTOMATIC_DISCOVERY_RANGE'] = discovery_range if peer is None: peer = '' env['ROS_STATIC_PEERS'] = peer From 24f519333f07892c6e86833fd1409977189724b8 Mon Sep 17 00:00:00 2001 From: Shane Loretz Date: Wed, 29 Mar 2023 23:28:36 +0000 Subject: [PATCH 14/20] More stuff to readme, not sure if table will render Signed-off-by: Shane Loretz --- test_discovery/README.md | 46 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/test_discovery/README.md b/test_discovery/README.md index e69de29b..7dd3082a 100644 --- a/test_discovery/README.md +++ b/test_discovery/README.md @@ -0,0 +1,46 @@ +# test_discovery + +This package tests the use of `ROS_AUTOMATIC_DISCOVERY_RANGE` and `ROS_STATIC_PEERS` environment variables. +There are two sets of tests: automated tests that run when testing this package, and semiautomated tests that must be run manually after building this package. + +# Automated tests + +The automated tests run when testing this package. +They test only the cases taht apply when two processes are on the same host. + +The expected commication with two processes on the same host is: + +|Same host |||Node B setting |||||| +|---|---|---|---|---|---|---|---|---| +||||No static peer |||With static peer ||| +|||Range |OFF |LOCALHOST |SUBNET |OFF |LOCALHOST |SUBNET| +|Node A setting |No static peer |OFF |:x: |:x: |:x: |:x: |:x: |:x:| +|||LOCALHOST |:x: |:white_check_mark: |:white_check_mark: |:x: |:white_check_mark: |:white_check_mark:| +|||SUBNET |:x: |:white_check_mark: |:white_check_mark: |:x: |:white_check_mark: |:white_check_mark:| +||With static peer |OFF |:x: |:x: |:x: |:x: |:x: |:x:| +|||LOCALHOST |:x: |:white_check_mark: |:white_check_mark: |:x: |:white_check_mark: |:white_check_mark:| +|||SUBNET |:x: |:white_check_mark: |:white_check_mark: |:x: |:white_check_mark: |:white_check_mark:| + +## Semi-automated tests + +The semiautomated tests use `mininet` to test discovery behavior across two different (virtual) hosts. +These tests require `root` access. + +The expected communication with two processes on different hosts is: + +|Different hosts |||Node B setting |||||| +||||No static peer |||With static peer ||| +|||Range |OFF |LOCALHOST |SUBNET |OFF |LOCALHOST |SUBNET| +|Node A setting |No static peer |OFF |:x: |:x: |:x: |:x: |:x: |:x:| +|||LOCALHOST |:x: |:x: |:x: |:x: |:white_check_mark: |:white_check_mark:| +|||SUBNET |:x: |:x: |:white_check_mark: |:x: |:white_check_mark: |:white_check_mark:| +||With static peer |OFF |:x: |:x: |:x: |:x: |:x: |:x:| +|||LOCALHOST |:white_check_mark: |:white_check_mark: |:white_check_mark: |:x: |:white_check_mark: |:white_check_mark:| +|||SUBNET |:white_check_mark: |:white_check_mark: |:white_check_mark: |:x: |:white_check_mark: |:white_check_mark:| +|||LOCALHOST |:x: |:white_check_mark: |:white_check_mark: |:x: |:white_check_mark: |:white_check_mark:| +|||SUBNET |:x: |:white_check_mark: |:white_check_mark: |:x: |:white_check_mark: |:white_check_mark:| + + +### Installing prerequisites + +### Running the semi-automated tests. From 6cbc122e047b4a186178e4e5d1e7f303bdcf7544 Mon Sep 17 00:00:00 2001 From: Shane Loretz Date: Thu, 30 Mar 2023 02:18:07 +0000 Subject: [PATCH 15/20] More readme edits Signed-off-by: Shane Loretz --- test_discovery/README.md | 52 ++++++++++++++++++---------------------- 1 file changed, 23 insertions(+), 29 deletions(-) diff --git a/test_discovery/README.md b/test_discovery/README.md index 7dd3082a..fd580efc 100644 --- a/test_discovery/README.md +++ b/test_discovery/README.md @@ -6,41 +6,35 @@ There are two sets of tests: automated tests that run when testing this package, # Automated tests The automated tests run when testing this package. -They test only the cases taht apply when two processes are on the same host. - -The expected commication with two processes on the same host is: - -|Same host |||Node B setting |||||| -|---|---|---|---|---|---|---|---|---| -||||No static peer |||With static peer ||| -|||Range |OFF |LOCALHOST |SUBNET |OFF |LOCALHOST |SUBNET| -|Node A setting |No static peer |OFF |:x: |:x: |:x: |:x: |:x: |:x:| -|||LOCALHOST |:x: |:white_check_mark: |:white_check_mark: |:x: |:white_check_mark: |:white_check_mark:| -|||SUBNET |:x: |:white_check_mark: |:white_check_mark: |:x: |:white_check_mark: |:white_check_mark:| -||With static peer |OFF |:x: |:x: |:x: |:x: |:x: |:x:| -|||LOCALHOST |:x: |:white_check_mark: |:white_check_mark: |:x: |:white_check_mark: |:white_check_mark:| -|||SUBNET |:x: |:white_check_mark: |:white_check_mark: |:x: |:white_check_mark: |:white_check_mark:| +They test only the cases that apply when two processes are on the same host. ## Semi-automated tests The semiautomated tests use `mininet` to test discovery behavior across two different (virtual) hosts. -These tests require `root` access. +These tests require `root` access, and a working `mininet` install. -The expected communication with two processes on different hosts is: +### Installing prerequisites -|Different hosts |||Node B setting |||||| -||||No static peer |||With static peer ||| -|||Range |OFF |LOCALHOST |SUBNET |OFF |LOCALHOST |SUBNET| -|Node A setting |No static peer |OFF |:x: |:x: |:x: |:x: |:x: |:x:| -|||LOCALHOST |:x: |:x: |:x: |:x: |:white_check_mark: |:white_check_mark:| -|||SUBNET |:x: |:x: |:white_check_mark: |:x: |:white_check_mark: |:white_check_mark:| -||With static peer |OFF |:x: |:x: |:x: |:x: |:x: |:x:| -|||LOCALHOST |:white_check_mark: |:white_check_mark: |:white_check_mark: |:x: |:white_check_mark: |:white_check_mark:| -|||SUBNET |:white_check_mark: |:white_check_mark: |:white_check_mark: |:x: |:white_check_mark: |:white_check_mark:| -|||LOCALHOST |:x: |:white_check_mark: |:white_check_mark: |:x: |:white_check_mark: |:white_check_mark:| -|||SUBNET |:x: |:white_check_mark: |:white_check_mark: |:x: |:white_check_mark: |:white_check_mark:| +A working `mininet` install has only been tested on an Ubuntu based machine. +If you're running in a container, that container will need root priviledges. +First install the necessary dependencies: -### Installing prerequisites +```bash +sudo apt install \ + iputils-ping \ + iproute2 \ + mininet \ + openvswitch-switch \ + openvswitch-testcontroller +``` + +Next, make sure the openvswitch service is running + +```bash +sudo service openvswitch-switch start +``` + +You're now ready to run the tests. -### Running the semi-automated tests. +### Running the semi-automated tests From 8a063c703265f816f04b7375a8fef1a9216c8be9 Mon Sep 17 00:00:00 2001 From: Shane Loretz Date: Thu, 30 Mar 2023 02:19:07 +0000 Subject: [PATCH 16/20] Check that node was successfully created Signed-off-by: Shane Loretz --- test_discovery/roottests/test_discovery.py | 4 ++++ test_discovery/src/subscribe_once.cpp | 2 ++ test_discovery/tests/test_discovery.py | 4 ++++ 3 files changed, 10 insertions(+) diff --git a/test_discovery/roottests/test_discovery.py b/test_discovery/roottests/test_discovery.py index 7ba02153..1dacad03 100644 --- a/test_discovery/roottests/test_discovery.py +++ b/test_discovery/roottests/test_discovery.py @@ -103,6 +103,10 @@ def test_differenthost(mn, ros_ws, rmw, pub_range, pub_peer, sub_range, sub_peer mn.h1.cmd(pub_cmd) result = mn.h2.cmd(sub_cmd) + + # Invalid node configuration could make OFF tests appear to succeed + assert "test_discovery: node successfully created" in result.strip() + message_received = 'test_discovery: message was received' in result.strip() if 'OFF' in (pub_range, sub_range): diff --git a/test_discovery/src/subscribe_once.cpp b/test_discovery/src/subscribe_once.cpp index cb5e6395..dafe7a2b 100644 --- a/test_discovery/src/subscribe_once.cpp +++ b/test_discovery/src/subscribe_once.cpp @@ -33,6 +33,8 @@ int main(int argc, char * argv[]) auto subscription = node->create_subscription("test_topic", 10, topic_callback); + std::cout << "test_discovery: node successfully created\n" << std::flush; + auto clock = node->get_clock(); auto end_time = clock->now() + rclcpp::Duration::from_seconds(kTimeout); while (rclcpp::ok() && clock->now() <= end_time) { diff --git a/test_discovery/tests/test_discovery.py b/test_discovery/tests/test_discovery.py index 643390ef..74205c02 100644 --- a/test_discovery/tests/test_discovery.py +++ b/test_discovery/tests/test_discovery.py @@ -93,8 +93,12 @@ def test_thishost(rmw, pub_range, pub_peer, sub_range, sub_peer): sub_cmd, env=sub_env, stdout=subprocess.PIPE, stderr=subprocess.PIPE) stdout, _ = communicate('sub', sub_proc) + # Invalid node configuration could make OFF tests appear to succeed + assert "test_discovery: node successfully created" in stdout + message_received = 'test_discovery: message was received' in stdout pub_proc.kill() + communicate('pub', pub_proc) if 'OFF' in (pub_range, sub_range): From 06a2ce879a9a912e680e3a280c3c8bf1b64e1069 Mon Sep 17 00:00:00 2001 From: Shane Loretz Date: Thu, 30 Mar 2023 02:27:53 +0000 Subject: [PATCH 17/20] Disable testing range not set to save time Signed-off-by: Shane Loretz --- test_discovery/roottests/test_discovery.py | 2 +- test_discovery/tests/test_discovery.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/test_discovery/roottests/test_discovery.py b/test_discovery/roottests/test_discovery.py index 1dacad03..7f375baf 100644 --- a/test_discovery/roottests/test_discovery.py +++ b/test_discovery/roottests/test_discovery.py @@ -24,7 +24,7 @@ RANGES = [ - None, +# None, # idential to LOCALHOST, but takes too long to test here 'OFF', 'SUBNET', 'LOCALHOST', diff --git a/test_discovery/tests/test_discovery.py b/test_discovery/tests/test_discovery.py index 74205c02..6223f337 100644 --- a/test_discovery/tests/test_discovery.py +++ b/test_discovery/tests/test_discovery.py @@ -23,7 +23,7 @@ RANGES = [ - None, +# None, # idential to LOCALHOST, but takes too long to test here 'OFF', 'SUBNET', 'LOCALHOST', From 2e970ec224823116827dcfc80c684355f1d373b7 Mon Sep 17 00:00:00 2001 From: Shane Loretz Date: Fri, 31 Mar 2023 00:54:51 +0000 Subject: [PATCH 18/20] Fix flake8 issues Signed-off-by: Shane Loretz --- test_discovery/roottests/test_discovery.py | 4 ++-- test_discovery/tests/test_discovery.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/test_discovery/roottests/test_discovery.py b/test_discovery/roottests/test_discovery.py index 7f375baf..377d10bb 100644 --- a/test_discovery/roottests/test_discovery.py +++ b/test_discovery/roottests/test_discovery.py @@ -24,7 +24,7 @@ RANGES = [ -# None, # idential to LOCALHOST, but takes too long to test here + # None, # idential to LOCALHOST, but takes too long to test here 'OFF', 'SUBNET', 'LOCALHOST', @@ -105,7 +105,7 @@ def test_differenthost(mn, ros_ws, rmw, pub_range, pub_peer, sub_range, sub_peer result = mn.h2.cmd(sub_cmd) # Invalid node configuration could make OFF tests appear to succeed - assert "test_discovery: node successfully created" in result.strip() + assert 'test_discovery: node successfully created' in result.strip() message_received = 'test_discovery: message was received' in result.strip() diff --git a/test_discovery/tests/test_discovery.py b/test_discovery/tests/test_discovery.py index 6223f337..86ef555b 100644 --- a/test_discovery/tests/test_discovery.py +++ b/test_discovery/tests/test_discovery.py @@ -23,7 +23,7 @@ RANGES = [ -# None, # idential to LOCALHOST, but takes too long to test here + # None, # idential to LOCALHOST, but takes too long to test here 'OFF', 'SUBNET', 'LOCALHOST', @@ -94,7 +94,7 @@ def test_thishost(rmw, pub_range, pub_peer, sub_range, sub_peer): stdout, _ = communicate('sub', sub_proc) # Invalid node configuration could make OFF tests appear to succeed - assert "test_discovery: node successfully created" in stdout + assert 'test_discovery: node successfully created' in stdout message_received = 'test_discovery: message was received' in stdout pub_proc.kill() From 2143918cce5b63a768224841a495c5dceb2c48aa Mon Sep 17 00:00:00 2001 From: Shane Loretz Date: Fri, 31 Mar 2023 21:32:27 +0000 Subject: [PATCH 19/20] Skip subnet samehost tests with static peer Signed-off-by: Shane Loretz --- test_discovery/tests/test_discovery.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/test_discovery/tests/test_discovery.py b/test_discovery/tests/test_discovery.py index 86ef555b..57eba9be 100644 --- a/test_discovery/tests/test_discovery.py +++ b/test_discovery/tests/test_discovery.py @@ -82,6 +82,20 @@ def communicate(name, proc): @pytest.mark.parametrize('pub_range', RANGES) @pytest.mark.parametrize('rmw', get_rmw_implementations()) def test_thishost(rmw, pub_range, pub_peer, sub_range, sub_peer): + # For same host tests, setting a static peer while using SUBNET + # doesn't make a lot of sense. + # Further, it cause test failures with rmw_fastrtps_* + # When there's an initial peer to localhost, Fast-DDS will try to discover + # peers on localhost using unicast discovery, but that uses the + # default maxInitialPeersRange value of 4, which is a very small number of + # peers. + # Any other ROS nodes on a system (such as daemons, or test processes that + # weren't cleaned up) will cause this test to fail. + if 'SUBNET' == sub_range and sub_peer is not None: + pytest.skip("Skipping samehost SUBNET with static peer") + if 'SUBNET' == pub_range and pub_peer is not None: + pytest.skip("Skipping samehost SUBNET with static peer") + pub_env = make_env(rmw, pub_range, pub_peer) sub_env = make_env(rmw, sub_range, sub_peer) pub_cmd = [get_executable('publish_once')] From f10b1fbd40da124aa9d6cca970b2e59569a0584d Mon Sep 17 00:00:00 2001 From: Shane Loretz Date: Fri, 31 Mar 2023 23:32:04 +0000 Subject: [PATCH 20/20] single quotes Signed-off-by: Shane Loretz --- test_discovery/tests/test_discovery.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test_discovery/tests/test_discovery.py b/test_discovery/tests/test_discovery.py index 57eba9be..a698e021 100644 --- a/test_discovery/tests/test_discovery.py +++ b/test_discovery/tests/test_discovery.py @@ -92,9 +92,9 @@ def test_thishost(rmw, pub_range, pub_peer, sub_range, sub_peer): # Any other ROS nodes on a system (such as daemons, or test processes that # weren't cleaned up) will cause this test to fail. if 'SUBNET' == sub_range and sub_peer is not None: - pytest.skip("Skipping samehost SUBNET with static peer") + pytest.skip('Skipping samehost SUBNET with static peer') if 'SUBNET' == pub_range and pub_peer is not None: - pytest.skip("Skipping samehost SUBNET with static peer") + pytest.skip('Skipping samehost SUBNET with static peer') pub_env = make_env(rmw, pub_range, pub_peer) sub_env = make_env(rmw, sub_range, sub_peer)