fix(ci): normalize dev launcher path assertions on Windows#4915
Merged
Conversation
The two Windows-targeted dev.js launcher tests added in #4728 mock existsSync with forward-slash suffix matching and assert spawn args via forward-slash stringContaining. On a real windows-latest runner dev.js builds these paths with path.join, which yields backslashes, so the existsSync mock never matches, dev.js takes the bare tsx.cmd shell fallback, and both tests fail. On macOS/Linux the platform() mock plus real forward-slash joins keep them green, which is why main CI has been red only on the Windows job since 423cac1. Normalize separators in both the existsSync mocks and the received spawn arguments before asserting, so the tests pass on every host OS. Same content as the fix bundled into #4840's merge commit 0e104e1, extracted into a standalone test-only change so main recovers without waiting on a core-behavior PR; both merge cleanly in either order. Co-authored-by: qqqys <qys177@gmail.com>
Collaborator
|
Thanks for the PR! Template looks good ✓ On direction: This is a straightforward CI fix — main's Windows job has been red since #4728 because the two Windows-targeted tests in On approach: Scope is tight and correct — one file, +18/-16, test-only. The Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 方向：这是一个直接的 CI 修复——自 #4728 以来 main 的 Windows 任务一直失败，因为 方案：范围紧凑合理——仅修改一个文件，+18/-16，纯测试改动。 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
wenshao
commented
Jun 10, 2026
wenshao
left a comment
Collaborator
Author
There was a problem hiding this comment.
No review findings. Downgraded from Approve to Comment: self-PR; CI still running. — DeepSeek/deepseek-v4-pro via Qwen Code /review
Collaborator
Code ReviewThe diff is clean and minimal. One helper ( No bugs, no security concerns, no regressions. The test-only scope means One minor observation: the first test now uses Test ResultsThis is a test-only fix for Windows CI. On Linux, Before (main branch)(On Linux both pass — the bug only manifests on real Windows runners where After (this PR)CI status: 4 successful, 0 failing, 5 pending (Test jobs still running on ubuntu/macos/windows). 中文说明代码审查diff 干净且极简。一个辅助函数（ 无 bug、无安全隐患、无回归。纯测试改动意味着 一个小观察：第一个测试现在对 command 断言使用 测试结果这是一个针对 Windows CI 的纯测试修复。在 Linux 上， — Qwen Code · qwen3.7-max |
Collaborator
ReflectionThis is one of those PRs where the fix is so targeted there's almost nothing to debate. The root cause is clear: The restructuring from monolithic On Linux, both versions pass because No concerns. Shipping this unblocks main's CI and stops the red-ness cascade onto other PRs. 中文说明反思这是那种修复目标非常明确的 PR，几乎没什么可争论的。根因很清楚： 将整体的 在 Linux 上两个版本都通过，因为 没有顾虑。合入此 PR 可以恢复 main 的 CI 并阻止红色蔓延到其他 PR。 — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
approved these changes
Jun 10, 2026
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
tanzhenxin
approved these changes
Jun 10, 2026
tanzhenxin
left a comment
Collaborator
There was a problem hiding this comment.
Test-only normalization fix, verified: this PR's Windows job is green while main is red on the same two assertions. Thanks for extracting it from #4840 so CI recovers independently.
This was referenced Jun 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Makes the two Windows-targeted
scripts/dev.jslauncher tests path-separator agnostic, so they pass on realwindows-latestrunners and main's Windows CI recovers.Why it's needed
Main's
Test (windows-latest, Node 22.x)job has been red on every push since423cac110(#4728) — last green main commit9e4c87a7e, withfb1432cf5,9533aface,1190ef316all failing on the same two tests (e.g. run 27235517277). This also turns the Windows check red on every open PR that includes current main (seen on #4519).Root cause: the tests (added in #4728) mock
existsSyncwith forward-slash suffix matching (endsWith('node_modules/tsx/dist/cli.mjs')) and assert spawn args with forward-slashstringContaining('packages/cli/index.ts'). They mockplatform()towin32, butdev.jsbuilds paths with the realpath.join— backslashes on a real Windows host. TheexistsSyncmock therefore never matches,dev.jsfalls into the baretsx.cmdshell fallback, and both assertions fail. On macOS/Linux the real joins use/, so the tests only fail on actual Windows — the platform they were written to cover.Fix: normalize separators (
normalizePath) in both theexistsSyncmocks and the received spawn arguments before asserting.Note: this is the same content as the fix bundled into #4840's merge commit
0e104e179(credit @qqqys, co-authored). Extracted into a standalone test-only PR so main's CI recovers without coupling to a core-behavior PR's merge timing — the file contents are identical, so #4840 and this PR merge cleanly in either order.Reviewer Test Plan
How to verify
Expected: 2/2 pass on macOS/Linux, and — the actual point — the
Test (windows-latest, Node 22.x)check on this PR goes green, while it is red on current main.Evidence (Before & After)
Mechanism-level A/B, reproduced on Linux by mocking
node:pathwithpath.win32sodev.jsbuilds backslash paths exactly as on a real Windows runner:existsSyncmock misses → spawn called with bare'tsx.cmd'+shell: trueinstead ofnode.exe+cli.mjs+shell: false; received args are backslash paths that forward-slashstringContainingrejects).Real-runner proof is this PR's own Windows CI job (main is red on the same job at the merge-base).
Tested on
Risk & Scope
scripts/dev.jsbehavior is untouched.Linked Issues
Unbreaks Windows CI broken by #4728; unblocks the Windows check on open PRs (e.g. #4519).