From ef66ee145cb4e74b4504d4806445cad8ad953390 Mon Sep 17 00:00:00 2001 From: Michael Orlov Date: Fri, 31 May 2024 17:13:21 -0700 Subject: [PATCH] Sweep cleanup in rosbag2 recorder CLI args verification code (#1633) * Remove outdated checks that only one option out of others can be used Signed-off-by: Michael Orlov * Move arguments checks to a separate method Signed-off-by: Michael Orlov * Fix wrong limitation on arguments in recorder CLI help section - On Jazzy and Rolling we have relaxed limitations in topic filter, and it is ok to apply --exclude-topics above --topics or --exclude-services above --services. Also --all, --all-topics or --all-services overrides --regex. Signed-off-by: Michael Orlov * Add warning that --all will override --topics and --services - Also adjust integration tests to check that --all, --all-topics and --all-services are allowed to be used together with --topics and --services CLI arguments. Signed-off-by: Michael Orlov * Add check for "len(args.services) > 0" per code review Signed-off-by: Michael Orlov * Add extra checks for arguments' validation and test coverage Signed-off-by: Michael Orlov --------- Signed-off-by: Michael Orlov --- ros2bag/ros2bag/verb/record.py | 145 +++++++++--------- ros2bag/test/test_recorder_args_parser.py | 99 ++++++++++++ .../test_rosbag2_record_end_to_end.cpp | 30 +++- 3 files changed, 196 insertions(+), 78 deletions(-) diff --git a/ros2bag/ros2bag/verb/record.py b/ros2bag/ros2bag/verb/record.py index 6818c9e27..733ff360a 100644 --- a/ros2bag/ros2bag/verb/record.py +++ b/ros2bag/ros2bag/verb/record.py @@ -21,6 +21,7 @@ from ros2bag.api import convert_service_to_service_event_topic from ros2bag.api import convert_yaml_to_qos_profile from ros2bag.api import print_error +from ros2bag.api import print_warn from ros2bag.api import SplitLineFormatter from ros2bag.verb import VerbExtension from ros2cli.node import NODE_NAME_PREFIX @@ -84,8 +85,7 @@ def add_recorder_arguments(parser: ArgumentParser) -> None: parser.add_argument( '-e', '--regex', default='', help='Record only topics and services containing provided regular expression. ' - 'Overrides --all, --all-topics and --all-services, applies on top of ' - 'topics list and service list.') + 'Note: --all, --all-topics or --all-services will override --regex.') parser.add_argument( '--exclude-regex', default='', help='Exclude topics and services containing provided regular expression. ' @@ -98,11 +98,11 @@ def add_recorder_arguments(parser: ArgumentParser) -> None: parser.add_argument( '--exclude-topics', type=str, metavar='Topic', nargs='+', help='Space-delimited list of topics not being recorded. ' - 'Works on top of --all, --all-topics, or --regex.') + 'Works on top of --all, --all-topics, --topics or --regex.') parser.add_argument( '--exclude-services', type=str, metavar='ServiceName', nargs='+', help='Space-delimited list of services not being recorded. ' - 'Works on top of --all, --all-services, or --regex.') + 'Works on top of --all, --all-services, --services or --regex.') # Discovery behavior parser.add_argument( @@ -205,76 +205,85 @@ def add_recorder_arguments(parser: ArgumentParser) -> None: 'Has no effect if no compression mode is chosen. Default: %(default)s.') +def check_necessary_argument(args): + # At least one options out of --all, --all-topics, --all-services, --services, --topics, + # --topic-types or --regex must be used + if not (args.all or args.all_topics or args.all_services or + (args.services and len(args.services) > 0) or + (args.topics and len(args.topics) > 0) or + (args.topic_types and len(args.topic_types) > 0) or args.regex): + return False + return True + + +def validate_parsed_arguments(args, uri) -> str: + if args.topics_positional: + print(print_warn('Positional "topics" argument deprecated. ' + 'Please use optional "--topics" argument instead.')) + args.topics = args.topics_positional + + if not check_necessary_argument(args): + return print_error('Need to specify at least one option out of --all, --all-topics, ' + '--all-services, --services, --topics, --topic-types or --regex') + + if args.exclude_regex and not \ + (args.all or args.all_topics or args.topic_types or args.all_services or + args.regex): + return print_error('--exclude-regex argument requires either --all, ' + '--all-topics, --topic-types, --all-services or --regex') + + if args.exclude_topics and not \ + (args.all or args.all_topics or args.topic_types or args.regex): + return print_error('--exclude-topics argument requires either --all, --all-topics, ' + '--topic-types or --regex') + + if args.exclude_topic_types and not \ + (args.all or args.all_topics or args.topic_types or args.regex): + return print_error('--exclude-topic-types argument requires either --all, ' + '--all-topics or --regex') + + if args.exclude_services and not (args.all or args.all_services or args.regex): + return print_error('--exclude-services argument requires either --all, --all-services ' + 'or --regex') + + if (args.all or args.all_services) and args.services: + print(print_warn('--all or --all-services will override --services')) + + if (args.all or args.all_topics) and args.topics: + print(print_warn('--all or --all-topics will override --topics')) + + if (args.all or args.all_topics or args.all_services) and args.regex: + print(print_warn('--all, --all-topics or --all-services will override --regex')) + + if os.path.isdir(uri): + return print_error("Output folder '{}' already exists.".format(uri)) + + if args.use_sim_time and args.no_discovery: + return print_error( + '--use-sim-time and --no-discovery both set, but are incompatible settings. ' + 'The /clock topic needs to be discovered to record with sim time.') + + if args.compression_format and args.compression_mode == 'none': + return print_error('Invalid choice: Cannot specify compression format ' + 'without a compression mode.') + + if args.compression_queue_size < 0: + return print_error('Compression queue size must be at least 0.') + + class RecordVerb(VerbExtension): """Record ROS data to a bag.""" def add_arguments(self, parser, cli_name): # noqa: D102 add_recorder_arguments(parser) - def _check_necessary_argument(self, args): - # At least one options out of --all, --all-topics, --all-services, --services, --topics, - # --topic-types or --regex must be used - if not (args.all or args.all_topics or args.all_services or - args.services or (args.topics and len(args.topics) > 0) or - (args.topic_types and len(args.topic_types) > 0) or args.regex): - return False - return True - def main(self, *, args): # noqa: D102 - if args.topics_positional: - print('Warning! Positional "topics" argument deprecated. ' - 'Please use optional "--topics" argument instead.') - args.topics = args.topics_positional - - if not self._check_necessary_argument(args): - return print_error('Need to specify one option out of --all, --all-topics, ' - '--all-services, --services, --topics, --topic-types and --regex') - - # Only one option out of --all, --all-services --services or --regex can be used - if (args.all and args.all_services) or \ - ((args.all or args.all_services) and args.regex) or \ - ((args.all or args.all_services or args.regex) and args.services): - return print_error('Must specify only one option out of --all, --all-services, ' - '--services or --regex') - - # Only one option out of --all, --all-topics, --topics, --topic-types or --regex can - # be used - if (args.all and args.all_topics) or \ - ((args.all or args.all_topics) and args.regex) or \ - ((args.all or args.all_topics or args.regex) and args.topics) or \ - ((args.all or args.all_topics or args.regex or args.topics) and args.topic_types): - return print_error('Must specify only one option out of --all, --all-topics, ' - '--topics, --topic-types or --regex') - - if (args.exclude_regex and - not (args.regex or args.all or args.all_topics or args.all_services)): - return print_error('--exclude-regex argument requires either --all, ' - '--all-topics, --all-services or --regex') - - if args.exclude_topics and not (args.regex or args.all or args.all_topics): - return print_error('--exclude-topics argument requires either --all, --all-topics ' - 'or --regex') - - if args.exclude_topic_types and not (args.regex or args.all or args.all_topics): - return print_error('--exclude-topic-types argument requires either --all, ' - '--all-topics or --regex') - - if args.exclude_services and not (args.regex or args.all or args.all_services): - return print_error('--exclude-services argument requires either --all, --all-services ' - 'or --regex') - uri = args.output or datetime.datetime.now().strftime('rosbag2_%Y_%m_%d-%H_%M_%S') - if os.path.isdir(uri): - return print_error("Output folder '{}' already exists.".format(uri)) - - if args.compression_format and args.compression_mode == 'none': - return print_error('Invalid choice: Cannot specify compression format ' - 'without a compression mode.') - - if args.compression_queue_size < 0: - return print_error('Compression queue size must be at least 0.') + error_str = validate_parsed_arguments(args, uri) + if error_str and len(error_str) > 0: + return error_str args.compression_mode = args.compression_mode.upper() @@ -282,16 +291,10 @@ def main(self, *, args): # noqa: D102 if args.qos_profile_overrides_path: qos_profile_dict = yaml.safe_load(args.qos_profile_overrides_path) try: - qos_profile_overrides = convert_yaml_to_qos_profile( - qos_profile_dict) + qos_profile_overrides = convert_yaml_to_qos_profile(qos_profile_dict) except (InvalidQoSProfileException, ValueError) as e: return print_error(str(e)) - if args.use_sim_time and args.no_discovery: - return print_error( - '--use-sim-time and --no-discovery both set, but are incompatible settings. ' - 'The /clock topic needs to be discovered to record with sim time.') - # Prepare custom_data dictionary custom_data = {} if args.custom_data: diff --git a/ros2bag/test/test_recorder_args_parser.py b/ros2bag/test/test_recorder_args_parser.py index d19562653..7d8c2def6 100644 --- a/ros2bag/test/test_recorder_args_parser.py +++ b/ros2bag/test/test_recorder_args_parser.py @@ -13,13 +13,17 @@ # limitations under the License. import argparse +import datetime from pathlib import Path import pytest from ros2bag.verb.record import add_recorder_arguments +from ros2bag.verb.record import validate_parsed_arguments RESOURCES_PATH = Path(__file__).parent / 'resources' +ERROR_STRING_MSG = 'ros2bag record CLI did not produce the expected output' \ + '\n Expected output pattern: {}\n Actual output: {}' @pytest.fixture(scope='function') @@ -128,3 +132,98 @@ def test_recorder_custom_data_list_argument(test_arguments_parser): ) assert ['Key1=Value1', 'key2=value2'] == args.custom_data assert output_path.as_posix() == args.output + + +def test_recorder_validate_exclude_regex_needs_inclusive_args(test_arguments_parser): + """Test that --exclude-regex needs inclusive arguments.""" + output_path = RESOURCES_PATH / 'ros2bag_tmp_file' + args = test_arguments_parser.parse_args( + ['--exclude-regex', '%s-%s', '--services', 'service1', '--topics', 'topic1', + '--output', output_path.as_posix()] + ) + assert '%s-%s' == args.exclude_regex + assert args.all is False + assert args.all_topics is False + assert [] == args.topic_types + assert args.all_services is False + assert '' == args.regex + + uri = args.output or datetime.datetime.now().strftime('rosbag2_%Y_%m_%d-%H_%M_%S') + error_str = validate_parsed_arguments(args, uri) + assert error_str is not None + expected_output = '--exclude-regex argument requires either --all, ' \ + '--all-topics, --topic-types, --all-services or --regex' + matches = expected_output in error_str + assert matches, ERROR_STRING_MSG.format(expected_output, error_str) + + +def test_recorder_validate_exclude_topics_needs_inclusive_args(test_arguments_parser): + """Test that --exclude-topics inclusive arguments.""" + output_path = RESOURCES_PATH / 'ros2bag_tmp_file' + args = test_arguments_parser.parse_args( + ['--exclude-topics', 'topic1', '--all-services', '--topics', 'topic1', + '--output', output_path.as_posix()] + ) + assert ['topic1'] == args.exclude_topics + assert args.all is False + assert args.all_topics is False + assert [] == args.topic_types + assert args.all_services is True + assert '' == args.regex + assert '' == args.exclude_regex + + uri = args.output or datetime.datetime.now().strftime('rosbag2_%Y_%m_%d-%H_%M_%S') + error_str = validate_parsed_arguments(args, uri) + assert error_str is not None + expected_output = '--exclude-topics argument requires either --all, --all-topics, ' \ + '--topic-types or --regex' + matches = expected_output in error_str + assert matches, ERROR_STRING_MSG.format(expected_output, error_str) + + +def test_recorder_validate_exclude_topics_types_needs_inclusive_args(test_arguments_parser): + """Test that --exclude-topic-types needs inclusive argument.""" + output_path = RESOURCES_PATH / 'ros2bag_tmp_file' + args = test_arguments_parser.parse_args( + ['--exclude-topic-types', '/topic_type1', '--all-services', '--topics', 'topic1', + '--output', output_path.as_posix()] + ) + assert ['/topic_type1'] == args.exclude_topic_types + assert args.all is False + assert args.all_topics is False + assert [] == args.topic_types + assert args.all_services is True + assert '' == args.regex + assert '' == args.exclude_regex + + uri = args.output or datetime.datetime.now().strftime('rosbag2_%Y_%m_%d-%H_%M_%S') + error_str = validate_parsed_arguments(args, uri) + assert error_str is not None + expected_output = '--exclude-topic-types argument requires either --all, ' \ + '--all-topics or --regex' + matches = expected_output in error_str + assert matches, ERROR_STRING_MSG.format(expected_output, error_str) + + +def test_recorder_validate_exclude_services_needs_inclusive_args(test_arguments_parser): + """Test that --exclude-services needs at least --all, --all-services or --regex arguments.""" + output_path = RESOURCES_PATH / 'ros2bag_tmp_file' + args = test_arguments_parser.parse_args( + ['--exclude-services', 'service1', '--services', 'service1', '--all-topics', + '--output', output_path.as_posix()] + ) + assert ['service1'] == args.exclude_services + assert args.all is False + assert args.all_topics is True + assert [] == args.topic_types + assert args.all_services is False + assert '' == args.regex + assert '' == args.exclude_regex + + uri = args.output or datetime.datetime.now().strftime('rosbag2_%Y_%m_%d-%H_%M_%S') + error_str = validate_parsed_arguments(args, uri) + assert error_str is not None + expected_output = '--exclude-services argument requires either --all, --all-services ' + 'or --regex' + matches = expected_output in error_str + assert matches, ERROR_STRING_MSG.format(expected_output, error_str) diff --git a/rosbag2_tests/test/rosbag2_tests/test_rosbag2_record_end_to_end.cpp b/rosbag2_tests/test/rosbag2_tests/test_rosbag2_record_end_to_end.cpp index d93bec9d1..3f7d38189 100644 --- a/rosbag2_tests/test/rosbag2_tests/test_rosbag2_record_end_to_end.cpp +++ b/rosbag2_tests/test/rosbag2_tests/test_rosbag2_record_end_to_end.cpp @@ -535,14 +535,30 @@ TEST_P(RecordFixture, record_fails_gracefully_if_bag_already_exists) { EXPECT_THAT(error_output, HasSubstr("Output folder 'empty_dir' already exists")); } -TEST_P(RecordFixture, record_fails_if_both_all_and_topic_list_is_specified) { - internal::CaptureStderr(); - auto exit_code = execute_and_wait_until_completion( - get_base_record_command() + " -a --topics /some_topic", temporary_dir_path_); - auto error_output = internal::GetCapturedStderr(); +TEST_P(RecordFixture, record_if_topic_list_service_list_and_all_are_specified) { + auto message = get_messages_strings()[0]; + message->string_value = "test"; - EXPECT_THAT(exit_code, Eq(EXIT_FAILURE)); - EXPECT_FALSE(error_output.empty()); + rosbag2_test_common::PublicationManager pub_manager; + pub_manager.setup_publisher("/test_topic", message, 10); + + internal::CaptureStdout(); + auto process_handle = start_execution( + get_base_record_command() + + " -a --all-topics --all-services --topics /test_topic --services /service1"); + auto cleanup_process_handle = rcpputils::make_scope_exit( + [process_handle]() { + stop_execution(process_handle); + }); + + ASSERT_TRUE(pub_manager.wait_for_matched("/test_topic")) << + "Expected find rosbag subscription"; + auto output = internal::GetCapturedStdout(); + stop_execution(process_handle); + cleanup_process_handle.cancel(); + + EXPECT_THAT(output, HasSubstr("--all or --all-topics will override --topics")); + EXPECT_THAT(output, HasSubstr("--all or --all-services will override --services")); } TEST_P(RecordFixture, record_fails_if_neither_all_nor_topic_list_are_specified) {