ci(test): run the test job on windows too

[Pixnop]

Issue #267 pointed out that the test job in ci.yml only ever runs on ubuntu, even though the suite has Windows-specific coverage: pathsHandlersWin32, the atomic-write crash tests whose rename semantics differ on Windows, and the backgroundHandlers symlink cases that skip themselves on win32. Any of those paths could regress on Windows and CI would still show green.

This change extends the test job to the same os matrix the build job already uses (ubuntu-latest and windows-latest), rather than spinning up a separate windows-only job that runs a subset of files. The build job proves the matrix pattern already works for this repo, and running the full suite is simpler to maintain than picking out which files count as "win32-relevant" and keeping that list in sync as tests get added. The diff is four lines.

The sonarcloud job is untouched. It already runs its own independent test:coverage step on ubuntu-latest only and feeds that into the SonarCloud scan, so this change doesn't add a second source of coverage data into Sonar. Windows just gets its own test run with no reporting side effect.

For the time cost, the build job's windows-latest run has been taking around one and a half to two minutes recently. Since jobs in a workflow run in parallel, this shouldn't add wall-clock time to a typical CI run, but it does add roughly that much in billed Actions minutes, and Windows runners bill at a 2x multiplier over Linux.

Correction to the commit message on 37e86de

That commit names EXECUTE_GAME as the user-visible failure. It is the wrong spawn, and the message stays as it is because the review cites the commit by hash and the green CI run hangs off it, so rewriting it costs more than it fixes. The record belongs here instead.

On EXECUTE_GAME the player saw no difference. MainMenu.tsx already wraps the invoke in try/catch and shows notifications.body.errorExecutingGame, and playOutcomeNotifications.ts maps launch-failed to that same key, so a rejection and a clean started: false produced the identical toast. What the fix buys there is the log lines and the _playing bookkeeping, not a new message.

The genuinely silent failure was the other spawn. useLookForAVersion.ts:34 calls lookForAGameVersion with no try/catch anywhere in detectFolder, so a rejected invoke skipped setFolder, setVersionFound and addNotification alike. On Windows, pointing the "add an existing installation" dialog at a folder whose Vintagestory.exe a stopped download had truncated did nothing at all: no version, no error, no reason.

[Pixnop]

The seventeen are dealt with and the Windows job is green: 137 files, 1609 passing, 28 skipped, coverage at 91.94 statements, 89.07 branches, 91.41 functions and 93.47 lines, every floor clear. Linux is unchanged at 1635 passing and 2 skipped.

Sixteen of the seventeen were the tests being wrong about Windows. One was not, and it was hiding behind twelve of them.

The bug

Every EXECUTE_GAME fixture in gameHandlers.test.ts writes a game binary called Vintagestory, and on Windows buildGameLaunchPlan only ever looks for Vintagestory.exe. So the version folder held no game the launcher could see, every test got no-executable back, and nothing past that point ran at all. That is the whole story for twelve failures, the two that looked like they diverged on session adoption with mismatched uids included: with no launch there is no session write, so there was never anything to adopt, and the file the assertions read back still held whatever the fixture had put there. Naming the binary per platform in one place fixed the diagnosis, and it also let the Windows job get far enough to spawn the thing, which is where the real problem was waiting.

child_process.spawn reports ENOENT, EACCES, EAGAIN, EMFILE and ENFILE through an error event. Everything else it throws where it stands. Windows answers a file that is not a valid executable with UNKNOWN, which is none of those five, and both spawns in gameHandlers.ts were written for the event alone. The throw went straight past the promise and out through the handler, so EXECUTE_GAME rejected instead of resolving launch-failed, which is precisely the anti-pattern gameProcessOutcomeToResult was written to end, and LOOK_FOR_A_GAME_VERSION rejected instead of reporting no version found.

A player reaches this with a game version whose executable a stopped download truncated, or that an antivirus emptied in place. On Linux that is EACCES, the event fires, and they get the ordinary "couldn't run it" notice. On Windows they got the generic error the renderer shows for any exception, and none of the log lines the failure path writes. Both spawns now catch the throw and settle it the way the error event does. Two tests pin it, and they drive the throw through a spawn wrapper rather than through a real Windows failure, so they hold the contract on Linux too rather than only where the bug shows.

The verdicts

Test	Cause	Fix, or reason for the skip	Proof
extraction: unpacks a wrapped archive straight into the target folder	Asked the filesystem whether vintagestory exists to prove the wrapping folder was flattened away. On a case-insensitive filesystem that question is about the Vintagestory file sitting next to it	Dropped that line. The full listing on the line above already says it, and says it better, since an extra folder could not hide from it either	Broke the flattening on Linux with the line gone, test still fails
extraction: coalesces 7-Zip progress and emits one terminal 100	Two 7-Zip processes over a 2000 file archive run past the five second default on a Windows runner	Skipped on win32. Nothing in the coalescing is platform-specific, so the ubuntu run covers it, and #274 replaces this test with a yauzl one that spawns nothing	The Windows job
EXECUTE_GAME: launch-failed when the executable is a symlink	Fixture binary name	Named per platform	Reproduced and re-broken on Linux, then the Windows job
EXECUTE_GAME: launch-failed when a real, executable-bit file fails to actually start, with no account	Fixture binary name, then the spawn throw	Named per platform, and the spawn now catches	Same, plus the pinning test
EXECUTE_GAME: launch-failed after successfully writing the account session first	Same	Same	Same
EXECUTE_GAME: session-write-failed when the account session cannot be written into clientsettings.json	Fixture binary name, and chmod 0o500 on the installation folder does not stop a write on NTFS, which gates on the file's own read-only attribute	Skipped on win32. There is no way to make that write fail through mode bits there	The Windows job
EXECUTE_GAME: clears another player's session before launching with no session of our own	Fixture binary name, then the spawn throw	Named per platform, and the spawn now catches	Reproduced on Linux, then the Windows job
EXECUTE_GAME: leaves a settings file with no foreign session alone when we have none of our own	Same	Same	Same
EXECUTE_GAME: session-write-failed when a foreign session cannot be cleared	Same mode bits problem as the write test above, on the same folder	Skipped on win32	The Windows job
EXECUTE_GAME: adopts the session the game refreshed instead of overwriting it	Fixture binary name, then the spawn throw. Adoption itself is identical on both platforms	Named per platform, and the spawn now catches	Reproduced on Linux, then the Windows job
EXECUTE_GAME: says an adoption happened without ever putting the session in a log line	Same	Same	Same
EXECUTE_GAME: signs in as the active account, not the first one saved	Same. The ENOENT on clientsettings.json was the file never being written, not a path the handler failed to find	Same	Same
EXECUTE_GAME: overwrites another player's refreshed session instead of adopting it into the active account	Same. The uid mismatch was the fixture's own content read back untouched, not a uid compare behaving differently	Same	Same
EXECUTE_GAME: adopts the refreshed session under the active account's uid, not the first saved account's	Same	Same	Same
CHANGE_PERMS: throws on an empty paths array	The handler returns false on anything that is not Linux before it looks at its arguments, so there is nothing to catch	Skipped on win32. The early return is right: POSIX mode bits are the only thing that handler has to apply	Made the early return fire on Linux, which reproduces all three failures exactly, timeout included
CHANGE_PERMS: throws on more than 128 paths	Same	Same	Same
CHANGE_PERMS: resolves true once the worker finishes	Same early return, so no worker is ever started and the fake worker never arrives	Skipped on win32	Same

On the adoption pair

Since the brief singled those two out, the reading is worth stating plainly. Nothing in the adoption path branches on platform. sessionToAdopt compares playeruid with !==, which is right for an identifier the auth service issues, and there is no case folding to do there. The settings file is found by joining clientsettings.json onto a path that has already been through assertManagedPath, and path policy compares through comparablePath, which already knows about Windows. Both failures were the launch dying two steps earlier. I checked before renaming anything, and the reproduction backs it up: pointing the fixture helper at a name the launcher does not know fails all twelve on Linux with the identical messages, mismatched uids and all.

Odds and ends

One test in gameHandlers.test.ts was passing on Windows for the wrong reason. It makes the version folder unlistable with chmod 0o000 to cover the readdir failure arm, and since NTFS has no such bits the folder lists fine, comes back empty, and yields no-executable down a different path. It skips on Windows now with that written on it, which is why the skip count moved by seven rather than six.

Two rounds of CI. The first took seventeen down to nine, all nine being the spawn throw the fixtures had been hiding. The second was green.

[Zaldaryon]

Requesting changes on one point, and it is not in the code. The production fix is correct, the premise behind it holds, every skip is honest, and the fixture rename is right. Local gates pass here: typecheck, lint:ci at 0 errors and 15 pre-existing warnings, format:check, test:coverage at 92.7 statements / 89.8 branches / 92.12 functions / 94.19 lines, all over the floors; gameHandlers.test.ts is 26 passing. CI on 37e86de is green including the new test (windows-latest).

Blocking: the matrix renames a required status check out of existence

dev branch protection requires these contexts, with enforce_admins: true:

typecheck, lint, test, build (ubuntu-latest), build (windows-latest)

Once test gains strategy.matrix.os, GitHub reports its runs as test (ubuntu-latest) and test (windows-latest). The context test is never produced again. The moment this merges, every PR to dev, this one included, sits at "Expected, waiting for status to be reported: test", and with enforce_admins: true nobody can merge past it. The repo has been here before: build (ubuntu-latest) / build (windows-latest) are in the required list precisely because build was matrixed earlier.

Two ways out, not equivalent:

1. An admin edits the required contexts from test to test (ubuntu-latest) + test (windows-latest). This needs a coordinated window (flip it early and every other open PR blocks on contexts it does not produce; flip it late and this PR cannot merge) and it needs a maintainer, so the PR cannot carry it.
2. Keep a job literally named test. Rename the matrix job test-matrix, add a gate job:

  test:
    needs: [test-matrix]
    if: always()
    runs-on: ubuntu-latest
    steps:
      - run: '[ "${{ needs.test-matrix.result }}" = "success" ]'

if: always() and the explicit result check matter: GitHub counts a skipped required job as satisfied, so a naive gate would pass a red matrix. Option 2 needs no protection change and no window. I would take it.

The production fix (commit 37e86de) is correct

Verified the synchronous-throw premise: spawn routes only ENOENT / EACCES / EAGAIN / EMFILE / ENFILE through the error event and throws everything else from the call itself. Reproduced locally: spawn("/etc/hostname/nope") throws ENOTDIR synchronously. So the try/catch is on the right side of the call. Pre-fix the throw landed inside the new Promise executor and rejected the promise, and the await in the handler turned that into a rejected invoke.

Both catch blocks match the error event path: EXECUTE_GAME's settle({ started: false, error }) is byte-identical to the event handler; the probe's resolve({ ok: false, stdout: "", error }) differs from the event only in stdout, which is provably "" at that point. The probe's resolve rather than settle is necessary, not stylistic: settle closes over const timer, which is declared after the try, so calling it from the catch would hit the TDZ and throw a ReferenceError inside the executor. The comment is accurate.

settle({ started: false }) reaches the renderer as launch-failed and the errorExecutingGame toast, not as a crash, and does not credit playtime. The change also restores the documented port contract in src/domain/ports.ts:310 ("resolves once the process exits, never rejecting: a spawn failure is reported through started: false"), which the bare spawn was violating. spawnInstaller in pathsHandlers.ts:527, the Inno installer spawn, was already wrapped this way, so this brings gameHandlers in line with the existing pattern. No other unguarded spawn with the same problem in the repo.

Worth addressing, not blocking

• The commit message names the wrong spawn as the user-visible bug. On EXECUTE_GAME, MainMenu.tsx:106 already wraps runGame in try/catch and shows the same errorExecutingGame toast that launch-failed maps to, so the player saw the identical message either way. The genuine silent failure is on the other spawn: useLookForAVersion.ts:39 calls lookForAGameVersion with no try/catch anywhere in detectFolder, so pre-fix a truncated or quarantined Vintagestory.exe on Windows rejected the invoke, setFolder and addNotification never ran, and the "add an existing install" dialog silently did nothing. That is the failure worth naming in the PR description.
• spawnThrow.next is not reset between tests. tests/ipc/gameHandlers.test.ts:57: afterEach only calls vi.restoreAllMocks(), which does not touch a vi.hoisted object. Both tests assert consumption so they pass today, but if either fails before reaching the spawn, the flag leaks and the next test's real spawn throws. One line in beforeEach: spawnThrow.next = false.
• settle/resolve asymmetry in the probe is safe now, fragile later. Anyone moving the timer declaration above the try silently reintroduces a double-settle path. Declaring let timer before settle and using settle in both catches removes the asymmetry the comment has to explain.
• The UNKNOWN error code in the comment and the fixture ({ code: "UNKNOWN", errno: -4094 }) may not be what Windows actually produced. ERROR_BAD_EXE_FORMAT maps to ENOEXEC, EFTYPE, or UNKNOWN depending on the libuv version. Behaviour is unaffected (none is in the five-code list, so the throw happens regardless, and production only calls getErrorMessage), but confirm the code from the failing Windows run and correct the comment if needed.
• No .gitattributes. The accountLoginFlow.test.ts CRLF fix is correct, but tests/security-boundaries.test.ts:98 and tests/renderer-dom/moddbVisibilityPrompt.test.tsx:50 read source text the same way and pass only because neither uses a \n-based assertion. A repo-level * text=auto eol=lf would immunise all three.

Verified as sound

Every one of the 13 new skipIf sites is process.platform === "win32" (none inverted). The chmod-readback skips are unwritable on NTFS, not merely failing. The CHANGE_PERMS skips are not hiding a gap: the handler returns false before touching arguments on non-Linux, and TaskManagerContext.tsx:290 already documents and discards that. The Vintagestory to Vintagestory.exe per-platform rename matches gameExecutableCandidates (.exe only on win32, [] on darwin where buildGameLaunchPlan short-circuits anyway). The removed existsSync(..., "vintagestory") === false assertion is subsumed by the full readdirSync listing on the line above. sonarcloud is untouched (own test:coverage on ubuntu, no needs:, not required, no artifact collision). fail-fast: false matches the build job. Windows npm ci is already proven by build (windows-latest). This repo is public, so the Windows runner 2x multiplier costs nothing.

One thing for the maintainer to be aware of: vitest.config.ts floors (89/87/85/85) were calibrated on Linux numbers. This PR makes them a second gate the Windows leg must clear on its own (Pixnop reports 91.94/89.07/91.41/93.47, statements the tightest at +2.07). Any future skipIf(win32) eats that margin and can fail only the Windows leg.

Not checked here

The exact Windows error code, and the Windows leg itself (no Windows host).

[Pixnop]

You are right, and I checked the protection rule myself before touching anything. dev requires exactly these contexts today, with enforce_admins: true:

typecheck, lint, test, build (ubuntu-latest), build (windows-latest)

So the four-line diff was a trap. Once test carries strategy.matrix.os, GitHub reports test (ubuntu-latest) and test (windows-latest), and the bare test context is never produced again. Every open PR against dev would sit forever on a status nothing can report, this one included, with no admin override. The scar tissue is already visible in the required list: build (ubuntu-latest) and build (windows-latest) are in there because build got matrixed at some point and someone had to go edit the rule afterwards.

Took your option 2, since it is the one the PR can carry without a coordinated window. The matrix job is now test-matrix, and a gate job keeps the required name alive:

  test:
    needs: [test-matrix]
    if: always()
    runs-on: ubuntu-latest
    steps:
      - run: |
          echo "test-matrix result: ${{ needs.test-matrix.result }}"
          [ "${{ needs.test-matrix.result }}" = "success" ]

Both halves are load bearing, as you said. Without always() the gate is skipped when a leg fails, and protection treats a skipped required job as satisfied, which is the failure mode where a red matrix merges anyway. And the check has to be against success rather than something looser: for a matrixed dependency, needs.test-matrix.result is success only when every leg succeeded, and it is failure, cancelled or skipped in every other case. The string comparison is false for all three, so the step exits non-zero and the gate goes red. Cancellation and a skipped matrix are covered by the same comparison, not just an errored leg.

About the failing case, I want to be honest about how far I went. I did not actually break a leg to watch the gate turn red, because doing that means pushing a deliberately broken commit onto this branch and then reverting it, which leaves noise in the history you are reviewing. What I have instead is the argument above plus the shape of the expression: [ "$x" = "success" ] under the default bash shell exits 1 for any other value, and needs.<job>.result has no values beyond those four. The echo line is there so a future red gate says in its own log which state it saw, rather than making someone reconstruct it. If you want the empirical version before approving, say so and I will run it once on a throwaway branch.

The proof that matters is on this run:

test                          pass  4s
test-matrix (ubuntu-latest)   pass  1m42s
test-matrix (windows-latest)  pass  2m44s
typecheck                     pass
lint                          pass
build (ubuntu-latest)         pass
build (windows-latest)        pass
sonarcloud                    pass

A context literally named test is reported again and it is green, so all five required contexts still exist. sonarcloud is untouched: no needs:, its own test:coverage run, still ubuntu only.

Local gates on this commit: typecheck clean, lint:ci at 0 errors and the same 15 pre-existing warnings, format:check clean, test:coverage at 92.72 / 89.84 / 92.12 / 94.19, matching your numbers and above the floors.

Leaving your non-blocking points for a separate pass rather than folding them into this one. And if the maintainers would rather have option 1 later, editing the required contexts to the two matrix names, then the gate job becomes dead weight and can be deleted in that same change.

[Pixnop]

Landed your four non-blocking points here rather than in a follow-up, so the same file set gets one review instead of two.

spawnThrow.next. Reset in beforeEach alongside the rest of the per-test state. Nothing was passing for the wrong reason: both tests that raise the flag assert it was consumed, so the leak needs a test to fail before it reaches the spawn, and the damage then lands on whatever runs next. Cheap to close, and it is the same cross-test pollution the renderer harness work went after.

The settle / resolve asymmetry. timer now starts as undefined above settle, so the spawn catch settles exactly the way the error event does and clearTimeout ignores the handle that does not exist yet. The catch also passes stdout rather than a literal "", which is the same value at that point and removes the last difference between the two paths. One thing worth knowing if you touch it: prefer-const flags a bare let timer declared and assigned once, and const is what puts the TDZ back, so the explicit = undefined is load bearing rather than noise.

.gitattributes. * text=auto eol=lf, plus binary on *.zip, *.bin, *.lzma1 and *.lzma2. The fixture markers are not ceremony: I checked every file under tests/fixtures, and not-a-zip.bin contains no NUL byte, so text=auto classifies it as text on its own. It carries no CR today, which is why nothing has broken yet, but regenerate it with one and the byte-exact assertions start disagreeing with the file. Images are marked too. Nothing in the index is CRLF right now, so this renormalises no existing file and the diff is the one new file. I left the .replace(/\r\n/g, "\n") in accountLoginFlow.test.ts alone, since existing clones keep their CRLF working copies until someone refreshes them.

The commit message on 37e86de. Verified your reading before writing anything down: MainMenu.tsx catches the rejection and shows notifications.body.errorExecutingGame, and playOutcomeNotifications.ts maps launch-failed to that same key, so the player saw one message either way. useLookForAVersion.ts:34 really is bare, and a rejected invoke there skips setFolder, setVersionFound and addNotification together, which is the dialog doing nothing at all. I did not amend the commit. You cite it by hash in your review and the green run hangs off it, so rewriting it would cost more than the wrong sentence does. The correction is in the PR description instead.

Checks on this push, with the required context still produced:

test                          pass  4s
test-matrix (ubuntu-latest)   pass  1m30s
test-matrix (windows-latest)  pass  2m19s
typecheck  pass | lint  pass | build (ubuntu-latest)  pass | build (windows-latest)  pass
sonarcloud pass

Locally: typecheck clean, lint:ci at 0 errors and the same 15 warnings, format:check clean, test:coverage at 92.79 / 89.87 / 92.21 / 94.28, and gameHandlers.test.ts still 26 passing.

[Zaldaryon]

Approved.

The follow-up commits close the blocking CI issue and the Windows test failures. The matrix now runs test-matrix on Ubuntu and Windows, while the required test gate runs with always() and fails unless needs.test-matrix.result is success. The dev protection rule requires typecheck, lint, test, build (ubuntu-latest), and build (windows-latest), and this run reports all five required contexts successfully.

The production fix in src/ipc/handlers/gameHandlers.ts catches synchronous spawn failures in both EXECUTE_GAME and LOOK_FOR_A_GAME_VERSION, preserving their existing failure contracts. The updated fixture names, Windows-specific skips, CRLF normalization, and spawn-throw tests are scoped to the platform behavior they cover. I found no remaining correctness or compatibility blocker.

Local verification after npm ci: npm run typecheck, npm run lint:ci, npm run format:check, npm run test:coverage, and npm run build:unpack all passed. Coverage was 92.77% statements, 89.84% branches, 92.21% functions, and 94.28% lines, with 1,635 tests passed and 2 skipped.