Skip to content

Commit 0ab87b1

Browse files
eleanorjboydeleanorjboyd
authored
Implement tests for manager timeout handling (#1297)
tests to follow along: #1295
1 parent 8b9890a commit 0ab87b1

File tree

2 files changed

+151
-41
lines changed

2 files changed

+151
-41
lines changed

‎src/features/common/managerReady.ts‎

Lines changed: 45 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import { installExtension } from '../../common/workbenchCommands';
1010
import { EnvironmentManagers, PythonProjectManager } from '../../internal.api';
1111
import { getDefaultEnvManagerSetting, getDefaultPkgManagerSetting } from '../settings/settingHelpers';
1212

13-
const MANAGER_READY_TIMEOUT_MS = 30_000;
13+
export const MANAGER_READY_TIMEOUT_MS = 30_000;
1414

1515
interface ManagerReady extends Disposable {
1616
waitForEnvManager(uris?: Uri[]): Promise<void>;
@@ -27,6 +27,48 @@ function getExtensionId(managerId: string): string | undefined {
2727
return parts ? parts[1] : undefined;
2828
}
2929

30+
/**
31+
* Wraps a deferred with a timeout so a missing/dead manager cannot block the API forever.
32+
* On timeout the deferred is resolved (not rejected) so callers proceed with degraded results
33+
* instead of hanging.
34+
*/
35+
export function withManagerTimeout(
36+
deferred: Deferred<void>,
37+
managerId: string,
38+
kind: 'environment' | 'package',
39+
): Promise<void> {
40+
if (deferred.completed) {
41+
return deferred.promise;
42+
}
43+
return new Promise<void>((resolve) => {
44+
const timer = setTimeout(() => {
45+
if (!deferred.completed) {
46+
traceWarn(
47+
`Timed out after ${MANAGER_READY_TIMEOUT_MS / 1000}s waiting for ${kind} manager "${managerId}" to register. ` +
48+
`The manager may not be installed or its extension failed to activate. Proceeding without it. ` +
49+
`To prevent this, check your "python-envs.defaultEnvManager" and "python-envs.pythonProjects" settings.`,
50+
);
51+
sendTelemetryEvent(EventNames.MANAGER_READY_TIMEOUT, undefined, {
52+
managerId,
53+
managerKind: kind,
54+
});
55+
deferred.resolve();
56+
}
57+
}, MANAGER_READY_TIMEOUT_MS);
58+
59+
deferred.promise.then(
60+
() => {
61+
clearTimeout(timer);
62+
resolve();
63+
},
64+
() => {
65+
clearTimeout(timer);
66+
resolve();
67+
},
68+
);
69+
});
70+
}
71+
3072
class ManagerReadyImpl implements ManagerReady {
3173
private readonly envManagers: Map<string, Deferred<void>> = new Map();
3274
private readonly pkgManagers: Map<string, Deferred<void>> = new Map();
@@ -111,44 +153,6 @@ class ManagerReadyImpl implements ManagerReady {
111153
}
112154
}
113155

114-
/**
115-
* Wraps a deferred with a timeout so a missing/dead manager cannot block the API forever.
116-
* On timeout the deferred is resolved (not rejected) so callers proceed with degraded results
117-
* instead of hanging.
118-
*/
119-
private _withTimeout(deferred: Deferred<void>, managerId: string, kind: string): Promise<void> {
120-
if (deferred.completed) {
121-
return deferred.promise;
122-
}
123-
return new Promise<void>((resolve) => {
124-
const timer = setTimeout(() => {
125-
if (!deferred.completed) {
126-
traceWarn(
127-
`Timed out after ${MANAGER_READY_TIMEOUT_MS / 1000}s waiting for ${kind} manager "${managerId}" to register. ` +
128-
`The manager may not be installed or its extension failed to activate. Proceeding without it. ` +
129-
`To prevent this, check your "python-envs.defaultEnvManager" and "python-envs.pythonProjects" settings.`,
130-
);
131-
sendTelemetryEvent(EventNames.MANAGER_READY_TIMEOUT, undefined, {
132-
managerId,
133-
managerKind: kind as 'environment' | 'package',
134-
});
135-
deferred.resolve();
136-
}
137-
}, MANAGER_READY_TIMEOUT_MS);
138-
139-
deferred.promise.then(
140-
() => {
141-
clearTimeout(timer);
142-
resolve();
143-
},
144-
() => {
145-
clearTimeout(timer);
146-
resolve();
147-
},
148-
);
149-
});
150-
}
151-
152156
public dispose(): void {
153157
this.disposables.forEach((d) => d.dispose());
154158
this.envManagers.clear();
@@ -161,7 +165,7 @@ class ManagerReadyImpl implements ManagerReady {
161165
}
162166
const deferred = createDeferred<void>();
163167
this.envManagers.set(managerId, deferred);
164-
return this._withTimeout(deferred, managerId, 'environment');
168+
return withManagerTimeout(deferred, managerId, 'environment');
165169
}
166170

167171
public async waitForEnvManager(uris?: Uri[]): Promise<void> {
@@ -210,7 +214,7 @@ class ManagerReadyImpl implements ManagerReady {
210214
}
211215
const deferred = createDeferred<void>();
212216
this.pkgManagers.set(managerId, deferred);
213-
return this._withTimeout(deferred, managerId, 'package');
217+
return withManagerTimeout(deferred, managerId, 'package');
214218
}
215219

216220
public async waitForPkgManager(uris?: Uri[]): Promise<void> {

‎src/test/features/common/managerReady.unit.test.ts‎

Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,106 @@
1+
import assert from 'assert';
2+
import * as sinon from 'sinon';
3+
import * as logging from '../../../common/logging';
4+
import { EventNames } from '../../../common/telemetry/constants';
5+
import * as telemetrySender from '../../../common/telemetry/sender';
6+
import { createDeferred } from '../../../common/utils/deferred';
7+
import { MANAGER_READY_TIMEOUT_MS, withManagerTimeout } from '../../../features/common/managerReady';
8+
9+
suite('withManagerTimeout', () => {
10+
let clock: sinon.SinonFakeTimers;
11+
let traceWarnStub: sinon.SinonStub;
12+
let sendTelemetryStub: sinon.SinonStub;
13+
14+
setup(() => {
15+
clock = sinon.useFakeTimers();
16+
traceWarnStub = sinon.stub(logging, 'traceWarn');
17+
sendTelemetryStub = sinon.stub(telemetrySender, 'sendTelemetryEvent');
18+
});
19+
20+
teardown(() => {
21+
clock.restore();
22+
sinon.restore();
23+
});
24+
25+
test('deferred never resolves → timeout fires, logs warning, sends telemetry', async () => {
26+
const deferred = createDeferred<void>();
27+
const promise = withManagerTimeout(deferred, 'test-ext:venv', 'environment');
28+
29+
// Advance past the timeout
30+
clock.tick(MANAGER_READY_TIMEOUT_MS);
31+
await clock.tickAsync(0); // flush microtasks
32+
33+
await promise;
34+
35+
// Warning was logged with manager ID
36+
assert.ok(traceWarnStub.calledOnce, 'traceWarn should be called once');
37+
assert.ok(traceWarnStub.firstCall.args[0].includes('test-ext:venv'), 'warning should contain the manager ID');
38+
39+
// Telemetry was sent
40+
assert.ok(sendTelemetryStub.calledOnce, 'sendTelemetryEvent should be called once');
41+
const [eventName, , properties] = sendTelemetryStub.firstCall.args;
42+
assert.strictEqual(eventName, EventNames.MANAGER_READY_TIMEOUT);
43+
assert.strictEqual(properties.managerId, 'test-ext:venv');
44+
assert.strictEqual(properties.managerKind, 'environment');
45+
});
46+
47+
test('deferred resolves before timeout → no warning, no telemetry', async () => {
48+
const deferred = createDeferred<void>();
49+
const promise = withManagerTimeout(deferred, 'test-ext:conda', 'environment');
50+
51+
// Resolve before timeout
52+
deferred.resolve();
53+
await clock.tickAsync(0);
54+
55+
await promise;
56+
57+
// Advance past the timeout to confirm it was cleared
58+
clock.tick(MANAGER_READY_TIMEOUT_MS);
59+
await clock.tickAsync(0);
60+
61+
assert.ok(traceWarnStub.notCalled, 'traceWarn should not be called');
62+
assert.ok(sendTelemetryStub.notCalled, 'sendTelemetryEvent should not be called');
63+
});
64+
65+
test('timeout resolves (not rejects) the deferred', async () => {
66+
const deferred = createDeferred<void>();
67+
const promise = withManagerTimeout(deferred, 'test-ext:missing', 'environment');
68+
69+
clock.tick(MANAGER_READY_TIMEOUT_MS);
70+
await clock.tickAsync(0);
71+
72+
// This must resolve — if it rejects, the test fails
73+
await promise;
74+
75+
assert.ok(deferred.resolved, 'deferred should be resolved, not rejected');
76+
assert.ok(!deferred.rejected, 'deferred should not be rejected');
77+
});
78+
79+
test('already-completed deferred returns immediately without timeout', async () => {
80+
const deferred = createDeferred<void>();
81+
deferred.resolve();
82+
83+
const promise = withManagerTimeout(deferred, 'test-ext:venv', 'environment');
84+
await promise;
85+
86+
// No timer was set, so nothing should fire
87+
clock.tick(MANAGER_READY_TIMEOUT_MS);
88+
await clock.tickAsync(0);
89+
90+
assert.ok(traceWarnStub.notCalled, 'traceWarn should not be called for completed deferred');
91+
assert.ok(sendTelemetryStub.notCalled, 'sendTelemetryEvent should not be called for completed deferred');
92+
});
93+
94+
test('package manager kind is passed through to telemetry', async () => {
95+
const deferred = createDeferred<void>();
96+
const promise = withManagerTimeout(deferred, 'test-ext:pip', 'package');
97+
98+
clock.tick(MANAGER_READY_TIMEOUT_MS);
99+
await clock.tickAsync(0);
100+
await promise;
101+
102+
const [, , properties] = sendTelemetryStub.firstCall.args;
103+
assert.strictEqual(properties.managerId, 'test-ext:pip');
104+
assert.strictEqual(properties.managerKind, 'package');
105+
});
106+
});

0 commit comments

Comments
 (0)