From 3b75f16380edeaea6fce7a5b1822de21f215aa58 Mon Sep 17 00:00:00 2001 From: eleanorjboyd <26030610+eleanorjboyd@users.noreply.github.com> Date: Tue, 9 Dec 2025 10:12:21 -0800 Subject: [PATCH 1/8] Fix env variable caching and add testing arch for environment variable functionality --- .../testing-workflow.instructions.md | 7 + .../terminal/terminalEnvVarInjector.ts | 163 +++++-- .../features/terminalEnvVarInjector.test.ts | 423 ++++++++++++++++++ .../terminalEnvVarInjectorBasic.unit.test.ts | 345 +++++++++++--- .../fixtures/terminalEnvVarInjector/basic.env | 4 + .../terminalEnvVarInjector/commented-out.env | 4 + .../custom-path/.env.custom | 2 + .../fixtures/terminalEnvVarInjector/empty.env | 1 + .../terminalEnvVarInjector/single-var.env | 2 + .../terminalEnvVarInjector/with-unset.env | 3 + .../terminalEnvVarInjector/workspace1/.env | 2 + .../terminalEnvVarInjector/workspace2/.env | 2 + 12 files changed, 862 insertions(+), 96 deletions(-) create mode 100644 src/test/features/terminalEnvVarInjector.test.ts create mode 100644 src/test/fixtures/terminalEnvVarInjector/basic.env create mode 100644 src/test/fixtures/terminalEnvVarInjector/commented-out.env create mode 100644 src/test/fixtures/terminalEnvVarInjector/custom-path/.env.custom create mode 100644 src/test/fixtures/terminalEnvVarInjector/empty.env create mode 100644 src/test/fixtures/terminalEnvVarInjector/single-var.env create mode 100644 src/test/fixtures/terminalEnvVarInjector/with-unset.env create mode 100644 src/test/fixtures/terminalEnvVarInjector/workspace1/.env create mode 100644 src/test/fixtures/terminalEnvVarInjector/workspace2/.env diff --git a/.github/instructions/testing-workflow.instructions.md b/.github/instructions/testing-workflow.instructions.md index 4c2a8270..8cb8b1d3 100644 --- a/.github/instructions/testing-workflow.instructions.md +++ b/.github/instructions/testing-workflow.instructions.md @@ -574,3 +574,10 @@ envConfig.inspect - Untestable Node.js APIs → Create proxy abstraction functions (use function overloads to preserve intelligent typing while making functions mockable) ## 🧠 Agent Learnings + +- VS Code file watchers only monitor workspace folders, not external temp directories (1) +- Use fixture-based testing with real files instead of mocking fs-extra, which has non-configurable property descriptors that prevent stubbing (1) +- Extension tests (.test.ts) should use real filesystem operations; unit tests (.unit.test.ts) should mock dependencies (1) +- Use `as unknown as TargetType` for type casting instead of `as any` to maintain type safety and avoid 'any' violations +- If tests frequently need private access consider that maybe methods should be protected, or public test utilities should exist for testing (1) +- When making systematic changes across many similar locations, fix one instance completely first to validate the approach before applying the pattern everywhere (1) diff --git a/src/features/terminal/terminalEnvVarInjector.ts b/src/features/terminal/terminalEnvVarInjector.ts index 039c4b3c..ffa27e2a 100644 --- a/src/features/terminal/terminalEnvVarInjector.ts +++ b/src/features/terminal/terminalEnvVarInjector.ts @@ -5,8 +5,10 @@ import * as fse from 'fs-extra'; import * as path from 'path'; import { Disposable, + EnvironmentVariableCollection, EnvironmentVariableScope, GlobalEnvironmentVariableCollection, + Uri, window, workspace, WorkspaceFolder, @@ -23,6 +25,13 @@ import { EnvVarManager } from '../execution/envVariableManager'; export class TerminalEnvVarInjector implements Disposable { private disposables: Disposable[] = []; + /** + * Track which environment variables we've set per workspace to properly handle + * variables that are removed/commented out in .env files. + * Key: workspace fsPath, Value: Set of env var keys we've set for that workspace + */ + private readonly trackedEnvVars: Map> = new Map(); + constructor( private readonly envVarCollection: GlobalEnvironmentVariableCollection, private readonly envVarManager: EnvVarManager, @@ -134,48 +143,22 @@ export class TerminalEnvVarInjector implements Disposable { */ private async injectEnvironmentVariablesForWorkspace(workspaceFolder: WorkspaceFolder): Promise { const workspaceUri = workspaceFolder.uri; - try { - const envVars = await this.envVarManager.getEnvironmentVariables(workspaceUri); + const workspaceKey = workspaceUri.fsPath; - // use scoped environment variable collection + try { const envVarScope = this.getEnvironmentVariableCollectionScoped({ workspaceFolder }); - // Check if env file injection is enabled - const config = getConfiguration('python', workspaceUri); - const useEnvFile = config.get('terminal.useEnvFile', false); - const envFilePath = config.get('envFile'); - - if (!useEnvFile) { - traceVerbose( - `TerminalEnvVarInjector: Env file injection disabled for workspace: ${workspaceUri.fsPath}`, - ); + // Check if we should inject and get the env file path + const shouldInject = await this.shouldInjectEnvVars(workspaceUri, envVarScope, workspaceKey); + if (!shouldInject) { return; } - // Track which .env file is being used for logging - const resolvedEnvFilePath: string | undefined = envFilePath - ? path.resolve(resolveVariables(envFilePath, workspaceUri)) - : undefined; - const defaultEnvFilePath: string = path.join(workspaceUri.fsPath, '.env'); - - let activeEnvFilePath: string = resolvedEnvFilePath || defaultEnvFilePath; - if (activeEnvFilePath && (await fse.pathExists(activeEnvFilePath))) { - traceVerbose(`TerminalEnvVarInjector: Using env file: ${activeEnvFilePath}`); - } else { - traceVerbose( - `TerminalEnvVarInjector: No .env file found for workspace: ${workspaceUri.fsPath}, not injecting environment variables.`, - ); - return; // No .env file to inject - } + // Get environment variables from the .env file + const envVars = await this.envVarManager.getEnvironmentVariables(workspaceUri); - for (const [key, value] of Object.entries(envVars)) { - if (value === undefined) { - // Remove the environment variable if the value is undefined - envVarScope.delete(key); - } else { - envVarScope.replace(key, value); - } - } + // Apply the environment variable changes + this.applyEnvVarChanges(envVarScope, envVars, workspaceKey); } catch (error) { traceError( `TerminalEnvVarInjector: Error injecting environment variables for workspace ${workspaceUri.fsPath}:`, @@ -184,6 +167,91 @@ export class TerminalEnvVarInjector implements Disposable { } } + /** + * Check if environment variables should be injected for the workspace. + * Returns true if injection should proceed, false otherwise. + */ + private async shouldInjectEnvVars( + workspaceUri: Uri, + envVarScope: EnvironmentVariableCollection, + workspaceKey: string, + ): Promise { + const config = getConfiguration('python', workspaceUri); + const useEnvFile = config.get('terminal.useEnvFile', false); + const envFilePath = config.get('envFile'); + + if (!useEnvFile) { + traceVerbose(`TerminalEnvVarInjector: Env file injection disabled for workspace: ${workspaceUri.fsPath}`); + this.cleanupTrackedVars(envVarScope, workspaceKey); + return false; + } + + // Determine which .env file to use + const resolvedEnvFilePath: string | undefined = envFilePath + ? path.resolve(resolveVariables(envFilePath, workspaceUri)) + : undefined; + const defaultEnvFilePath: string = path.join(workspaceUri.fsPath, '.env'); + const activeEnvFilePath: string = resolvedEnvFilePath || defaultEnvFilePath; + + if (activeEnvFilePath && (await fse.pathExists(activeEnvFilePath))) { + traceVerbose(`TerminalEnvVarInjector: Using env file: ${activeEnvFilePath}`); + return true; + } else { + traceVerbose( + `TerminalEnvVarInjector: No .env file found for workspace: ${workspaceUri.fsPath}, not injecting environment variables.`, + ); + this.cleanupTrackedVars(envVarScope, workspaceKey); + return false; + } + } + + /** + * Apply environment variable changes by comparing current and previous state. + */ + private applyEnvVarChanges( + envVarScope: EnvironmentVariableCollection, + envVars: { [key: string]: string | undefined }, + workspaceKey: string, + ): void { + const previousKeys = this.trackedEnvVars.get(workspaceKey) || new Set(); + const currentKeys = new Set(); + + // Update/add current env vars from .env file + for (const [key, value] of Object.entries(envVars)) { + if (value === undefined || value === '') { + // Variable explicitly unset in .env (e.g., VAR=) + envVarScope.delete(key); + } else { + envVarScope.replace(key, value); + currentKeys.add(key); + } + } + + // Delete keys that were previously set but are now gone from .env + for (const oldKey of previousKeys) { + if (!currentKeys.has(oldKey)) { + traceVerbose( + `TerminalEnvVarInjector: Removing previously set env var '${oldKey}' from workspace ${workspaceKey}`, + ); + envVarScope.delete(oldKey); + } + } + + // Update our tracking for this workspace + this.trackedEnvVars.set(workspaceKey, currentKeys); + } + + /** + * Clean up previously tracked environment variables for a workspace. + */ + private cleanupTrackedVars(envVarScope: EnvironmentVariableCollection, workspaceKey: string): void { + const previousKeys = this.trackedEnvVars.get(workspaceKey); + if (previousKeys) { + previousKeys.forEach((key) => envVarScope.delete(key)); + this.trackedEnvVars.delete(workspaceKey); + } + } + /** * Dispose of the injector and clean up resources. */ @@ -192,8 +260,20 @@ export class TerminalEnvVarInjector implements Disposable { this.disposables.forEach((disposable) => disposable.dispose()); this.disposables = []; - // Clear all environment variables from the collection - this.envVarCollection.clear(); + // Clear only the environment variables that we've set, preserving others (e.g., BASH_ENV) + for (const [workspaceKey, trackedKeys] of this.trackedEnvVars.entries()) { + try { + // Try to find the workspace folder for proper scoping + const workspaceFolder = workspace.workspaceFolders?.find((wf) => wf.uri.fsPath === workspaceKey); + if (workspaceFolder) { + const scope = this.getEnvironmentVariableCollectionScoped({ workspaceFolder }); + trackedKeys.forEach((key) => scope.delete(key)); + } + } catch (error) { + traceError(`Failed to clean up environment variables for workspace ${workspaceKey}:`, error); + } + } + this.trackedEnvVars.clear(); } private getEnvironmentVariableCollectionScoped(scope: EnvironmentVariableScope = {}) { @@ -205,9 +285,16 @@ export class TerminalEnvVarInjector implements Disposable { * Clear all environment variables for a workspace. */ private clearWorkspaceVariables(workspaceFolder: WorkspaceFolder): void { + const workspaceKey = workspaceFolder.uri.fsPath; try { const scope = this.getEnvironmentVariableCollectionScoped({ workspaceFolder }); - scope.clear(); + + // Only delete env vars that we've set, not ones set by other managers (e.g., BASH_ENV) + const trackedKeys = this.trackedEnvVars.get(workspaceKey); + if (trackedKeys) { + trackedKeys.forEach((key) => scope.delete(key)); + this.trackedEnvVars.delete(workspaceKey); + } } catch (error) { traceError(`Failed to clear environment variables for workspace ${workspaceFolder.uri.fsPath}:`, error); } diff --git a/src/test/features/terminalEnvVarInjector.test.ts b/src/test/features/terminalEnvVarInjector.test.ts new file mode 100644 index 00000000..7cdbf0dc --- /dev/null +++ b/src/test/features/terminalEnvVarInjector.test.ts @@ -0,0 +1,423 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import * as assert from 'assert'; +import * as fs from 'fs-extra'; +import * as os from 'os'; +import * as path from 'path'; +import * as sinon from 'sinon'; +import { FileChangeType, GlobalEnvironmentVariableCollection, Uri, WorkspaceFolder } from 'vscode'; +import * as workspaceApis from '../../common/workspace.apis'; +import { PythonEnvVariableManager } from '../../features/execution/envVariableManager'; +import { TerminalEnvVarInjector } from '../../features/terminal/terminalEnvVarInjector'; +import { PythonProjectManager } from '../../internal.api'; + +suite('TerminalEnvVarInjector - Integration Tests with Fixtures', () => { + let tempDir: string; + let mockGetConfiguration: sinon.SinonStub; + let mockEnvVarCollection: GlobalEnvironmentVariableCollection; + let envVarManager: PythonEnvVariableManager; + let injector: TerminalEnvVarInjector; + let scopedCollectionStubs: Map< + string, + { replace: sinon.SinonStub; delete: sinon.SinonStub; clear: sinon.SinonStub; get: sinon.SinonStub } + >; + + const fixturesPath = path.join(__dirname, '..', 'fixtures', 'terminalEnvVarInjector'); + + setup(async () => { + // Create a unique temp directory for this test run + tempDir = await fs.mkdtemp(path.join(os.tmpdir(), 'terminalEnvVarInjector-test-')); + + // Stub workspace configuration + mockGetConfiguration = sinon.stub(workspaceApis, 'getConfiguration'); + const mockConfig = { + get: sinon.stub(), + }; + mockConfig.get.withArgs('terminal.useEnvFile', false).returns(true); + mockConfig.get.withArgs('envFile').returns(undefined); + mockGetConfiguration.returns(mockConfig); + + // Track scoped collections per workspace + scopedCollectionStubs = new Map(); + + // Mock GlobalEnvironmentVariableCollection + mockEnvVarCollection = { + getScoped: (scope: { workspaceFolder: WorkspaceFolder }) => { + const key = scope.workspaceFolder.uri.fsPath; + if (!scopedCollectionStubs.has(key)) { + scopedCollectionStubs.set(key, { + replace: sinon.stub(), + delete: sinon.stub(), + clear: sinon.stub(), + get: sinon.stub(), + }); + } + return scopedCollectionStubs.get(key)!; + }, + clear: sinon.stub(), + } as unknown as GlobalEnvironmentVariableCollection; + + // Create PythonEnvVariableManager instance with mock project manager + const mockProjectManager = { + get: sinon.stub().returns(undefined), + }; + envVarManager = new PythonEnvVariableManager(mockProjectManager as unknown as PythonProjectManager); + }); + + teardown(async () => { + sinon.restore(); + injector?.dispose(); + + // Clean up temp directory + if (tempDir && (await fs.pathExists(tempDir))) { + await fs.remove(tempDir); + } + }); + + async function copyFixture(fixtureName: string, targetDir: string, targetName = '.env'): Promise { + const sourcePath = path.join(fixturesPath, fixtureName); + const targetPath = path.join(targetDir, targetName); + await fs.ensureDir(path.dirname(targetPath)); + await fs.copy(sourcePath, targetPath); + return targetPath; + } + + suite('File Existence Tests', () => { + test('should inject variables when .env file exists', async () => { + // Arrange - copy fixture to temp workspace + const workspaceDir = path.join(tempDir, 'workspace1'); + await copyFixture('basic.env', workspaceDir); + + const workspaceFolder: WorkspaceFolder = { + uri: Uri.file(workspaceDir), + name: 'workspace1', + index: 0, + }; + + injector = new TerminalEnvVarInjector(mockEnvVarCollection, envVarManager); + + // Act - trigger injection via the public initialize flow + envVarManager['_onDidChangeEnvironmentVariables'].fire({ + uri: workspaceFolder.uri, + changeType: FileChangeType.Changed, + }); + + // Wait for async operations + await new Promise((resolve) => setTimeout(resolve, 100)); + + // Assert + const stubs = scopedCollectionStubs.get(workspaceDir); + assert.ok(stubs, 'Should have created scoped collection for workspace'); + assert.ok(stubs!.replace.calledWith('FOO', 'bar'), 'Should inject FOO'); + assert.ok(stubs!.replace.calledWith('BAR', 'baz'), 'Should inject BAR'); + assert.ok(stubs!.replace.calledWith('BAZ', 'qux'), 'Should inject BAZ'); + }); + + test('should not inject when .env file does not exist', async () => { + // Arrange - workspace without .env file + const workspaceDir = path.join(tempDir, 'no-env-workspace'); + await fs.ensureDir(workspaceDir); + + const workspaceFolder: WorkspaceFolder = { + uri: Uri.file(workspaceDir), + name: 'no-env-workspace', + index: 0, + }; + + injector = new TerminalEnvVarInjector(mockEnvVarCollection, envVarManager); + + // Act + envVarManager['_onDidChangeEnvironmentVariables'].fire({ + uri: workspaceFolder.uri, + changeType: FileChangeType.Changed, + }); + + await new Promise((resolve) => setTimeout(resolve, 100)); + + // Assert + const stubs = scopedCollectionStubs.get(workspaceDir); + if (stubs) { + assert.strictEqual(stubs.replace.called, false, 'Should not inject any variables'); + } + }); + + test('should use custom env file path when configured', async () => { + // Arrange - copy custom fixture + const workspaceDir = path.join(tempDir, 'custom-path-workspace'); + const customPath = path.join(workspaceDir, 'config', '.env.custom'); + await fs.ensureDir(path.dirname(customPath)); + await fs.copy(path.join(fixturesPath, 'custom-path', '.env.custom'), customPath); + + // Configure custom path + const mockConfig = mockGetConfiguration.returnValues[0]; + mockConfig.get.withArgs('envFile').returns(customPath); + + const workspaceFolder: WorkspaceFolder = { + uri: Uri.file(workspaceDir), + name: 'custom-path-workspace', + index: 0, + }; + + injector = new TerminalEnvVarInjector(mockEnvVarCollection, envVarManager); + + // Act + envVarManager['_onDidChangeEnvironmentVariables'].fire({ + uri: workspaceFolder.uri, + changeType: FileChangeType.Changed, + }); + + await new Promise((resolve) => setTimeout(resolve, 100)); + + // Assert + const stubs = scopedCollectionStubs.get(workspaceDir); + assert.ok(stubs?.replace.calledWith('CUSTOM_VAR', 'custom_value'), 'Should inject from custom file'); + }); + }); + + suite('Configuration Changes', () => { + test('should cleanup tracked vars when useEnvFile is disabled after being enabled', async () => { + // Arrange - Start with env file and injection enabled + const workspaceDir = path.join(tempDir, 'disable-workspace'); + await copyFixture('basic.env', workspaceDir); + + const workspaceFolder: WorkspaceFolder = { + uri: Uri.file(workspaceDir), + name: 'disable-workspace', + index: 0, + }; + + injector = new TerminalEnvVarInjector(mockEnvVarCollection, envVarManager); + + // Initial injection + envVarManager['_onDidChangeEnvironmentVariables'].fire({ + uri: workspaceFolder.uri, + changeType: FileChangeType.Changed, + }); + await new Promise((resolve) => setTimeout(resolve, 100)); + + const stubs = scopedCollectionStubs.get(workspaceDir)!; + assert.ok(stubs.replace.calledWith('FOO', 'bar'), 'Initial FOO should be set'); + + // Act - Disable useEnvFile + const mockConfig = mockGetConfiguration.returnValues[0]; + mockConfig.get.withArgs('terminal.useEnvFile', false).returns(false); + + stubs.replace.resetHistory(); + stubs.delete.resetHistory(); + + envVarManager['_onDidChangeEnvironmentVariables'].fire({ + uri: workspaceFolder.uri, + changeType: FileChangeType.Changed, + }); + await new Promise((resolve) => setTimeout(resolve, 100)); + + // Assert - Should cleanup previously tracked variables + assert.ok(stubs.delete.calledWith('FOO'), 'Should delete FOO'); + assert.ok(stubs.delete.calledWith('BAR'), 'Should delete BAR'); + assert.ok(stubs.delete.calledWith('BAZ'), 'Should delete BAZ'); + }); + + test('should cleanup tracked vars when .env file is deleted', async () => { + // Arrange - Start with env file + const workspaceDir = path.join(tempDir, 'delete-file-workspace'); + const envFilePath = await copyFixture('single-var.env', workspaceDir); + + const workspaceFolder: WorkspaceFolder = { + uri: Uri.file(workspaceDir), + name: 'delete-file-workspace', + index: 0, + }; + + injector = new TerminalEnvVarInjector(mockEnvVarCollection, envVarManager); + + // Initial injection + envVarManager['_onDidChangeEnvironmentVariables'].fire({ + uri: workspaceFolder.uri, + changeType: FileChangeType.Changed, + }); + await new Promise((resolve) => setTimeout(resolve, 100)); + + const stubs = scopedCollectionStubs.get(workspaceDir)!; + assert.ok(stubs.replace.calledWith('FOO', 'bar'), 'Initial FOO should be set'); + + // Act - Delete the .env file + await fs.remove(envFilePath); + stubs.delete.resetHistory(); + + envVarManager['_onDidChangeEnvironmentVariables'].fire({ + uri: workspaceFolder.uri, + changeType: FileChangeType.Deleted, + }); + await new Promise((resolve) => setTimeout(resolve, 100)); + + // Assert - Should cleanup + assert.ok(stubs.delete.calledWith('FOO'), 'Should delete FOO after file deletion'); + }); + }); + + suite('File Modification Scenarios', () => { + test('Scenario: Commenting out a variable removes it from terminals', async () => { + // Arrange - Start with basic.env + const workspaceDir = path.join(tempDir, 'comment-workspace'); + const envFilePath = await copyFixture('basic.env', workspaceDir); + + const workspaceFolder: WorkspaceFolder = { + uri: Uri.file(workspaceDir), + name: 'comment-workspace', + index: 0, + }; + + injector = new TerminalEnvVarInjector(mockEnvVarCollection, envVarManager); + + // Initial injection + envVarManager['_onDidChangeEnvironmentVariables'].fire({ + uri: workspaceFolder.uri, + changeType: FileChangeType.Changed, + }); + await new Promise((resolve) => setTimeout(resolve, 100)); + + const stubs = scopedCollectionStubs.get(workspaceDir)!; + assert.ok(stubs.replace.calledWith('BAR', 'baz'), 'BAR should be initially set'); + + // Act - Comment out BAR in the file + await fs.writeFile(envFilePath, '# Basic .env file\nFOO=bar\n# BAR=baz\nBAZ=qux\n'); + + stubs.replace.resetHistory(); + stubs.delete.resetHistory(); + + envVarManager['_onDidChangeEnvironmentVariables'].fire({ + uri: workspaceFolder.uri, + changeType: FileChangeType.Changed, + }); + await new Promise((resolve) => setTimeout(resolve, 100)); + + // Assert - BAR should be deleted + assert.ok(stubs.delete.calledWith('BAR'), 'Should delete commented out BAR'); + assert.ok(stubs.replace.calledWith('FOO', 'bar'), 'Should keep FOO'); + assert.ok(stubs.replace.calledWith('BAZ', 'qux'), 'Should keep BAZ'); + }); + + test('Scenario: Adding a new variable injects it', async () => { + // Arrange - Start with single var + const workspaceDir = path.join(tempDir, 'add-var-workspace'); + const envFilePath = await copyFixture('single-var.env', workspaceDir); + + const workspaceFolder: WorkspaceFolder = { + uri: Uri.file(workspaceDir), + name: 'add-var-workspace', + index: 0, + }; + + injector = new TerminalEnvVarInjector(mockEnvVarCollection, envVarManager); + + // Initial injection + envVarManager['_onDidChangeEnvironmentVariables'].fire({ + uri: workspaceFolder.uri, + changeType: FileChangeType.Changed, + }); + await new Promise((resolve) => setTimeout(resolve, 100)); + + const stubs = scopedCollectionStubs.get(workspaceDir)!; + + // Act - Add NEW_VAR to file + await fs.appendFile(envFilePath, 'NEW_VAR=new_value\n'); + + stubs.replace.resetHistory(); + + envVarManager['_onDidChangeEnvironmentVariables'].fire({ + uri: workspaceFolder.uri, + changeType: FileChangeType.Changed, + }); + await new Promise((resolve) => setTimeout(resolve, 100)); + + // Assert + assert.ok(stubs.replace.calledWith('NEW_VAR', 'new_value'), 'Should add NEW_VAR'); + assert.ok(stubs.replace.calledWith('FOO', 'bar'), 'Should keep FOO'); + }); + + test('Scenario: Unsetting a variable (VAR=) removes it', async () => { + // Arrange - Start with variables + const workspaceDir = path.join(tempDir, 'unset-workspace'); + const envFilePath = await copyFixture('with-unset.env', workspaceDir); + + const workspaceFolder: WorkspaceFolder = { + uri: Uri.file(workspaceDir), + name: 'unset-workspace', + index: 0, + }; + + injector = new TerminalEnvVarInjector(mockEnvVarCollection, envVarManager); + + // Initial injection + envVarManager['_onDidChangeEnvironmentVariables'].fire({ + uri: workspaceFolder.uri, + changeType: FileChangeType.Changed, + }); + await new Promise((resolve) => setTimeout(resolve, 100)); + + const stubs = scopedCollectionStubs.get(workspaceDir)!; + assert.ok(stubs.replace.calledWith('TO_UNSET', 'value'), 'TO_UNSET should be initially set'); + + // Act - Unset TO_UNSET (VAR=) + await fs.writeFile(envFilePath, 'FOO=bar\nTO_UNSET=\n'); + + stubs.delete.resetHistory(); + + envVarManager['_onDidChangeEnvironmentVariables'].fire({ + uri: workspaceFolder.uri, + changeType: FileChangeType.Changed, + }); + await new Promise((resolve) => setTimeout(resolve, 100)); + + // Assert + assert.ok(stubs.delete.calledWith('TO_UNSET'), 'Should delete unset variable'); + }); + }); + + suite('Multi-Workspace Scenarios', () => { + test('Scenario: Multiple workspaces maintain independent tracking', async () => { + // Arrange - Two workspaces with different env files + const workspace1Dir = path.join(tempDir, 'workspace1'); + const workspace2Dir = path.join(tempDir, 'workspace2'); + + await copyFixture('workspace1/.env', workspace1Dir); + await copyFixture('workspace2/.env', workspace2Dir); + + const workspaceFolder1: WorkspaceFolder = { + uri: Uri.file(workspace1Dir), + name: 'workspace1', + index: 0, + }; + + const workspaceFolder2: WorkspaceFolder = { + uri: Uri.file(workspace2Dir), + name: 'workspace2', + index: 1, + }; + + injector = new TerminalEnvVarInjector(mockEnvVarCollection, envVarManager); + + // Act - Inject for both workspaces + envVarManager['_onDidChangeEnvironmentVariables'].fire({ + uri: workspaceFolder1.uri, + changeType: FileChangeType.Changed, + }); + envVarManager['_onDidChangeEnvironmentVariables'].fire({ + uri: workspaceFolder2.uri, + changeType: FileChangeType.Changed, + }); + await new Promise((resolve) => setTimeout(resolve, 100)); + + // Assert - Each workspace should have its own variables + const stubs1 = scopedCollectionStubs.get(workspace1Dir)!; + const stubs2 = scopedCollectionStubs.get(workspace2Dir)!; + + assert.ok(stubs1.replace.calledWith('WS1_VAR', 'workspace1_value'), 'Workspace 1 should have WS1_VAR'); + assert.strictEqual(stubs1.replace.calledWith('WS2_VAR'), false, 'Workspace 1 should not have WS2_VAR'); + + assert.ok(stubs2.replace.calledWith('WS2_VAR', 'workspace2_value'), 'Workspace 2 should have WS2_VAR'); + assert.strictEqual(stubs2.replace.calledWith('WS1_VAR'), false, 'Workspace 2 should not have WS1_VAR'); + }); + }); +}); diff --git a/src/test/features/terminalEnvVarInjectorBasic.unit.test.ts b/src/test/features/terminalEnvVarInjectorBasic.unit.test.ts index fc69844f..2cfcca71 100644 --- a/src/test/features/terminalEnvVarInjectorBasic.unit.test.ts +++ b/src/test/features/terminalEnvVarInjectorBasic.unit.test.ts @@ -1,30 +1,60 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. +import * as assert from 'assert'; import * as sinon from 'sinon'; import * as typeMoq from 'typemoq'; -import { GlobalEnvironmentVariableCollection, workspace } from 'vscode'; +import { + Disposable, + EnvironmentVariableCollection, + GlobalEnvironmentVariableCollection, + Uri, + workspace, + WorkspaceFolder, +} from 'vscode'; import { EnvVarManager } from '../../features/execution/envVariableManager'; import { TerminalEnvVarInjector } from '../../features/terminal/terminalEnvVarInjector'; -interface MockScopedCollection { - clear: sinon.SinonStub; - replace: sinon.SinonStub; - delete: sinon.SinonStub; +/** + * Test interface to access private methods for unit testing. + * This is preferable to using 'as any' and maintains type safety. + */ +interface TerminalEnvVarInjectorTestable { + applyEnvVarChanges( + envVarScope: EnvironmentVariableCollection, + envVars: { [key: string]: string | undefined }, + workspaceKey: string, + ): void; + cleanupTrackedVars(envVarScope: EnvironmentVariableCollection, workspaceKey: string): void; + clearWorkspaceVariables(workspaceFolder: WorkspaceFolder): void; + trackedEnvVars: Map>; + dispose(): void; } -suite('TerminalEnvVarInjector Basic Tests', () => { +suite('TerminalEnvVarInjector - Core Functionality', () => { let envVarCollection: typeMoq.IMock; let envVarManager: typeMoq.IMock; let injector: TerminalEnvVarInjector; - let mockScopedCollection: MockScopedCollection; - // eslint-disable-next-line @typescript-eslint/no-explicit-any - let workspaceFoldersStub: any; + let testableInjector: TerminalEnvVarInjectorTestable; + let mockScopedCollection: EnvironmentVariableCollection; + let clearStub: sinon.SinonStub; + let replaceStub: sinon.SinonStub; + let deleteStub: sinon.SinonStub; + let getStub: sinon.SinonStub; + let mockWorkspaceFolder: WorkspaceFolder; + let workspaceFoldersStub: WorkspaceFolder[]; setup(() => { envVarCollection = typeMoq.Mock.ofType(); envVarManager = typeMoq.Mock.ofType(); + // Create mock workspace folder + mockWorkspaceFolder = { + uri: Uri.file('/test/workspace'), + name: 'test-workspace', + index: 0, + }; + // Mock workspace.workspaceFolders property workspaceFoldersStub = []; Object.defineProperty(workspace, 'workspaceFolders', { @@ -32,31 +62,26 @@ suite('TerminalEnvVarInjector Basic Tests', () => { configurable: true, }); - // Setup scoped collection mock + // Setup scoped collection mock with sinon stubs + clearStub = sinon.stub(); + replaceStub = sinon.stub(); + deleteStub = sinon.stub(); + getStub = sinon.stub(); + mockScopedCollection = { - clear: sinon.stub(), - replace: sinon.stub(), - delete: sinon.stub(), - }; + clear: clearStub, + replace: replaceStub, + delete: deleteStub, + get: getStub, + } as unknown as EnvironmentVariableCollection; // Setup environment variable collection to return scoped collection - envVarCollection - .setup((x) => x.getScoped(typeMoq.It.isAny())) - .returns( - () => mockScopedCollection as unknown as ReturnType, - ); + envVarCollection.setup((x) => x.getScoped(typeMoq.It.isAny())).returns(() => mockScopedCollection); envVarCollection.setup((x) => x.clear()).returns(() => {}); - // Setup minimal mocks for event subscriptions - envVarManager - .setup((m) => m.onDidChangeEnvironmentVariables) - .returns( - () => - ({ - dispose: () => {}, - // eslint-disable-next-line @typescript-eslint/no-explicit-any - } as any), - ); + // Setup minimal mocks for event subscriptions - return a function matching Event signature + const mockDisposable: Disposable = { dispose: () => {} }; + envVarManager.setup((m) => m.onDidChangeEnvironmentVariables).returns(() => () => mockDisposable); }); teardown(() => { @@ -64,41 +89,245 @@ suite('TerminalEnvVarInjector Basic Tests', () => { injector?.dispose(); }); - test('should initialize without errors', () => { - // Arrange & Act - injector = new TerminalEnvVarInjector(envVarCollection.object, envVarManager.object); + suite('applyEnvVarChanges', () => { + setup(() => { + injector = new TerminalEnvVarInjector(envVarCollection.object, envVarManager.object); + testableInjector = injector as unknown as TerminalEnvVarInjectorTestable; + }); + + test('should add new variables from .env file', () => { + // Mock - new variables + const envVars = { + FOO: 'bar', + BAZ: 'qux', + }; + + // Run - access private method via test interface + testableInjector.applyEnvVarChanges(mockScopedCollection, envVars, '/test/workspace'); + + // Assert - should call replace for each variable + assert.ok(replaceStub.calledWith('FOO', 'bar'), 'Should set FOO'); + assert.ok(replaceStub.calledWith('BAZ', 'qux'), 'Should set BAZ'); + assert.strictEqual(replaceStub.callCount, 2, 'Should set exactly 2 variables'); + }); + + test('should update existing variables', () => { + // Mock - First set initial variables + const initialVars = { FOO: 'initial' }; + testableInjector.applyEnvVarChanges(mockScopedCollection, initialVars, '/test/workspace'); + + replaceStub.resetHistory(); + + // Now update + const updatedVars = { FOO: 'updated' }; + + // Run + testableInjector.applyEnvVarChanges(mockScopedCollection, updatedVars, '/test/workspace'); + + // Assert - should replace with new value + assert.ok(replaceStub.calledWith('FOO', 'updated'), 'Should update FOO to new value'); + }); + + test('should delete variables with empty values', () => { + // Mock - variable set to empty + const envVars = { + FOO: 'bar', + EMPTY: '', + UNDEFINED: undefined, + }; + + // Run + testableInjector.applyEnvVarChanges(mockScopedCollection, envVars, '/test/workspace'); + + // Assert - should delete empty/undefined values + assert.ok(deleteStub.calledWith('EMPTY'), 'Should delete EMPTY'); + assert.ok(deleteStub.calledWith('UNDEFINED'), 'Should delete UNDEFINED'); + assert.ok(replaceStub.calledWith('FOO', 'bar'), 'Should set FOO'); + }); + + test('should remove previously tracked vars no longer in .env (commented out scenario)', () => { + // Mock - First set: FOO, BAR, BAZ + const initialVars = { + FOO: 'bar', + BAR: 'baz', + BAZ: 'qux', + }; + testableInjector.applyEnvVarChanges(mockScopedCollection, initialVars, '/test/workspace'); + + replaceStub.resetHistory(); + deleteStub.resetHistory(); + + // Now update - BAR is commented out (removed) + const updatedVars = { + FOO: 'bar', + BAZ: 'qux', + // BAR is missing (commented out) + }; + + // Run + testableInjector.applyEnvVarChanges(mockScopedCollection, updatedVars, '/test/workspace'); + + // Assert - should delete BAR + assert.ok(deleteStub.calledWith('BAR'), 'Should delete commented out variable BAR'); + assert.ok(replaceStub.calledWith('FOO', 'bar'), 'Should keep FOO'); + assert.ok(replaceStub.calledWith('BAZ', 'qux'), 'Should keep BAZ'); + }); + + test('should handle first-time initialization with no previous keys', () => { + // Mock - brand new workspace + const envVars = { + NEW_VAR: 'value', + }; + + // Run - apply to workspace that has no tracked vars yet + testableInjector.applyEnvVarChanges(mockScopedCollection, envVars, '/new/workspace'); + + // Assert - should just add the new variable + assert.ok(replaceStub.calledWith('NEW_VAR', 'value'), 'Should set NEW_VAR'); + assert.strictEqual(deleteStub.called, false, 'Should not delete anything'); + }); + + test('should update tracking map correctly', () => { + // Mock - set some variables + const envVars = { + TRACKED_1: 'value1', + TRACKED_2: 'value2', + }; + + // Run + testableInjector.applyEnvVarChanges(mockScopedCollection, envVars, '/test/workspace'); + + // Assert - verify tracking map contains the keys + const trackedVars = testableInjector.trackedEnvVars.get('/test/workspace'); + assert.ok(trackedVars, 'Should have tracking entry for workspace'); + assert.ok(trackedVars.has('TRACKED_1'), 'Should track TRACKED_1'); + assert.ok(trackedVars.has('TRACKED_2'), 'Should track TRACKED_2'); + assert.strictEqual(trackedVars.size, 2, 'Should track exactly 2 variables'); + }); + }); + + suite('cleanupTrackedVars', () => { + setup(() => { + injector = new TerminalEnvVarInjector(envVarCollection.object, envVarManager.object); + }); + + test('should delete all tracked variables', () => { + // Mock - Set up some tracked variables + const envVars = { + VAR1: 'value1', + VAR2: 'value2', + VAR3: 'value3', + }; + testableInjector.applyEnvVarChanges(mockScopedCollection, envVars, '/test/workspace'); + + deleteStub.resetHistory(); + + // Run - cleanup + testableInjector.cleanupTrackedVars(mockScopedCollection, '/test/workspace'); + + // Assert - should delete all tracked variables + assert.strictEqual(deleteStub.callCount, 3, 'Should delete all 3 variables'); + assert.ok(deleteStub.calledWith('VAR1'), 'Should delete VAR1'); + assert.ok(deleteStub.calledWith('VAR2'), 'Should delete VAR2'); + assert.ok(deleteStub.calledWith('VAR3'), 'Should delete VAR3'); + }); + + test('should remove workspace from tracking map', () => { + // Mock - Set up tracked variables + const envVars = { TEST: 'value' }; + testableInjector.applyEnvVarChanges(mockScopedCollection, envVars, '/test/workspace'); - // Assert - should not throw - sinon.assert.match(injector, sinon.match.object); + // Run - cleanup + testableInjector.cleanupTrackedVars(mockScopedCollection, '/test/workspace'); + + // Assert - tracking should be removed + const trackedVars = testableInjector.trackedEnvVars.get('/test/workspace'); + assert.strictEqual(trackedVars, undefined, 'Should remove workspace from tracking map'); + }); + + test('should handle case when no tracked vars exist (no-op)', () => { + // Run - cleanup workspace with no tracked vars + testableInjector.cleanupTrackedVars(mockScopedCollection, '/nonexistent/workspace'); + + // Assert - should not throw and not delete anything + assert.strictEqual(deleteStub.called, false, 'Should not delete anything'); + }); }); - test('should dispose cleanly', () => { - // Arrange - injector = new TerminalEnvVarInjector(envVarCollection.object, envVarManager.object); + suite('clearWorkspaceVariables', () => { + setup(() => { + injector = new TerminalEnvVarInjector(envVarCollection.object, envVarManager.object); + }); + + test('should only delete tracked variables', () => { + // Mock - Set up tracked variables + const envVars = { + MY_VAR: 'value', + ANOTHER_VAR: 'value2', + }; + testableInjector.applyEnvVarChanges(mockScopedCollection, envVars, '/test/workspace'); + + deleteStub.resetHistory(); - // Act - injector.dispose(); + // Run - clear workspace variables + testableInjector.clearWorkspaceVariables(mockWorkspaceFolder); - // Assert - should clear on dispose - envVarCollection.verify((c) => c.clear(), typeMoq.Times.atLeastOnce()); + // Assert - should only delete our tracked variables + assert.strictEqual(deleteStub.callCount, 2, 'Should delete exactly 2 variables'); + assert.ok(deleteStub.calledWith('MY_VAR'), 'Should delete MY_VAR'); + assert.ok(deleteStub.calledWith('ANOTHER_VAR'), 'Should delete ANOTHER_VAR'); + }); + + test('should not delete non-tracked variables like BASH_ENV', () => { + // Mock - Set up only one tracked variable + const envVars = { MY_VAR: 'value' }; + testableInjector.applyEnvVarChanges(mockScopedCollection, envVars, '/test/workspace'); + + // Simulate BASH_ENV being set by another manager (not tracked by us) + getStub.withArgs('BASH_ENV').returns({ value: 'some_bash_command' }); + + deleteStub.resetHistory(); + + // Run + testableInjector.clearWorkspaceVariables(mockWorkspaceFolder); + + // Assert - should only delete MY_VAR, not BASH_ENV + assert.strictEqual(deleteStub.callCount, 1, 'Should delete only tracked variable'); + assert.ok(deleteStub.calledWith('MY_VAR'), 'Should delete MY_VAR'); + assert.strictEqual(deleteStub.calledWith('BASH_ENV'), false, 'Should not delete BASH_ENV'); + }); + + test('should handle errors gracefully', () => { + // Mock - Make delete throw an error + deleteStub.throws(new Error('Collection error')); + + // Run - should not throw + assert.doesNotThrow(() => testableInjector.clearWorkspaceVariables(mockWorkspaceFolder)); + }); }); - test('should register environment variable change event handler', () => { - // Arrange - let eventHandlerRegistered = false; - envVarManager.reset(); - envVarManager - .setup((m) => m.onDidChangeEnvironmentVariables) - .returns((_handler) => { - eventHandlerRegistered = true; - // eslint-disable-next-line @typescript-eslint/no-explicit-any - return { dispose: () => {} } as any; - }); - - // Act - injector = new TerminalEnvVarInjector(envVarCollection.object, envVarManager.object); - - // Assert - sinon.assert.match(eventHandlerRegistered, true); + suite('Basic Tests', () => { + test('should initialize without errors', () => { + // Arrange & Act + injector = new TerminalEnvVarInjector(envVarCollection.object, envVarManager.object); + + // Assert - should not throw + sinon.assert.match(injector, sinon.match.object); + }); + + test('should register environment variable change event handler', () => { + // Arrange + let eventHandlerRegistered = false; + envVarManager.reset(); + // Setup minimal mocks for event subscriptions - return a function matching Event signature + const mockDisposable: Disposable = { dispose: () => {} }; + envVarManager.setup((m) => m.onDidChangeEnvironmentVariables).returns(() => () => mockDisposable); + + // Act + injector = new TerminalEnvVarInjector(envVarCollection.object, envVarManager.object); + + // Assert + sinon.assert.match(eventHandlerRegistered, true); + }); }); }); diff --git a/src/test/fixtures/terminalEnvVarInjector/basic.env b/src/test/fixtures/terminalEnvVarInjector/basic.env new file mode 100644 index 00000000..f60b94ce --- /dev/null +++ b/src/test/fixtures/terminalEnvVarInjector/basic.env @@ -0,0 +1,4 @@ +# Basic .env file with three variables +FOO=bar +BAR=baz +BAZ=qux diff --git a/src/test/fixtures/terminalEnvVarInjector/commented-out.env b/src/test/fixtures/terminalEnvVarInjector/commented-out.env new file mode 100644 index 00000000..52e18c96 --- /dev/null +++ b/src/test/fixtures/terminalEnvVarInjector/commented-out.env @@ -0,0 +1,4 @@ +# File with some variables commented out +FOO=bar +# BAR=baz +BAZ=qux diff --git a/src/test/fixtures/terminalEnvVarInjector/custom-path/.env.custom b/src/test/fixtures/terminalEnvVarInjector/custom-path/.env.custom new file mode 100644 index 00000000..ff06271f --- /dev/null +++ b/src/test/fixtures/terminalEnvVarInjector/custom-path/.env.custom @@ -0,0 +1,2 @@ +# Custom path .env file +CUSTOM_VAR=custom_value diff --git a/src/test/fixtures/terminalEnvVarInjector/empty.env b/src/test/fixtures/terminalEnvVarInjector/empty.env new file mode 100644 index 00000000..0fc89653 --- /dev/null +++ b/src/test/fixtures/terminalEnvVarInjector/empty.env @@ -0,0 +1 @@ +# Empty .env file diff --git a/src/test/fixtures/terminalEnvVarInjector/single-var.env b/src/test/fixtures/terminalEnvVarInjector/single-var.env new file mode 100644 index 00000000..a6d2855d --- /dev/null +++ b/src/test/fixtures/terminalEnvVarInjector/single-var.env @@ -0,0 +1,2 @@ +# Single variable +FOO=bar diff --git a/src/test/fixtures/terminalEnvVarInjector/with-unset.env b/src/test/fixtures/terminalEnvVarInjector/with-unset.env new file mode 100644 index 00000000..d1f0d4d4 --- /dev/null +++ b/src/test/fixtures/terminalEnvVarInjector/with-unset.env @@ -0,0 +1,3 @@ +# Variables with one to be unset +FOO=bar +TO_UNSET=value diff --git a/src/test/fixtures/terminalEnvVarInjector/workspace1/.env b/src/test/fixtures/terminalEnvVarInjector/workspace1/.env new file mode 100644 index 00000000..2f82da8a --- /dev/null +++ b/src/test/fixtures/terminalEnvVarInjector/workspace1/.env @@ -0,0 +1,2 @@ +# Workspace 1 .env file +WS1_VAR=workspace1_value diff --git a/src/test/fixtures/terminalEnvVarInjector/workspace2/.env b/src/test/fixtures/terminalEnvVarInjector/workspace2/.env new file mode 100644 index 00000000..c5a77b54 --- /dev/null +++ b/src/test/fixtures/terminalEnvVarInjector/workspace2/.env @@ -0,0 +1,2 @@ +# Workspace 2 .env file +WS2_VAR=workspace2_value From 6657fe9a865a44af7416b52bbeb40957b9b98e27 Mon Sep 17 00:00:00 2001 From: eleanorjboyd <26030610+eleanorjboyd@users.noreply.github.com> Date: Tue, 9 Dec 2025 10:52:41 -0800 Subject: [PATCH 2/8] fix test --- src/features/terminal/terminalEnvVarInjector.ts | 6 +++++- .../terminalEnvVarInjectorBasic.unit.test.ts | 16 +++++----------- 2 files changed, 10 insertions(+), 12 deletions(-) diff --git a/src/features/terminal/terminalEnvVarInjector.ts b/src/features/terminal/terminalEnvVarInjector.ts index ffa27e2a..ef0ad949 100644 --- a/src/features/terminal/terminalEnvVarInjector.ts +++ b/src/features/terminal/terminalEnvVarInjector.ts @@ -257,7 +257,11 @@ export class TerminalEnvVarInjector implements Disposable { */ dispose(): void { traceVerbose('TerminalEnvVarInjector: Disposing'); - this.disposables.forEach((disposable) => disposable.dispose()); + this.disposables.forEach((disposable) => { + if (disposable) { + disposable.dispose(); + } + }); this.disposables = []; // Clear only the environment variables that we've set, preserving others (e.g., BASH_ENV) diff --git a/src/test/features/terminalEnvVarInjectorBasic.unit.test.ts b/src/test/features/terminalEnvVarInjectorBasic.unit.test.ts index 2cfcca71..fc3edbea 100644 --- a/src/test/features/terminalEnvVarInjectorBasic.unit.test.ts +++ b/src/test/features/terminalEnvVarInjectorBasic.unit.test.ts @@ -79,9 +79,9 @@ suite('TerminalEnvVarInjector - Core Functionality', () => { envVarCollection.setup((x) => x.getScoped(typeMoq.It.isAny())).returns(() => mockScopedCollection); envVarCollection.setup((x) => x.clear()).returns(() => {}); - // Setup minimal mocks for event subscriptions - return a function matching Event signature + // Setup minimal mocks for event subscriptions - return disposable when handler is registered const mockDisposable: Disposable = { dispose: () => {} }; - envVarManager.setup((m) => m.onDidChangeEnvironmentVariables).returns(() => () => mockDisposable); + envVarManager.setup((m) => m.onDidChangeEnvironmentVariables(typeMoq.It.isAny())).returns(() => mockDisposable); }); teardown(() => { @@ -316,18 +316,12 @@ suite('TerminalEnvVarInjector - Core Functionality', () => { }); test('should register environment variable change event handler', () => { - // Arrange - let eventHandlerRegistered = false; - envVarManager.reset(); - // Setup minimal mocks for event subscriptions - return a function matching Event signature - const mockDisposable: Disposable = { dispose: () => {} }; - envVarManager.setup((m) => m.onDidChangeEnvironmentVariables).returns(() => () => mockDisposable); - + // Arrange - Use the global setup's mock configuration // Act injector = new TerminalEnvVarInjector(envVarCollection.object, envVarManager.object); - // Assert - sinon.assert.match(eventHandlerRegistered, true); + // Assert - Verify that the mock's setup was used (handler was registered) + envVarManager.verify((m) => m.onDidChangeEnvironmentVariables(typeMoq.It.isAny()), typeMoq.Times.once()); }); }); }); From 16ee1ad17cc19a2ce1ca9b8ceb8159bcd94dd92b Mon Sep 17 00:00:00 2001 From: eleanorjboyd <26030610+eleanorjboyd@users.noreply.github.com> Date: Tue, 9 Dec 2025 11:00:11 -0800 Subject: [PATCH 3/8] updates --- .github/instructions/testing-workflow.instructions.md | 2 ++ src/test/features/terminalEnvVarInjectorBasic.unit.test.ts | 5 +++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/.github/instructions/testing-workflow.instructions.md b/.github/instructions/testing-workflow.instructions.md index 8cb8b1d3..3b297a3f 100644 --- a/.github/instructions/testing-workflow.instructions.md +++ b/.github/instructions/testing-workflow.instructions.md @@ -581,3 +581,5 @@ envConfig.inspect - Use `as unknown as TargetType` for type casting instead of `as any` to maintain type safety and avoid 'any' violations - If tests frequently need private access consider that maybe methods should be protected, or public test utilities should exist for testing (1) - When making systematic changes across many similar locations, fix one instance completely first to validate the approach before applying the pattern everywhere (1) +- Always recompile tests after making changes before running them, especially when changing imports or type definitions (1) +- When using paths as Map keys for tracking, you MUST use Uri.fsPath consistently throughout the test - mixing hardcoded strings with Uri.fsPath causes key mismatches on Windows (1) diff --git a/src/test/features/terminalEnvVarInjectorBasic.unit.test.ts b/src/test/features/terminalEnvVarInjectorBasic.unit.test.ts index fc3edbea..78d9f378 100644 --- a/src/test/features/terminalEnvVarInjectorBasic.unit.test.ts +++ b/src/test/features/terminalEnvVarInjectorBasic.unit.test.ts @@ -257,6 +257,7 @@ suite('TerminalEnvVarInjector - Core Functionality', () => { suite('clearWorkspaceVariables', () => { setup(() => { injector = new TerminalEnvVarInjector(envVarCollection.object, envVarManager.object); + testableInjector = injector as unknown as TerminalEnvVarInjectorTestable; }); test('should only delete tracked variables', () => { @@ -265,7 +266,7 @@ suite('TerminalEnvVarInjector - Core Functionality', () => { MY_VAR: 'value', ANOTHER_VAR: 'value2', }; - testableInjector.applyEnvVarChanges(mockScopedCollection, envVars, '/test/workspace'); + testableInjector.applyEnvVarChanges(mockScopedCollection, envVars, mockWorkspaceFolder.uri.fsPath); deleteStub.resetHistory(); @@ -281,7 +282,7 @@ suite('TerminalEnvVarInjector - Core Functionality', () => { test('should not delete non-tracked variables like BASH_ENV', () => { // Mock - Set up only one tracked variable const envVars = { MY_VAR: 'value' }; - testableInjector.applyEnvVarChanges(mockScopedCollection, envVars, '/test/workspace'); + testableInjector.applyEnvVarChanges(mockScopedCollection, envVars, mockWorkspaceFolder.uri.fsPath); // Simulate BASH_ENV being set by another manager (not tracked by us) getStub.withArgs('BASH_ENV').returns({ value: 'some_bash_command' }); From 78b89b628e2d39850075035a3b98cee3473dd5c1 Mon Sep 17 00:00:00 2001 From: Eleanor Boyd <26030610+eleanorjboyd@users.noreply.github.com> Date: Tue, 9 Dec 2025 11:09:39 -0800 Subject: [PATCH 4/8] Update src/features/terminal/terminalEnvVarInjector.ts Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- src/features/terminal/terminalEnvVarInjector.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/features/terminal/terminalEnvVarInjector.ts b/src/features/terminal/terminalEnvVarInjector.ts index ef0ad949..7b15a7a1 100644 --- a/src/features/terminal/terminalEnvVarInjector.ts +++ b/src/features/terminal/terminalEnvVarInjector.ts @@ -258,9 +258,7 @@ export class TerminalEnvVarInjector implements Disposable { dispose(): void { traceVerbose('TerminalEnvVarInjector: Disposing'); this.disposables.forEach((disposable) => { - if (disposable) { - disposable.dispose(); - } + disposable.dispose(); }); this.disposables = []; From 5b869061480635e70d634505ab575f204419eae8 Mon Sep 17 00:00:00 2001 From: eleanorjboyd <26030610+eleanorjboyd@users.noreply.github.com> Date: Tue, 9 Dec 2025 11:36:16 -0800 Subject: [PATCH 5/8] fix tests --- src/features/execution/envVariableManager.ts | 7 + .../terminal/terminalEnvVarInjector.ts | 8 +- .../features/terminalEnvVarInjector.test.ts | 130 +++++++++++------- .../terminalEnvVarInjectorBasic.unit.test.ts | 100 +++----------- 4 files changed, 110 insertions(+), 135 deletions(-) diff --git a/src/features/execution/envVariableManager.ts b/src/features/execution/envVariableManager.ts index d0317f24..9d6aa68f 100644 --- a/src/features/execution/envVariableManager.ts +++ b/src/features/execution/envVariableManager.ts @@ -77,6 +77,13 @@ export class PythonEnvVariableManager implements EnvVarManager { onDidChangeEnvironmentVariables: Event; + /** + * @internal For testing only - manually trigger environment variable change event + */ + triggerEnvironmentVariableChange(event: DidChangeEnvironmentVariablesEventArgs): void { + this._onDidChangeEnvironmentVariables.fire(event); + } + dispose(): void { this.disposables.forEach((disposable) => disposable.dispose()); } diff --git a/src/features/terminal/terminalEnvVarInjector.ts b/src/features/terminal/terminalEnvVarInjector.ts index 7b15a7a1..fdc5677d 100644 --- a/src/features/terminal/terminalEnvVarInjector.ts +++ b/src/features/terminal/terminalEnvVarInjector.ts @@ -30,7 +30,7 @@ export class TerminalEnvVarInjector implements Disposable { * variables that are removed/commented out in .env files. * Key: workspace fsPath, Value: Set of env var keys we've set for that workspace */ - private readonly trackedEnvVars: Map> = new Map(); + protected readonly trackedEnvVars: Map> = new Map(); constructor( private readonly envVarCollection: GlobalEnvironmentVariableCollection, @@ -208,7 +208,7 @@ export class TerminalEnvVarInjector implements Disposable { /** * Apply environment variable changes by comparing current and previous state. */ - private applyEnvVarChanges( + protected applyEnvVarChanges( envVarScope: EnvironmentVariableCollection, envVars: { [key: string]: string | undefined }, workspaceKey: string, @@ -244,7 +244,7 @@ export class TerminalEnvVarInjector implements Disposable { /** * Clean up previously tracked environment variables for a workspace. */ - private cleanupTrackedVars(envVarScope: EnvironmentVariableCollection, workspaceKey: string): void { + protected cleanupTrackedVars(envVarScope: EnvironmentVariableCollection, workspaceKey: string): void { const previousKeys = this.trackedEnvVars.get(workspaceKey); if (previousKeys) { previousKeys.forEach((key) => envVarScope.delete(key)); @@ -286,7 +286,7 @@ export class TerminalEnvVarInjector implements Disposable { /** * Clear all environment variables for a workspace. */ - private clearWorkspaceVariables(workspaceFolder: WorkspaceFolder): void { + protected clearWorkspaceVariables(workspaceFolder: WorkspaceFolder): void { const workspaceKey = workspaceFolder.uri.fsPath; try { const scope = this.getEnvironmentVariableCollectionScoped({ workspaceFolder }); diff --git a/src/test/features/terminalEnvVarInjector.test.ts b/src/test/features/terminalEnvVarInjector.test.ts index 7cdbf0dc..501140f2 100644 --- a/src/test/features/terminalEnvVarInjector.test.ts +++ b/src/test/features/terminalEnvVarInjector.test.ts @@ -83,6 +83,28 @@ suite('TerminalEnvVarInjector - Integration Tests with Fixtures', () => { return targetPath; } + /** + * Wait for a condition to be true, polling at regular intervals. + * @param condition Function that returns true when the condition is met + * @param timeoutMs Maximum time to wait in milliseconds + * @param pollIntervalMs How often to check the condition + */ + async function waitForCondition( + condition: () => boolean, + timeoutMs: number = 2000, + pollIntervalMs: number = 10, + ): Promise { + const startTime = Date.now(); + while (!condition()) { + if (Date.now() - startTime > timeoutMs) { + throw new Error(`Timeout waiting for condition after ${timeoutMs}ms`); + } + // Allow async operations to process + await new Promise((resolve) => setImmediate(resolve)); + await new Promise((resolve) => setTimeout(resolve, pollIntervalMs)); + } + } + suite('File Existence Tests', () => { test('should inject variables when .env file exists', async () => { // Arrange - copy fixture to temp workspace @@ -97,21 +119,24 @@ suite('TerminalEnvVarInjector - Integration Tests with Fixtures', () => { injector = new TerminalEnvVarInjector(mockEnvVarCollection, envVarManager); - // Act - trigger injection via the public initialize flow - envVarManager['_onDidChangeEnvironmentVariables'].fire({ + // Act - trigger injection via the test helper + envVarManager.triggerEnvironmentVariableChange({ uri: workspaceFolder.uri, changeType: FileChangeType.Changed, }); - // Wait for async operations - await new Promise((resolve) => setTimeout(resolve, 100)); + // Wait for async operations to complete + await new Promise((resolve) => setTimeout(resolve, 200)); // Assert const stubs = scopedCollectionStubs.get(workspaceDir); - assert.ok(stubs, 'Should have created scoped collection for workspace'); - assert.ok(stubs!.replace.calledWith('FOO', 'bar'), 'Should inject FOO'); - assert.ok(stubs!.replace.calledWith('BAR', 'baz'), 'Should inject BAR'); - assert.ok(stubs!.replace.calledWith('BAZ', 'qux'), 'Should inject BAZ'); + if (!stubs) { + assert.fail('Should have created scoped collection for workspace'); + return; + } + assert.ok(stubs.replace.calledWith('FOO', 'bar'), 'Should inject FOO'); + assert.ok(stubs.replace.calledWith('BAR', 'baz'), 'Should inject BAR'); + assert.ok(stubs.replace.calledWith('BAZ', 'qux'), 'Should inject BAZ'); }); test('should not inject when .env file does not exist', async () => { @@ -128,12 +153,13 @@ suite('TerminalEnvVarInjector - Integration Tests with Fixtures', () => { injector = new TerminalEnvVarInjector(mockEnvVarCollection, envVarManager); // Act - envVarManager['_onDidChangeEnvironmentVariables'].fire({ + envVarManager.triggerEnvironmentVariableChange({ uri: workspaceFolder.uri, changeType: FileChangeType.Changed, }); - await new Promise((resolve) => setTimeout(resolve, 100)); + // Wait a bit to ensure no injection happens (negative assertion) + await new Promise((resolve) => setTimeout(resolve, 50)); // Assert const stubs = scopedCollectionStubs.get(workspaceDir); @@ -162,15 +188,16 @@ suite('TerminalEnvVarInjector - Integration Tests with Fixtures', () => { injector = new TerminalEnvVarInjector(mockEnvVarCollection, envVarManager); // Act - envVarManager['_onDidChangeEnvironmentVariables'].fire({ + envVarManager.triggerEnvironmentVariableChange({ uri: workspaceFolder.uri, changeType: FileChangeType.Changed, }); - await new Promise((resolve) => setTimeout(resolve, 100)); + // Wait for async operations to complete + const stubs = scopedCollectionStubs.get(workspaceDir); + await waitForCondition(() => !!stubs && stubs.replace.called); // Assert - const stubs = scopedCollectionStubs.get(workspaceDir); assert.ok(stubs?.replace.calledWith('CUSTOM_VAR', 'custom_value'), 'Should inject from custom file'); }); }); @@ -190,27 +217,28 @@ suite('TerminalEnvVarInjector - Integration Tests with Fixtures', () => { injector = new TerminalEnvVarInjector(mockEnvVarCollection, envVarManager); // Initial injection - envVarManager['_onDidChangeEnvironmentVariables'].fire({ + envVarManager.triggerEnvironmentVariableChange({ uri: workspaceFolder.uri, changeType: FileChangeType.Changed, }); - await new Promise((resolve) => setTimeout(resolve, 100)); - const stubs = scopedCollectionStubs.get(workspaceDir)!; - assert.ok(stubs.replace.calledWith('FOO', 'bar'), 'Initial FOO should be set'); + await waitForCondition(() => stubs.replace.calledWith('FOO', 'bar')); - // Act - Disable useEnvFile - const mockConfig = mockGetConfiguration.returnValues[0]; - mockConfig.get.withArgs('terminal.useEnvFile', false).returns(false); + // Set up a new mock config with useEnvFile disabled for the next call + const disabledEnvFileConfig = { + get: sinon.stub().withArgs('terminal.useEnvFile', false).returns(false), + // Add any other methods/properties as needed by the code under test + }; + mockGetConfiguration.returns(disabledEnvFileConfig); stubs.replace.resetHistory(); stubs.delete.resetHistory(); - envVarManager['_onDidChangeEnvironmentVariables'].fire({ + envVarManager.triggerEnvironmentVariableChange({ uri: workspaceFolder.uri, changeType: FileChangeType.Changed, }); - await new Promise((resolve) => setTimeout(resolve, 100)); + await waitForCondition(() => stubs.delete.callCount >= 3); // Assert - Should cleanup previously tracked variables assert.ok(stubs.delete.calledWith('FOO'), 'Should delete FOO'); @@ -232,24 +260,24 @@ suite('TerminalEnvVarInjector - Integration Tests with Fixtures', () => { injector = new TerminalEnvVarInjector(mockEnvVarCollection, envVarManager); // Initial injection - envVarManager['_onDidChangeEnvironmentVariables'].fire({ + envVarManager.triggerEnvironmentVariableChange({ uri: workspaceFolder.uri, changeType: FileChangeType.Changed, }); - await new Promise((resolve) => setTimeout(resolve, 100)); - const stubs = scopedCollectionStubs.get(workspaceDir)!; + await waitForCondition(() => stubs.replace.calledWith('FOO', 'bar')); + assert.ok(stubs.replace.calledWith('FOO', 'bar'), 'Initial FOO should be set'); // Act - Delete the .env file await fs.remove(envFilePath); stubs.delete.resetHistory(); - envVarManager['_onDidChangeEnvironmentVariables'].fire({ + envVarManager.triggerEnvironmentVariableChange({ uri: workspaceFolder.uri, changeType: FileChangeType.Deleted, }); - await new Promise((resolve) => setTimeout(resolve, 100)); + await waitForCondition(() => stubs.delete.calledWith('FOO')); // Assert - Should cleanup assert.ok(stubs.delete.calledWith('FOO'), 'Should delete FOO after file deletion'); @@ -271,13 +299,14 @@ suite('TerminalEnvVarInjector - Integration Tests with Fixtures', () => { injector = new TerminalEnvVarInjector(mockEnvVarCollection, envVarManager); // Initial injection - envVarManager['_onDidChangeEnvironmentVariables'].fire({ + envVarManager.triggerEnvironmentVariableChange({ uri: workspaceFolder.uri, changeType: FileChangeType.Changed, }); - await new Promise((resolve) => setTimeout(resolve, 100)); + let stubs = scopedCollectionStubs.get(workspaceDir); + await waitForCondition(() => !!stubs && stubs!.replace.calledWith('BAR', 'baz')); + stubs = scopedCollectionStubs.get(workspaceDir)!; - const stubs = scopedCollectionStubs.get(workspaceDir)!; assert.ok(stubs.replace.calledWith('BAR', 'baz'), 'BAR should be initially set'); // Act - Comment out BAR in the file @@ -286,11 +315,11 @@ suite('TerminalEnvVarInjector - Integration Tests with Fixtures', () => { stubs.replace.resetHistory(); stubs.delete.resetHistory(); - envVarManager['_onDidChangeEnvironmentVariables'].fire({ + envVarManager.triggerEnvironmentVariableChange({ uri: workspaceFolder.uri, changeType: FileChangeType.Changed, }); - await new Promise((resolve) => setTimeout(resolve, 100)); + await waitForCondition(() => stubs.delete.calledWith('BAR')); // Assert - BAR should be deleted assert.ok(stubs.delete.calledWith('BAR'), 'Should delete commented out BAR'); @@ -312,24 +341,24 @@ suite('TerminalEnvVarInjector - Integration Tests with Fixtures', () => { injector = new TerminalEnvVarInjector(mockEnvVarCollection, envVarManager); // Initial injection - envVarManager['_onDidChangeEnvironmentVariables'].fire({ + envVarManager.triggerEnvironmentVariableChange({ uri: workspaceFolder.uri, changeType: FileChangeType.Changed, }); - await new Promise((resolve) => setTimeout(resolve, 100)); - - const stubs = scopedCollectionStubs.get(workspaceDir)!; + let stubs = scopedCollectionStubs.get(workspaceDir); + await waitForCondition(() => !!stubs && stubs!.replace.calledWith('FOO', 'bar')); + stubs = scopedCollectionStubs.get(workspaceDir)!; // Act - Add NEW_VAR to file await fs.appendFile(envFilePath, 'NEW_VAR=new_value\n'); stubs.replace.resetHistory(); - envVarManager['_onDidChangeEnvironmentVariables'].fire({ + envVarManager.triggerEnvironmentVariableChange({ uri: workspaceFolder.uri, changeType: FileChangeType.Changed, }); - await new Promise((resolve) => setTimeout(resolve, 100)); + await waitForCondition(() => stubs.replace.calledWith('NEW_VAR', 'new_value')); // Assert assert.ok(stubs.replace.calledWith('NEW_VAR', 'new_value'), 'Should add NEW_VAR'); @@ -350,13 +379,14 @@ suite('TerminalEnvVarInjector - Integration Tests with Fixtures', () => { injector = new TerminalEnvVarInjector(mockEnvVarCollection, envVarManager); // Initial injection - envVarManager['_onDidChangeEnvironmentVariables'].fire({ + envVarManager.triggerEnvironmentVariableChange({ uri: workspaceFolder.uri, changeType: FileChangeType.Changed, }); - await new Promise((resolve) => setTimeout(resolve, 100)); + let stubs = scopedCollectionStubs.get(workspaceDir); + await waitForCondition(() => !!stubs && stubs!.replace.calledWith('TO_UNSET', 'value')); + stubs = scopedCollectionStubs.get(workspaceDir)!; - const stubs = scopedCollectionStubs.get(workspaceDir)!; assert.ok(stubs.replace.calledWith('TO_UNSET', 'value'), 'TO_UNSET should be initially set'); // Act - Unset TO_UNSET (VAR=) @@ -364,11 +394,11 @@ suite('TerminalEnvVarInjector - Integration Tests with Fixtures', () => { stubs.delete.resetHistory(); - envVarManager['_onDidChangeEnvironmentVariables'].fire({ + envVarManager.triggerEnvironmentVariableChange({ uri: workspaceFolder.uri, changeType: FileChangeType.Changed, }); - await new Promise((resolve) => setTimeout(resolve, 100)); + await waitForCondition(() => stubs.delete.calledWith('TO_UNSET')); // Assert assert.ok(stubs.delete.calledWith('TO_UNSET'), 'Should delete unset variable'); @@ -399,19 +429,23 @@ suite('TerminalEnvVarInjector - Integration Tests with Fixtures', () => { injector = new TerminalEnvVarInjector(mockEnvVarCollection, envVarManager); // Act - Inject for both workspaces - envVarManager['_onDidChangeEnvironmentVariables'].fire({ + envVarManager.triggerEnvironmentVariableChange({ uri: workspaceFolder1.uri, changeType: FileChangeType.Changed, }); - envVarManager['_onDidChangeEnvironmentVariables'].fire({ + envVarManager.triggerEnvironmentVariableChange({ uri: workspaceFolder2.uri, changeType: FileChangeType.Changed, }); - await new Promise((resolve) => setTimeout(resolve, 100)); + + // Wait for async operations for both workspaces + let stubs1 = scopedCollectionStubs.get(workspace1Dir); + let stubs2 = scopedCollectionStubs.get(workspace2Dir); + await waitForCondition(() => !!stubs1 && !!stubs2 && stubs1!.replace.called && stubs2!.replace.called); + stubs1 = scopedCollectionStubs.get(workspace1Dir)!; + stubs2 = scopedCollectionStubs.get(workspace2Dir)!; // Assert - Each workspace should have its own variables - const stubs1 = scopedCollectionStubs.get(workspace1Dir)!; - const stubs2 = scopedCollectionStubs.get(workspace2Dir)!; assert.ok(stubs1.replace.calledWith('WS1_VAR', 'workspace1_value'), 'Workspace 1 should have WS1_VAR'); assert.strictEqual(stubs1.replace.calledWith('WS2_VAR'), false, 'Workspace 1 should not have WS2_VAR'); diff --git a/src/test/features/terminalEnvVarInjectorBasic.unit.test.ts b/src/test/features/terminalEnvVarInjectorBasic.unit.test.ts index 78d9f378..51f960b8 100644 --- a/src/test/features/terminalEnvVarInjectorBasic.unit.test.ts +++ b/src/test/features/terminalEnvVarInjectorBasic.unit.test.ts @@ -15,27 +15,10 @@ import { import { EnvVarManager } from '../../features/execution/envVariableManager'; import { TerminalEnvVarInjector } from '../../features/terminal/terminalEnvVarInjector'; -/** - * Test interface to access private methods for unit testing. - * This is preferable to using 'as any' and maintains type safety. - */ -interface TerminalEnvVarInjectorTestable { - applyEnvVarChanges( - envVarScope: EnvironmentVariableCollection, - envVars: { [key: string]: string | undefined }, - workspaceKey: string, - ): void; - cleanupTrackedVars(envVarScope: EnvironmentVariableCollection, workspaceKey: string): void; - clearWorkspaceVariables(workspaceFolder: WorkspaceFolder): void; - trackedEnvVars: Map>; - dispose(): void; -} - suite('TerminalEnvVarInjector - Core Functionality', () => { let envVarCollection: typeMoq.IMock; let envVarManager: typeMoq.IMock; let injector: TerminalEnvVarInjector; - let testableInjector: TerminalEnvVarInjectorTestable; let mockScopedCollection: EnvironmentVariableCollection; let clearStub: sinon.SinonStub; let replaceStub: sinon.SinonStub; @@ -92,29 +75,12 @@ suite('TerminalEnvVarInjector - Core Functionality', () => { suite('applyEnvVarChanges', () => { setup(() => { injector = new TerminalEnvVarInjector(envVarCollection.object, envVarManager.object); - testableInjector = injector as unknown as TerminalEnvVarInjectorTestable; - }); - - test('should add new variables from .env file', () => { - // Mock - new variables - const envVars = { - FOO: 'bar', - BAZ: 'qux', - }; - - // Run - access private method via test interface - testableInjector.applyEnvVarChanges(mockScopedCollection, envVars, '/test/workspace'); - - // Assert - should call replace for each variable - assert.ok(replaceStub.calledWith('FOO', 'bar'), 'Should set FOO'); - assert.ok(replaceStub.calledWith('BAZ', 'qux'), 'Should set BAZ'); - assert.strictEqual(replaceStub.callCount, 2, 'Should set exactly 2 variables'); }); test('should update existing variables', () => { // Mock - First set initial variables const initialVars = { FOO: 'initial' }; - testableInjector.applyEnvVarChanges(mockScopedCollection, initialVars, '/test/workspace'); + injector['applyEnvVarChanges'](mockScopedCollection, initialVars, '/test/workspace'); replaceStub.resetHistory(); @@ -122,7 +88,7 @@ suite('TerminalEnvVarInjector - Core Functionality', () => { const updatedVars = { FOO: 'updated' }; // Run - testableInjector.applyEnvVarChanges(mockScopedCollection, updatedVars, '/test/workspace'); + injector['applyEnvVarChanges'](mockScopedCollection, updatedVars, '/test/workspace'); // Assert - should replace with new value assert.ok(replaceStub.calledWith('FOO', 'updated'), 'Should update FOO to new value'); @@ -137,7 +103,7 @@ suite('TerminalEnvVarInjector - Core Functionality', () => { }; // Run - testableInjector.applyEnvVarChanges(mockScopedCollection, envVars, '/test/workspace'); + injector['applyEnvVarChanges'](mockScopedCollection, envVars, '/test/workspace'); // Assert - should delete empty/undefined values assert.ok(deleteStub.calledWith('EMPTY'), 'Should delete EMPTY'); @@ -145,34 +111,6 @@ suite('TerminalEnvVarInjector - Core Functionality', () => { assert.ok(replaceStub.calledWith('FOO', 'bar'), 'Should set FOO'); }); - test('should remove previously tracked vars no longer in .env (commented out scenario)', () => { - // Mock - First set: FOO, BAR, BAZ - const initialVars = { - FOO: 'bar', - BAR: 'baz', - BAZ: 'qux', - }; - testableInjector.applyEnvVarChanges(mockScopedCollection, initialVars, '/test/workspace'); - - replaceStub.resetHistory(); - deleteStub.resetHistory(); - - // Now update - BAR is commented out (removed) - const updatedVars = { - FOO: 'bar', - BAZ: 'qux', - // BAR is missing (commented out) - }; - - // Run - testableInjector.applyEnvVarChanges(mockScopedCollection, updatedVars, '/test/workspace'); - - // Assert - should delete BAR - assert.ok(deleteStub.calledWith('BAR'), 'Should delete commented out variable BAR'); - assert.ok(replaceStub.calledWith('FOO', 'bar'), 'Should keep FOO'); - assert.ok(replaceStub.calledWith('BAZ', 'qux'), 'Should keep BAZ'); - }); - test('should handle first-time initialization with no previous keys', () => { // Mock - brand new workspace const envVars = { @@ -180,7 +118,7 @@ suite('TerminalEnvVarInjector - Core Functionality', () => { }; // Run - apply to workspace that has no tracked vars yet - testableInjector.applyEnvVarChanges(mockScopedCollection, envVars, '/new/workspace'); + injector['applyEnvVarChanges'](mockScopedCollection, envVars, '/new/workspace'); // Assert - should just add the new variable assert.ok(replaceStub.calledWith('NEW_VAR', 'value'), 'Should set NEW_VAR'); @@ -195,10 +133,10 @@ suite('TerminalEnvVarInjector - Core Functionality', () => { }; // Run - testableInjector.applyEnvVarChanges(mockScopedCollection, envVars, '/test/workspace'); + injector['applyEnvVarChanges'](mockScopedCollection, envVars, '/test/workspace'); // Assert - verify tracking map contains the keys - const trackedVars = testableInjector.trackedEnvVars.get('/test/workspace'); + const trackedVars = injector['trackedEnvVars'].get('/test/workspace'); assert.ok(trackedVars, 'Should have tracking entry for workspace'); assert.ok(trackedVars.has('TRACKED_1'), 'Should track TRACKED_1'); assert.ok(trackedVars.has('TRACKED_2'), 'Should track TRACKED_2'); @@ -218,12 +156,12 @@ suite('TerminalEnvVarInjector - Core Functionality', () => { VAR2: 'value2', VAR3: 'value3', }; - testableInjector.applyEnvVarChanges(mockScopedCollection, envVars, '/test/workspace'); + injector['applyEnvVarChanges'](mockScopedCollection, envVars, '/test/workspace'); deleteStub.resetHistory(); // Run - cleanup - testableInjector.cleanupTrackedVars(mockScopedCollection, '/test/workspace'); + injector['cleanupTrackedVars'](mockScopedCollection, '/test/workspace'); // Assert - should delete all tracked variables assert.strictEqual(deleteStub.callCount, 3, 'Should delete all 3 variables'); @@ -235,19 +173,19 @@ suite('TerminalEnvVarInjector - Core Functionality', () => { test('should remove workspace from tracking map', () => { // Mock - Set up tracked variables const envVars = { TEST: 'value' }; - testableInjector.applyEnvVarChanges(mockScopedCollection, envVars, '/test/workspace'); + injector['applyEnvVarChanges'](mockScopedCollection, envVars, '/test/workspace'); // Run - cleanup - testableInjector.cleanupTrackedVars(mockScopedCollection, '/test/workspace'); + injector['cleanupTrackedVars'](mockScopedCollection, '/test/workspace'); // Assert - tracking should be removed - const trackedVars = testableInjector.trackedEnvVars.get('/test/workspace'); + const trackedVars = injector['trackedEnvVars'].get('/test/workspace'); assert.strictEqual(trackedVars, undefined, 'Should remove workspace from tracking map'); }); test('should handle case when no tracked vars exist (no-op)', () => { // Run - cleanup workspace with no tracked vars - testableInjector.cleanupTrackedVars(mockScopedCollection, '/nonexistent/workspace'); + injector['cleanupTrackedVars'](mockScopedCollection, '/nonexistent/workspace'); // Assert - should not throw and not delete anything assert.strictEqual(deleteStub.called, false, 'Should not delete anything'); @@ -257,7 +195,6 @@ suite('TerminalEnvVarInjector - Core Functionality', () => { suite('clearWorkspaceVariables', () => { setup(() => { injector = new TerminalEnvVarInjector(envVarCollection.object, envVarManager.object); - testableInjector = injector as unknown as TerminalEnvVarInjectorTestable; }); test('should only delete tracked variables', () => { @@ -266,12 +203,12 @@ suite('TerminalEnvVarInjector - Core Functionality', () => { MY_VAR: 'value', ANOTHER_VAR: 'value2', }; - testableInjector.applyEnvVarChanges(mockScopedCollection, envVars, mockWorkspaceFolder.uri.fsPath); + injector['applyEnvVarChanges'](mockScopedCollection, envVars, mockWorkspaceFolder.uri.fsPath); deleteStub.resetHistory(); // Run - clear workspace variables - testableInjector.clearWorkspaceVariables(mockWorkspaceFolder); + injector['clearWorkspaceVariables'](mockWorkspaceFolder); // Assert - should only delete our tracked variables assert.strictEqual(deleteStub.callCount, 2, 'Should delete exactly 2 variables'); @@ -282,15 +219,12 @@ suite('TerminalEnvVarInjector - Core Functionality', () => { test('should not delete non-tracked variables like BASH_ENV', () => { // Mock - Set up only one tracked variable const envVars = { MY_VAR: 'value' }; - testableInjector.applyEnvVarChanges(mockScopedCollection, envVars, mockWorkspaceFolder.uri.fsPath); - - // Simulate BASH_ENV being set by another manager (not tracked by us) - getStub.withArgs('BASH_ENV').returns({ value: 'some_bash_command' }); + injector['applyEnvVarChanges'](mockScopedCollection, envVars, mockWorkspaceFolder.uri.fsPath); deleteStub.resetHistory(); // Run - testableInjector.clearWorkspaceVariables(mockWorkspaceFolder); + injector['clearWorkspaceVariables'](mockWorkspaceFolder); // Assert - should only delete MY_VAR, not BASH_ENV assert.strictEqual(deleteStub.callCount, 1, 'Should delete only tracked variable'); @@ -303,7 +237,7 @@ suite('TerminalEnvVarInjector - Core Functionality', () => { deleteStub.throws(new Error('Collection error')); // Run - should not throw - assert.doesNotThrow(() => testableInjector.clearWorkspaceVariables(mockWorkspaceFolder)); + assert.doesNotThrow(() => injector['clearWorkspaceVariables'](mockWorkspaceFolder)); }); }); From a230490e25d9cfc27c92ade65a770b18dabf5452 Mon Sep 17 00:00:00 2001 From: eleanorjboyd <26030610+eleanorjboyd@users.noreply.github.com> Date: Tue, 9 Dec 2025 11:39:54 -0800 Subject: [PATCH 6/8] learning --- .github/instructions/testing-workflow.instructions.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.github/instructions/testing-workflow.instructions.md b/.github/instructions/testing-workflow.instructions.md index 3b297a3f..f9dcbe88 100644 --- a/.github/instructions/testing-workflow.instructions.md +++ b/.github/instructions/testing-workflow.instructions.md @@ -583,3 +583,7 @@ envConfig.inspect - When making systematic changes across many similar locations, fix one instance completely first to validate the approach before applying the pattern everywhere (1) - Always recompile tests after making changes before running them, especially when changing imports or type definitions (1) - When using paths as Map keys for tracking, you MUST use Uri.fsPath consistently throughout the test - mixing hardcoded strings with Uri.fsPath causes key mismatches on Windows (1) +- Avoid accessing private members in tests using bracket notation or test interfaces - instead add explicit test helper methods or make methods `protected` rather than `private` for better encapsulation and less brittle tests (1) +- Check for redundant test coverage between unit and integration test files - integration tests should test end-to-end behavior while unit tests focus on internal logic and edge cases (1) +- Replace hardcoded `setTimeout` delays with condition-based polling that waits for the actual expected state - this makes tests faster and more reliable across different CI environments (1) +- Fixture files in `src/test/fixtures/` must be copied to `out/test/fixtures/` for compiled tests to access them - create a copy script or add to the build process (1) From 07aca52c198cd2e3279716262c42f93cab05cb54 Mon Sep 17 00:00:00 2001 From: eleanorjboyd <26030610+eleanorjboyd@users.noreply.github.com> Date: Tue, 9 Dec 2025 13:02:22 -0800 Subject: [PATCH 7/8] updates for better job --- .github/instructions/testing-workflow.instructions.md | 4 ++-- src/test/features/terminalEnvVarInjector.test.ts | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/instructions/testing-workflow.instructions.md b/.github/instructions/testing-workflow.instructions.md index f9dcbe88..c241c731 100644 --- a/.github/instructions/testing-workflow.instructions.md +++ b/.github/instructions/testing-workflow.instructions.md @@ -585,5 +585,5 @@ envConfig.inspect - When using paths as Map keys for tracking, you MUST use Uri.fsPath consistently throughout the test - mixing hardcoded strings with Uri.fsPath causes key mismatches on Windows (1) - Avoid accessing private members in tests using bracket notation or test interfaces - instead add explicit test helper methods or make methods `protected` rather than `private` for better encapsulation and less brittle tests (1) - Check for redundant test coverage between unit and integration test files - integration tests should test end-to-end behavior while unit tests focus on internal logic and edge cases (1) -- Replace hardcoded `setTimeout` delays with condition-based polling that waits for the actual expected state - this makes tests faster and more reliable across different CI environments (1) -- Fixture files in `src/test/fixtures/` must be copied to `out/test/fixtures/` for compiled tests to access them - create a copy script or add to the build process (1) +- For async test timing, prefer event-driven or promise-based approaches over delays; when testing fire-and-forget event handlers with no completion signal, use condition-based polling (`waitForCondition`) instead of hardcoded `setTimeout` - faster and more reliable than arbitrary delays (1) +- When accessing fixture files in compiled tests, use `path.join(__dirname, '..', '..', '..', 'src', 'test', 'fixtures')` to read directly from source instead of copying to `out/` - `__dirname` points to the compiled location so navigate up and into `src/` (1) diff --git a/src/test/features/terminalEnvVarInjector.test.ts b/src/test/features/terminalEnvVarInjector.test.ts index 501140f2..75955923 100644 --- a/src/test/features/terminalEnvVarInjector.test.ts +++ b/src/test/features/terminalEnvVarInjector.test.ts @@ -23,7 +23,7 @@ suite('TerminalEnvVarInjector - Integration Tests with Fixtures', () => { { replace: sinon.SinonStub; delete: sinon.SinonStub; clear: sinon.SinonStub; get: sinon.SinonStub } >; - const fixturesPath = path.join(__dirname, '..', 'fixtures', 'terminalEnvVarInjector'); + const fixturesPath = path.join(__dirname, '..', '..', '..', 'src', 'test', 'fixtures', 'terminalEnvVarInjector'); setup(async () => { // Create a unique temp directory for this test run From 5071ff827d1476ec8082cc4973dbf6aae79307e2 Mon Sep 17 00:00:00 2001 From: eleanorjboyd <26030610+eleanorjboyd@users.noreply.github.com> Date: Tue, 9 Dec 2025 13:18:00 -0800 Subject: [PATCH 8/8] fix tests --- .../features/terminalEnvVarInjector.test.ts | 20 ++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/src/test/features/terminalEnvVarInjector.test.ts b/src/test/features/terminalEnvVarInjector.test.ts index 75955923..5084c272 100644 --- a/src/test/features/terminalEnvVarInjector.test.ts +++ b/src/test/features/terminalEnvVarInjector.test.ts @@ -319,12 +319,12 @@ suite('TerminalEnvVarInjector - Integration Tests with Fixtures', () => { uri: workspaceFolder.uri, changeType: FileChangeType.Changed, }); - await waitForCondition(() => stubs.delete.calledWith('BAR')); + await waitForCondition(() => stubs!.delete.calledWith('BAR')); // Assert - BAR should be deleted - assert.ok(stubs.delete.calledWith('BAR'), 'Should delete commented out BAR'); - assert.ok(stubs.replace.calledWith('FOO', 'bar'), 'Should keep FOO'); - assert.ok(stubs.replace.calledWith('BAZ', 'qux'), 'Should keep BAZ'); + assert.ok(stubs!.delete.calledWith('BAR'), 'Should delete commented out BAR'); + assert.ok(stubs!.replace.calledWith('FOO', 'bar'), 'Should keep FOO'); + assert.ok(stubs!.replace.calledWith('BAZ', 'qux'), 'Should keep BAZ'); }); test('Scenario: Adding a new variable injects it', async () => { @@ -348,6 +348,7 @@ suite('TerminalEnvVarInjector - Integration Tests with Fixtures', () => { let stubs = scopedCollectionStubs.get(workspaceDir); await waitForCondition(() => !!stubs && stubs!.replace.calledWith('FOO', 'bar')); stubs = scopedCollectionStubs.get(workspaceDir)!; + stubs = scopedCollectionStubs.get(workspaceDir)!; // Act - Add NEW_VAR to file await fs.appendFile(envFilePath, 'NEW_VAR=new_value\n'); @@ -358,11 +359,11 @@ suite('TerminalEnvVarInjector - Integration Tests with Fixtures', () => { uri: workspaceFolder.uri, changeType: FileChangeType.Changed, }); - await waitForCondition(() => stubs.replace.calledWith('NEW_VAR', 'new_value')); + await waitForCondition(() => stubs!.replace.calledWith('NEW_VAR', 'new_value')); // Assert - assert.ok(stubs.replace.calledWith('NEW_VAR', 'new_value'), 'Should add NEW_VAR'); - assert.ok(stubs.replace.calledWith('FOO', 'bar'), 'Should keep FOO'); + assert.ok(stubs!.replace.calledWith('NEW_VAR', 'new_value'), 'Should add NEW_VAR'); + assert.ok(stubs!.replace.calledWith('FOO', 'bar'), 'Should keep FOO'); }); test('Scenario: Unsetting a variable (VAR=) removes it', async () => { @@ -386,6 +387,7 @@ suite('TerminalEnvVarInjector - Integration Tests with Fixtures', () => { let stubs = scopedCollectionStubs.get(workspaceDir); await waitForCondition(() => !!stubs && stubs!.replace.calledWith('TO_UNSET', 'value')); stubs = scopedCollectionStubs.get(workspaceDir)!; + stubs = scopedCollectionStubs.get(workspaceDir)!; assert.ok(stubs.replace.calledWith('TO_UNSET', 'value'), 'TO_UNSET should be initially set'); @@ -398,10 +400,10 @@ suite('TerminalEnvVarInjector - Integration Tests with Fixtures', () => { uri: workspaceFolder.uri, changeType: FileChangeType.Changed, }); - await waitForCondition(() => stubs.delete.calledWith('TO_UNSET')); + await waitForCondition(() => stubs!.delete.calledWith('TO_UNSET')); // Assert - assert.ok(stubs.delete.calledWith('TO_UNSET'), 'Should delete unset variable'); + assert.ok(stubs!.delete.calledWith('TO_UNSET'), 'Should delete unset variable'); }); });