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

Readded WaitForTopics utility #333

Merged
merged 2 commits into from
Jan 4, 2022
Merged

Readded WaitForTopics utility #333

merged 2 commits into from
Jan 4, 2022

Conversation

adityapande-1995
Copy link
Contributor

@adityapande-1995 adityapande-1995 commented Dec 14, 2021 •
edited
Loading

This PR aims to revert the changes made in #326, as the upstream issue with unit tests in rclpy has been fixed ros2/launch_ros#278, and WaitForTopics has been added back to launch_testing_ros ros2/launch_ros#288

Signed-off-by: Aditya aditya050995@gmail.com

@adityapande-1995
Readded WaitForTopics utility
7e0d6bb 
Signed-off-by: Aditya <aditya050995@gmail.com>
clalancette
clalancette approved these changes Dec 14, 2021
View reviewed changes
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

Generally looks reasonable to me, though I think we need a new release of launch_ros to be done for the Rpr job to pass. I'll go ahead and do that. Also we'll want to really, really make sure that this passes on Windows (probably by running both Windows and Windows Debug CI, at least).

@clalancette
Copy link
Contributor

ros/rosdistro#31478 is a new release of launch_ros. It will probably take a couple of hours for that to rebuild, but once that is done we can re-run the Rpr job.

@adityapande-1995
Copy link
Contributor Author

@ros-pull-request-builder retest this please

1 similar comment
@clalancette
Copy link
Contributor

@ros-pull-request-builder retest this please

@adityapande-1995
Copy link
Contributor Author

adityapande-1995 commented Dec 15, 2021 •
edited
Loading

CI :

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status
  • Windows debug : Build Status

Windows CI round 2 : Build Status

@adityapande-1995
Copy link
Contributor Author

adityapande-1995 commented Dec 21, 2021 •
edited
Loading

Running full CI as discussed with @ivanpauno :
build : --packages-above-and-dependencies launch_testing_examples rclpy ament_package launch_ros
test: --packages-above launch_testing_examples rclpy ament_package launch_ros

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status
  • Windows debug : Build Status

Errors in CI seem to be in rclcpp, rosbag2 are unrelated to the changes here.
Vanilla rolling CI :

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

ivanpauno
ivanpauno approved these changes Dec 23, 2021
View reviewed changes
Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

LGTM!

@adityapande-1995
Copy link
Contributor Author

adityapande-1995 commented Dec 23, 2021 •
edited
Loading

I'll disable set_param_launch_test for now, as it is failing on windows. Originally added here with green CI : #324

@adityapande-1995
Disabled set parameter test on windows
4cd6bc3 
Signed-off-by: Aditya <aditya050995@gmail.com>
@adityapande-1995
Copy link
Contributor Author

adityapande-1995 commented Jan 4, 2022 •
edited
Loading

Windows CI : Build Status
Vanilla Windows CI : Build Status

The only error (test_record_) not common in these 2 seems to be unrelated to the changes made here.

ivanpauno
ivanpauno approved these changes Jan 4, 2022
View reviewed changes
Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

LGTM!

@adityapande-1995 adityapande-1995 merged commit 6e76f78 into master Jan 4, 2022
@delete-merged-branch delete-merged-branch bot deleted the aditya/readd_wait_for_topics branch January 4, 2022 18:31
@adityapande-1995 adityapande-1995 mentioned this pull request Jan 6, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clalancette clalancette clalancette approved these changes

@ivanpauno ivanpauno ivanpauno 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.
3 participants
@adityapande-1995 @clalancette @ivanpauno