Skip to content

Commit 122d731

Browse files
edvilmeCopilot
andcommitted
fix: await setImmediate in setEnvironment so events fire before return
The root cause of flaky integration tests was that setEnvironment fired change events via setImmediate but returned before they fired. This made it impossible for callers to know when events were done. Now setEnvironment awaits the setImmediate callback, guaranteeing that onDidChangeActiveEnvironment has fired when the promise resolves. This eliminates all race conditions and removes the need for test-side workarounds (onceEvent, setEnvironmentAndWait, drain loops). Tests are simplified to just 'await api.setEnvironment(...)' — no helpers, no predictions, no timeouts. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 598b5c9 commit 122d731

2 files changed

Lines changed: 62 additions & 88 deletions

File tree

src/features/envManagers.ts

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -363,9 +363,12 @@ export class PythonEnvironmentManagers implements EnvironmentManagers {
363363
const oldEnv = this._activeSelection.get(key);
364364
if (oldEnv?.envId.id !== environment?.envId.id) {
365365
this._activeSelection.set(key, environment);
366-
setImmediate(() =>
367-
this._onDidChangeActiveEnvironment.fire({ uri: project?.uri, new: environment, old: oldEnv }),
368-
);
366+
await new Promise<void>((resolve) => {
367+
setImmediate(() => {
368+
this._onDidChangeActiveEnvironment.fire({ uri: project?.uri, new: environment, old: oldEnv });
369+
resolve();
370+
});
371+
});
369372
}
370373
}
371374

@@ -443,7 +446,12 @@ export class PythonEnvironmentManagers implements EnvironmentManagers {
443446
if (shouldPersistSettings) {
444447
await setAllManagerSettings(settings);
445448
}
446-
setImmediate(() => events.forEach((e) => this._onDidChangeActiveEnvironment.fire(e)));
449+
await new Promise<void>((resolve) => {
450+
setImmediate(() => {
451+
events.forEach((e) => this._onDidChangeActiveEnvironment.fire(e));
452+
resolve();
453+
});
454+
});
447455
} else {
448456
const promises: Promise<void>[] = [];
449457
const events: DidChangeEnvironmentEventArgs[] = [];
@@ -488,7 +496,12 @@ export class PythonEnvironmentManagers implements EnvironmentManagers {
488496
}
489497
}
490498
await Promise.all(promises);
491-
setImmediate(() => events.forEach((e) => this._onDidChangeActiveEnvironment.fire(e)));
499+
await new Promise<void>((resolve) => {
500+
setImmediate(() => {
501+
events.forEach((e) => this._onDidChangeActiveEnvironment.fire(e));
502+
resolve();
503+
});
504+
});
492505
}
493506
}
494507

src/test/integration/interpreterSelection.integration.test.ts

Lines changed: 44 additions & 83 deletions
Original file line numberDiff line numberDiff line change
@@ -23,57 +23,10 @@
2323

2424
import * as assert from 'assert';
2525
import * as vscode from 'vscode';
26-
import { DidChangeEnvironmentEventArgs, PythonEnvironment, PythonEnvironmentApi, SetEnvironmentScope } from '../../api';
26+
import { DidChangeEnvironmentEventArgs, PythonEnvironment, PythonEnvironmentApi } from '../../api';
2727
import { ENVS_EXTENSION_ID } from '../constants';
2828
import { waitForCondition } from '../testUtils';
2929

30-
/**
31-
* Returns a promise that resolves with the first event fired by the given
32-
* VS Code event emitter. Automatically disposes the subscription after
33-
* the event fires.
34-
*/
35-
function onceEvent<T>(event: vscode.Event<T>): Promise<T> {
36-
return new Promise<T>((resolve) => {
37-
const disposable = event((e) => {
38-
disposable.dispose();
39-
resolve(e);
40-
});
41-
});
42-
}
43-
44-
/**
45-
* Calls setEnvironment and waits for the async event chain to fully settle.
46-
*
47-
* Compares the current environment with the target to determine whether
48-
* a change event is expected. If a change is expected, subscribes to
49-
* onDidChangeEnvironment BEFORE calling setEnvironment and awaits the
50-
* event. If no change is expected (idempotent set), calls setEnvironment
51-
* and returns immediately.
52-
*
53-
* Returns the change event if one was fired, or undefined for idempotent sets.
54-
*/
55-
async function setEnvironmentAndWait(
56-
api: PythonEnvironmentApi,
57-
scope: SetEnvironmentScope,
58-
env: PythonEnvironment | undefined,
59-
): Promise<DidChangeEnvironmentEventArgs | undefined> {
60-
// Determine if this set will actually change the environment.
61-
// For array scopes, check the first URI (the primary project).
62-
const getScope = Array.isArray(scope) ? scope[0] : scope;
63-
const current = await api.getEnvironment(getScope);
64-
const expectsChange = current?.envId.id !== env?.envId.id;
65-
66-
if (!expectsChange) {
67-
await api.setEnvironment(scope, env);
68-
return undefined;
69-
}
70-
71-
// Subscribe before calling setEnvironment so we don't miss the event
72-
const eventPromise = onceEvent(api.onDidChangeEnvironment);
73-
await api.setEnvironment(scope, env);
74-
return eventPromise;
75-
}
76-
7730
suite('Integration: Interpreter Selection Priority', function () {
7831
this.timeout(60_000);
7932

@@ -99,11 +52,9 @@ suite('Integration: Interpreter Selection Priority', function () {
9952
});
10053

10154
// Reset to original state after each test to prevent state pollution.
102-
// Uses setEnvironmentAndWait to ensure all async events drain before
103-
// the next test starts — prevents stale events from leaking.
10455
teardown(async function () {
10556
try {
106-
await setEnvironmentAndWait(api, undefined, originalEnv);
57+
await api.setEnvironment(undefined, originalEnv);
10758
} catch {
10859
// Ignore errors during reset
10960
}
@@ -141,8 +92,8 @@ suite('Integration: Interpreter Selection Priority', function () {
14192

14293
const envToSet = environments[0];
14394

144-
// Set environment and wait for async event chain to settle
145-
await setEnvironmentAndWait(api, undefined, envToSet);
95+
// Set environment globally — await guarantees the event has fired
96+
await api.setEnvironment(undefined, envToSet);
14697

14798
const retrieved = await api.getEnvironment(undefined);
14899

@@ -173,11 +124,11 @@ suite('Integration: Interpreter Selection Priority', function () {
173124
const projectEnv = environments[1];
174125
const project = projects[0];
175126

176-
// Set global environment and wait for event chain to settle
177-
await setEnvironmentAndWait(api, undefined, globalEnv);
127+
// Set global environment
128+
await api.setEnvironment(undefined, globalEnv);
178129

179-
// Set different environment for project and wait
180-
await setEnvironmentAndWait(api, project.uri, projectEnv);
130+
// Set different environment for project
131+
await api.setEnvironment(project.uri, projectEnv);
181132

182133
// Verify global is unchanged
183134
const globalRetrieved = await api.getEnvironment(undefined);
@@ -215,24 +166,34 @@ suite('Integration: Interpreter Selection Priority', function () {
215166
const oldEnv = environments[0];
216167
const newEnv = environments[1];
217168

218-
// Set initial environment and wait for all async events to drain
219-
await setEnvironmentAndWait(api, undefined, oldEnv);
169+
// Set initial environment
170+
await api.setEnvironment(undefined, oldEnv);
220171

221-
// Set new environment — the returned event contains the change payload
222-
const event = await setEnvironmentAndWait(api, undefined, newEnv);
172+
// Subscribe to capture the change event from the next set
173+
let capturedEvent: DidChangeEnvironmentEventArgs | undefined;
174+
const disposable = api.onDidChangeEnvironment((e) => {
175+
capturedEvent = e;
176+
});
223177

224-
assert.ok(event, 'Change event should have fired');
225-
assert.ok(event.new, 'Event should have new environment');
226-
assert.strictEqual(
227-
event.new.envId.id,
228-
newEnv.envId.id,
229-
'Event new envId should match the environment we set',
230-
);
231-
assert.strictEqual(
232-
event.new.environmentPath.fsPath,
233-
newEnv.environmentPath.fsPath,
234-
'New should match set environment',
235-
);
178+
try {
179+
// Change to new environment — await guarantees the event has fired
180+
await api.setEnvironment(undefined, newEnv);
181+
182+
assert.ok(capturedEvent, 'Change event should have fired');
183+
assert.ok(capturedEvent.new, 'Event should have new environment');
184+
assert.strictEqual(
185+
capturedEvent.new.envId.id,
186+
newEnv.envId.id,
187+
'Event new envId should match the environment we set',
188+
);
189+
assert.strictEqual(
190+
capturedEvent.new.environmentPath.fsPath,
191+
newEnv.environmentPath.fsPath,
192+
'New should match set environment',
193+
);
194+
} finally {
195+
disposable.dispose();
196+
}
236197
});
237198

238199
/**
@@ -253,8 +214,8 @@ suite('Integration: Interpreter Selection Priority', function () {
253214
const project = projects[0];
254215
const env = environments[0];
255216

256-
// Set project environment and wait for event chain to settle
257-
await setEnvironmentAndWait(api, project.uri, env);
217+
// Set project environment
218+
await api.setEnvironment(project.uri, env);
258219

259220
// Query for a file inside the project
260221
const fileUri = vscode.Uri.joinPath(project.uri, 'subdir', 'script.py');
@@ -308,11 +269,11 @@ suite('Integration: Interpreter Selection Priority', function () {
308269

309270
const env = environments[0];
310271

311-
// Set environment first time and wait for event chain to settle
312-
await setEnvironmentAndWait(api, undefined, env);
272+
// Set environment first time
273+
await api.setEnvironment(undefined, env);
313274

314-
// Set same environment again and wait for any events to drain
315-
await setEnvironmentAndWait(api, undefined, env);
275+
// Set same environment again
276+
await api.setEnvironment(undefined, env);
316277

317278
// Verify functional idempotency: after setting the same environment
318279
// twice, getEnvironment should still return the same environment.
@@ -343,13 +304,13 @@ suite('Integration: Interpreter Selection Priority', function () {
343304
return;
344305
}
345306

346-
// Set an explicit environment and wait for event chain to settle
347-
await setEnvironmentAndWait(api, undefined, environments[0]);
307+
// Set an explicit environment
308+
await api.setEnvironment(undefined, environments[0]);
348309
const beforeClear = await api.getEnvironment(undefined);
349310
assert.ok(beforeClear, 'Should have environment before clearing');
350311

351-
// Clear the selection and wait for event chain to settle
352-
await setEnvironmentAndWait(api, undefined, undefined);
312+
// Clear the selection
313+
await api.setEnvironment(undefined, undefined);
353314

354315
// Get environment - should return auto-discovered environment
355316
const autoEnv = await api.getEnvironment(undefined);

0 commit comments

Comments
 (0)