-
Notifications
You must be signed in to change notification settings - Fork 412
Add ROS 2 integration tests #1024
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
Conversation
| const expectedTopics = [ | ||
| /* | ||
| * '/turtle1/cmd_vel', '/turtle1/color_sensor', '/turtle1/pose', | ||
| * '/turtle2/cmd_vel', '/turtle2/color_sensor', '/turtle2/pose', | ||
| */ | ||
| "/tf2_web_republisher/status", | ||
| "/tf2_web_republisher/feedback", | ||
| // '/tf2_web_republisher/goal', '/tf2_web_republisher/result', | ||
| "/fibonacci/feedback", | ||
| "/fibonacci/status", | ||
| "/fibonacci/result", | ||
| ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test data is ROS-1-only and wasn't even sensical.. it didn't check for the one thing unique to our test suites, the /listener topic 🤦🏻♂️
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
496132e to
e05d367
Compare
16c97c9 to
5bad2c5
Compare
|
@EzraBrooks It seems some tests related to Parameters are failing. Just FYI, I reimplemented the |
9ebdad7 to
9914ed3
Compare
|
edit: turns out the |
| // Known issue with getting params being able to hang in Humble because it had no timeout. | ||
| // This doesn't even work with `ros2 param list` at the CLI in our test environment :( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fyi
| <launch> | ||
| <!-- Include rosbridge websocket launch file for ROS 2 --> | ||
| <include file="$(find-pkg-share rosbridge_server)/launch/rosbridge_websocket_launch.xml"> | ||
| <arg name="call_services_in_new_thread" value="true"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
turns out our tests accidentally depend on this behavior which was not default in humble
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe leave a comment related to this so that when humble goes eol someone can see this and delete it
I'd also enable the action equivalent param send_action_goals_in_new_thread (going off memory) for the same reason
This comment was marked as resolved.
This comment was marked as resolved.
53f73c9 to
0636126
Compare
|
With how much duplicated effort is going on here, I think roslibjs 2.x should be the final major release to support ROS 1. I think there are a lot of great improvements in 2.0 that we should provide the ROS 1 users, but we should move on pretty quickly to a 3.0 that has no ROS 1 support. |
|
I enabled testing on rolling and then immediately disabled it because something outside my control was broken. Sorry, not gonna happen - at least not right now. |
It just seems like whatever ros version is being used in the backend for the rolling test doesn't contain this change |
|
It honestly may get easily fixed by doing an apt-get upgrade here: Line 9 in 76fc87e
Although that has implications for caching, of course. Ideally the docker image is just up to date lol |
|
I'm still not a fan of having my code be tested against the unstable branch of someone else's code, regardless. |
The usual model (at least that makes sense to me) is to have rolling in CI so you get the early feedback, but not having it be a required check to pass |
|
And therein lies the problem where people in the ROS community become inoculated to the sight of failing CI jobs. imo, next steps for the tests in this repo are to eliminate all the dependencies other than |
sea-bass
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
woof, you are right the ros 1/2 branching logic is something we should seek to get rid of. Nice job figuring all that out though.
| const PARAM_NAME = | ||
| process.env.ROS_DISTRO === "noetic" | ||
| ? "/test/foo" | ||
| : // Is it crazy to muck around with use_sim_time here? I feel like it shouldn't matter.. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine IMO
| <launch> | ||
| <!-- Include rosbridge websocket launch file for ROS 2 --> | ||
| <include file="$(find-pkg-share rosbridge_server)/launch/rosbridge_websocket_launch.xml"> | ||
| <arg name="call_services_in_new_thread" value="true"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe leave a comment related to this so that when humble goes eol someone can see this and delete it
I'd also enable the action equivalent param send_action_goals_in_new_thread (going off memory) for the same reason
These packages won't be able to easily stop depending on "core" libraries like But you're right, I don't think it's unreasonable to simply not test rolling. It means less risk of "red is okay" but also means by the time a new stable distro is cut the number of surprises has potentially amassed. |
|
I think it's easier to squash all the bugs at once than it is to be stressed out from random CI fires for a year ;) |
Public API Changes
None.
Description
noeticCloses #746