Don't warn for unknown types if topics are not selected (#1466)

* Don't warn for unknown types if topics are not in the list Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Add unit tests to verify regression Signed-off-by: Michael Orlov <michael.orlov@apex.ai> * Move checks for single type and hidden topics to the bottom Rationale: To avoid irrelevant warning messages if topic name hasn't been selected by topic list or regex. Signed-off-by: Michael Orlov <michael.orlov@apex.ai> --------- Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
ros2 · Sep 13, 2023 · bf4038d · bf4038d
1 parent 777612f
commit bf4038d
Show file tree

Hide file tree

Showing 2 changed files with 54 additions and 16 deletions.
diff --git a/rosbag2_transport/src/rosbag2_transport/topic_filter.cpp b/rosbag2_transport/src/rosbag2_transport/topic_filter.cpp
@@ -117,22 +117,6 @@ std::unordered_map<std::string, std::string> TopicFilter::filter_topics(
 bool TopicFilter::take_topic(
   const std::string & topic_name, const std::vector<std::string> & topic_types)
 {
-  if (!has_single_type(topic_name, topic_types)) {
-    return false;
-  }
-
-  const std::string & topic_type = topic_types[0];
-  if (!allow_unknown_types_ && !type_is_known(topic_name, topic_type)) {
-    return false;
-  }
-
-  if (!record_options_.include_hidden_topics && topic_is_hidden(topic_name)) {
-    RCUTILS_LOG_WARN_ONCE_NAMED(
-      ROSBAG2_TRANSPORT_PACKAGE_NAME,
-      "Hidden topics are not recorded. Enable them with --include-hidden-topics");
-    return false;
-  }
-
   if (!record_options_.include_unpublished_topics && node_graph_ &&
     topic_is_unpublished(topic_name, *node_graph_))
   {
@@ -163,6 +147,22 @@ bool TopicFilter::take_topic(
     return false;
   }
 
+  if (!has_single_type(topic_name, topic_types)) {
+    return false;
+  }
+
+  if (!record_options_.include_hidden_topics && topic_is_hidden(topic_name)) {
+    RCUTILS_LOG_WARN_ONCE_NAMED(
+      ROSBAG2_TRANSPORT_PACKAGE_NAME,
+      "Hidden topics are not recorded. Enable them with --include-hidden-topics");
+    return false;
+  }
+
+  const std::string & topic_type = topic_types[0];
+  if (!allow_unknown_types_ && !type_is_known(topic_name, topic_type)) {
+    return false;
+  }
+
   return true;
 }
 

diff --git a/rosbag2_transport/test/rosbag2_transport/test_topic_filter.cpp b/rosbag2_transport/test/rosbag2_transport/test_topic_filter.cpp
@@ -203,3 +203,41 @@ TEST_F(RegexFixture, regex_all_and_filter)
   auto filtered_topics = filter.filter_topics(topics_and_types_);
   EXPECT_THAT(filtered_topics, SizeIs(6));
 }
+
+TEST_F(RegexFixture, do_not_print_warning_about_unknown_types_if_topic_is_not_selected) {
+  {  // Check for topics explicitly selected via "topics" list
+    rosbag2_transport::RecordOptions record_options;
+    // Select only one topic with name "/planning" via topic list
+    record_options.topics = {"/planning"};
+    record_options.all = false;
+    rosbag2_transport::TopicFilter filter{record_options, nullptr, false};
+    testing::internal::CaptureStderr();
+    auto filtered_topics = filter.filter_topics(topics_and_types_);
+    std::string test_output = testing::internal::GetCapturedStderr();
+    ASSERT_EQ(0u, filtered_topics.size());
+    EXPECT_TRUE(
+      test_output.find(
+        "Topic '/invalid_topic' has unknown type 'invalid_topic_type'") == std::string::npos);
+    EXPECT_TRUE(
+      test_output.find(
+        "Topic '/planning' has unknown type 'planning_topic_type'") != std::string::npos);
+  }
+
+  {  // Check for topics selected via regex
+    rosbag2_transport::RecordOptions record_options;
+    // Select only one topic with name "/planning" via regex
+    record_options.regex = "^/planning";
+    record_options.all = false;
+    rosbag2_transport::TopicFilter filter{record_options, nullptr, false};
+    testing::internal::CaptureStderr();
+    auto filtered_topics = filter.filter_topics(topics_and_types_);
+    std::string test_output = testing::internal::GetCapturedStderr();
+    ASSERT_EQ(0u, filtered_topics.size());
+    EXPECT_TRUE(
+      test_output.find(
+        "Topic '/invalid_topic' has unknown type 'invalid_topic_type'") == std::string::npos);
+    EXPECT_TRUE(
+      test_output.find(
+        "Topic '/planning' has unknown type 'planning_topic_type'") != std::string::npos);
+  }
+}