Skip to content

fix(scripts): make the test script runnable on Windows - #3040

Open
KallinikosMil wants to merge 1 commit into
infinitered:masterfrom
KallinikosMil:fix/test-script-windows
Open

fix(scripts): make the test script runnable on Windows#3040
KallinikosMil wants to merge 1 commit into
infinitered:masterfrom
KallinikosMil:fix/test-script-windows

Conversation

@KallinikosMil

Copy link
Copy Markdown

Description

pnpm test cannot run at all on Windows. The test script prefixes jest with a POSIX inline environment variable assignment:

"test": "TS_JEST_DISABLE_VER_CHECKER=true jest"

cmd.exe has no equivalent syntax, so the shell tries to execute the assignment as a command and the run dies before jest starts:

> ignite-cli@11.5.0 test C:\...\ignite
> TS_JEST_DISABLE_VER_CHECKER=true jest

'TS_JEST_DISABLE_VER_CHECKER' is not recognized as an internal or external command,
operable program or batch file.
 ELIFECYCLE  Test failed.

That's a fresh git clone && pnpm install && pnpm test on Windows 11 — a contributor on Windows can't run the suite before opening a PR.

Why removing the variable rather than wrapping it

TS_JEST_DISABLE_VER_CHECKER opted out of a TypeScript version-mismatch warning that ts-jest has since dropped. In the ts-jest this repo resolves (29.4.6, from ts-jest: ^29.1.1), the name appears only in CHANGELOG.mdgrep -ril ver_checker node_modules/…/ts-jest/dist/ returns nothing. Running jest without it emits no warning of any kind:

$ npx jest src/tools          # no env var set
Test Suites: 3 passed, 3 total
Tests:       19 passed, 19 total

So the prefix costs a hard failure on Windows and buys nothing.

There's also a consistency argument: watch, watch:debug and coverage already invoke jest bare. test was the only one carrying the prefix.

If you'd rather keep the variable — say, to stay covered should the checker ever return — the cross-platform form is cross-env:

"test": "cross-env TS_JEST_DISABLE_VER_CHECKER=true jest"

That needs a new devDependency, which is why I didn't reach for it first. Happy to switch if you prefer it.

  • Issues:

Checklist

  • README.md and other relevant documentation has been updated with my changes — no documentation references this script's env var.
  • I have manually tested this, including by generating a new app locally. — I ran the unit suite (src/tools, 19 tests, 3 suites) before and after on Windows 11 / Node 22.23.2 / pnpm 10.9.0. I did not run the test/vanilla e2e specs, which generate real apps and need a full RN toolchain; this change doesn't touch generation, only how jest is invoked. CI (CircleCI, cimg/node:20.19.4) runs Linux only, so the whole suite there is unaffected either way — jest picks up the same config with or without the variable.

Verification

Before (on master)

$ pnpm test
'TS_JEST_DISABLE_VER_CHECKER' is not recognized as an internal or external command
ELIFECYCLE Test failed.        # exit 1, zero tests run

After

$ pnpm test src/tools
Test Suites: 3 passed, 3 total
Tests:       19 passed, 19 total          # exit 0

On Linux/macOS the behaviour is unchanged: the variable was already a no-op there, so jest receives exactly the same configuration.

The `test` script prefixed jest with a POSIX inline environment variable
assignment. cmd.exe has no such syntax, so on Windows `pnpm test` fails before
jest starts:

    'TS_JEST_DISABLE_VER_CHECKER' is not recognized as an internal or external
    command, operable program or batch file.

The variable is also no longer doing anything. `TS_JEST_DISABLE_VER_CHECKER`
opted out of a version-mismatch warning that ts-jest has since removed; in the
resolved ts-jest (29.4.6) the name appears only in its CHANGELOG, not in any
runtime code. Running jest without it produces no warning.

`watch`, `watch:debug` and `coverage` already invoke jest bare, so dropping
the prefix also makes `test` consistent with them.
@KallinikosMil

Copy link
Copy Markdown
Author

The CircleCI failure here is pre-existing and unrelated to this change — flagging it so the red X doesn't stall the review.

What actually fails: two cases in test/vanilla/ignite-new.test.ts, both on the same assertion — the generated app's compile script emits a TypeScript error, and the test asserts the output doesn't contain "error":

app/navigators/navigationUtilities.ts:71:44 - error TS2345:
Argument of type 'Readonly<{...}> | undefined' is not assignable to parameter of
type 'Readonly<{...}> | PartialState<...>'.
  Type 'undefined' is not assignable to ...

  71       const routeName = getActiveRouteName(navigationRef.getRootState())

getRootState() is typed as possibly undefined in @react-navigation/native v7, while getActiveRouteName(state: NavigationState | PartialState<NavigationState>) doesn't accept it. Nothing to do with how jest is invoked.

Evidence it isn't mine: builds 5117 and 5116, on the unrelated feat/expo-sdk-56 branch, fail the same file with the same Validation Error in the --packager=yarn case. My branch additionally trips the --packager=bun case, which is the same TS error surfacing in a second packager variant rather than a new failure.

Also worth noting the run reached jest at all — the log opens with > ignite-cli@11.5.0 test / > jest, and the seven other suites pass. On Linux the old prefix was harmless; this PR only changes what happens on Windows, where the command couldn't start.

Happy to open a separate PR for the navigationUtilities.ts type error if that'd be useful — it looks like a one-liner, but the right behaviour when the root state is genuinely undefined is your call, so I'd rather ask than guess.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant