search

atom / github

:octocat: Git and GitHub integration for Atom
https://github.atom.io
MIT License
1.12k stars 394 forks source link

CI: Use 'windows-2019' image for Windows tests (makes CI passing) #2758

Closed DeeDeeG closed 2 years ago

DeeDeeG commented 2 years ago

Description of the Change

In CI, use the 'windows-2019' image instead of 'windows-latest'.

Reason for change

This gives us an older version of Visual Studio compatible with apm's old copy of node-gyp, so CI can proceed past installing dependencies/not get stuck, on the Windows jobs.

(context: apm is still built around npm 6 --> node-gyp 5.x. Visual Studio 2022 support was only added in node-gyp 8.4.0. So we need older Visual Studio to accommodate apm's old copy of node-gyp. the 'windows-2019' image has the older Visual Studio we're looking for.)

Applicable Issues

None filed, but CI isn't passing.

More details about the issue

(I assume the Windows jobs started failing when the 'windows-latest' CI image tag was updated to point to/be an alias for the 'windows-2022' image.)

CI's Windows jobs get stuck on the "install dependencies" step, because they need to build native C/C++ code in some of the packages, but apm's old copy of node-gyp doesn't understand how to find find Visual Studio 2022 or newer, so the dependency installation steps fail. Normally the test step has a "fails allowed" setting, but this was failing even before the tests ran, causing the overall windows jobs to report failing, and thus all PRs are going to report as failing, making it much harder to tell at a glance which PRs actually pass CI on macOS and Linux. A reminder that the Windows tests have been known failing and allowed to fail without making CI red for a long time now. Failing before the tests ran is a new thing.

codecov[bot] commented 2 years ago

Codecov Report

Merging #2758 (f867d24) into master (be2baf3) will increase coverage by 0.00%. The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2758   +/-   ##
=======================================
  Coverage   93.46%   93.46%           
=======================================
  Files         237      237           
  Lines       13213    13213           
  Branches     1900     1900           
=======================================
+ Hits        12349    12350    +1     
+ Misses        864      863    -1
Impacted Files Coverage Δ
lib/atom/gutter.js 92.85% <0.00%> (+2.38%) :arrow_up:

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 be2baf3...f867d24. Read the comment docs.

DeeDeeG commented 2 years ago

I accidentally hit enter before I was done editing the PR body and picking a good title.

So that's been updated now.

Basically this lets the Windows CI jobs successfully install dependencies, which gets them to the "run tests" step which is already allowed to fail. So Windows CI should show as passing/green again, and overall CI should show passing/green now.

smashwilson commented 2 years ago

😍 Excellent!

[..] which gets them to the "run tests" step which is already allowed to fail.

Yeah, it looks like we still have some not-obvious-to-fix failures lurking in there. This is a great step forward, though, thank you 🙇🏻