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

Redesign record_services tests to make them more deterministic #1122

Merged
merged 1 commit into from
Oct 26, 2022
Merged

Redesign record_services tests to make them more deterministic #1122

merged 1 commit into from
Oct 26, 2022

Conversation

MichaelOrlov
Copy link
Contributor

@MichaelOrlov MichaelOrlov commented Oct 12, 2022 •
edited
Loading

cc: @rshanor

@MichaelOrlov MichaelOrlov marked this pull request as ready for review October 12, 2022 23:05
@MichaelOrlov MichaelOrlov requested a review from a team as a code owner October 12, 2022 23:05
@MichaelOrlov MichaelOrlov requested review from gbiggs, jhdcs, clalancette and emersonknapp and removed request for a team October 12, 2022 23:05
@rshanor
Copy link
Contributor

rshanor commented Oct 12, 2022

Nice!

clalancette
clalancette reviewed Oct 13, 2022
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.

The changes seem generally reasonable to me; I particularly like the loops that wait 10 seconds, but check every 30 ms for events to happen. That should improve reliability.

I'm not going to approve because I don't really have great context on this code. What I will say is that it would be fantastic to run CI on this with --retest-until-fail 50 or something to make sure that it is truly reliable.

@MichaelOrlov
Copy link
Contributor Author

Gist: https://gist.githubusercontent.com/MichaelOrlov/3ebb8badcc544b55d28449b44c18210d/raw/f306bdded1820d5a60a87773389d1af1aaac99e8/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_transport
TEST args: --packages-above rosbag2_transport
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/10992

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

@MichaelOrlov
Copy link
Contributor Author

Failure on Windows CI job seems unrelated

Creating library C:/ci/ws/build/rmw_connextdds_common/Release/rmw_connextdds_common_pro.lib and object C:/ci/ws/build/rmw_connextdds_common/Release/rmw_connextdds_common_pro.exp
CVTRES : fatal error CVT1107: 'C:\Program Files\rti_connext_dds-6.0.1\lib\x64Win64VS2017\nddscored.lib' is corrupt [C:\ci\ws\build\rmw_connextdds_common\rmw_connextdds_common_pro.vcxproj]
LINK : fatal error LNK1123: failure during conversion to COFF: file invalid or corrupt [C:\ci\ws\build\rmw_connextdds_common\rmw_connextdds_common_pro.vcxproj]

@MichaelOrlov
Copy link
Contributor Author

Running Linux CI job with --retest-until-fail 50

  • Linux Build Status

@MichaelOrlov
Copy link
Contributor Author

@clalancette Challenge accepted. 😉 Linux CI job with --retest-until-fail 50 passing green.
Hope it's bullet CI proof to be approved.

Re-running Windows build since it was some unrelated issues with rti_dds

  • Windows Build Status

@rshanor
Copy link
Contributor

rshanor commented Oct 19, 2022

@christophebedard @gbiggs @jhdcs @emersonknapp any chance this could get approval? Need this to land to address PR feedback in #1131

@MichaelOrlov
Copy link
Contributor Author

@clalancette @emersonknapp Kindly ping for re-iterate on code review.
IMO the PR is CI proofed. It was running with --retest-until-fail 50 with no errors.

clalancette
clalancette approved these changes Oct 26, 2022
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.

It seems reasonable to me. Especially since it is in test code, we can always address anything else in a follow-up. Thanks for doing the torture-tests in CI, that gives us a lot better confidence in it.

@MichaelOrlov MichaelOrlov merged commit 5f0c7d4 into rolling Oct 26, 2022
@delete-merged-branch delete-merged-branch bot deleted the morlov/redesign-in-record_services-tests branch October 26, 2022 22:11
@MichaelOrlov MichaelOrlov mentioned this pull request Oct 27, 2022
Merged
@MichaelOrlov
Copy link
Contributor Author

https://github.com/Mergifyio backport humble

mergify bot pushed a commit that referenced this pull request Oct 29, 2022
@MichaelOrlov @mergify
Redesign record_services tests to make them more deterministic (#1122)
70111b1 
Signed-off-by: Michael Orlov <[email protected]>

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

# Conflicts:
#	rosbag2_transport/test/rosbag2_transport/mock_sequential_writer.hpp
#	rosbag2_transport/test/rosbag2_transport/test_record_services.cpp
@mergify mergify bot mentioned this pull request Oct 29, 2022
Merged
@mergify
Copy link

mergify bot commented Oct 29, 2022

backport humble

✅ Backports have been created

emersonknapp
emersonknapp reviewed Nov 2, 2022
View reviewed changes
Copy link
Collaborator

@emersonknapp emersonknapp left a comment

Choose a reason for hiding this comment

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

after the fact - this looks good to me too

MichaelOrlov added a commit that referenced this pull request Apr 3, 2023
@MichaelOrlov
BP. Redesign record_services tests to make them more deterministic (#…
fd17bf5 
…1122)

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

Signed-off-by: Michael Orlov <[email protected]>
(cherry picked from commit 5f0c7d4)
MichaelOrlov added a commit that referenced this pull request Apr 3, 2023
@MichaelOrlov
[humble] Redesign record_services tests to make them more determinist…
a3f66a7 
…ic (#1122)

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

Signed-off-by: Michael Orlov <[email protected]>
(cherry picked from commit 5f0c7d4)
MichaelOrlov added a commit that referenced this pull request Apr 10, 2023
@MichaelOrlov
[humble] Redesign record_services tests to make them more determinist…
ece2da4 
…ic (#1122)

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

Signed-off-by: Michael Orlov <[email protected]>
(cherry picked from commit 5f0c7d4)
MichaelOrlov added a commit that referenced this pull request Apr 11, 2023
@MichaelOrlov
[humble] Redesign record_services tests to make them more determinist…
c17a896 
…ic (#1122)

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

Signed-off-by: Michael Orlov <[email protected]>
(cherry picked from commit 5f0c7d4)
MichaelOrlov added a commit that referenced this pull request Apr 12, 2023
@mergify @MichaelOrlov
[humble] Redesign record_services tests to make them more determinist…
e30daed 
…ic (#1122) (#1142)

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

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

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
Reviewers

@emersonknapp emersonknapp emersonknapp left review comments

@clalancette clalancette clalancette approved these changes

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

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

@christophebedard christophebedard Awaiting requested review from christophebedard

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@MichaelOrlov @rshanor @clalancette @emersonknapp