Skip to content

Commit 5b86906

Browse files
eleanorjboydeleanorjboyd
committed
fix tests
1 parent 78b89b6 commit 5b86906

File tree

4 files changed

+110
-135
lines changed

4 files changed

+110
-135
lines changed

‎src/features/execution/envVariableManager.ts‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,13 @@ export class PythonEnvVariableManager implements EnvVarManager {
7777

7878
onDidChangeEnvironmentVariables: Event<DidChangeEnvironmentVariablesEventArgs>;
7979

80+
/**
81+
* @internal For testing only - manually trigger environment variable change event
82+
*/
83+
triggerEnvironmentVariableChange(event: DidChangeEnvironmentVariablesEventArgs): void {
84+
this._onDidChangeEnvironmentVariables.fire(event);
85+
}
86+
8087
dispose(): void {
8188
this.disposables.forEach((disposable) => disposable.dispose());
8289
}

‎src/features/terminal/terminalEnvVarInjector.ts‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ export class TerminalEnvVarInjector implements Disposable {
3030
* variables that are removed/commented out in .env files.
3131
* Key: workspace fsPath, Value: Set of env var keys we've set for that workspace
3232
*/
33-
private readonly trackedEnvVars: Map<string, Set<string>> = new Map();
33+
protected readonly trackedEnvVars: Map<string, Set<string>> = new Map();
3434

3535
constructor(
3636
private readonly envVarCollection: GlobalEnvironmentVariableCollection,
@@ -208,7 +208,7 @@ export class TerminalEnvVarInjector implements Disposable {
208208
/**
209209
* Apply environment variable changes by comparing current and previous state.
210210
*/
211-
private applyEnvVarChanges(
211+
protected applyEnvVarChanges(
212212
envVarScope: EnvironmentVariableCollection,
213213
envVars: { [key: string]: string | undefined },
214214
workspaceKey: string,
@@ -244,7 +244,7 @@ export class TerminalEnvVarInjector implements Disposable {
244244
/**
245245
* Clean up previously tracked environment variables for a workspace.
246246
*/
247-
private cleanupTrackedVars(envVarScope: EnvironmentVariableCollection, workspaceKey: string): void {
247+
protected cleanupTrackedVars(envVarScope: EnvironmentVariableCollection, workspaceKey: string): void {
248248
const previousKeys = this.trackedEnvVars.get(workspaceKey);
249249
if (previousKeys) {
250250
previousKeys.forEach((key) => envVarScope.delete(key));
@@ -286,7 +286,7 @@ export class TerminalEnvVarInjector implements Disposable {
286286
/**
287287
* Clear all environment variables for a workspace.
288288
*/
289-
private clearWorkspaceVariables(workspaceFolder: WorkspaceFolder): void {
289+
protected clearWorkspaceVariables(workspaceFolder: WorkspaceFolder): void {
290290
const workspaceKey = workspaceFolder.uri.fsPath;
291291
try {
292292
const scope = this.getEnvironmentVariableCollectionScoped({ workspaceFolder });

‎src/test/features/terminalEnvVarInjector.test.ts‎

Lines changed: 82 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,28 @@ suite('TerminalEnvVarInjector - Integration Tests with Fixtures', () => {
8383
return targetPath;
8484
}
8585

86+
/**
87+
* Wait for a condition to be true, polling at regular intervals.
88+
* @param condition Function that returns true when the condition is met
89+
* @param timeoutMs Maximum time to wait in milliseconds
90+
* @param pollIntervalMs How often to check the condition
91+
*/
92+
async function waitForCondition(
93+
condition: () => boolean,
94+
timeoutMs: number = 2000,
95+
pollIntervalMs: number = 10,
96+
): Promise<void> {
97+
const startTime = Date.now();
98+
while (!condition()) {
99+
if (Date.now() - startTime > timeoutMs) {
100+
throw new Error(`Timeout waiting for condition after ${timeoutMs}ms`);
101+
}
102+
// Allow async operations to process
103+
await new Promise((resolve) => setImmediate(resolve));
104+
await new Promise((resolve) => setTimeout(resolve, pollIntervalMs));
105+
}
106+
}
107+
86108
suite('File Existence Tests', () => {
87109
test('should inject variables when .env file exists', async () => {
88110
// Arrange - copy fixture to temp workspace
@@ -97,21 +119,24 @@ suite('TerminalEnvVarInjector - Integration Tests with Fixtures', () => {
97119

98120
injector = new TerminalEnvVarInjector(mockEnvVarCollection, envVarManager);
99121

100-
// Act - trigger injection via the public initialize flow
101-
envVarManager['_onDidChangeEnvironmentVariables'].fire({
122+
// Act - trigger injection via the test helper
123+
envVarManager.triggerEnvironmentVariableChange({
102124
uri: workspaceFolder.uri,
103125
changeType: FileChangeType.Changed,
104126
});
105127

106-
// Wait for async operations
107-
await new Promise((resolve) => setTimeout(resolve, 100));
128+
// Wait for async operations to complete
129+
await new Promise((resolve) => setTimeout(resolve, 200));
108130

109131
// Assert
110132
const stubs = scopedCollectionStubs.get(workspaceDir);
111-
assert.ok(stubs, 'Should have created scoped collection for workspace');
112-
assert.ok(stubs!.replace.calledWith('FOO', 'bar'), 'Should inject FOO');
113-
assert.ok(stubs!.replace.calledWith('BAR', 'baz'), 'Should inject BAR');
114-
assert.ok(stubs!.replace.calledWith('BAZ', 'qux'), 'Should inject BAZ');
133+
if (!stubs) {
134+
assert.fail('Should have created scoped collection for workspace');
135+
return;
136+
}
137+
assert.ok(stubs.replace.calledWith('FOO', 'bar'), 'Should inject FOO');
138+
assert.ok(stubs.replace.calledWith('BAR', 'baz'), 'Should inject BAR');
139+
assert.ok(stubs.replace.calledWith('BAZ', 'qux'), 'Should inject BAZ');
115140
});
116141

117142
test('should not inject when .env file does not exist', async () => {
@@ -128,12 +153,13 @@ suite('TerminalEnvVarInjector - Integration Tests with Fixtures', () => {
128153
injector = new TerminalEnvVarInjector(mockEnvVarCollection, envVarManager);
129154

130155
// Act
131-
envVarManager['_onDidChangeEnvironmentVariables'].fire({
156+
envVarManager.triggerEnvironmentVariableChange({
132157
uri: workspaceFolder.uri,
133158
changeType: FileChangeType.Changed,
134159
});
135160

136-
await new Promise((resolve) => setTimeout(resolve, 100));
161+
// Wait a bit to ensure no injection happens (negative assertion)
162+
await new Promise((resolve) => setTimeout(resolve, 50));
137163

138164
// Assert
139165
const stubs = scopedCollectionStubs.get(workspaceDir);
@@ -162,15 +188,16 @@ suite('TerminalEnvVarInjector - Integration Tests with Fixtures', () => {
162188
injector = new TerminalEnvVarInjector(mockEnvVarCollection, envVarManager);
163189

164190
// Act
165-
envVarManager['_onDidChangeEnvironmentVariables'].fire({
191+
envVarManager.triggerEnvironmentVariableChange({
166192
uri: workspaceFolder.uri,
167193
changeType: FileChangeType.Changed,
168194
});
169195

170-
await new Promise((resolve) => setTimeout(resolve, 100));
196+
// Wait for async operations to complete
197+
const stubs = scopedCollectionStubs.get(workspaceDir);
198+
await waitForCondition(() => !!stubs && stubs.replace.called);
171199

172200
// Assert
173-
const stubs = scopedCollectionStubs.get(workspaceDir);
174201
assert.ok(stubs?.replace.calledWith('CUSTOM_VAR', 'custom_value'), 'Should inject from custom file');
175202
});
176203
});
@@ -190,27 +217,28 @@ suite('TerminalEnvVarInjector - Integration Tests with Fixtures', () => {
190217
injector = new TerminalEnvVarInjector(mockEnvVarCollection, envVarManager);
191218

192219
// Initial injection
193-
envVarManager['_onDidChangeEnvironmentVariables'].fire({
220+
envVarManager.triggerEnvironmentVariableChange({
194221
uri: workspaceFolder.uri,
195222
changeType: FileChangeType.Changed,
196223
});
197-
await new Promise((resolve) => setTimeout(resolve, 100));
198-
199224
const stubs = scopedCollectionStubs.get(workspaceDir)!;
200-
assert.ok(stubs.replace.calledWith('FOO', 'bar'), 'Initial FOO should be set');
225+
await waitForCondition(() => stubs.replace.calledWith('FOO', 'bar'));
201226

202-
// Act - Disable useEnvFile
203-
const mockConfig = mockGetConfiguration.returnValues[0];
204-
mockConfig.get.withArgs('terminal.useEnvFile', false).returns(false);
227+
// Set up a new mock config with useEnvFile disabled for the next call
228+
const disabledEnvFileConfig = {
229+
get: sinon.stub().withArgs('terminal.useEnvFile', false).returns(false),
230+
// Add any other methods/properties as needed by the code under test
231+
};
232+
mockGetConfiguration.returns(disabledEnvFileConfig);
205233

206234
stubs.replace.resetHistory();
207235
stubs.delete.resetHistory();
208236

209-
envVarManager['_onDidChangeEnvironmentVariables'].fire({
237+
envVarManager.triggerEnvironmentVariableChange({
210238
uri: workspaceFolder.uri,
211239
changeType: FileChangeType.Changed,
212240
});
213-
await new Promise((resolve) => setTimeout(resolve, 100));
241+
await waitForCondition(() => stubs.delete.callCount >= 3);
214242

215243
// Assert - Should cleanup previously tracked variables
216244
assert.ok(stubs.delete.calledWith('FOO'), 'Should delete FOO');
@@ -232,24 +260,24 @@ suite('TerminalEnvVarInjector - Integration Tests with Fixtures', () => {
232260
injector = new TerminalEnvVarInjector(mockEnvVarCollection, envVarManager);
233261

234262
// Initial injection
235-
envVarManager['_onDidChangeEnvironmentVariables'].fire({
263+
envVarManager.triggerEnvironmentVariableChange({
236264
uri: workspaceFolder.uri,
237265
changeType: FileChangeType.Changed,
238266
});
239-
await new Promise((resolve) => setTimeout(resolve, 100));
240-
241267
const stubs = scopedCollectionStubs.get(workspaceDir)!;
268+
await waitForCondition(() => stubs.replace.calledWith('FOO', 'bar'));
269+
242270
assert.ok(stubs.replace.calledWith('FOO', 'bar'), 'Initial FOO should be set');
243271

244272
// Act - Delete the .env file
245273
await fs.remove(envFilePath);
246274
stubs.delete.resetHistory();
247275

248-
envVarManager['_onDidChangeEnvironmentVariables'].fire({
276+
envVarManager.triggerEnvironmentVariableChange({
249277
uri: workspaceFolder.uri,
250278
changeType: FileChangeType.Deleted,
251279
});
252-
await new Promise((resolve) => setTimeout(resolve, 100));
280+
await waitForCondition(() => stubs.delete.calledWith('FOO'));
253281

254282
// Assert - Should cleanup
255283
assert.ok(stubs.delete.calledWith('FOO'), 'Should delete FOO after file deletion');
@@ -271,13 +299,14 @@ suite('TerminalEnvVarInjector - Integration Tests with Fixtures', () => {
271299
injector = new TerminalEnvVarInjector(mockEnvVarCollection, envVarManager);
272300

273301
// Initial injection
274-
envVarManager['_onDidChangeEnvironmentVariables'].fire({
302+
envVarManager.triggerEnvironmentVariableChange({
275303
uri: workspaceFolder.uri,
276304
changeType: FileChangeType.Changed,
277305
});
278-
await new Promise((resolve) => setTimeout(resolve, 100));
306+
let stubs = scopedCollectionStubs.get(workspaceDir);
307+
await waitForCondition(() => !!stubs && stubs!.replace.calledWith('BAR', 'baz'));
308+
stubs = scopedCollectionStubs.get(workspaceDir)!;
279309

280-
const stubs = scopedCollectionStubs.get(workspaceDir)!;
281310
assert.ok(stubs.replace.calledWith('BAR', 'baz'), 'BAR should be initially set');
282311

283312
// Act - Comment out BAR in the file
@@ -286,11 +315,11 @@ suite('TerminalEnvVarInjector - Integration Tests with Fixtures', () => {
286315
stubs.replace.resetHistory();
287316
stubs.delete.resetHistory();
288317

289-
envVarManager['_onDidChangeEnvironmentVariables'].fire({
318+
envVarManager.triggerEnvironmentVariableChange({
290319
uri: workspaceFolder.uri,
291320
changeType: FileChangeType.Changed,
292321
});
293-
await new Promise((resolve) => setTimeout(resolve, 100));
322+
await waitForCondition(() => stubs.delete.calledWith('BAR'));
294323

295324
// Assert - BAR should be deleted
296325
assert.ok(stubs.delete.calledWith('BAR'), 'Should delete commented out BAR');
@@ -312,24 +341,24 @@ suite('TerminalEnvVarInjector - Integration Tests with Fixtures', () => {
312341
injector = new TerminalEnvVarInjector(mockEnvVarCollection, envVarManager);
313342

314343
// Initial injection
315-
envVarManager['_onDidChangeEnvironmentVariables'].fire({
344+
envVarManager.triggerEnvironmentVariableChange({
316345
uri: workspaceFolder.uri,
317346
changeType: FileChangeType.Changed,
318347
});
319-
await new Promise((resolve) => setTimeout(resolve, 100));
320-
321-
const stubs = scopedCollectionStubs.get(workspaceDir)!;
348+
let stubs = scopedCollectionStubs.get(workspaceDir);
349+
await waitForCondition(() => !!stubs && stubs!.replace.calledWith('FOO', 'bar'));
350+
stubs = scopedCollectionStubs.get(workspaceDir)!;
322351

323352
// Act - Add NEW_VAR to file
324353
await fs.appendFile(envFilePath, 'NEW_VAR=new_value\n');
325354

326355
stubs.replace.resetHistory();
327356

328-
envVarManager['_onDidChangeEnvironmentVariables'].fire({
357+
envVarManager.triggerEnvironmentVariableChange({
329358
uri: workspaceFolder.uri,
330359
changeType: FileChangeType.Changed,
331360
});
332-
await new Promise((resolve) => setTimeout(resolve, 100));
361+
await waitForCondition(() => stubs.replace.calledWith('NEW_VAR', 'new_value'));
333362

334363
// Assert
335364
assert.ok(stubs.replace.calledWith('NEW_VAR', 'new_value'), 'Should add NEW_VAR');
@@ -350,25 +379,26 @@ suite('TerminalEnvVarInjector - Integration Tests with Fixtures', () => {
350379
injector = new TerminalEnvVarInjector(mockEnvVarCollection, envVarManager);
351380

352381
// Initial injection
353-
envVarManager['_onDidChangeEnvironmentVariables'].fire({
382+
envVarManager.triggerEnvironmentVariableChange({
354383
uri: workspaceFolder.uri,
355384
changeType: FileChangeType.Changed,
356385
});
357-
await new Promise((resolve) => setTimeout(resolve, 100));
386+
let stubs = scopedCollectionStubs.get(workspaceDir);
387+
await waitForCondition(() => !!stubs && stubs!.replace.calledWith('TO_UNSET', 'value'));
388+
stubs = scopedCollectionStubs.get(workspaceDir)!;
358389

359-
const stubs = scopedCollectionStubs.get(workspaceDir)!;
360390
assert.ok(stubs.replace.calledWith('TO_UNSET', 'value'), 'TO_UNSET should be initially set');
361391

362392
// Act - Unset TO_UNSET (VAR=)
363393
await fs.writeFile(envFilePath, 'FOO=bar\nTO_UNSET=\n');
364394

365395
stubs.delete.resetHistory();
366396

367-
envVarManager['_onDidChangeEnvironmentVariables'].fire({
397+
envVarManager.triggerEnvironmentVariableChange({
368398
uri: workspaceFolder.uri,
369399
changeType: FileChangeType.Changed,
370400
});
371-
await new Promise((resolve) => setTimeout(resolve, 100));
401+
await waitForCondition(() => stubs.delete.calledWith('TO_UNSET'));
372402

373403
// Assert
374404
assert.ok(stubs.delete.calledWith('TO_UNSET'), 'Should delete unset variable');
@@ -399,19 +429,23 @@ suite('TerminalEnvVarInjector - Integration Tests with Fixtures', () => {
399429
injector = new TerminalEnvVarInjector(mockEnvVarCollection, envVarManager);
400430

401431
// Act - Inject for both workspaces
402-
envVarManager['_onDidChangeEnvironmentVariables'].fire({
432+
envVarManager.triggerEnvironmentVariableChange({
403433
uri: workspaceFolder1.uri,
404434
changeType: FileChangeType.Changed,
405435
});
406-
envVarManager['_onDidChangeEnvironmentVariables'].fire({
436+
envVarManager.triggerEnvironmentVariableChange({
407437
uri: workspaceFolder2.uri,
408438
changeType: FileChangeType.Changed,
409439
});
410-
await new Promise((resolve) => setTimeout(resolve, 100));
440+
441+
// Wait for async operations for both workspaces
442+
let stubs1 = scopedCollectionStubs.get(workspace1Dir);
443+
let stubs2 = scopedCollectionStubs.get(workspace2Dir);
444+
await waitForCondition(() => !!stubs1 && !!stubs2 && stubs1!.replace.called && stubs2!.replace.called);
445+
stubs1 = scopedCollectionStubs.get(workspace1Dir)!;
446+
stubs2 = scopedCollectionStubs.get(workspace2Dir)!;
411447

412448
// Assert - Each workspace should have its own variables
413-
const stubs1 = scopedCollectionStubs.get(workspace1Dir)!;
414-
const stubs2 = scopedCollectionStubs.get(workspace2Dir)!;
415449

416450
assert.ok(stubs1.replace.calledWith('WS1_VAR', 'workspace1_value'), 'Workspace 1 should have WS1_VAR');
417451
assert.strictEqual(stubs1.replace.calledWith('WS2_VAR'), false, 'Workspace 1 should not have WS2_VAR');

0 commit comments

Comments
 (0)