From e36238576252f2a94ac75f487b730e7a147fcdd2 Mon Sep 17 00:00:00 2001 From: Stella Huang Date: Fri, 30 Jan 2026 10:41:12 -0800 Subject: [PATCH 1/3] fix .env file issue --- .../terminal/terminalEnvVarInjector.ts | 16 +- .../terminalEnvVarInjectorBasic.unit.test.ts | 140 +++++++++++++++++- 2 files changed, 149 insertions(+), 7 deletions(-) diff --git a/src/features/terminal/terminalEnvVarInjector.ts b/src/features/terminal/terminalEnvVarInjector.ts index 039c4b3c..df6d29f6 100644 --- a/src/features/terminal/terminalEnvVarInjector.ts +++ b/src/features/terminal/terminalEnvVarInjector.ts @@ -149,6 +149,8 @@ export class TerminalEnvVarInjector implements Disposable { traceVerbose( `TerminalEnvVarInjector: Env file injection disabled for workspace: ${workspaceUri.fsPath}`, ); + // Clear any previously set variables when injection is disabled + envVarScope.clear(); return; } @@ -165,14 +167,18 @@ export class TerminalEnvVarInjector implements Disposable { traceVerbose( `TerminalEnvVarInjector: No .env file found for workspace: ${workspaceUri.fsPath}, not injecting environment variables.`, ); - return; // No .env file to inject + // Clear any previously set variables when no .env file exists + envVarScope.clear(); + return; } + // Clear all previously set variables before re-injecting. + // This ensures that when variables are commented out or removed from .env, + // they are properly removed from the terminal environment. + envVarScope.clear(); + for (const [key, value] of Object.entries(envVars)) { - if (value === undefined) { - // Remove the environment variable if the value is undefined - envVarScope.delete(key); - } else { + if (value !== undefined) { envVarScope.replace(key, value); } } diff --git a/src/test/features/terminalEnvVarInjectorBasic.unit.test.ts b/src/test/features/terminalEnvVarInjectorBasic.unit.test.ts index fc69844f..40e6efff 100644 --- a/src/test/features/terminalEnvVarInjectorBasic.unit.test.ts +++ b/src/test/features/terminalEnvVarInjectorBasic.unit.test.ts @@ -3,7 +3,8 @@ import * as sinon from 'sinon'; import * as typeMoq from 'typemoq'; -import { GlobalEnvironmentVariableCollection, workspace } from 'vscode'; +import { GlobalEnvironmentVariableCollection, Uri, workspace, WorkspaceFolder } from 'vscode'; +import * as workspaceApis from '../../common/workspace.apis'; import { EnvVarManager } from '../../features/execution/envVariableManager'; import { TerminalEnvVarInjector } from '../../features/terminal/terminalEnvVarInjector'; @@ -55,8 +56,14 @@ suite('TerminalEnvVarInjector Basic Tests', () => { ({ dispose: () => {}, // eslint-disable-next-line @typescript-eslint/no-explicit-any - } as any), + }) as any, ); + // Mock workspace.onDidChangeConfiguration to return a proper disposable + Object.defineProperty(workspace, 'onDidChangeConfiguration', { + value: () => ({ dispose: () => {} }), + configurable: true, + writable: true, + }); }); teardown(() => { @@ -102,3 +109,132 @@ suite('TerminalEnvVarInjector Basic Tests', () => { sinon.assert.match(eventHandlerRegistered, true); }); }); + +/** + * Tests for variable clearing: Ensure that when .env file variables are commented out or removed, + * they are properly removed from the terminal environment. + * + * These tests verify the clear() behavior when useEnvFile setting is disabled. + * Tests for file-existence scenarios are integration-level and not covered here. + */ +suite('TerminalEnvVarInjector - Variable Clearing', () => { + let envVarCollection: typeMoq.IMock; + let injector: TerminalEnvVarInjector; + let mockScopedCollection: MockScopedCollection; + let mockGetConfiguration: sinon.SinonStub; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + let workspaceFoldersStub: any; + let mockWorkspaceFolder: WorkspaceFolder; + let mockEnvVarManager: { + onDidChangeEnvironmentVariables: sinon.SinonStub; + getEnvironmentVariables: sinon.SinonStub; + }; + + interface MockWorkspaceConfig { + get: sinon.SinonStub; + } + + setup(() => { + envVarCollection = typeMoq.Mock.ofType(); + + // Create mock EnvVarManager using sinon stubs + mockEnvVarManager = { + onDidChangeEnvironmentVariables: sinon.stub().returns({ dispose: () => {} }), + getEnvironmentVariables: sinon.stub().resolves({}), + }; + + // Create a mock workspace folder + mockWorkspaceFolder = { + uri: Uri.file('/test/workspace'), + name: 'test-workspace', + index: 0, + }; + + // Mock workspace.workspaceFolders property + workspaceFoldersStub = [mockWorkspaceFolder]; + Object.defineProperty(workspace, 'workspaceFolders', { + get: () => workspaceFoldersStub, + configurable: true, + }); + + // Setup scoped collection mock + mockScopedCollection = { + clear: sinon.stub(), + replace: sinon.stub(), + delete: sinon.stub(), + }; + + // 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.clear()).returns(() => {}); + + // Mock getConfiguration + mockGetConfiguration = sinon.stub(workspaceApis, 'getConfiguration'); + + // Mock workspace.onDidChangeConfiguration to return a proper disposable + Object.defineProperty(workspace, 'onDidChangeConfiguration', { + value: () => ({ dispose: () => {} }), + configurable: true, + writable: true, + }); + }); + + teardown(() => { + sinon.restore(); + injector?.dispose(); + }); + + test('should call clear() when useEnvFile setting is disabled', async () => { + // Arrange - mock configuration with useEnvFile disabled + const mockConfig: MockWorkspaceConfig = { + get: sinon.stub(), + }; + mockConfig.get.withArgs('terminal.useEnvFile', false).returns(false); + mockConfig.get.withArgs('envFile').returns(undefined); + mockGetConfiguration.returns(mockConfig); + + // Mock getEnvironmentVariables + mockEnvVarManager.getEnvironmentVariables.resolves({ TEST_VAR: 'value' }); + + // Act + injector = new TerminalEnvVarInjector(envVarCollection.object, mockEnvVarManager as unknown as EnvVarManager); + + // Wait for async initialization + await new Promise((resolve) => setTimeout(resolve, 150)); + + // Assert - clear() should be called, but replace() should NOT be called + sinon.assert.called(mockScopedCollection.clear); + sinon.assert.notCalled(mockScopedCollection.replace); + }); + + test('should not inject variables when useEnvFile is disabled even if env vars exist', async () => { + // Arrange - mock configuration with useEnvFile disabled + const mockConfig: MockWorkspaceConfig = { + get: sinon.stub(), + }; + mockConfig.get.withArgs('terminal.useEnvFile', false).returns(false); + mockConfig.get.withArgs('envFile').returns('.env'); + mockGetConfiguration.returns(mockConfig); + + // Mock getEnvironmentVariables to return multiple variables + mockEnvVarManager.getEnvironmentVariables.resolves({ + API_KEY: 'secret123', + DATABASE_URL: 'postgres://localhost/db', + DEBUG: 'true', + }); + + // Act + injector = new TerminalEnvVarInjector(envVarCollection.object, mockEnvVarManager as unknown as EnvVarManager); + + // Wait for async initialization + await new Promise((resolve) => setTimeout(resolve, 150)); + + // Assert - clear() should be called to remove any previous variables, but replace() should NOT be called + sinon.assert.called(mockScopedCollection.clear); + sinon.assert.notCalled(mockScopedCollection.replace); + }); +}); From 6572576c317531a80386ab7aee1a3f4a2e149e68 Mon Sep 17 00:00:00 2001 From: Stella Huang Date: Fri, 30 Jan 2026 16:17:11 -0800 Subject: [PATCH 2/3] clean up tests --- .../terminalEnvVarInjectorBasic.unit.test.ts | 138 +----------------- 1 file changed, 1 insertion(+), 137 deletions(-) diff --git a/src/test/features/terminalEnvVarInjectorBasic.unit.test.ts b/src/test/features/terminalEnvVarInjectorBasic.unit.test.ts index 40e6efff..d36ccb51 100644 --- a/src/test/features/terminalEnvVarInjectorBasic.unit.test.ts +++ b/src/test/features/terminalEnvVarInjectorBasic.unit.test.ts @@ -3,8 +3,7 @@ import * as sinon from 'sinon'; import * as typeMoq from 'typemoq'; -import { GlobalEnvironmentVariableCollection, Uri, workspace, WorkspaceFolder } from 'vscode'; -import * as workspaceApis from '../../common/workspace.apis'; +import { GlobalEnvironmentVariableCollection, workspace } from 'vscode'; import { EnvVarManager } from '../../features/execution/envVariableManager'; import { TerminalEnvVarInjector } from '../../features/terminal/terminalEnvVarInjector'; @@ -58,12 +57,6 @@ suite('TerminalEnvVarInjector Basic Tests', () => { // eslint-disable-next-line @typescript-eslint/no-explicit-any }) as any, ); - // Mock workspace.onDidChangeConfiguration to return a proper disposable - Object.defineProperty(workspace, 'onDidChangeConfiguration', { - value: () => ({ dispose: () => {} }), - configurable: true, - writable: true, - }); }); teardown(() => { @@ -109,132 +102,3 @@ suite('TerminalEnvVarInjector Basic Tests', () => { sinon.assert.match(eventHandlerRegistered, true); }); }); - -/** - * Tests for variable clearing: Ensure that when .env file variables are commented out or removed, - * they are properly removed from the terminal environment. - * - * These tests verify the clear() behavior when useEnvFile setting is disabled. - * Tests for file-existence scenarios are integration-level and not covered here. - */ -suite('TerminalEnvVarInjector - Variable Clearing', () => { - let envVarCollection: typeMoq.IMock; - let injector: TerminalEnvVarInjector; - let mockScopedCollection: MockScopedCollection; - let mockGetConfiguration: sinon.SinonStub; - // eslint-disable-next-line @typescript-eslint/no-explicit-any - let workspaceFoldersStub: any; - let mockWorkspaceFolder: WorkspaceFolder; - let mockEnvVarManager: { - onDidChangeEnvironmentVariables: sinon.SinonStub; - getEnvironmentVariables: sinon.SinonStub; - }; - - interface MockWorkspaceConfig { - get: sinon.SinonStub; - } - - setup(() => { - envVarCollection = typeMoq.Mock.ofType(); - - // Create mock EnvVarManager using sinon stubs - mockEnvVarManager = { - onDidChangeEnvironmentVariables: sinon.stub().returns({ dispose: () => {} }), - getEnvironmentVariables: sinon.stub().resolves({}), - }; - - // Create a mock workspace folder - mockWorkspaceFolder = { - uri: Uri.file('/test/workspace'), - name: 'test-workspace', - index: 0, - }; - - // Mock workspace.workspaceFolders property - workspaceFoldersStub = [mockWorkspaceFolder]; - Object.defineProperty(workspace, 'workspaceFolders', { - get: () => workspaceFoldersStub, - configurable: true, - }); - - // Setup scoped collection mock - mockScopedCollection = { - clear: sinon.stub(), - replace: sinon.stub(), - delete: sinon.stub(), - }; - - // 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.clear()).returns(() => {}); - - // Mock getConfiguration - mockGetConfiguration = sinon.stub(workspaceApis, 'getConfiguration'); - - // Mock workspace.onDidChangeConfiguration to return a proper disposable - Object.defineProperty(workspace, 'onDidChangeConfiguration', { - value: () => ({ dispose: () => {} }), - configurable: true, - writable: true, - }); - }); - - teardown(() => { - sinon.restore(); - injector?.dispose(); - }); - - test('should call clear() when useEnvFile setting is disabled', async () => { - // Arrange - mock configuration with useEnvFile disabled - const mockConfig: MockWorkspaceConfig = { - get: sinon.stub(), - }; - mockConfig.get.withArgs('terminal.useEnvFile', false).returns(false); - mockConfig.get.withArgs('envFile').returns(undefined); - mockGetConfiguration.returns(mockConfig); - - // Mock getEnvironmentVariables - mockEnvVarManager.getEnvironmentVariables.resolves({ TEST_VAR: 'value' }); - - // Act - injector = new TerminalEnvVarInjector(envVarCollection.object, mockEnvVarManager as unknown as EnvVarManager); - - // Wait for async initialization - await new Promise((resolve) => setTimeout(resolve, 150)); - - // Assert - clear() should be called, but replace() should NOT be called - sinon.assert.called(mockScopedCollection.clear); - sinon.assert.notCalled(mockScopedCollection.replace); - }); - - test('should not inject variables when useEnvFile is disabled even if env vars exist', async () => { - // Arrange - mock configuration with useEnvFile disabled - const mockConfig: MockWorkspaceConfig = { - get: sinon.stub(), - }; - mockConfig.get.withArgs('terminal.useEnvFile', false).returns(false); - mockConfig.get.withArgs('envFile').returns('.env'); - mockGetConfiguration.returns(mockConfig); - - // Mock getEnvironmentVariables to return multiple variables - mockEnvVarManager.getEnvironmentVariables.resolves({ - API_KEY: 'secret123', - DATABASE_URL: 'postgres://localhost/db', - DEBUG: 'true', - }); - - // Act - injector = new TerminalEnvVarInjector(envVarCollection.object, mockEnvVarManager as unknown as EnvVarManager); - - // Wait for async initialization - await new Promise((resolve) => setTimeout(resolve, 150)); - - // Assert - clear() should be called to remove any previous variables, but replace() should NOT be called - sinon.assert.called(mockScopedCollection.clear); - sinon.assert.notCalled(mockScopedCollection.replace); - }); -}); From 16818ac213803f26395dbdd484481d48ee55f0ea Mon Sep 17 00:00:00 2001 From: Stella Huang Date: Fri, 30 Jan 2026 16:19:06 -0800 Subject: [PATCH 3/3] hygiene --- src/test/features/terminalEnvVarInjectorBasic.unit.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/features/terminalEnvVarInjectorBasic.unit.test.ts b/src/test/features/terminalEnvVarInjectorBasic.unit.test.ts index d36ccb51..fc69844f 100644 --- a/src/test/features/terminalEnvVarInjectorBasic.unit.test.ts +++ b/src/test/features/terminalEnvVarInjectorBasic.unit.test.ts @@ -55,7 +55,7 @@ suite('TerminalEnvVarInjector Basic Tests', () => { ({ dispose: () => {}, // eslint-disable-next-line @typescript-eslint/no-explicit-any - }) as any, + } as any), ); });