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 for regression in open_succeeds_twice and minimal_writer_example tests #1667

Merged
merged 2 commits into from
May 21, 2024

Conversation

MichaelOrlov
Copy link
Contributor

@MichaelOrlov MichaelOrlov commented May 21, 2024

The error message from CI reference build:

/home/jenkins-agent/workspace/nightly_linux-aarch64_repeated/ws/src/ros2/rosbag2/rosbag2_compression/test/rosbag2_compression/test_sequential_compression_writer.cpp:242
Expected: writer_->open(storage_options, {serialization_format_, serialization_format_}) doesn't throw an exception.
  Actual: it throws std::runtime_error with description "Bag directory already exists (/tmp/path_not_empty), can't overwrite existing bag
  • This PR addresses the failure in the rosbag2_compression.SequentialCompressionWriterTest.open_succeeds_twice on a second run due to the leftovers in the temp folder.
    It also addresses the flakiness in TestRosbag2CPPAPI::minimal_writer_example due to using shared ptr to the serialized message twice in the writer.write(msg) call.
    Note. The "serialized_msg2" in the TestRosbag2CPPAPI::minimal_writer_example does not own the serialized data after the first call writer.write(serialized_msg2,..). i.e. need to use another message or another API in the test for the second call to the writer.write(msg).

- The `serialized_msg2` is not owning the serialized data after the
first call writer.write(serialized_msg2,..). i.e. need to use another
message or another API in test for second call to writer.write(msg).

Signed-off-by: Michael Orlov <[email protected]>
@MichaelOrlov MichaelOrlov marked this pull request as ready for review May 21, 2024 17:44
@MichaelOrlov MichaelOrlov requested a review from a team as a code owner May 21, 2024 17:44
@MichaelOrlov MichaelOrlov requested review from gbiggs and james-rms and removed request for a team May 21, 2024 17:44
@MichaelOrlov MichaelOrlov requested review from clalancette and claraberendsen and removed request for gbiggs and james-rms May 21, 2024 17:46
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.

Looks good to me with green CI.

@MichaelOrlov
Copy link
Contributor Author

Pulls: #1667
Gist: https://gist.githubusercontent.com/MichaelOrlov/d4ba5a9c4c9c2f8dfa1bec28797574ad/raw/a85d2f3ae526f8081c0285cdf0cb1e2d7d9ba243/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_compression rosbag2_tests
TEST args: --packages-above rosbag2_compression rosbag2_tests
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/13936

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

@MichaelOrlov MichaelOrlov merged commit 8523995 into rolling May 21, 2024
14 checks passed
@delete-merged-branch delete-merged-branch bot deleted the morlov/fix_for_ci_regression_on_rolling branch May 21, 2024 20:23
@MichaelOrlov
Copy link
Contributor Author

https://github.com/Mergifyio backport iron

Copy link

mergify bot commented May 21, 2024

backport iron

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request May 21, 2024
…le` tests (#1667)

* Use test-specific temp subfolder to avoid leftovers from previous run

Signed-off-by: Michael Orlov <[email protected]>

* Address flakiness in TestRosbag2CPPAPI::minimal_writer_example

- The `serialized_msg2` is not owning the serialized data after the
first call writer.write(serialized_msg2,..). i.e. need to use another
message or another API in test for second call to writer.write(msg).

Signed-off-by: Michael Orlov <[email protected]>

---------

Signed-off-by: Michael Orlov <[email protected]>
(cherry picked from commit 8523995)

# Conflicts:
#	rosbag2_compression/test/rosbag2_compression/test_sequential_compression_writer.cpp
@MichaelOrlov
Copy link
Contributor Author

Pulls: #1667
Gist: https://gist.githubusercontent.com/MichaelOrlov/1aca57dfbcadf2cc7fde1994d79adae3/raw/b21940c48d7c010e294f043927a6ecf57f052447/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_compression rosbag2_tests
TEST args: --packages-above rosbag2_compression rosbag2_tests
ROS Distro: iron
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/13940

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

MichaelOrlov added a commit that referenced this pull request May 22, 2024
…r_example` tests (backport #1667) (#1669)

* Fix for "open_succeeds_twice" test failure on second run

- Use std::filesystem for temp files and folders operation. For some
reason rcpputils::fs::delete_all(folder_name) wasn't able to delete temp
folder with subfolders.

Signed-off-by: Michael Orlov <[email protected]>

* Address flakiness in TestRosbag2CPPAPI::minimal_writer_example

- The `serialized_msg2` is not owning the serialized data after the
first call writer.write(serialized_msg2,..). i.e. need to use another
message or another API in test for second call to writer.write(msg).

Signed-off-by: Michael Orlov <[email protected]>

---------

Signed-off-by: Michael Orlov <[email protected]>
Co-authored-by: Michael Orlov <[email protected]>
@MichaelOrlov
Copy link
Contributor Author

https://github.com/Mergifyio backport jazzy

Copy link

mergify bot commented May 23, 2024

backport jazzy

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request May 23, 2024
…le` tests (#1667)

* Use test-specific temp subfolder to avoid leftovers from previous run

Signed-off-by: Michael Orlov <[email protected]>

* Address flakiness in TestRosbag2CPPAPI::minimal_writer_example

- The `serialized_msg2` is not owning the serialized data after the
first call writer.write(serialized_msg2,..). i.e. need to use another
message or another API in test for second call to writer.write(msg).

Signed-off-by: Michael Orlov <[email protected]>

---------

Signed-off-by: Michael Orlov <[email protected]>
(cherry picked from commit 8523995)
MichaelOrlov added a commit that referenced this pull request May 24, 2024
…le` tests (#1667) (#1675)

* Use test-specific temp subfolder to avoid leftovers from previous run

Signed-off-by: Michael Orlov <[email protected]>

* Address flakiness in TestRosbag2CPPAPI::minimal_writer_example

- The `serialized_msg2` is not owning the serialized data after the
first call writer.write(serialized_msg2,..). i.e. need to use another
message or another API in test for second call to writer.write(msg).

Signed-off-by: Michael Orlov <[email protected]>

---------

Signed-off-by: Michael Orlov <[email protected]>
(cherry picked from commit 8523995)

Co-authored-by: Michael Orlov <[email protected]>
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