test: make path assertions platform-agnostic so the suite passes on Windows - #1450
Open
KallinikosMil wants to merge 1 commit into
Open
test: make path assertions platform-agnostic so the suite passes on Windows#1450KallinikosMil wants to merge 1 commit into
KallinikosMil wants to merge 1 commit into
Conversation
…indows Five assertions in two suites compared platform-native paths against hard-coded POSIX literals, so they failed on Windows even though the code under test is correct -- it builds those paths with `path.join` / `path.resolve`, which use backslashes there. Expected: "path/to/react-native/scripts/compose-source-maps.js" Received: "path\to\react-native\scripts\compose-source-maps.js" Run the expectations through `path.join` / `path.resolve` instead of comparing raw strings. Both are no-ops on POSIX, so Linux and macOS runs are unchanged. In resolveProjectPath.test.ts the root is resolved as well. Without a drive letter, up-level navigation that reaches the root collapses to a bare backslash, which Windows then reads as the beginning of a UNC share -- the test produced \\shared\utils.js rather than a local path.
|
@KallinikosMil is attempting to deploy a commit to the Callstack Team on Vercel. A member of the Team first needs to authorize it. |
|
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
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.
Summary
Five tests across two suites fail on Windows because they compare platform-native paths against hard-coded POSIX literals, so
pnpm testis red even though the code under test is correct:getHermesCLIPathandresolveProjectPathbuild those paths withpath.join/path.resolve, which is exactly what they should do — the separators are right for the platform. It's the expectations that are POSIX-only.The fix runs the expectations through
path.join/path.resolverather than comparing raw strings. Both are no-ops on POSIX, so Linux and macOS runs are byte-for-byte unchanged; the literals stay readable as POSIX in the source.One case needed a bit more
In
resolveProjectPath.test.tsthe root is resolved as well, not just the expectation. With a POSIX root and no drive letter, up-level navigation that reaches the root collapses to a bare\, and Windows then reads\+/shared/...as the start of a UNC share:Resolving the root first gives it a drive letter, so
path.joinkeeps the drive when it walks up and the result stays local. To be clear about scope: I don't think this is reachable in production, becauserootDirthere comes from a real project directory and always carries a drive on Windows. It's a property of feeding the function a POSIX absolute path on Windows, so I've fixed it in the test and leftresolveProjectPathalone. Flagging it because the failure mode is a surprising one and worth knowing about.Test plan
Windows 11, Node 22.23.2, pnpm 10.34.5.
Before —
@callstack/repack:After:
pnpm lint(biome) is clean.metro-compat-test(47 passed),resolver-cases-test(3 files) andrepack-dev-server(5 tests) all pass on Windows both before and after.Not addressed here:
tests/integrationfails on Windows withModule build failed— 4 files, 50 tests, 24 snapshots. I verified this is pre-existing and unrelated by stashing my changes and re-running on a clean tree, which produced the identical50 failed | 8 passed. It looks like a separate Windows issue and I didn't want to bundle a fix for it into this PR. Happy to look into it separately if that'd be useful.I noticed
test-main-matrix.ymlalready has anosmatrix axis, currently[ubuntu-latest]. If you'd like these kept honest, addingwindows-latestthere would do it — glad to open that as its own PR rather than assume you want the extra CI minutes.