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

Skip failing Windows tests #1205

Merged
merged 10 commits into from
Nov 19, 2021
Merged

Skip failing Windows tests #1205

merged 10 commits into from
Nov 19, 2021

Conversation

Blast545
Copy link
Contributor

Signed-off-by: Jorge Perez [email protected]

First step towards #1175

Summary

As the title says, skips Windows tests for ign-gazebo5. Will get green Windows buildfarm before merging.

Checklist

  • Signed all commits for DCO
  • Added tests (actually removing tests)
  • All tests passed (See test coverage)

Note to maintainers: Remember to use Squash-Merge

@github-actions github-actions bot added the 🏢 edifice Ignition Edifice label Nov 15, 2021
@Blast545 Blast545 self-assigned this Nov 15, 2021
@Blast545 Blast545 added tests Broken or missing tests / testing infra Windows Windows support labels Nov 15, 2021
@Blast545
Copy link
Contributor Author

Blast545 commented Nov 15, 2021

Worskpace temp error on the Windows pr job:

  • WIndows Build Status

Manual rebuild:

  • WIndows Build Status

Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Nice! This is going in a good direction! There are some compilation and codecheck errors though

@@ -92,7 +92,8 @@ class EntityFeatureMapFixture: public InternalFixture<::testing::Test>
public: EnginePtrType engine;
};

TEST_F(EntityFeatureMapFixture, AddCastRemoveEntity)
// See https://github.com/ignitionrobotics/ign-gazebo/issues/1175
TEST_F(EntityFeatureMapFixture, IGN_UTILS_TEST_ENABLE_ONLY_LINUX(AddCastRemoveEntity))
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be disabled only on Windows?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, my bad there. Addressed here: 5901c2b

@Blast545 Blast545 force-pushed the blast545/skip_windows_tests branch from 4905869 to 5901c2b Compare November 16, 2021 02:08
@codecov
Copy link

codecov bot commented Nov 16, 2021

Codecov Report

Merging #1205 (b324c43) into ign-gazebo5 (adadda7) will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##           ign-gazebo5    #1205      +/-   ##
===============================================
- Coverage        65.99%   65.99%   -0.01%     
===============================================
  Files              255      255              
  Lines            20137    20137              
===============================================
- Hits             13290    13289       -1     
- Misses            6847     6848       +1     
Impacted Files Coverage Δ
src/Server.cc 84.33% <0.00%> (-0.61%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update adadda7...b324c43. Read the comment docs.

@Blast545
Copy link
Contributor Author

Blast545 commented Nov 16, 2021

  • WIndows Build Status

Cleaned up the workspace, and retrying.
This make take some failure attempts before getting a good one x.x

  • WIndows Build Status

@Blast545
Copy link
Contributor Author

@ros-pull-request-builder retest this please

@chapulina
Copy link
Contributor

@ros-pull-request-builder retest this please

Whaaat did that work? Are @osrf-jenkins and @ros-pull-request-builder doing exactly the same thing? 😱

@Blast545
Copy link
Contributor Author

@chapulina I actually tried and wasn't expecting it to work, I was surprised in a good way.

@Blast545
Copy link
Contributor Author

There seems to be an error with the PR jobs. For some reason, at least for this PR, after I add commits to retrigger the PR logic it tries to test against gazeboMain instead of gazebo5. However, the "retest please" works fine.

@Blast545
Copy link
Contributor Author

@ros-pull-request-builder retest this please

@Blast545
Copy link
Contributor Author

The PR job is not showing anymore failures for Windows, I would say this is ready for merge:
https://build.osrfoundation.org/job/ign_gazebo-pr-win/3420/

It has 100+ warnings though. I think we can iterate on those after #990 gets merged.

Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Thanks for all the effort, @Blast545 !

@chapulina chapulina merged commit a06198f into ign-gazebo5 Nov 19, 2021
@chapulina chapulina deleted the blast545/skip_windows_tests branch November 19, 2021 06:09
@chapulina chapulina mentioned this pull request Jan 12, 2022
7 tasks
@osrf-triage
Copy link

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-01-24-citadel-edifice-fortress/1241/1

@osrf-triage
Copy link

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-04-13-fortress-edifice/1367/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏢 edifice Ignition Edifice tests Broken or missing tests / testing infra Windows Windows support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants