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

Refactor play until and duration duration tests #1024

Merged
merged 4 commits into from
Jul 6, 2022
Merged

Refactor play until and duration duration tests #1024

merged 4 commits into from
Jul 6, 2022

Conversation

agalbachicar
Copy link
Contributor

@agalbachicar agalbachicar commented Jun 13, 2022 •
edited
Loading

As requested in #1005 (review) , this PR refactors test_play_until and test_play_for (now called test_play_duration) to unify common code.

@agalbachicar agalbachicar force-pushed the agalbachicar/play_until_duration_test_refactor branch from 88c85a0 to 28be5c2 Compare June 13, 2022 12:27
@agalbachicar agalbachicar changed the title Refactor pllay until and duration duration tests Refactor play until and duration duration tests Jun 13, 2022
@agalbachicar agalbachicar force-pushed the agalbachicar/play_until_duration_test_refactor branch from 28be5c2 to fc36496 Compare June 13, 2022 12:36
@agalbachicar agalbachicar mentioned this pull request Jun 13, 2022
Merged
@agalbachicar agalbachicar force-pushed the agalbachicar/play_until_duration_test_refactor branch from fc36496 to 52f83eb Compare June 27, 2022 10:32
@agalbachicar agalbachicar marked this pull request as ready for review June 27, 2022 10:35
@agalbachicar agalbachicar requested a review from a team as a code owner June 27, 2022 10:35
@agalbachicar agalbachicar requested review from gbiggs and MichaelOrlov and removed request for a team June 27, 2022 10:35
@audrow audrow changed the base branch from master to rolling June 28, 2022 14:50
@agalbachicar
Refactors the common parts of play_until and play_duration tests.
927fc46 
- Migrates test fixtures of play with duration and until a timestamp options to a common header file.
- Moves test_play_for and PlayFor to use duration instead.

Signed-off-by: Agustin Alba Chicar <ag.albachicar@ekumenlabs.com>
@agalbachicar agalbachicar force-pushed the agalbachicar/play_until_duration_test_refactor branch from 52f83eb to 927fc46 Compare July 4, 2022 09:08
@agalbachicar
Copy link
Contributor Author

@MichaelOrlov it should be OK now to be reviewed.
Thanks in advance!

MichaelOrlov
MichaelOrlov requested changes Jul 5, 2022
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.

@agalbachicar Thanks for your contribution.
Overall looks good to me among a couple nitpicks in code formatting.

rosbag2_transport/test/rosbag2_transport/test_play_until.cpp Outdated Show resolved Hide resolved
rosbag2_transport/test/rosbag2_transport/test_play_until.cpp Outdated Show resolved Hide resolved
agalbachicar and others added 3 commits July 6, 2022 14:25
@agalbachicar @MichaelOrlov
Update rosbag2_transport/test/rosbag2_transport/test_play_until.cpp
bcf69a1 
Co-authored-by: Michael Orlov <morlovmr@gmail.com>
Signed-off-by: Agustin Alba Chicar <ag.albachicar@ekumenlabs.com>
@agalbachicar @MichaelOrlov
Update rosbag2_transport/test/rosbag2_transport/test_play_until.cpp
c6337cf 
Co-authored-by: Michael Orlov <morlovmr@gmail.com>
Signed-off-by: Agustin Alba Chicar <ag.albachicar@ekumenlabs.com>
@agalbachicar
Fixes formatting.
3a96ed3 
Signed-off-by: Agustin Alba Chicar <ag.albachicar@ekumenlabs.com>
@agalbachicar agalbachicar force-pushed the agalbachicar/play_until_duration_test_refactor branch from c70fd60 to 3a96ed3 Compare July 6, 2022 12:26
@MichaelOrlov
Copy link
Contributor

MichaelOrlov commented Jul 6, 2022 •
edited
Loading

Running CI:
Gist: https://gist.githubusercontent.com/MichaelOrlov/cfd91695252c91ead9685e1aaf26c96a/raw/b16c24d9a9149d9d8cb73ffa906dcf2ff9973dea/ros2.repos
BUILD args: --packages-above-and-dependencies rosbag2_transport rosbag2_tests
TEST args: --packages-above rosbag2_transport rosbag2_tests
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/10479

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

MichaelOrlov
MichaelOrlov approved these changes Jul 6, 2022
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.

@agalbachicar Thanks.
Approving now.

@agalbachicar
Copy link
Contributor Author

I've checked CI and it seems the problem on it was the flaky test.

@MichaelOrlov
Copy link
Contributor

I've checked CI and it seems the problem on it was the flaky test.

Yes. I agree. I am also running CI with build farm on 3 platforms.

@MichaelOrlov MichaelOrlov merged commit 856540e into ros2:rolling Jul 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MichaelOrlov MichaelOrlov MichaelOrlov approved these changes

@gbiggs gbiggs Awaiting requested review from gbiggs gbiggs 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.
2 participants
@agalbachicar @MichaelOrlov