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 <michael.orlov@apex.ai>

* Move arguments checks to a separate method

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

* 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 <michael.orlov@apex.ai>

* 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 <michael.orlov@apex.ai>

* Add check for "len(args.services) > 0" per code review

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

* Add extra checks for arguments' validation and test coverage

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

---------

Signed-off-by: Michael Orlov <michael.orlov@apex.ai>

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,93 +205,96 @@ 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()
 
         qos_profile_overrides = {}  # Specify a valid default
         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:

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)

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) {