Fix race condition for ingesting/dispensing and disable uncrustify tests by default (#362)

Yadunund · luca-della-vedova · mxgrey · web-flow · commit a5d411aeacd9 · 2024-06-08T02:18:15.000Z
* Disable uncrustify tests by default Signed-off-by: Yadunund <yadunund@intrinsic.ai> * Try to increase timeout Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai> * Revert "Try to increase timeout" This reverts commit 9104594. Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai> * Fix failing test in rolling Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai> * Fix race condition for ingesting and dispensing Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai> --------- Signed-off-by: Yadunund <yadunund@intrinsic.ai> Signed-off-by: Luca Della Vedova <lucadv@intrinsic.ai> Signed-off-by: Michael X. Grey <mxgrey@intrinsic.ai> Co-authored-by: Luca Della Vedova <lucadv@intrinsic.ai> Co-authored-by: Michael X. Grey <mxgrey@intrinsic.ai>
diff --git a/rmf_fleet_adapter/CMakeLists.txt b/rmf_fleet_adapter/CMakeLists.txt
@@ -49,7 +49,9 @@ foreach(pkg ${dep_pkgs})
   find_package(${pkg} REQUIRED)
 endforeach()
 
-if(BUILD_TESTING)
+# Disable uncrustify tests by default.
+set(TEST_UNCRUSTIFY "Off")
+if(BUILD_TESTING AND TEST_UNCRUSTIFY)
   find_package(ament_cmake_uncrustify REQUIRED)
   find_file(uncrustify_config_file
     NAMES "rmf_code_style.cfg"
diff --git a/rmf_fleet_adapter/src/rmf_fleet_adapter/phases/DispenseItem.cpp b/rmf_fleet_adapter/src/rmf_fleet_adapter/phases/DispenseItem.cpp
@@ -160,11 +160,13 @@ LegacyTask::StatusMsg DispenseItem::ActivePhase::_get_status(
   status.state = LegacyTask::StatusMsg::STATE_ACTIVE;
 
   using rmf_dispenser_msgs::msg::DispenserResult;
-  if (dispenser_result
-    && dispenser_result->request_guid == _request_guid
-    && is_newer(dispenser_result->time, _last_msg))
+  if (dispenser_result && dispenser_result->request_guid == _request_guid)
   {
-    _last_msg = dispenser_result->time;
+    if (is_newer(dispenser_result->time, _last_msg))
+    {
+      _last_msg = dispenser_result->time;
+    }
+
     switch (dispenser_result->status)
     {
       case DispenserResult::ACKNOWLEDGED:
diff --git a/rmf_fleet_adapter/src/rmf_fleet_adapter/phases/IngestItem.cpp b/rmf_fleet_adapter/src/rmf_fleet_adapter/phases/IngestItem.cpp
@@ -159,11 +159,13 @@ LegacyTask::StatusMsg IngestItem::ActivePhase::_get_status(
   status.state = LegacyTask::StatusMsg::STATE_ACTIVE;
 
   using rmf_ingestor_msgs::msg::IngestorResult;
-  if (ingestor_result
-    && ingestor_result->request_guid == _request_guid
-    && is_newer(ingestor_result->time, _last_msg))
+  if (ingestor_result && ingestor_result->request_guid == _request_guid)
   {
-    _last_msg = ingestor_result->time;
+    if (is_newer(ingestor_result->time, _last_msg))
+    {
+      _last_msg = ingestor_result->time;
+    }
+
     switch (ingestor_result->status)
     {
       case IngestorResult::ACKNOWLEDGED:
diff --git a/rmf_fleet_adapter/src/rmf_fleet_adapter/phases/Utils.cpp b/rmf_fleet_adapter/src/rmf_fleet_adapter/phases/Utils.cpp
@@ -23,7 +23,7 @@ namespace phases {
 bool is_newer(const builtin_interfaces::msg::Time& a,
   const builtin_interfaces::msg::Time& b)
 {
-  return a.sec > b.sec || (a.sec == b.sec && a.nanosec > b.nanosec);
+  return a.sec > b.sec || (a.sec == b.sec && a.nanosec >= b.nanosec);
 }
 
 } // namespace phases
diff --git a/rmf_task_ros2/CMakeLists.txt b/rmf_task_ros2/CMakeLists.txt
@@ -55,20 +55,22 @@ ament_export_targets(rmf_task_ros2 HAS_LIBRARY_TARGET)
 ament_export_dependencies(rmf_traffic rmf_task_msgs rclcpp nlohmann_json)
 
 #===============================================================================
-
+set(TEST_UNCRUSTIFY "Off")
 if(BUILD_TESTING)
   find_package(ament_cmake_catch2 QUIET)
-  find_package(ament_cmake_uncrustify REQUIRED)
-  find_file(uncrustify_config_file
-    NAMES "rmf_code_style.cfg"
-    PATHS "${rmf_utils_DIR}/../../../share/rmf_utils/")
-
-  ament_uncrustify(
-    ARGN include src test
-    CONFIG_FILE ${uncrustify_config_file}
-    LANGUAGE C++
-    MAX_LINE_LENGTH 80
-  )
+  if (TEST_UNCRUSTIFY)
+    find_package(ament_cmake_uncrustify REQUIRED)
+    find_file(uncrustify_config_file
+      NAMES "rmf_code_style.cfg"
+      PATHS "${rmf_utils_DIR}/../../../share/rmf_utils/")
+
+    ament_uncrustify(
+      ARGN include src test
+      CONFIG_FILE ${uncrustify_config_file}
+      LANGUAGE C++
+      MAX_LINE_LENGTH 80
+    )
+  endif()
 
   file(GLOB_RECURSE unit_test_srcs "test/*.cpp")
 
diff --git a/rmf_task_ros2/test/bidding/test_SelfBid.cpp b/rmf_task_ros2/test/bidding/test_SelfBid.cpp
@@ -90,24 +90,24 @@ SCENARIO("Auction with 2 Bids", "[TwoBids]")
   exec_options.context = rcl_context;
   rclcpp::executors::SingleThreadedExecutor executor(exec_options);
   executor.add_node(node);
+  const auto now = std::chrono::steady_clock::now();
 
   auto bidder1 = AsyncBidder::make(
     node,
-    [&test_notice_bidder1](const auto& notice, auto respond)
+    [&test_notice_bidder1, &now](const auto& notice, auto respond)
     {
       Response::Proposal best_robot_estimate;
       test_notice_bidder1 = notice.request;
       best_robot_estimate.fleet_name = "bidder1";
-      best_robot_estimate.finish_time =
-      std::chrono::steady_clock::time_point::max();
+      best_robot_estimate.finish_time = now + std::chrono::seconds(1000);
 
       respond(Response{best_robot_estimate, {}});
     }
   );
 
   auto bidder2 = AsyncBidder::make(
     node,
-    [&test_notice_bidder2](const auto& notice, auto respond)
+    [&test_notice_bidder2, &now](const auto& notice, auto respond)
     {
       auto request = nlohmann::json::parse(notice.request);
       if (request["category"] == "patrol")
@@ -117,8 +117,7 @@ SCENARIO("Auction with 2 Bids", "[TwoBids]")
       Response::Proposal best_robot_estimate;
       best_robot_estimate.new_cost = 2.3; // lower cost than bidder1
       best_robot_estimate.fleet_name = "bidder2";
-      best_robot_estimate.finish_time =
-      std::chrono::steady_clock::time_point::min();
+      best_robot_estimate.finish_time = now + std::chrono::seconds(1);
       test_notice_bidder2 = notice.request;
 
       respond(Response{best_robot_estimate, {}});
diff --git a/rmf_traffic_ros2/CMakeLists.txt b/rmf_traffic_ros2/CMakeLists.txt
@@ -41,19 +41,24 @@ if (rmf_traffic_FOUND)
   message(STATUS "rmf_traffic_INCLUDE_DIRS: ${rmf_traffic_INCLUDE_DIRS}")
 endif()
 
+# Disable uncrustify tests by default.
+set(TEST_UNCRUSTIFY "Off")
 if(BUILD_TESTING)
   find_package(ament_cmake_catch2 QUIET)
-  find_package(ament_cmake_uncrustify REQUIRED)
-  find_file(uncrustify_config_file
-    NAMES "rmf_code_style.cfg"
-    PATHS "${rmf_utils_DIR}/../../../share/rmf_utils/")
-
-  ament_uncrustify(
-    ARGN include src examples
-    CONFIG_FILE ${uncrustify_config_file}
-    LANGUAGE C++
-    MAX_LINE_LENGTH 80
-  )
+
+  if(TEST_UNCRUSTIFY)
+    find_package(ament_cmake_uncrustify REQUIRED)
+    find_file(uncrustify_config_file
+      NAMES "rmf_code_style.cfg"
+      PATHS "${rmf_utils_DIR}/../../../share/rmf_utils/")
+
+    ament_uncrustify(
+      ARGN include src examples
+      CONFIG_FILE ${uncrustify_config_file}
+      LANGUAGE C++
+      MAX_LINE_LENGTH 80
+    )
+  endif()
 
   file(GLOB_RECURSE unit_test_srcs "test/unit/*.cpp")
 
diff --git a/rmf_websocket/CMakeLists.txt b/rmf_websocket/CMakeLists.txt
@@ -88,18 +88,22 @@ install(
   ARCHIVE DESTINATION lib
 )
 
+# Disable uncrustify tests by default.
+set(TEST_UNCRUSTIFY "Off")
 if(BUILD_TESTING)
-  find_package(ament_cmake_uncrustify REQUIRED)
-  find_file(uncrustify_config_file
-    NAMES "rmf_code_style.cfg"
-    PATHS "${rmf_utils_DIR}/../../../share/rmf_utils/")
-
-  ament_uncrustify(
-    include src
-    CONFIG_FILE ${uncrustify_config_file}
-    LANGUAGE C++
-    MAX_LINE_LENGTH 80
-  )
+  if(TEST_UNCRUSTIFY)
+    find_package(ament_cmake_uncrustify REQUIRED)
+    find_file(uncrustify_config_file
+      NAMES "rmf_code_style.cfg"
+      PATHS "${rmf_utils_DIR}/../../../share/rmf_utils/")
+
+    ament_uncrustify(
+      include src
+      CONFIG_FILE ${uncrustify_config_file}
+      LANGUAGE C++
+      MAX_LINE_LENGTH 80
+    )
+  endif()
 
   # unit test
   find_package(ament_cmake_catch2 REQUIRED)

Original file line number	Diff line number	Diff line change
`@@ -23,7 +23,7 @@ namespace phases {`
`23`	`23`	`bool is_newer(const builtin_interfaces::msg::Time& a,`
`24`	`24`	`const builtin_interfaces::msg::Time& b)`
`25`	`25`	`{`
`26`		`- return a.sec > b.sec \|\| (a.sec == b.sec && a.nanosec > b.nanosec);`
	`26`	`+ return a.sec > b.sec \|\| (a.sec == b.sec && a.nanosec >= b.nanosec);`
`27`	`27`	`}`
`28`	`28`
`29`	`29`	`} // namespace phases`