GitHub Actions: npm test is failing Windows tests on M1 Macs

[cclauss]

Why are Windows tests running on Macs?

Fixes #2993

npm test is running and failing Windows find-visualstudio tests on an M1 Mac!!!

These tests are either not being run or are not failing on Intel Macs.

GitHub Actions blog:

Over the next 12 weeks, jobs using the macos-latest runner label will migrate from macOS 12 (Monterey) to macOS 14 (Sonoma).

https://github.blog/changelog/2024-04-01-macos-14-sonoma-is-generally-available-and-the-latest-macos-runner-image

Discovered by @GeoffreyPlitt in:

• node-gyp fails on Sonoma with CLI only and not XCode (Mac M1 / Apple Silicon) #2992
• Adding macos-14 github action #2993

Checklist

• npm install && npm run lint && npm test passes
• tests are included
• documentation is changed or added
• commit message follows commit guidelines

Description of change

@jarig

[GeoffreyPlitt]

Nice!

[StefanStojanovic]

@cclauss a quick question - Is the goal of this PR to add the failing configuration, or to address the issue on M1 Mac as well?

[cclauss]

The goal of this pull request is to encourage someone to fix Windows-specific npm tests to only run on Windows or to pass on all operating systems. To me, running Windows tests only on Windows makes sense.

This should be done before GitHub Actions migrates all macOS jobs off of Intel and onto ARM.

[StefanStojanovic]

Understood. I made a branch from this one. If skipping VS tests outside Windows is wanted, this commit should be enough.

Feel free to cherry-pick it, or apply an inline patch file

From ddbf1beb7a97645146e621982c09a002f62d90ff Mon Sep 17 00:00:00 2001
From: StefanStojanovic <[email protected]>
Date: Mon, 8 Apr 2024 11:42:20 +0200
Subject: [PATCH 1/1] test: run find visual studio tests only on windows

---
 test/test-find-visualstudio.js | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/test/test-find-visualstudio.js b/test/test-find-visualstudio.js
index 2c3f4e1..80cd564 100644
--- a/test/test-find-visualstudio.js
+++ b/test/test-find-visualstudio.js
@@ -21,6 +21,11 @@ class TestVisualStudioFinder extends VisualStudioFinder {
   }
 }
 
+// Only run "Find Visual Studio" tests on Windows.
+if  (process.platform !== 'win32') {
+  return
+}
+
 describe('find-visualstudio', function () {
   this.beforeAll(function () {
     // Condition to skip the test suite
-- 
2.44.0.windows.1

[StefanStojanovic]

From d09beefea668bcf98dee60973d0d4aa9d2596d62 Mon Sep 17 00:00:00 2001
From: StefanStojanovic <[email protected]>
Date: Mon, 8 Apr 2024 12:22:53 +0200
Subject: [PATCH 1/1] test: run find visual studio tests only on windows

---
 test/test-find-visualstudio.js | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/test/test-find-visualstudio.js b/test/test-find-visualstudio.js
index 80cd564..bea0bd9 100644
--- a/test/test-find-visualstudio.js
+++ b/test/test-find-visualstudio.js
@@ -22,11 +22,13 @@ class TestVisualStudioFinder extends VisualStudioFinder {
 }
 
 // Only run "Find Visual Studio" tests on Windows.
-if  (process.platform !== 'win32') {
-  return
-}
+const shouldSkip = process.platform !== 'win32'
 
 describe('find-visualstudio', function () {
+  if (shouldSkip) {
+    return
+  }
+
   this.beforeAll(function () {
     // Condition to skip the test suite
     if (process.env.SystemRoot === undefined) {
-- 
2.44.0.windows.1

This should satisfy the linter.

[StefanStojanovic]

LGTM

[richardlau]

FWIW, without commenting on whether skipping the test on macOS is the correct thing to do (I trust @StefanStojanovic's judgement here) -- the test failure that was occurring looks to be a mismatch between the test and code:

node-gyp/test/test-find-visualstudio.js

Lines 479 to 481 in 77737e4

	await verifyVSSetupData('VSSetup_VS_2019_Professional_workload.txt', 'Professional', 2019,
	'10.0.19041.0', 'v142', '16.11.34407.143', 'Program Files (x86)')
	})

loads test/fixtures/VSSetup_VS_2019_Professional_workload.txt, which contains:

node-gyp/test/fixtures/VSSetup_VS_2019_Professional_workload.txt

Line 91 in 77737e4

"Id": "Microsoft.VisualStudio.VC.MSBuild.Base",

In lib/find-visualstudio.js, this matches this conditional:

node-gyp/lib/find-visualstudio.js

Lines 357 to 368 in 77737e4

	const pkg = 'Microsoft.VisualStudio.VC.MSBuild.Base'
	const msbuildPath = path.join(info.path, 'MSBuild', 'Current', 'Bin', 'MSBuild.exe')
	const msbuildPathArm64 = path.join(info.path, 'MSBuild', 'Current', 'Bin', 'arm64', 'MSBuild.exe')
	if (info.packages.indexOf(pkg) !== -1) {
	this.log.silly('- found VC.MSBuild.Base')
	if (versionYear === 2017) {
	return path.join(info.path, 'MSBuild', '15.0', 'Bin', 'MSBuild.exe')
	}
	if (versionYear === 2019) {
	return msbuildPath
	}
	}

However the test seems to be adding arm64 to the expected MSBuild path:

node-gyp/test/test-find-visualstudio.js

Lines 499 to 503 in 77737e4

	const msBuildPath = process.arch === 'arm64'
	? `C:\\${expectedProgramFilesPath}\\Microsoft Visual Studio\\${vsYear}\\` +
	`${vsType}\\MSBuild\\Current\\Bin\\arm64\\MSBuild.exe`
	: `C:\\${expectedProgramFilesPath}\\Microsoft Visual Studio\\${vsYear}\\` +
	`${vsType}\\MSBuild\\Current\\Bin\\MSBuild.exe`

which looks like it's meant to match:

node-gyp/lib/find-visualstudio.js

Lines 369 to 378 in 77737e4

	/**
	* Visual Studio 2022 doesn't have the MSBuild package.
	* Support for compiling _on_ ARM64 was added in MSVC 14.32.31326,
	* so let's leverage it if the user has an ARM64 device.
	*/
	if (process.arch === 'arm64' && this.msBuildPathExists(msbuildPathArm64)) {
	return msbuildPathArm64
	} else if (this.msBuildPathExists(msbuildPath)) {
	return msbuildPath
	}

but we don't hit that code in the failing test because it returns earlier from the function due to the presences of Microsoft.VisualStudio.VC.MSBuild.Base.

I suspect the test will fail on Windows on ARM64 (which I don't think is being tested in GitHub Actions).

[StefanStojanovic]

I'm not 100% sure, but I also think this is more likely to fail on ARM64 Windows than not. @richardlau thanks for the analysis, we (@huseyinacacak-janea or me) can fix it in a follow-up PR to handle the ARM64 case correctly. Although it is not tested in GH actions, we have access to ARM64 Windows machines where we could develop the change. Once we have it, we can revert b113c39 in order to start testing ARM64 VS detection correctly. Disabling tests now seems like a better approach, as I do not know if we'll get to fixing this before the GitHub Actions migration for macOS.

[cclauss]

A huge hats off to @GeoffreyPlitt for

• node-gyp fails on Sonoma with CLI only and not XCode (Mac M1 / Apple Silicon) #2992
• Adding macos-14 github action #2993
  which elucidated both the problem and solution. It would be fun to see more contributions from Geoffrey in the future!