diff --git a/.github/instructions/testing-workflow.instructions.md b/.github/instructions/testing-workflow.instructions.md index be56e4a8..92922fc5 100644 --- a/.github/instructions/testing-workflow.instructions.md +++ b/.github/instructions/testing-workflow.instructions.md @@ -574,6 +574,17 @@ 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) +- 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) +- 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) - Avoid testing exact error messages or log output - assert only that errors are thrown or rejection occurs to prevent brittle tests (1) - Create shared mock helpers (e.g., `createMockLogOutputChannel()`) instead of duplicating mock setup across multiple test files (1) - 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 039c4b3c..fdc5677d 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 + */ + protected 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,16 +167,115 @@ 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. + */ + protected 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. + */ + protected 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. */ dispose(): void { traceVerbose('TerminalEnvVarInjector: Disposing'); - this.disposables.forEach((disposable) => disposable.dispose()); + 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 = {}) { @@ -204,10 +286,17 @@ 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 }); - 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..5084c272 --- /dev/null +++ b/src/test/features/terminalEnvVarInjector.test.ts @@ -0,0 +1,459 @@ +// 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, '..', '..', '..', 'src', 'test', '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; + } + + /** + * 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 + 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 test helper + envVarManager.triggerEnvironmentVariableChange({ + uri: workspaceFolder.uri, + changeType: FileChangeType.Changed, + }); + + // Wait for async operations to complete + await new Promise((resolve) => setTimeout(resolve, 200)); + + // Assert + const stubs = scopedCollectionStubs.get(workspaceDir); + 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 () => { + // 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.triggerEnvironmentVariableChange({ + uri: workspaceFolder.uri, + changeType: FileChangeType.Changed, + }); + + // Wait a bit to ensure no injection happens (negative assertion) + await new Promise((resolve) => setTimeout(resolve, 50)); + + // 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.triggerEnvironmentVariableChange({ + uri: workspaceFolder.uri, + changeType: FileChangeType.Changed, + }); + + // Wait for async operations to complete + const stubs = scopedCollectionStubs.get(workspaceDir); + await waitForCondition(() => !!stubs && stubs.replace.called); + + // Assert + 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.triggerEnvironmentVariableChange({ + uri: workspaceFolder.uri, + changeType: FileChangeType.Changed, + }); + const stubs = scopedCollectionStubs.get(workspaceDir)!; + await waitForCondition(() => stubs.replace.calledWith('FOO', 'bar')); + + // 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.triggerEnvironmentVariableChange({ + uri: workspaceFolder.uri, + changeType: FileChangeType.Changed, + }); + await waitForCondition(() => stubs.delete.callCount >= 3); + + // 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.triggerEnvironmentVariableChange({ + uri: workspaceFolder.uri, + changeType: FileChangeType.Changed, + }); + 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.triggerEnvironmentVariableChange({ + uri: workspaceFolder.uri, + changeType: FileChangeType.Deleted, + }); + await waitForCondition(() => stubs.delete.calledWith('FOO')); + + // 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.triggerEnvironmentVariableChange({ + uri: workspaceFolder.uri, + changeType: FileChangeType.Changed, + }); + let stubs = scopedCollectionStubs.get(workspaceDir); + await waitForCondition(() => !!stubs && stubs!.replace.calledWith('BAR', 'baz')); + 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.triggerEnvironmentVariableChange({ + uri: workspaceFolder.uri, + changeType: FileChangeType.Changed, + }); + 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'); + }); + + 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.triggerEnvironmentVariableChange({ + uri: workspaceFolder.uri, + changeType: FileChangeType.Changed, + }); + 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'); + + stubs.replace.resetHistory(); + + envVarManager.triggerEnvironmentVariableChange({ + uri: workspaceFolder.uri, + changeType: FileChangeType.Changed, + }); + 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'); + }); + + 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.triggerEnvironmentVariableChange({ + uri: workspaceFolder.uri, + changeType: FileChangeType.Changed, + }); + 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'); + + // Act - Unset TO_UNSET (VAR=) + await fs.writeFile(envFilePath, 'FOO=bar\nTO_UNSET=\n'); + + stubs.delete.resetHistory(); + + envVarManager.triggerEnvironmentVariableChange({ + uri: workspaceFolder.uri, + changeType: FileChangeType.Changed, + }); + await waitForCondition(() => stubs!.delete.calledWith('TO_UNSET')); + + // 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.triggerEnvironmentVariableChange({ + uri: workspaceFolder1.uri, + changeType: FileChangeType.Changed, + }); + envVarManager.triggerEnvironmentVariableChange({ + uri: workspaceFolder2.uri, + changeType: FileChangeType.Changed, + }); + + // 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 + + 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..51f960b8 100644 --- a/src/test/features/terminalEnvVarInjectorBasic.unit.test.ts +++ b/src/test/features/terminalEnvVarInjectorBasic.unit.test.ts @@ -1,30 +1,43 @@ // 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; -} - -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 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 +45,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 disposable when handler is registered + const mockDisposable: Disposable = { dispose: () => {} }; + envVarManager.setup((m) => m.onDidChangeEnvironmentVariables(typeMoq.It.isAny())).returns(() => mockDisposable); }); teardown(() => { @@ -64,41 +72,191 @@ 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); + }); + + test('should update existing variables', () => { + // Mock - First set initial variables + const initialVars = { FOO: 'initial' }; + injector['applyEnvVarChanges'](mockScopedCollection, initialVars, '/test/workspace'); + + replaceStub.resetHistory(); + + // Now update + const updatedVars = { FOO: 'updated' }; + + // Run + injector['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 + injector['applyEnvVarChanges'](mockScopedCollection, envVars, '/test/workspace'); - // Assert - should not throw - sinon.assert.match(injector, sinon.match.object); + // 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 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 + injector['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 + injector['applyEnvVarChanges'](mockScopedCollection, envVars, '/test/workspace'); + + // Assert - verify tracking map contains the keys + 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'); + assert.strictEqual(trackedVars.size, 2, 'Should track exactly 2 variables'); + }); }); - test('should dispose cleanly', () => { - // Arrange - injector = new TerminalEnvVarInjector(envVarCollection.object, envVarManager.object); + 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', + }; + injector['applyEnvVarChanges'](mockScopedCollection, envVars, '/test/workspace'); - // Act - injector.dispose(); + deleteStub.resetHistory(); + + // Run - cleanup + injector['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'); + }); - // Assert - should clear on dispose - envVarCollection.verify((c) => c.clear(), typeMoq.Times.atLeastOnce()); + test('should remove workspace from tracking map', () => { + // Mock - Set up tracked variables + const envVars = { TEST: 'value' }; + injector['applyEnvVarChanges'](mockScopedCollection, envVars, '/test/workspace'); + + // Run - cleanup + injector['cleanupTrackedVars'](mockScopedCollection, '/test/workspace'); + + // Assert - tracking should be removed + 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 + injector['cleanupTrackedVars'](mockScopedCollection, '/nonexistent/workspace'); + + // Assert - should not throw and not delete anything + assert.strictEqual(deleteStub.called, false, 'Should not delete anything'); + }); }); - 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('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', + }; + injector['applyEnvVarChanges'](mockScopedCollection, envVars, mockWorkspaceFolder.uri.fsPath); + + deleteStub.resetHistory(); + + // Run - clear workspace variables + injector['clearWorkspaceVariables'](mockWorkspaceFolder); + + // 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' }; + injector['applyEnvVarChanges'](mockScopedCollection, envVars, mockWorkspaceFolder.uri.fsPath); + + deleteStub.resetHistory(); + + // Run + injector['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(() => injector['clearWorkspaceVariables'](mockWorkspaceFolder)); + }); + }); + + 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 - Use the global setup's mock configuration + // Act + injector = new TerminalEnvVarInjector(envVarCollection.object, envVarManager.object); + + // Assert - Verify that the mock's setup was used (handler was registered) + envVarManager.verify((m) => m.onDidChangeEnvironmentVariables(typeMoq.It.isAny()), typeMoq.Times.once()); + }); }); }); 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