Skip to content

Add in a test for unbounded serialized take.#592

Merged
clalancette merged 1 commit intorollingfrom
clalancette/add-serialized-take-test
May 5, 2026
Merged

Add in a test for unbounded serialized take.#592
clalancette merged 1 commit intorollingfrom
clalancette/add-serialized-take-test

Conversation

@clalancette
Copy link
Copy Markdown
Contributor

@clalancette clalancette commented Apr 30, 2026

Description

We already had a test for serialized_take, but it was only a simple type. Unbounded types are handled differently in rmw_fastrtps_cpp now, so we should have a separate test for it.

This will go along with a series of fixes to rmw_fastrtps_cpp

Is this user-facing behavior change?

No.

Did you use Generative AI?

Yes, I used Claude Opus 4.7

Additional Information

This should be merged after ros2/rmw_fastrtps#880 , ros2/rmw_fastrtps#879 , and ros2/rmw_fastrtps#881 .

This should be backported to Lyrical.

This is actually handled differently in rmw_fastrtps_cpp
now, so we should have a separate test for it.

Signed-off-by: Chris Lalancette <clalancette@gmail.com>
@clalancette
Copy link
Copy Markdown
Contributor Author

Here is CI with this and all of the other PRs listed here:

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

@clalancette
Copy link
Copy Markdown
Contributor Author

clalancette commented May 4, 2026

Here's another build with the updates in place:

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

@clalancette
Copy link
Copy Markdown
Contributor Author

I think the CI for Windows failure is a red herring (get it?).

The one failing test is test_play_publish_clock__rmw_fastrtps_cpp . That test expects publish-to-publish latency to be 7.5ms, but that seems like far too short a gap to reliably get on Windows. It actually saw 9ms, which seems within reason, so I think this is a bug in the rosbag2 tests. The rest of the test failures also seem like Windows-specific issues unrelated to this change.

It is a FAILURE rather than UNSTABLE because I think ros2/launch_ros#540 is incorrect for pytest 7 (which is what Windows is still on). We can address that separately.

I'm tempted to merge this series in as-is, but I'll want consent on it because these Windows failures look problematic.

@clalancette
Copy link
Copy Markdown
Contributor Author

OK, after discussion, the consensus is to merge these in and then deal with the rest of the Windows fallout separately. So going ahead and merging in.

@clalancette clalancette merged commit 1a0d6a0 into rolling May 5, 2026
3 checks passed
@clalancette clalancette deleted the clalancette/add-serialized-take-test branch May 5, 2026 19:38
@clalancette
Copy link
Copy Markdown
Contributor Author

@Mergifyio backport lyrical

@mergify
Copy link
Copy Markdown

mergify Bot commented May 5, 2026

backport lyrical

✅ Backports have been created

Details

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.

2 participants