Skip to content

Use main function instead of global variable to setup the test environment.#4948

Merged
SteveMacenski merged 7 commits intoros-navigation:mainfrom
evshary:use_main_setup_env
Mar 6, 2025
Merged

Use main function instead of global variable to setup the test environment.#4948
SteveMacenski merged 7 commits intoros-navigation:mainfrom
evshary:use_main_setup_env

Conversation

@evshary
Copy link
Contributor

@evshary evshary commented Feb 25, 2025


Basic Info

Info Please fill out this column
Ticket(s) this addresses -
Primary OS tested on Ubuntu
Robotic platform tested on -
Does this PR contain AI generated software? No

Description of contribution in a few bullet points

Now the unit test relies on using global variables to setup the environment.
For example,

RclCppFixture g_rclcppfixture;

I think it would be better to use the main function instead.

We discovered that while testing rmw_zenoh.
Due to the issue, rmw_zenoh couldn't shutdown in atexit stage. That means calling shutdown in the destructor of a global variable will cause panic.
We believe it would be better to avoid using global variables.

Here is the list of affected tests: ZettaScaleLabs/rmw_zenoh#54

Description of documentation updates required from your changes

Not required

Description of how this change was tested

Run the CI test.


Future work that may be required in bullet points

For Maintainers:

  • Check that any new parameters added are updated in docs.nav2.org
  • Check that any significant change is added to the migration guide
  • Check that any new features OR changes to existing behaviors are reflected in the tuning guide
  • Check that any new functions have Doxygen added
  • Check that any new features have test coverage
  • Check that any new plugins is added to the plugins page
  • If BT Node, Additionally: add to BT's XML index of nodes for groot, BT package's readme table, and BT library lists

@codecov
Copy link

codecov bot commented Feb 25, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

see 3 files with indirect coverage changes

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@SteveMacenski SteveMacenski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@evshary thanks for the PR!

I broadly speaking agree and approve, though I'd like an explanation for the changes in nav2_behavior_tree/test/plugins/action/test_remove_in_collision_goals_action.cpp which don't match the same changes made in the other files.

Otherwise, LGTM and happy to merge once I understand why only this one test is otherwise adjusted

@evshary
Copy link
Contributor Author

evshary commented Mar 5, 2025

Hi @SteveMacenski

Thank you for the review! Yes, a more detailed explanation for that should be better.
Originally, RemoveInCollisionGoalsTestFixture is used to initialize/clean the test environment by static void SetUpTestCase() and static void TearDownTestCase(). However, the fixture will touch the rclcpp::Node and destruct in atexit stage because it's a static variable.

In rmw_zenoh, there is a limitation in that we can't run anything in the atexit stage because some context is no longer available, and it will panic. So to avoid initializing/cleaning the test environment with static, I refactored the class and only initialized/cleaned it inside the main function.

Please let me know if you want this in a separate PR or you have any concern on this.

@YuanYuYuan
Copy link

Hi @evshary! Regarding the fix on nav2_behavior_tree/test/plugins/action/test_remove_in_collision_goals_action.cpp, I believe this patch could be a solution.

diff --git a/nav2_behavior_tree/test/plugins/action/test_remove_in_collision_goals_action.cpp b/nav2_behavior_tree/test/plugins/action/test_remove_in_collision_goals_action.cpp
index 307bd6e3..ea750fe4 100644
--- a/nav2_behavior_tree/test/plugins/action/test_remove_in_collision_goals_action.cpp
+++ b/nav2_behavior_tree/test/plugins/action/test_remove_in_collision_goals_action.cpp
@@ -106,6 +106,7 @@ public:
     config_ = nullptr;
     node_.reset();
     success_server_.reset();
+    failure_server_.reset();
     factory_.reset();
   }

evshary added 6 commits March 5, 2025 20:55
Signed-off-by: ChenYing Kuo <evshary@gmail.com>
Signed-off-by: ChenYing Kuo <evshary@gmail.com>
Signed-off-by: ChenYing Kuo <evshary@gmail.com>
Signed-off-by: ChenYing Kuo <evshary@gmail.com>
This reverts commit 3d0e468.

Signed-off-by: ChenYing Kuo <evshary@gmail.com>
Signed-off-by: ChenYing Kuo <evshary@gmail.com>
@evshary evshary force-pushed the use_main_setup_env branch from 271ecc0 to 9c2a5e6 Compare March 5, 2025 12:55
@SteveMacenski
Copy link
Member

@evshary can you fix the merge conflict? Then happy to merge this in

Thanks for the help to make this work well with Zenoh!

@mergify
Copy link
Contributor

mergify bot commented Mar 6, 2025

This pull request is in conflict. Could you fix it @evshary?

Signed-off-by: ChenYing Kuo <evshary@gmail.com>
@evshary
Copy link
Contributor Author

evshary commented Mar 6, 2025

@SteveMacenski It's resolved now. Hope we can make rmw_zenoh pass all the tests in navigation2 in the near future 😄

@SteveMacenski SteveMacenski merged commit 9f0af3b into ros-navigation:main Mar 6, 2025
11 checks passed
@evshary evshary deleted the use_main_setup_env branch March 7, 2025 02:39
RBT22 added a commit to EnjoyRobotics/navigation2 that referenced this pull request Mar 18, 2025
stevedanomodolor pushed a commit to stevedanomodolor/navigation2 that referenced this pull request Apr 29, 2025
…nment. (ros-navigation#4948)

* Use the main function to setup the test environment.

Signed-off-by: ChenYing Kuo <evshary@gmail.com>

* Apply the modification to all the tests.

Signed-off-by: ChenYing Kuo <evshary@gmail.com>

* Refactor the test_remove_in_collision_goals_action.

Signed-off-by: ChenYing Kuo <evshary@gmail.com>

* Add the missing test items.

Signed-off-by: ChenYing Kuo <evshary@gmail.com>

* Revert "Refactor the test_remove_in_collision_goals_action."

This reverts commit 3d0e468.

Signed-off-by: ChenYing Kuo <evshary@gmail.com>

* Reset failure_server_

Signed-off-by: ChenYing Kuo <evshary@gmail.com>

---------

Signed-off-by: ChenYing Kuo <evshary@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants