Skip to content

Commit 03701e1

Browse files
committed
test(e2e): address the Copilot review on the merged e2e PR
Three findings from the Copilot review of #984: - server-env.test.ts assumed JLAB_DESKTOP_CONFIG_DIR was unset, so a developer or CI with it set would fail the userData-dir assertion. Save and clear it in beforeEach, restore in afterEach, so both cases are hermetic. - start() polls the server URL via http/https.request and reschedules a 500ms setTimeout retry; with no real server that left background timers and connection attempts running after the assertions. Mock http/https so the poll reports the server up immediately and nothing stays scheduled, and swallow the now-resolving start() promise. - the e2e install step only confirmed require('electron') resolved a path; add an explicit test -x on the launcher (and keep the macOS framework test -f) so a dropped or non-executable binary fails the step with a clear error. Note: AI-assisted (Claude Code). Manually verified: the two server-env tests pass in ~0.3s with no hang, full unit suite 432 pass (eslint/tsc deferred to CI: this branch is off master's TS 4.2 / ESLint 8 and the local node_modules is on the Electron-42 branch's toolchain).
1 parent df4fce4 commit 03701e1

2 files changed

Lines changed: 48 additions & 26 deletions

File tree

.github/workflows/e2e.yml

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,9 +57,11 @@ jobs:
5757
ditto -x -k "$zip" "$dist"
5858
printf 'Electron.app/Contents/MacOS/Electron' > node_modules/electron/path.txt
5959
test -f "$dist/Electron.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Electron Framework"
60+
test -x "$dist/Electron.app/Contents/MacOS/Electron"
6061
else
6162
unzip -q -o "$zip" -d "$dist"
6263
printf 'electron' > node_modules/electron/path.txt
64+
test -x "$dist/electron"
6365
fi
6466
node -e "console.log('resolved electron:', require('electron'))"
6567
- run: yarn build

test/unit/server-env.test.ts

Lines changed: 46 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,23 @@ vi.mock('../../src/main/pythonenvdialog/pythonenvdialog', () => ({
9595
ManagePythonEnvironmentDialog: { Tab: { Settings: 'settings' } }
9696
}));
9797

98+
// start() polls the server URL via http/https.request until it is up. Without a
99+
// real server those requests fail and reschedule a setTimeout retry, leaving
100+
// background timers and connection attempts running after the assertions. Report
101+
// the URL as up immediately so the poll resolves and nothing is left scheduled.
102+
vi.mock('http', () => ({
103+
request: (_url: unknown, cb: (res: { statusCode: number }) => void) => {
104+
queueMicrotask(() => cb({ statusCode: 200 }));
105+
return { on: vi.fn(), end: vi.fn() };
106+
}
107+
}));
108+
vi.mock('https', () => ({
109+
request: (_url: unknown, cb: (res: { statusCode: number }) => void) => {
110+
queueMicrotask(() => cb({ statusCode: 200 }));
111+
return { on: vi.fn(), end: vi.fn() };
112+
}
113+
}));
114+
98115
import { JupyterServer } from '../../src/main/server';
99116
import { IEnvironmentType } from '../../src/main/tokens';
100117

@@ -116,13 +133,25 @@ async function flush() {
116133
}
117134
}
118135

136+
// Saved/cleared so the userData-dir assertion is hermetic: a developer or CI
137+
// with JLAB_DESKTOP_CONFIG_DIR set in the ambient env would otherwise make
138+
// start() legitimately prefer it and fail the first test.
139+
let originalConfigDir: string | undefined;
140+
119141
beforeEach(() => {
120142
vi.clearAllMocks();
121143
mockExecFile.mockReturnValue(makeChild());
144+
originalConfigDir = process.env.JLAB_DESKTOP_CONFIG_DIR;
145+
delete process.env.JLAB_DESKTOP_CONFIG_DIR;
122146
});
123147

124148
afterEach(() => {
125149
vi.restoreAllMocks();
150+
if (originalConfigDir === undefined) {
151+
delete process.env.JLAB_DESKTOP_CONFIG_DIR;
152+
} else {
153+
process.env.JLAB_DESKTOP_CONFIG_DIR = originalConfigDir;
154+
}
126155
});
127156

128157
describe('JupyterServer.start env isolation', () => {
@@ -137,7 +166,7 @@ describe('JupyterServer.start env isolation', () => {
137166
}
138167
});
139168

140-
server.start();
169+
void server.start().catch(() => undefined);
141170
await flush();
142171

143172
expect(mockExecFile).toHaveBeenCalledTimes(1);
@@ -146,32 +175,23 @@ describe('JupyterServer.start env isolation', () => {
146175
});
147176

148177
it('prefers the JLAB_DESKTOP_CONFIG_DIR override over the userData dir', async () => {
149-
const original = process.env.JLAB_DESKTOP_CONFIG_DIR;
150178
process.env.JLAB_DESKTOP_CONFIG_DIR = '/override/config';
151-
try {
152-
const server = new JupyterServer({
153-
environment: {
154-
path: '/envs/e2e/bin/python',
155-
name: 'e2e',
156-
type: IEnvironmentType.VirtualEnv,
157-
versions: {},
158-
defaultKernel: 'python3'
159-
}
160-
});
161-
162-
server.start();
163-
await flush();
164-
165-
const options = mockExecFile.mock.calls[0][1] as {
166-
env: NodeJS.ProcessEnv;
167-
};
168-
expect(options.env.JUPYTER_CONFIG_DIR).toBe('/override/config');
169-
} finally {
170-
if (original === undefined) {
171-
delete process.env.JLAB_DESKTOP_CONFIG_DIR;
172-
} else {
173-
process.env.JLAB_DESKTOP_CONFIG_DIR = original;
179+
const server = new JupyterServer({
180+
environment: {
181+
path: '/envs/e2e/bin/python',
182+
name: 'e2e',
183+
type: IEnvironmentType.VirtualEnv,
184+
versions: {},
185+
defaultKernel: 'python3'
174186
}
175-
}
187+
});
188+
189+
void server.start().catch(() => undefined);
190+
await flush();
191+
192+
const options = mockExecFile.mock.calls[0][1] as {
193+
env: NodeJS.ProcessEnv;
194+
};
195+
expect(options.env.JUPYTER_CONFIG_DIR).toBe('/override/config');
176196
});
177197
});

0 commit comments

Comments
 (0)