Skip to content

Commit 90452df

Browse files
EhabYEhabY
authored
fix: Windows compatibility for tests, scripts, and Vitest Explorer (#876)
Add cross-env for environment variables in npm scripts, resolve the Electron binary path platform-agnostically, replace .cmd-dependent execFile calls with process.execPath, normalize path assertions with path.join(), and compile integration tests as part of the build step.
1 parent c2aa5ca commit 90452df

8 files changed

Lines changed: 205 additions & 51 deletions

File tree

‎.vscode/settings.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,5 +14,5 @@
1414
"vitest.nodeEnv": {
1515
"ELECTRON_RUN_AS_NODE": "1"
1616
},
17-
"vitest.nodeExecutable": "node_modules/.bin/electron"
17+
"vitest.nodeExecutable": "node_modules/electron/dist/electron"
1818
}

‎package.json‎

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -18,19 +18,20 @@
1818
"type": "commonjs",
1919
"main": "./dist/extension.js",
2020
"scripts": {
21-
"build": "concurrently -g -n webviews,extension \"pnpm build:webviews\" \"node esbuild.mjs\"",
22-
"build:production": "NODE_ENV=production pnpm build",
21+
"build": "concurrently -g -n webviews,extension,compile-tests:integration \"pnpm build:webviews\" \"node esbuild.mjs\" \"pnpm compile-tests:integration\"",
22+
"build:production": "cross-env NODE_ENV=production pnpm build",
2323
"build:webviews": "pnpm -r --filter \"./packages/*\" --parallel build",
24+
"compile-tests:integration": "tsc -p test/integration --outDir out --noCheck",
2425
"format": "prettier --write --cache --cache-strategy content .",
2526
"format:check": "prettier --check --cache --cache-strategy content .",
2627
"lint": "eslint --cache --cache-strategy content .",
2728
"lint:fix": "pnpm lint --fix",
2829
"package": "pnpm build:production && vsce package --no-dependencies",
2930
"package:prerelease": "pnpm build:production && vsce package --pre-release --no-dependencies",
30-
"test": "CI=true ELECTRON_RUN_AS_NODE=1 electron node_modules/vitest/vitest.mjs",
31-
"test:extension": "ELECTRON_RUN_AS_NODE=1 electron node_modules/vitest/vitest.mjs --project extension",
32-
"test:integration": "tsc -p test/integration --outDir out --noCheck && node esbuild.mjs && vscode-test",
33-
"test:webview": "ELECTRON_RUN_AS_NODE=1 electron node_modules/vitest/vitest.mjs --project webview",
31+
"test": "cross-env CI=true ELECTRON_RUN_AS_NODE=1 electron node_modules/vitest/vitest.mjs",
32+
"test:extension": "cross-env ELECTRON_RUN_AS_NODE=1 electron node_modules/vitest/vitest.mjs --project extension",
33+
"test:integration": "pnpm compile-tests:integration && node esbuild.mjs && vscode-test",
34+
"test:webview": "cross-env ELECTRON_RUN_AS_NODE=1 electron node_modules/vitest/vitest.mjs --project webview",
3435
"typecheck": "concurrently -g -n extension,tests,packages \"tsc --noEmit\" \"tsc --noEmit -p test\" \"pnpm typecheck:packages\"",
3536
"typecheck:packages": "pnpm -r --filter \"./packages/*\" --parallel typecheck",
3637
"watch": "concurrently -g -n extension,webviews \"pnpm watch:extension\" \"pnpm watch:webviews\"",
@@ -631,8 +632,9 @@
631632
"bufferutil": "^4.1.0",
632633
"coder": "catalog:",
633634
"concurrently": "^9.2.1",
635+
"cross-env": "^10.1.0",
634636
"dayjs": "^1.11.20",
635-
"electron": "39.8.1",
637+
"electron": "39.8.5",
636638
"esbuild": "^0.28.0",
637639
"eslint": "^10.2.0",
638640
"eslint-config-prettier": "^10.1.8",

‎pnpm-lock.yaml‎

Lines changed: 27 additions & 10 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎test/unit/cliConfig.test.ts‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ import {
1414
} from "@/settings/cli";
1515

1616
import { MockConfigurationProvider } from "../mocks/testHelpers";
17-
import { isWindows } from "../utils/platform";
17+
import { quoteCommand } from "../utils/platform";
1818

1919
vi.mock("node:os");
2020

@@ -320,9 +320,3 @@ describe("cliConfig", () => {
320320
});
321321
});
322322
});
323-
324-
function quoteCommand(value: string): string {
325-
// Used to escape environment variables in commands. See `getHeaderArgs` in src/headers.ts
326-
const quote = isWindows() ? '"' : "'";
327-
return `${quote}${value}${quote}`;
328-
}

‎test/unit/core/cliExec.test.ts‎

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,22 @@
11
import fs from "fs/promises";
22
import os from "os";
33
import path from "path";
4-
import { beforeAll, describe, expect, it } from "vitest";
5-
6-
import * as cliExec from "@/core/cliExec";
4+
import { beforeAll, describe, expect, it, vi } from "vitest";
75

86
import { MockConfigurationProvider } from "../../mocks/testHelpers";
9-
import { isWindows, writeExecutable } from "../../utils/platform";
7+
import { isWindows, quoteCommand, writeExecutable } from "../../utils/platform";
108

119
import type { CliEnv } from "@/core/cliExec";
1210

11+
// Shim execFile so .js test scripts are run through node cross-platform.
12+
vi.mock("node:child_process", async (importOriginal) => {
13+
const { shimExecFile } = await import("../../utils/platform");
14+
return shimExecFile(await importOriginal());
15+
});
16+
17+
// Import after mock so the module picks up the shimmed execFile.
18+
const cliExec = await import("@/core/cliExec");
19+
1320
describe("cliExec", () => {
1421
const tmp = path.join(os.tmpdir(), "vscode-coder-tests-cliExec");
1522
let echoArgsBin: string;
@@ -140,7 +147,7 @@ describe("cliExec", () => {
140147
"--url",
141148
"http://localhost:3000",
142149
"--header-command",
143-
"'my-header-cmd'",
150+
quoteCommand("my-header-cmd"),
144151
"speedtest",
145152
"owner/workspace",
146153
"--output",
@@ -185,7 +192,7 @@ describe("cliExec", () => {
185192
"--url",
186193
"http://localhost:3000",
187194
"--header-command",
188-
"'my-header-cmd'",
195+
quoteCommand("my-header-cmd"),
189196
"support",
190197
"bundle",
191198
"owner/workspace",

‎test/unit/remote/sshProcess.test.ts‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import find from "find-process";
22
import { vol } from "memfs";
33
import * as fsPromises from "node:fs/promises";
4+
import path from "node:path";
45
import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
56

67
import {
@@ -313,8 +314,10 @@ describe("SshProcessMonitor", () => {
313314
});
314315
const logPath = await waitForEvent(monitor.onLogFilePathChange);
315316

316-
expect(logPath).toBe("/proxy-logs/999.log");
317-
expect(monitor.getLogFilePath()).toBe("/proxy-logs/999.log");
317+
expect(logPath).toBe(path.join("/proxy-logs", "999.log"));
318+
expect(monitor.getLogFilePath()).toBe(
319+
path.join("/proxy-logs", "999.log"),
320+
);
318321
});
319322

320323
it("finds log file with prefix pattern", async () => {
@@ -330,7 +333,7 @@ describe("SshProcessMonitor", () => {
330333
});
331334
const logPath = await waitForEvent(monitor.onLogFilePathChange);
332335

333-
expect(logPath).toBe("/proxy-logs/coder-ssh-999.log");
336+
expect(logPath).toBe(path.join("/proxy-logs", "coder-ssh-999.log"));
334337
});
335338

336339
it("returns undefined when no proxyLogDir set", async () => {
@@ -373,7 +376,7 @@ describe("SshProcessMonitor", () => {
373376
});
374377
const logPath = await waitForEvent(monitor.onLogFilePathChange);
375378

376-
expect(logPath).toBe("/proxy-logs/2024-01-03-999.log");
379+
expect(logPath).toBe(path.join("/proxy-logs", "2024-01-03-999.log"));
377380
});
378381

379382
it("sorts log files using localeCompare for consistent cross-platform ordering", async () => {
@@ -395,7 +398,7 @@ describe("SshProcessMonitor", () => {
395398

396399
// With localeCompare: ["a", "Z"] -> reversed -> "Z" first
397400
// With plain sort(): ["Z", "a"] -> reversed -> "a" first (WRONG)
398-
expect(logPath).toBe("/proxy-logs/Z-999.log");
401+
expect(logPath).toBe(path.join("/proxy-logs", "Z-999.log"));
399402
});
400403
});
401404

‎test/utils/platform.test.ts‎

Lines changed: 85 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,18 @@
1-
import { describe, expect, it } from "vitest";
1+
import * as cp from "node:child_process";
2+
import fs from "node:fs/promises";
3+
import os from "node:os";
4+
import path from "node:path";
5+
import { promisify } from "node:util";
6+
import { beforeAll, describe, expect, it } from "vitest";
27

38
import {
49
expectPathsEqual,
510
exitCommand,
11+
isWindows,
612
printCommand,
713
printEnvCommand,
8-
isWindows,
14+
shimExecFile,
15+
writeExecutable,
916
} from "./platform";
1017

1118
describe("platform utils", () => {
@@ -83,4 +90,80 @@ describe("platform utils", () => {
8390
},
8491
);
8592
});
93+
94+
describe("writeExecutable", () => {
95+
const tmp = path.join(os.tmpdir(), "vscode-coder-tests-platform");
96+
97+
beforeAll(async () => {
98+
await fs.rm(tmp, { recursive: true, force: true });
99+
await fs.mkdir(tmp, { recursive: true });
100+
});
101+
102+
it("writes a .js file and returns its path", async () => {
103+
const result = await writeExecutable(tmp, "test-script", "// hello");
104+
expect(result).toBe(path.join(tmp, "test-script.js"));
105+
expect(await fs.readFile(result, "utf-8")).toBe("// hello");
106+
});
107+
108+
it("overwrites existing files", async () => {
109+
await writeExecutable(tmp, "overwrite", "first");
110+
const result = await writeExecutable(tmp, "overwrite", "second");
111+
expect(await fs.readFile(result, "utf-8")).toBe("second");
112+
});
113+
});
114+
115+
describe("shimExecFile", () => {
116+
const tmp = path.join(os.tmpdir(), "vscode-coder-tests-shim");
117+
const mod = shimExecFile(cp);
118+
const execFileAsync = promisify(mod.execFile);
119+
120+
beforeAll(async () => {
121+
await fs.rm(tmp, { recursive: true, force: true });
122+
await fs.mkdir(tmp, { recursive: true });
123+
});
124+
125+
it("runs .js files through node", async () => {
126+
const script = await writeExecutable(
127+
tmp,
128+
"echo",
129+
'process.stdout.write("ok");',
130+
);
131+
const { stdout } = await execFileAsync(script);
132+
expect(stdout).toBe("ok");
133+
});
134+
135+
it("passes args through to the script", async () => {
136+
const script = await writeExecutable(
137+
tmp,
138+
"echo-args",
139+
"process.stdout.write(process.argv.slice(2).join(','));",
140+
);
141+
const { stdout } = await execFileAsync(script, ["a", "b", "c"]);
142+
expect(stdout).toBe("a,b,c");
143+
});
144+
145+
it("does not rewrite non-.js files", async () => {
146+
await expect(execFileAsync("/nonexistent/binary")).rejects.toThrow(
147+
"ENOENT",
148+
);
149+
});
150+
151+
it("preserves the callback form", async () => {
152+
const script = await writeExecutable(
153+
tmp,
154+
"cb-echo",
155+
'process.stdout.write("cb");',
156+
);
157+
const stdout = await new Promise<string>((resolve, reject) => {
158+
mod.execFile(script, (err, out) =>
159+
err ? reject(new Error(err.message)) : resolve(out),
160+
);
161+
});
162+
expect(stdout).toBe("cb");
163+
});
164+
165+
it("does not touch spawn", () => {
166+
expect(mod.spawn).toBe(cp.spawn);
167+
});
168+
});
86169
});

0 commit comments

Comments
 (0)