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.

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

fix some tests #2962

Merged
merged 4 commits into from
May 27, 2022
Merged

fix some tests #2962

merged 4 commits into from
May 27, 2022

Conversation

wep21
Copy link
Contributor

@wep21 wep21 commented May 21, 2022


Basic Info

Info Please fill out this column
Ticket(s) this addresses (add tickets here #1)
Primary OS tested on (Ubuntu, MacOS, Windows)
Robotic platform tested on (Steve's Robot, gazebo simulation of Tally, hardware turtlebot)

Description of contribution in a few bullet points

Description of documentation updates required from your changes


Future work that may be required in bullet points

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

@mergify
Copy link
Contributor

mergify bot commented May 21, 2022

@wep21, your PR has failed to build. Please check CI outputs and resolve issues.
You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

@mergify
Copy link
Contributor

mergify bot commented May 21, 2022

@wep21, please properly fill in PR template in the future. @SteveMacenski, use this instead.

  • 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 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

@wep21 wep21 force-pushed the ci-jammy-rolling branch 2 times, most recently from 96be68a to bbb9447 Compare May 22, 2022 07:12
@SteveMacenski
Copy link
Member

There's 1 linting error copyright=Intel Corporation. (2018), Florian Gramss (2020), license=<unknown> but otherwise the other 3 tests are usually flaky so I assume those are actually fine if everything else is working now!

Thanks for taking this on! What's the next steps here / changes you have in your fork that need to be made here to have CI turn over for Rolling?

@wep21
Copy link
Contributor Author

wep21 commented May 24, 2022

@SteveMacenski Could you create a jammy based builder image like
docker build . -t ghcr.io/ros-planning/navigation2:main-jammy --target builder and push it because I don't have a write access to this organization? (How you tag the image is up to you.) I will change the base image after that.

@SteveMacenski
Copy link
Member

@ruffsl is our master of such things, I want him to weigh in on this and decide if that's the right call

@mergify
Copy link
Contributor

mergify bot commented May 25, 2022

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

@wep21 wep21 changed the title Ci jammy rolling trial fix some tests May 25, 2022
@mergify
Copy link
Contributor

mergify bot commented May 25, 2022

@wep21, your PR has failed to build. Please check CI outputs and resolve issues.
You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

@SteveMacenski
Copy link
Member

@ruffsl should the user rebase? CI failing with gazebo/ros/dev/plugins and similar missing. I retriggered CI in case it was just an order thing

@wep21
Copy link
Contributor Author

wep21 commented May 25, 2022

The docker push action seemed to be failed.
https://github.com/ros-planning/navigation2/runs/6583947311?check_suite_focus=true

@ruffsl
Copy link
Member

ruffsl commented May 25, 2022

Yeah, it looks like the GitHub actions for rebuilding the CI image has failed due to broken apt package conflicts. I seem to have been lucky with my local rebuild just hours prior, so I'm not sure what must have changed and that short time window.

In the meantime I'm pushing the builder stage I had built and tagged locally up to GitHub to bootstrap the CI for jammy. However I live in the boonies, so with my upload speed it will take a few hours. 🐌

@mergify
Copy link
Contributor

mergify bot commented May 27, 2022

@wep21, your PR has failed to build. Please check CI outputs and resolve issues.
You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

@wep21 wep21 force-pushed the ci-jammy-rolling branch 2 times, most recently from de07be0 to 93eb044 Compare May 27, 2022 11:58
@wep21
Copy link
Contributor Author

wep21 commented May 27, 2022

@SteveMacenski I fixed tests except for costmap filters, and I found out bt_navigator is never activated because of stucking
here. Have you ever seen this behavior?

@SteveMacenski
Copy link
Member

SteveMacenski commented May 27, 2022

The issue I see in CI first is Failed to load node 'controller_server' of type 'nav2_controller::ControllerServer' in container 'nav2_container': Component constructor threw an exception: parameter 'goal_checker_plugins' has invalid type: Wrong parameter type, parameter {goal_checker_plugins} is of type {string_array}, setting it to {string} is not allowed.

Which is weird, some PR rebasing weirdness must have reverted that, @AlexeyMerzlyakov fixed that a few weeks ago. If you change

to goal_checker_plugins: ["goal_checker"] that should handle the issue. Or maybe you need to pull in changes and your base branch is out of date?

That could cause the lifecycle manager to fail bringup if the controller server can't be activated so it can't active the BT navigator server. If it did, somehow, get into the activate state, the loadBehaviorTree is called and could hang because we'd be infinitely waiting for the Controller Server to come up https://github.com/ros-planning/navigation2/blob/8a007c8eee054de3a91bddfe80e8c26e0b666fde/nav2_behavior_tree/include/nav2_behavior_tree/bt_action_node.hpp#L93 which it won't ever.

@SteveMacenski
Copy link
Member

#2974 I opened this to see if that change fixes all of the issues, but I suspect it will

Daisuke Nishimatsu added 4 commits May 28, 2022 03:01
Signed-off-by: Daisuke Nishimatsu <border_goldenmarket@yahoo.co.jp>
Signed-off-by: Daisuke Nishimatsu <border_goldenmarket@yahoo.co.jp>
Signed-off-by: Daisuke Nishimatsu <border_goldenmarket@yahoo.co.jp>
Signed-off-by: Daisuke Nishimatsu <border_goldenmarket@yahoo.co.jp>
@SteveMacenski SteveMacenski merged commit 35561ba into ros-navigation:main May 27, 2022
redvinaa pushed a commit to EnjoyRobotics/navigation2 that referenced this pull request Jun 30, 2022
* fix test

Signed-off-by: Daisuke Nishimatsu <border_goldenmarket@yahoo.co.jp>

* add bt navigator into lifecycle manager

Signed-off-by: Daisuke Nishimatsu <border_goldenmarket@yahoo.co.jp>

* fix goal checker plugin

Signed-off-by: Daisuke Nishimatsu <border_goldenmarket@yahoo.co.jp>

* load keepout filter param

Signed-off-by: Daisuke Nishimatsu <border_goldenmarket@yahoo.co.jp>
savalena pushed a commit to savalena/navigation2 that referenced this pull request Jul 5, 2024
* fix test

Signed-off-by: Daisuke Nishimatsu <border_goldenmarket@yahoo.co.jp>

* add bt navigator into lifecycle manager

Signed-off-by: Daisuke Nishimatsu <border_goldenmarket@yahoo.co.jp>

* fix goal checker plugin

Signed-off-by: Daisuke Nishimatsu <border_goldenmarket@yahoo.co.jp>

* load keepout filter param

Signed-off-by: Daisuke Nishimatsu <border_goldenmarket@yahoo.co.jp>
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