Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve coverage for waypoint_follower package #2407

Merged
merged 6 commits into from
Jun 18, 2021
Merged

Improve coverage for waypoint_follower package #2407

merged 6 commits into from
Jun 18, 2021

Conversation

lucabonamini
Copy link
Contributor

@lucabonamini lucabonamini commented Jun 14, 2021 •
edited
Loading

Basic Info

Info Please fill out this column
Ticket(s) this addresses None
Primary OS tested on Ubuntu
Robotic platform tested on None

Description of contribution in a few bullet points

  • Added filled image message data to PhotoAtWaypoint test in order to test uncovered lines of photo_at_waypoint.cpp plugin

Future work that may be required in bullet points

  • Other tests might be necessary to get 100% coverage

For Maintainers:

  • Check that any new parameters added are updated in navigation.ros.org
  • Check that any significant change is added to the migration 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

@SteveMacenski
Copy link
Member

Hi,
You are right, some of these build failures are not your fault, we had some issues after the Galactic sync and we're still attempting to recover. Our CI isn't in the best spot at the moment so I haven't spent the time to debug these issues until its all stabilized. Docker/ROS/GitHub/etc all decided to have a bunch of issues all at once.

If you could, however, resolve these, it would be appreciated:

succeeds_node_name
build/include_order [4] (/opt/overlay_ws/src/navigation2/nav2_dwb_controller/nav_2d_utils/src/conversions.cpp:38)
build/include_order [4] (/opt/overlay_ws/src/navigation2/nav2_dwb_controller/nav_2d_utils/src/conversions.cpp:39)
build/include_order [4] (/opt/overlay_ws/src/navigation2/nav2_dwb_controller/nav_2d_utils/src/conversions.cpp:40)
build/include_order [4] (/opt/overlay_ws/src/navigation2/nav2_dwb_controller/nav_2d_utils/src/conversions.cpp:41)
build/include_order [4] (/opt/overlay_ws/src/navigation2/nav2_dwb_controller/nav_2d_utils/src/conversions.cpp:42)

It should be simple, they're just included in the wrong order after some linter profile updates from Galactic 🙄

I am looking at the codecov results https://codecov.io/gh/ros-planning/navigation2/tree/ab4f95b6d43e82ba633015bcafac61b21ddba622/nav2_waypoint_follower and seeing about ~2% additional testing to 86%! Any chance you took a look at the remaining code not covered / main server to see how we could push that package to 90%?

@lucabonamini
Copy link
Contributor Author

If you could, however, resolve these, it would be appreciated:

succeeds_node_name
build/include_order [4] (/opt/overlay_ws/src/navigation2/nav2_dwb_controller/nav_2d_utils/src/conversions.cpp:38)
build/include_order [4] (/opt/overlay_ws/src/navigation2/nav2_dwb_controller/nav_2d_utils/src/conversions.cpp:39)
build/include_order [4] (/opt/overlay_ws/src/navigation2/nav2_dwb_controller/nav_2d_utils/src/conversions.cpp:40)
build/include_order [4] (/opt/overlay_ws/src/navigation2/nav2_dwb_controller/nav_2d_utils/src/conversions.cpp:41)
build/include_order [4] (/opt/overlay_ws/src/navigation2/nav2_dwb_controller/nav_2d_utils/src/conversions.cpp:42)

It should be simple, they're just included in the wrong order after some linter profile updates from Galactic roll_eyes

Sure, no problem!

Any chance you took a look at the remaining code not covered / main server to see how we could push that package to 90%?

I'm already working on it 😅

@lucabonamini
Copy link
Contributor Author

lucabonamini commented Jun 15, 2021 •
edited
Loading

If you could, however, resolve these, it would be appreciated:

succeeds_node_name
build/include_order [4] (/opt/overlay_ws/src/navigation2/nav2_dwb_controller/nav_2d_utils/src/conversions.cpp:38)
build/include_order [4] (/opt/overlay_ws/src/navigation2/nav2_dwb_controller/nav_2d_utils/src/conversions.cpp:39)
build/include_order [4] (/opt/overlay_ws/src/navigation2/nav2_dwb_controller/nav_2d_utils/src/conversions.cpp:40)
build/include_order [4] (/opt/overlay_ws/src/navigation2/nav2_dwb_controller/nav_2d_utils/src/conversions.cpp:41)
build/include_order [4] (/opt/overlay_ws/src/navigation2/nav2_dwb_controller/nav_2d_utils/src/conversions.cpp:42)

It should be simple, they're just included in the wrong order after some linter profile updates from Galactic roll_eyes

Sure, no problem!

Done. Tested on my laptop and already pushed.

Any chance you took a look at the remaining code not covered / main server to see how we could push that package to 90%?

I'm already working on it sweat_smile

How do you test coverage locally? I have set up Codecov locally to easily check for coverage improvements. However the percentages I get are different from those reported on codecov.io.

@SteveMacenski
Copy link
Member

How do you test coverage locally? I have set up Codecov locally to easily check for coverage improvements. However the percentages I get are different from those reported on codecov.io.

Unfortunately, I don't have a good guide ready at the moment. This ticket ros-navigation/docs.nav2.org#88 introduces the idea of a tutorial for that with some instructions I've used in the past when I've been working on test coverage, they might help!

@lucabonamini
Copy link
Contributor Author

@SteveMacenski Last commit pushed plugins coverage to 95.04%, but main server coverage dramatically drop to 41.03%. I can't figured out what's happened.

@SteveMacenski
Copy link
Member

I'm seeing that it dropped (https://codecov.io/gh/ros-planning/navigation2/tree/ceb3bc4ff61c8567a74c3a0031b21b20cf60dc80), probably because the waypoint tests failed https://app.circleci.com/pipelines/github/ros-planning/navigation2/5532/workflows/a63c326a-d859-4c4c-97e2-57d6d2a5636c/jobs/20255/tests#failed-test-4

[tester_node-13] [ERROR] [1623961740.722004379] [nav2_waypoint_tester]: Goal rejected
[tester_node-13] Traceback (most recent call last):
[tester_node-13]   File "/opt/overlay_ws/src/navigation2/nav2_system_tests/src/waypoint_follower/tester.py", line 230, in <module>
[tester_node-13]     main()
[tester_node-13]   File "/opt/overlay_ws/src/navigation2/nav2_system_tests/src/waypoint_follower/tester.py", line 198, in main
[tester_node-13]     assert result
[tester_node-13] AssertionError

the tester died

@lucabonamini
Copy link
Contributor Author

Yes, I saw the result of that test. Any idea why it failed? I've just made some changes to the plugins module

@SteveMacenski
Copy link
Member

https://github.com/ros-planning/navigation2/blob/main/nav2_system_tests/src/waypoint_follower/tester.py#L198

198 means that it failed. I'd check the logs and see what failed to run properly. It looks like there's a print that the goal was rejected. It was not up in lifecycle yet which make sense (looking at the logs above, still trying to get the initial pose).

Going a bit up more you see a [ERROR] [gzserver-1]: process has died [pid 7225, exit code -6, cmd 'gzserver -s libgazebo_ros_init.so --minimal_comms /opt/overlay_ws/src/navigation2/nav2_system_tests/worlds/turtlebot3_ros2_demo.world']. so gazebo died. Probably not your fault, just transient. I'll retrigger CI to see how it does a second time.

CI issues are always hard to debug, my recommendation would be to take it 1 step at a time, usually you can find the problem if you dont get overwhelmed by the logs and just focus in on 1 thing at a time

@lucabonamini
Copy link
Contributor Author

You are definitely right. Since I can't have CircleCI builds locally, I just wanted to avoid doing test commits to debug.

Probably not your fault, just transient. I'll retrigger CI to see how it does a second time.

Let's see how this new build ends!

@lucabonamini
Copy link
Contributor Author

Total coverage for nav2_waypoint_follower is now 90.24%. Still improvable.

SteveMacenski
SteveMacenski approved these changes Jun 18, 2021
View reviewed changes
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.

Awesome! Thanks so much for this, another package to 90%!

@SteveMacenski SteveMacenski merged commit 12fa591 into ros-navigation:main Jun 18, 2021
ruffsl pushed a commit to ruffsl/navigation2 that referenced this pull request Jul 2, 2021
@ruffsl
Improve coverage for waypoint_follower package (ros-navigation#2407)
b00dfc0 
* Working on improve coverage for waypoint_follower package

* Fix typo

* Fix cpplint and uncrustify errors

* Fix nav_2d_utils/src/conversions.cpp include order errors (ros-navigation#2407)

* Improve coverage for wait_at_waypoint plugin

* Improve coverage for photo_at_waypoint plugin
@lucabonamini lucabonamini deleted the tests-waypoint_follower branch September 5, 2021 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SteveMacenski SteveMacenski SteveMacenski approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@lucabonamini @SteveMacenski