Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 11 additions & 5 deletions src/features/terminal/terminalEnvVarInjector.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Comment thread
anthonykim1 marked this conversation as resolved.
Outdated
Comment thread
anthonykim1 marked this conversation as resolved.
Outdated
return;
}

Expand All @@ -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);
}
}
Expand Down
140 changes: 138 additions & 2 deletions src/test/features/terminalEnvVarInjectorBasic.unit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -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(() => {
Expand Down Expand Up @@ -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<GlobalEnvironmentVariableCollection>;
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<GlobalEnvironmentVariableCollection>();

// 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<GlobalEnvironmentVariableCollection['getScoped']>,
);
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);
});
});
Loading