test(cli): make the test suite win32-clean - #1412
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 19 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (10)
WalkthroughThe pull request expands Windows compatibility across CI and tests. It enforces LF checkout behavior, runs the test suite in the Windows smoke job, and replaces hard-coded path expectations with platform-aware construction or normalization. Runtime environment tests gain shared temporary-directory cleanup and symlink-target assertions. The in-memory filesystem now normalizes Windows path forms, while flow, archive, environment, runner, and export-resolution tests update their fixtures and expectations accordingly. Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
readdir and readdirWithTypes held the same prefix-scanning loop, and seven methods repeated the parent-exists check. Behavior is unchanged.
Around 30 test files asserted POSIX separators, file modes, or exact readlink output, so `bun run test` could not join the windows-smoke CI job. Fix the assertions; production behavior was already correct. makeMemoryFs now keys every entry in one POSIX form, which fixes the whole class at the double instead of in each of its 36 consumers. expectLinkTarget canonicalizes the target a win32 junction reports, keeping the exact-target comparison rather than resolving through further links. extract.test.ts builds its symlink fixture as a raw tar entry so the symlink-escape guard still runs where win32 refuses to create a symlink without Developer Mode.
The windows-latest runner checks out with core.autocrlf=true. Two tests compare committed text to a value carrying "\n": skill.test.ts reads the SKILL.md frontmatter, which genSkillMd preserves verbatim because it splices only the commands table, and help.test.ts compares committed snapshots to help text generated with "\n". A CRLF working tree fails both. No committed file contains CRLF today, so this renormalizes nothing.
The teardown deleted tracked temp dirs concurrently, so a junction's target could go before the dir holding the junction. win32 then failed with EFAULT on the dangling junction. Delete newest first, one at a time, because a later dir is the one that links into an earlier one.
The first Windows run of the suite exposed 55 failures across 12 files. The ticket listed neither these files nor two of the causes, and my earlier claim that the makeMemoryFs fix covered them transitively was wrong. loadConfig, resolveExport, android, installAndroid, handler and stage asserted POSIX separators against node:path output, and resolve() adds a drive letter on win32. expand compares glob output, which stays forward-slashed on win32 while join does not, so both sides normalize. loadFlowDefault compares a bundled import literal, where the bundler doubles win32 backslashes. npmInstall now expects the cmd.exe wrapper that resolveNpmCommand produces on win32. stage asserts parsed dotenv values because serializeDotenv escapes backslashes by design and dotenv.test.ts already pins that format. Drops the junction fixture from symlinkDir.testUtils.test.ts: it built a target string by hand, passed on darwin, and made readlink throw EINVAL on win32. The two remaining cases exercise the real platform. src/commands/resolveDepsRoot.test.ts stays untouched. Its walk is host-coupled, and WIZ-11281 threads a platform argument through it.
Five test files each kept their own copy of the tracker's makeTmpDir and its concurrent teardown. I fixed the delete order in three of them and missed prepareRunDir.stageCollision.test.ts, which then failed on Windows with the same EFAULT. The copies are identical to makeTmpDirTracker, so they now call it and there is one implementation left to fix.
ffe9dd4 to
fe12fa9
Compare
resolveDepsRoot walks up the flow path with node:path, so the POSIX fixture path "/user/project" comes back drive-relative as "\user\project" on win32. WIZ-11281's platform argument selects the bin shim and does not change path spelling, so it left this assertion failing.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/domains/flows/ensureDeps.test.ts`:
- Line 9: Move makeTmpDirTracker out of the runtimeEnv domain into an allowed
shared layer such as shell, or define a flows-local test helper, then update
ensureDeps.test.ts and any other references to use the new location. Ensure
flows tests no longer import from sibling domain runtimeEnv while preserving the
helper’s behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7c6daac3-a7e4-4bfa-b281-599951f15b8a
📒 Files selected for processing (41)
.gitattributes.github/workflows/ci.ymlsrc/commands/resolveDepsRoot.test.tssrc/domains/config/loadConfig.test.tssrc/domains/doctor/checks/android.test.tssrc/domains/doctor/checks/fileAssets.test.tssrc/domains/flows/ensureDeps.test.tssrc/domains/flows/expand.test.tssrc/domains/flows/list.agent.test.tssrc/domains/flows/list.json.test.tssrc/domains/flows/list.test.tssrc/domains/flows/pull/bundle.test.tssrc/domains/flows/pull/envVars.test.tssrc/domains/flows/pull/extract.test.tssrc/domains/flows/pull/handler.test.tssrc/domains/flows/pull/stage.test.tssrc/domains/install/android/installAndroid.test.tssrc/domains/runner/createLaunch.contexts.test.tssrc/domains/runner/loadFlowDefault.test.tssrc/domains/runner/runWebFlow.har.test.tssrc/domains/runner/runWebFlow.trace.test.tssrc/domains/runner/runnerDeps.test.tssrc/domains/runner/web/contextSetup.test.tssrc/domains/runner/web/contextSetup.trace.test.tssrc/domains/runtimeEnv/execSubpathImports.test.tssrc/domains/runtimeEnv/innerHop.test.tssrc/domains/runtimeEnv/npmInstall.test.tssrc/domains/runtimeEnv/outerHop.install.test.tssrc/domains/runtimeEnv/outerHop.test.tssrc/domains/runtimeEnv/prepareRunDir.outerHop.test.tssrc/domains/runtimeEnv/prepareRunDir.stageCollision.test.tssrc/domains/runtimeEnv/prepareRunDir.test.tssrc/domains/runtimeEnv/runDirFixtures.testUtils.tssrc/domains/runtimeEnv/symlinkDir.testUtils.test.tssrc/domains/runtimeEnv/symlinkDir.testUtils.tssrc/shell/appium/createAndroidLaunchContext.test.tssrc/shell/fs.testUtils.test.tssrc/shell/fs.testUtils.tssrc/shell/manifest/lookup.test.tssrc/shell/reporter/createJUnitReporter.test.tssrc/shell/resolveExport.test.ts
Routing every teardown through the tracker made src/domains/flows/ensureDeps.test.ts import it from src/domains/runtimeEnv, and a domain must not import a sibling domain. The tracker only wraps mkdtemp and rm, so it belongs in shell. No override in .oxlintrc.json covers runtimeEnv, so lint did not catch it.
2 participants
Relates to WIZ-11279. Follow-up audit filed as WIZ-11287.
Overview of Changes
The
windows-smokejob deliberately skippedbun run testbecause the suite failed on win32. This makes the suite pass there and adds the step. Only tests, test utilities and CI changed. No production code, and every failure traced to a test artifact rather than a product bug.The ticket scoped ~30 files across four causes. The first real Windows run showed 55 failures across 12 files, including causes nobody had listed, so the fix is wider than the ticket: separator and drive-letter assertions against
node:pathoutput,makeMemoryFskeying (one fix covering all 36 of its consumers), junctionreadlinktargets, POSIX file modes, glob output that stays forward-slashed on win32, a bundler that doubles backslashes in an emitted import literal,cmd.exeargument wrapping, and dotenv backslash escaping. Two changes keep coverage a skip would have dropped:extract.test.tsbuilds its symlink fixture as a raw tar entry so the symlink-escape guard still runs where win32 needs Developer Mode, and the.env0600check is win32-skipped rather than deleted..gitattributespins an LF working tree because the runner checks out withcore.autocrlf=true, which broke theSKILL.mdfrontmatter and help-snapshot comparisons; no committed file contains CRLF, so it renormalizes nothing.Two fixes are worth a reviewer's attention because production was right and the test was wrong.
stage.test.tsnow asserts parsed dotenv values, sinceserializeDotenvescapes backslashes by design anddotenv.test.tsalready pins that byte format. Temp-dir teardown now runs newest-first, one at a time: it deleted concurrently, so a junction's target could go before the dir holding it and win32 failed withEFAULTon the dangling junction. Five files each kept a private copy of that teardown, which is how I fixed three and missed a fourth that then failed on the next run, so they all route throughmakeTmpDirTrackernow.Testing
bun run testruns in thewindows-smokejob as of this PR, so Windows is covered by CI rather than by argument. That job is green here, along with the checks below on Linux. Each of the 7 commits is independently green, so the branch bisects.bun run typecheck bun run lint bun run format:check bun run knip bun run test bun run build1230 tests pass locally on darwin, 0 fail.
Not fixed here, deliberately: on win32
expandPatternsreturns forward-slash paths from tinyglobby while the rest of the CLI joins with backslashes. Nothing is known to compare the two, so the tests normalize both sides and production is untouched. WIZ-11287 tracks the audit and says to verify before changing anything.Checklist