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

Mark play_end_to_end test as xfail in Windows #1452

Merged
merged 2 commits into from
Aug 28, 2023
Merged

Mark play_end_to_end test as xfail in Windows #1452

merged 2 commits into from
Aug 28, 2023

Conversation

Crola1702
Copy link
Contributor

As mentioned in #1437

@Crola1702 Crola1702 requested a review from a team as a code owner August 23, 2023 19:34
@Crola1702 Crola1702 requested review from gbiggs, jhdcs and clalancette and removed request for a team August 23, 2023 19:34
@Crola1702 Crola1702 mentioned this pull request Aug 23, 2023
Open
@Crola1702
Copy link
Contributor Author

CI:

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

@emersonknapp
Copy link
Collaborator

Should we only do this on windows?

MichaelOrlov
MichaelOrlov requested changes Aug 28, 2023
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.

I think it makes sense to mark the test as xfail only for the Windows platform seems it only regularly fails on Windows.

rosbag2_tests/CMakeLists.txt Outdated Show resolved Hide resolved
@Crola1702
Disable only for Windows
e284025 
Signed-off-by: Crola1702 <[email protected]>
@Crola1702 Crola1702 changed the title Mark play_end_to_end test as xfail Mark play_end_to_end test as xfail in Windows Aug 28, 2023
@Crola1702
Copy link
Contributor Author

CI:

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

clalancette
clalancette approved these changes Aug 28, 2023
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.

Looks reasonable to me, but I'd like to wait for final confirmation from @MichaelOrlov before merging.

MichaelOrlov
MichaelOrlov approved these changes Aug 28, 2023
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.

👍

@Crola1702 Crola1702 merged commit a48001a into rolling Aug 28, 2023
13 checks passed
@delete-merged-branch delete-merged-branch bot deleted the Crola1702/disasble-play_end_to_end-test branch August 28, 2023 17:38
@MichaelOrlov
Copy link
Contributor

@clalancette Sorry, I haven't had a chance yet to make a comprehensive analysis about the reason why this test fails on Windows. But as I said before, I don't mind disabling this test for Windows for a while. Anyway, I will have to reproduce the failure locally.

@clalancette
Copy link
Contributor

@clalancette Sorry, I haven't had a chance yet to make a comprehensive analysis about the reason why this test fails on Windows. But as I said before, I don't mind disabling this test for Windows for a while. Anyway, I will have to reproduce the failure locally.

No worries at all, thanks for any time you spend on it. I think disabling it just for Windows is a good compromise for now.

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

@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

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
@Crola1702 @emersonknapp @MichaelOrlov @clalancette