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

Restore Windows CI runs #6024

Merged
merged 1 commit into from
Dec 21, 2021
Merged

Restore Windows CI runs #6024

merged 1 commit into from
Dec 21, 2021

Conversation

basil
Copy link
Member

@basil basil commented Dec 7, 2021

Downstream of #6026, #6029, #6030, #6034, #6035, and jenkinsci/jenkins-test-harness#357. With all those changes and with the topmost commit of this PR, the tests pass on Windows in CI.

@basil basil added the work-in-progress The PR is under active development, not ready to the final review label Dec 7, 2021
@MarkEWaite
Copy link
Contributor

Likely will hit test timeouts because the tests are detectably slower on Windows than on Linux

@basil basil force-pushed the windows branch 7 times, most recently from 9dec7ba to dcd3abc Compare December 7, 2021 05:57
@basil
Copy link
Member Author

basil commented Dec 7, 2021

Well with the changes from #6026 I got the smoke tests passing locally, but I can't seem to get past the unit tests in CI. The unit tests seem to hang at the end before the build proceeds to building the WAR and running the integration tests. Since I can't reproduce this locally any help would be appreciated.

@basil basil force-pushed the windows branch 4 times, most recently from c2d8109 to f04c931 Compare December 7, 2021 08:08
@basil
Copy link
Member Author

basil commented Dec 7, 2021

Well with the changes from #6026 I got the smoke tests passing locally, but I can't seem to get past the unit tests in CI. The unit tests seem to hang at the end before the build proceeds to building the WAR and running the integration tests. Since I can't reproduce this locally any help would be appreciated.

Interestingly enough the hang on CI was in LoadStatisticsTest#graph, which passes for me locally on Windows. Not sure why it fails on CI, but for now I can get the CI build to move forward by skipping that test.

@basil basil force-pushed the windows branch 11 times, most recently from 02e1af8 to 0ea06ba Compare December 8, 2021 19:41
@basil basil changed the title Do the tests still work on Windows? Restore Windows CI runs Dec 8, 2021
@basil basil added skip-changelog Should not be shown in the changelog and removed work-in-progress The PR is under active development, not ready to the final review labels Dec 8, 2021
@timja
Copy link
Member

timja commented Dec 8, 2021

1hr 20 slower than linux 😢

@basil
Copy link
Member Author

basil commented Dec 9, 2021

Caught a bug in #6025 before it ever shipped, so worth it I suppose

@basil basil force-pushed the windows branch 3 times, most recently from 3e9e9f3 to 241fece Compare December 12, 2021 23:39
@jglick
Copy link
Member

jglick commented Dec 13, 2021

#4447 (comment) FTR

node(buildType == 'Linux' ? (jdk == 8 ? 'maven' : 'maven-11') : buildType.toLowerCase()) {
String agentContainerLabel = jdk == 8 ? 'maven' : 'maven-11'
if (buildType == 'Windows') {
agentContainerLabel += '-windows'
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need four combinations? To save resources, and sometimes wall clock time, I think it would suffice to run, say, Linux builds on Java 11 and Windows builds on Java 8.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nah, I was planning to reduce this to two runs before marking this change as ready for review. But in the meantime I've been running the draft PR with 4 builds to shake out any flakiness.

BTW #6029

Copy link
Member

Choose a reason for hiding this comment

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

before marking this change as ready for review

#6024 (comment) ?

Copy link
Member Author

Choose a reason for hiding this comment

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

My bad. I marked this as draft.

@basil basil marked this pull request as draft December 13, 2021 15:26
@basil basil force-pushed the windows branch 11 times, most recently from e86f0f2 to a0c748a Compare December 20, 2021 16:19
@basil basil marked this pull request as ready for review December 20, 2021 17:12
@basil
Copy link
Member Author

basil commented Dec 20, 2021

After shaking out flakiness for the past few days (weeks?) and merging several other preparatory PRs, I think this PR is finally ready to land. Yesterday's run passed on Java 8/11 on both Linux/Windows.

Do we really need four combinations? To save resources, and sometimes wall clock time, I think it would suffice to run, say, Linux builds on Java 11 and Windows builds on Java 8.

For now I am retaining the Linux Java 8 and 11 builds but adding a Windows Java 11 build. This tests the recommended use case (Java 11) on both Linux and Windows while also testing the compatibility use case (Java 8) just on Linux. Linux builds are fast, so running two of them has no effect on wall clock time. The idea would be to drop the Linux Java 8 build once we drop support for Java 8 (hopefully sooner rather than later) and replace it with a Java 17 build once that infrastructure exists.

Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

🎉

@basil
Copy link
Member Author

basil commented Dec 20, 2021

This PR is now ready for merge. We will merge it after approximately 24 hours if there is no negative feedback. Please see the merge process documentation for more information about the merge process. Thanks!

@basil basil added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Dec 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback skip-changelog Should not be shown in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants