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

Small cleanups to the rosbag2 tests. #1792

Merged
merged 2 commits into from
Aug 23, 2024
Merged

Small cleanups to the rosbag2 tests. #1792

merged 2 commits into from
Aug 23, 2024

Conversation

clalancette
Copy link
Contributor

  1. Rename "wait_for_srvice_to_be_ready" to "wait_for_service_to_be_ready".
  2. Make some of the constants constexpr, so we no longer have to capture them.

@clalancette
Small cleanups to the rosbag2 tests.
c332afc 
1.  Rename "wait_for_srvice_to_be_ready" to "wait_for_service_to_be_ready".
2.  Make some of the constants constexpr, so we no longer have to capture them.

Signed-off-by: Chris Lalancette <[email protected]>
@clalancette clalancette requested a review from a team as a code owner August 22, 2024 19:09
@clalancette clalancette requested review from emersonknapp and james-rms and removed request for a team August 22, 2024 19:09
@clalancette
Copy link
Contributor Author

Pulls: #1792
Gist: https://gist.githubusercontent.com/clalancette/4c2e6903c337a2a5d82aed0211cbb5d0/raw/e3663b1fa7b46c7bf84d94b838fff1622c215dbc/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_test_common rosbag2_tests rosbag2_transport
TEST args: --packages-above rosbag2_test_common rosbag2_tests rosbag2_transport
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14462

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

MichaelOrlov
MichaelOrlov approved these changes Aug 23, 2024
View reviewed changes
Copy link
Contributor

@MichaelOrlov MichaelOrlov left a comment

Choose a reason for hiding this comment

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

LGTM

MichaelOrlov
MichaelOrlov requested changes Aug 23, 2024
View reviewed changes
Copy link
Contributor

@MichaelOrlov MichaelOrlov left a comment

Choose a reason for hiding this comment

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

The second part i.e., removing expected_messages from capture list in lambdas doesn't work well on Windows.

@clalancette
Feedback from review.
7781cff 
Signed-off-by: Chris Lalancette <[email protected]>
@clalancette
Copy link
Contributor Author

The second part i.e., removing expected_messages from capture list in lambdas doesn't work well on Windows.

Ah, good call. Thanks for pointing that out. Fixed in 7781cff.

@clalancette
Copy link
Contributor Author

Pulls: #1792
Gist: https://gist.githubusercontent.com/clalancette/2b423f907038e2714f3b2e6e6f4e7687/raw/e3663b1fa7b46c7bf84d94b838fff1622c215dbc/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_test_common rosbag2_tests rosbag2_transport
TEST args: --packages-above rosbag2_test_common rosbag2_tests rosbag2_transport
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14464

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

MichaelOrlov
MichaelOrlov approved these changes Aug 23, 2024
View reviewed changes
Copy link
Contributor

@MichaelOrlov MichaelOrlov left a comment

Choose a reason for hiding this comment

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

LGTM now.

@clalancette clalancette merged commit 604cebc into rolling Aug 23, 2024
12 checks passed
@clalancette clalancette deleted the clalancette/rosbag2-test-cleanup branch August 23, 2024 18:54
@clalancette
Copy link
Contributor Author

Pulls: #1792
Gist: https://gist.githubusercontent.com/clalancette/47f256bdb35c83a7386955ba719181ca/raw/e3663b1fa7b46c7bf84d94b838fff1622c215dbc/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_test_common rosbag2_tests rosbag2_transport
TEST args: --packages-above rosbag2_test_common rosbag2_tests rosbag2_transport
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/14468

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@clalancette
Copy link
Contributor Author

Don't mind this last run; I made a mistake when running a script locally. It isn't relevant here.

@MichaelOrlov
Copy link
Contributor

https://github.com/Mergifyio backport jazzy

@mergify Mergify
Copy link

mergify bot commented Aug 23, 2024 •
edited
Loading

backport jazzy

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Aug 23, 2024
@clalancette @mergify
Small cleanups to the rosbag2 tests. (#1792)
d6fd40f 
* Small cleanups to the rosbag2 tests.

1.  Rename "wait_for_srvice_to_be_ready" to "wait_for_service_to_be_ready".
2.  Make some of the constants constexpr, so we no longer have to capture them.

Signed-off-by: Chris Lalancette <[email protected]>
(cherry picked from commit 604cebc)
@mergify mergify bot mentioned this pull request Aug 23, 2024
Merged
@clalancette clalancette mentioned this pull request Aug 27, 2024
Merged
MichaelOrlov pushed a commit that referenced this pull request Sep 5, 2024
@mergify @clalancette
Small cleanups to the rosbag2 tests. (#1792) (#1793)
865b864 
* Small cleanups to the rosbag2 tests.

1.  Rename "wait_for_srvice_to_be_ready" to "wait_for_service_to_be_ready".
2.  Make some of the constants constexpr, so we no longer have to capture them.

Signed-off-by: Chris Lalancette <[email protected]>
(cherry picked from commit 604cebc)

Co-authored-by: Chris Lalancette <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahcorde ahcorde ahcorde approved these changes

@MichaelOrlov MichaelOrlov MichaelOrlov approved these changes

@emersonknapp emersonknapp Awaiting requested review from emersonknapp emersonknapp is a code owner automatically assigned from ros2/tooling-reviewers

@james-rms james-rms Awaiting requested review from james-rms james-rms is a code owner automatically assigned from ros2/tooling-reviewers

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
@clalancette @MichaelOrlov @ahcorde