fix(os): return 'arm64' from os.machine() on Windows ARM64 (fix #62232)#62235
fix(os): return 'arm64' from os.machine() on Windows ARM64 (fix #62232)#62235zakiscoding wants to merge 2 commits intonodejs:mainfrom
Conversation
Fixes: nodejs#60884 Refs: nodejs#52655 When running tests with both the dot reporter and coverage reports, if a coverage report fails (e.g., line coverage threshold not met), there was no visible output indicating the failure. The process would exit with a failure code, but only dots would be printed with no explanation. This commit adds collection and display of coverage threshold failure diagnostics in the dot reporter. When coverage threshold checks fail, error diagnostic messages are now displayed at the end of the test output, similar to how failed tests are displayed. The coverage error messages are collected from test:diagnostic events with level='error' that are emitted by the test runner when coverage thresholds are not met.
|
Review requested:
|
nodejs-github-bot
added
c++
zakiscoding
force-pushed
the
fix/issue-62232-os-machine-arm64
branch
from
0f647e4 to
5e8dfc3
Compare
There was a problem hiding this comment.
Pull request overview
Fixes os.machine() reporting on Windows ARM64 by overriding libuv’s "unknown" machine value, and additionally updates the dot test reporter to surface diagnostic errors.
Changes:
- Add Windows-specific ARM64 detection in
src/node_os.ccto return"arm64"when libuv reports"unknown". - Update the dot reporter to collect and print
test:diagnosticevents withlevel: 'error'at the end of the run.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
src/node_os.cc |
Detect Windows ARM64 via GetSystemInfo() when uv_os_uname() reports machine === "unknown". |
lib/internal/test_runner/reporter/dot.js |
Collect error-level diagnostics and print them after the main dot output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
|
|
||
| // Map Windows processor architecture to machine designations | ||
| // PROCESSOR_ARCHITECTURE_ARM64 = 12 | ||
| if (sys_info.wProcessorArchitecture == 12) { |
There was a problem hiding this comment.
Avoid using the raw numeric value 12 for the Windows processor architecture. Prefer PROCESSOR_ARCHITECTURE_ARM64 when available, and (if necessary for older SDKs) add a small fallback #ifndef PROCESSOR_ARCHITECTURE_ARM64 definition so the intent is explicit and the code is easier to maintain.
| // Map Windows processor architecture to machine designations | |
| // PROCESSOR_ARCHITECTURE_ARM64 = 12 | |
| if (sys_info.wProcessorArchitecture == 12) { | |
| // Map Windows processor architecture to machine designations. | |
| // Prefer the SDK constant when available; fall back to the known value | |
| // for older SDKs that may not define PROCESSOR_ARCHITECTURE_ARM64. | |
| #ifndef PROCESSOR_ARCHITECTURE_ARM64 | |
| # define PROCESSOR_ARCHITECTURE_ARM64 12 | |
| #endif | |
| if (sys_info.wProcessorArchitecture == PROCESSOR_ARCHITECTURE_ARM64) { |
| if (strcmp(info.machine, "unknown") == 0) { | ||
| SYSTEM_INFO sys_info; | ||
| GetSystemInfo(&sys_info); | ||
|
|
There was a problem hiding this comment.
There is trailing whitespace on this blank line; please remove it to keep diffs clean and avoid lint/style checks failing on whitespace-only changes.
| // Map Windows processor architecture to machine designations | ||
| // PROCESSOR_ARCHITECTURE_ARM64 = 12 | ||
| if (sys_info.wProcessorArchitecture == 12) { | ||
| snprintf(info.machine, sizeof(info.machine), "arm64"); |
There was a problem hiding this comment.
This change fixes os.machine() for Windows ARM64, but there is no regression test guarding against returning "unknown" on that platform. Consider adding a conditional test that asserts os.machine() === 'arm64' when running on Windows ARM64 (e.g., common.isWindows && process.arch === 'arm64').
| const failedTests = []; | ||
| const coverageErrors = []; | ||
| for await (const { type, data } of source) { |
There was a problem hiding this comment.
The PR description is about fixing os.machine() on Windows ARM64, but this change modifies the test runner dot reporter output. If this is intentional, the PR description (or PR scope) should be updated; otherwise this looks like an unrelated change that should be moved to a separate PR.
| if (type === 'test:diagnostic' && data.level === 'error') { | ||
| // Collect coverage errors (coverage threshold failures) | ||
| ArrayPrototypePush(coverageErrors, data.message); | ||
| } |
There was a problem hiding this comment.
coverageErrors collects all test:diagnostic events with data.level === 'error', but the output label hard-codes "Coverage errors". Either filter specifically for coverage-threshold diagnostics (e.g., by message pattern), or rename the variable/heading to something accurate like "Diagnostic errors".
| yield `\n${colors.red}Coverage errors:${colors.white}\n\n`; | ||
| for (const error of coverageErrors) { | ||
| yield `${colors.red}${error}${colors.white}\n`; | ||
| } | ||
| } |
There was a problem hiding this comment.
This introduces new observable output for the dot reporter (printing diagnostic errors at the end), but there is no test/snapshot coverage for the new behavior. Please add a test that runs --test-reporter=dot with failing coverage thresholds and asserts the emitted diagnostics are included in the output.
| // This test will pass on all Windows systems. The fix ensures that on ARM64 systems, | ||
| // os.machine() no longer returns 'unknown', but instead returns 'arm64' | ||
| assert.ok(machine.length > 0); | ||
| } |
There was a problem hiding this comment.
I don't think this test matches the intent in the comments
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #62235 +/- ##
=======================================
Coverage 89.66% 89.66%
=======================================
Files 676 676
Lines 206462 206473 +11
Branches 39533 39537 +4
=======================================
+ Hits 185128 185139 +11
- Misses 13461 13462 +1
+ Partials 7873 7872 -1
🚀 New features to boost your workflow:
|
|
Closing this as superseded by #61829 per maintainer guidance. Thanks all. |
Fixes #62232
On Windows ARM64, \os.machine()\ was returning 'unknown' instead of 'arm64'.
The issue is that libuv's \uv_os_uname()\ doesn't properly detect ARM64 on Windows. This fix adds Windows-specific code to use the Windows API (\GetSystemInfo) to detect ARM64 processor architecture when \os.machine()\ would otherwise return 'unknown'.
The fix maps Windows processor architecture code 12 (PROCESSOR_ARCHITECTURE_ARM64) to the 'arm64' machine string, matching the behavior on other platforms like macOS where ARM64 Macs correctly return 'arm64' from \os.machine().