diff --git a/.github/instructions/testing-workflow.instructions.md b/.github/instructions/testing-workflow.instructions.md index be56e4a8..eaa32d4e 100644 --- a/.github/instructions/testing-workflow.instructions.md +++ b/.github/instructions/testing-workflow.instructions.md @@ -235,9 +235,25 @@ interface FunctionAnalysis { import * as sinon from 'sinon'; import * as workspaceApis from '../../common/workspace.apis'; // Wrapper functions -// Stub wrapper functions, not VS Code APIs directly -// Always mock wrapper functions (e.g., workspaceApis.getConfiguration()) instead of -// VS Code APIs directly to avoid stubbing issues +// ❌ WRONG - stubbing raw VS Code modules causes errors +import { commands } from 'vscode'; +const mockExecuteCommand = sinon.stub(commands, 'executeCommand').resolves(); +// Error: TypeError: Cannot stub non-existent property executeCommand + +// ✅ CORRECT - stub the wrapper function from common API layer +import * as commandApi from '../../common/command.api'; +const mockExecuteCommand = sinon.stub(commandApi, 'executeCommand').resolves(); + +// CRITICAL: Always check the implementation's imports first +// If the code uses wrapper functions like these, stub the wrapper: +// - commandApi.executeCommand() → stub commandApi.executeCommand +// - workspaceApis.getConfiguration() → stub workspaceApis.getConfiguration +// - windowApis.showInformationMessage() → stub windowApis.showInformationMessage +// - workspaceApis.getWorkspaceFolder() → stub workspaceApis.getWorkspaceFolder +// +// Why? Raw VS Code modules aren't exported in the test environment, causing +// "Cannot stub non-existent property" errors when trying to stub them directly. + const mockGetConfiguration = sinon.stub(workspaceApis, 'getConfiguration'); ``` @@ -574,6 +590,7 @@ envConfig.inspect - Untestable Node.js APIs → Create proxy abstraction functions (use function overloads to preserve intelligent typing while making functions mockable) ## 🧠 Agent Learnings + - 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) - +- When stubbing VS Code API functions in unit tests, always check the implementation imports first - if the code uses wrapper functions (like `commandApi.executeCommand()`, `workspaceApis.getConfiguration()`, `windowApis.showInformationMessage()`), stub the wrapper function instead of the raw VS Code module. Stubbing raw VS Code modules (e.g., `sinon.stub(commands, 'executeCommand')`) causes "Cannot stub non-existent property" errors because those modules aren't exported in the test environment (2) diff --git a/src/common/localize.ts b/src/common/localize.ts index fd9a8843..213300dc 100644 --- a/src/common/localize.ts +++ b/src/common/localize.ts @@ -15,6 +15,8 @@ export namespace Common { export const ok = l10n.t('Ok'); export const quickCreate = l10n.t('Quick Create'); export const installPython = l10n.t('Install Python'); + export const dontShowAgain = l10n.t("Don't Show Again"); + export const openSettings = l10n.t('Open Settings'); } export namespace WorkbenchStrings { diff --git a/src/features/terminal/terminalEnvVarInjector.ts b/src/features/terminal/terminalEnvVarInjector.ts index 039c4b3c..f95adc64 100644 --- a/src/features/terminal/terminalEnvVarInjector.ts +++ b/src/features/terminal/terminalEnvVarInjector.ts @@ -7,12 +7,15 @@ import { Disposable, EnvironmentVariableScope, GlobalEnvironmentVariableCollection, - window, workspace, WorkspaceFolder, } from 'vscode'; +import { executeCommand } from '../../common/command.api'; +import { Common } from '../../common/localize'; import { traceError, traceVerbose } from '../../common/logging'; +import { getGlobalPersistentState } from '../../common/persistentState'; import { resolveVariables } from '../../common/utils/internalVariables'; +import * as windowApis from '../../common/window.apis'; import { getConfiguration, getWorkspaceFolder } from '../../common/workspace.apis'; import { EnvVarManager } from '../execution/envVariableManager'; @@ -22,6 +25,7 @@ import { EnvVarManager } from '../execution/envVariableManager'; */ export class TerminalEnvVarInjector implements Disposable { private disposables: Disposable[] = []; + private static readonly DONT_SHOW_ENV_FILE_NOTIFICATION_KEY = 'dontShowEnvFileNotification'; constructor( private readonly envVarCollection: GlobalEnvironmentVariableCollection, @@ -63,9 +67,9 @@ export class TerminalEnvVarInjector implements Disposable { // Only show notification when env vars change and we have an env file but injection is disabled if (!useEnvFile && envFilePath) { - window.showInformationMessage( - 'An environment file is configured but terminal environment injection is disabled. Enable "python.terminal.useEnvFile" to use environment variables from .env files in terminals.', - ); + this.showEnvFileNotification().catch((error) => { + traceError('Failed to show environment file notification:', error); + }); } if (args.changeType === 2) { @@ -184,6 +188,36 @@ export class TerminalEnvVarInjector implements Disposable { } } + /** + * Show notification about environment file configuration with "Don't Show Again" option. + */ + private async showEnvFileNotification(): Promise { + const state = await getGlobalPersistentState(); + const dontShowAgain = await state.get( + TerminalEnvVarInjector.DONT_SHOW_ENV_FILE_NOTIFICATION_KEY, + false, + ); + + if (dontShowAgain) { + traceVerbose('TerminalEnvVarInjector: Env file notification suppressed by user preference'); + return; + } + + const result = await windowApis.showInformationMessage( + 'An environment file is configured but terminal environment injection is disabled. Enable "python.terminal.useEnvFile" to use environment variables from .env files in terminals.', + Common.openSettings, + Common.dontShowAgain, + ); + + if (result === Common.openSettings) { + await executeCommand('workbench.action.openSettings', 'python.terminal.useEnvFile'); + traceVerbose('TerminalEnvVarInjector: User opened settings for useEnvFile'); + } else if (result === Common.dontShowAgain) { + await state.set(TerminalEnvVarInjector.DONT_SHOW_ENV_FILE_NOTIFICATION_KEY, true); + traceVerbose('TerminalEnvVarInjector: User chose to not show env file notification again'); + } + } + /** * Dispose of the injector and clean up resources. */ diff --git a/src/test/features/terminalEnvVarInjectorNotification.unit.test.ts b/src/test/features/terminalEnvVarInjectorNotification.unit.test.ts new file mode 100644 index 00000000..12e827b6 --- /dev/null +++ b/src/test/features/terminalEnvVarInjectorNotification.unit.test.ts @@ -0,0 +1,342 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +import * as assert from 'node:assert'; +import * as sinon from 'sinon'; +import * as typeMoq from 'typemoq'; +import { GlobalEnvironmentVariableCollection, Uri, workspace } from 'vscode'; +import * as commandApi from '../../common/command.api'; +import { Common } from '../../common/localize'; +import * as persistentState from '../../common/persistentState'; +import * as windowApis from '../../common/window.apis'; +import * as workspaceApis from '../../common/workspace.apis'; +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 Notification Tests', () => { + let envVarCollection: typeMoq.IMock; + let envVarManager: typeMoq.IMock; + let injector: TerminalEnvVarInjector; + let mockScopedCollection: MockScopedCollection; + let mockState: { get: sinon.SinonStub; set: sinon.SinonStub; clear: sinon.SinonStub }; + let mockGetConfiguration: sinon.SinonStub; + let mockGetWorkspaceFolder: sinon.SinonStub; + let mockShowInformationMessage: sinon.SinonStub; + let mockExecuteCommand: sinon.SinonStub; + let envVarChangeHandler: (args: { uri?: Uri; changeType?: number }) => void; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + let workspaceFoldersStub: any; + + setup(() => { + envVarCollection = typeMoq.Mock.ofType(); + envVarManager = typeMoq.Mock.ofType(); + + // Mock workspace.workspaceFolders property + workspaceFoldersStub = []; + 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(() => {}); + + // Setup persistent state mocks + mockState = { + get: sinon.stub(), + set: sinon.stub().resolves(), + clear: sinon.stub().resolves(), + }; + sinon.stub(persistentState, 'getGlobalPersistentState').resolves(mockState); + + // Setup workspace API mocks + mockGetConfiguration = sinon.stub(workspaceApis, 'getConfiguration'); + mockGetWorkspaceFolder = sinon.stub(workspaceApis, 'getWorkspaceFolder'); + + // Setup showInformationMessage mock + mockShowInformationMessage = sinon.stub(windowApis, 'showInformationMessage').resolves(undefined); + + // Setup executeCommand mock + mockExecuteCommand = sinon.stub(commandApi, 'executeCommand').resolves(); + + // Setup environment variable change event handler - will be overridden in tests + envVarManager + .setup((m) => m.onDidChangeEnvironmentVariables) + .returns( + () => + ({ + dispose: () => {}, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + } as any), + ); + }); + + teardown(() => { + sinon.restore(); + injector?.dispose(); + }); + + test('should show notification when env file exists and useEnvFile is false (first time)', async () => { + // Arrange - user has not dismissed the notification before + mockState.get.resolves(false); + + // Setup environment variable change handler to capture it + envVarManager.reset(); + envVarCollection.reset(); + + // Re-setup scoped collection after reset + envVarCollection + .setup((x) => x.getScoped(typeMoq.It.isAny())) + .returns( + () => mockScopedCollection as unknown as ReturnType, + ); + envVarCollection.setup((x) => x.clear()).returns(() => {}); + + // Setup event handler to capture it + envVarManager + .setup((m) => m.onDidChangeEnvironmentVariables) + .returns((handler) => { + envVarChangeHandler = handler; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + return { dispose: () => {} } as any; + }); + + const mockConfig = { + get: sinon.stub(), + }; + mockConfig.get.withArgs('terminal.useEnvFile', false).returns(false); + mockConfig.get.withArgs('envFile').returns('.env'); + mockGetConfiguration.returns(mockConfig); + + const testUri = Uri.file('/workspace'); + mockGetWorkspaceFolder.returns({ uri: testUri }); + + // Act - create injector and trigger env var change + injector = new TerminalEnvVarInjector(envVarCollection.object, envVarManager.object); + await new Promise((resolve) => setTimeout(resolve, 10)); // Allow async initialization + + envVarChangeHandler({ uri: testUri }); + await new Promise((resolve) => setTimeout(resolve, 10)); // Allow async notification + + // Assert - notification should be shown with both buttons + assert.ok(mockShowInformationMessage.called, 'showInformationMessage should be called'); + const notificationCall = mockShowInformationMessage.getCall(0); + assert.ok( + notificationCall.args[0].includes('environment file is configured'), + 'Notification should mention environment file', + ); + assert.strictEqual( + notificationCall.args[1], + Common.openSettings, + 'Notification should have "Open Settings" button', + ); + assert.strictEqual( + notificationCall.args[2], + Common.dontShowAgain, + 'Notification should have "Don\'t Show Again" button', + ); + }); + + test('should not show notification when user has clicked "Don\'t Show Again"', async () => { + // Arrange - user has previously dismissed the notification + mockState.get.resolves(true); + + // Setup environment variable change handler to capture it + envVarManager.reset(); + envVarManager + .setup((m) => m.onDidChangeEnvironmentVariables) + .returns((handler) => { + envVarChangeHandler = handler; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + return { dispose: () => {} } as any; + }); + + const mockConfig = { + get: sinon.stub(), + }; + mockConfig.get.withArgs('terminal.useEnvFile', false).returns(false); + mockConfig.get.withArgs('envFile').returns('.env'); + mockGetConfiguration.returns(mockConfig); + + const testUri = Uri.file('/workspace'); + mockGetWorkspaceFolder.returns({ uri: testUri }); + + // Act - create injector and trigger env var change + injector = new TerminalEnvVarInjector(envVarCollection.object, envVarManager.object); + await new Promise((resolve) => setTimeout(resolve, 10)); // Allow async initialization + + envVarChangeHandler({ uri: testUri }); + await new Promise((resolve) => setTimeout(resolve, 10)); // Allow async notification + + // Assert - notification should NOT be shown + assert.ok(!mockShowInformationMessage.called, 'showInformationMessage should not be called'); + }); + + test('should store preference when user clicks "Don\'t Show Again"', async () => { + // Arrange - user clicks the "Don't Show Again" button + mockState.get.resolves(false); + mockShowInformationMessage.resolves(Common.dontShowAgain); + + // Setup environment variable change handler to capture it + envVarManager.reset(); + envVarManager + .setup((m) => m.onDidChangeEnvironmentVariables) + .returns((handler) => { + envVarChangeHandler = handler; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + return { dispose: () => {} } as any; + }); + + const mockConfig = { + get: sinon.stub(), + }; + mockConfig.get.withArgs('terminal.useEnvFile', false).returns(false); + mockConfig.get.withArgs('envFile').returns('.env'); + mockGetConfiguration.returns(mockConfig); + + const testUri = Uri.file('/workspace'); + mockGetWorkspaceFolder.returns({ uri: testUri }); + + // Act - create injector and trigger env var change + injector = new TerminalEnvVarInjector(envVarCollection.object, envVarManager.object); + await new Promise((resolve) => setTimeout(resolve, 10)); // Allow async initialization + + envVarChangeHandler({ uri: testUri }); + await new Promise((resolve) => setTimeout(resolve, 50)); // Allow async notification and state update + + // Assert - state should be set to true + assert.ok(mockState.set.called, 'state.set should be called'); + const setCall = mockState.set.getCall(0); + assert.strictEqual(setCall.args[0], 'dontShowEnvFileNotification', 'Should use correct state key'); + assert.strictEqual(setCall.args[1], true, 'Should set state to true'); + }); + + test('should open settings when user clicks "Open Settings" button', async () => { + // Arrange - user clicks the "Open Settings" button + mockState.get.resolves(false); + mockShowInformationMessage.resolves(Common.openSettings); + + // Setup environment variable change handler to capture it + envVarManager.reset(); + envVarManager + .setup((m) => m.onDidChangeEnvironmentVariables) + .returns((handler) => { + envVarChangeHandler = handler; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + return { dispose: () => {} } as any; + }); + + const mockConfig = { + get: sinon.stub(), + }; + mockConfig.get.withArgs('terminal.useEnvFile', false).returns(false); + mockConfig.get.withArgs('envFile').returns('.env'); + mockGetConfiguration.returns(mockConfig); + + const testUri = Uri.file('/workspace'); + mockGetWorkspaceFolder.returns({ uri: testUri }); + + // Act - create injector and trigger env var change + injector = new TerminalEnvVarInjector(envVarCollection.object, envVarManager.object); + await new Promise((resolve) => setTimeout(resolve, 10)); // Allow async initialization + + envVarChangeHandler({ uri: testUri }); + await new Promise((resolve) => setTimeout(resolve, 50)); // Allow async notification and command execution + + // Assert - executeCommand should be called to open settings + assert.ok(mockExecuteCommand.called, 'executeCommand should be called'); + const commandCall = mockExecuteCommand.getCall(0); + assert.strictEqual(commandCall.args[0], 'workbench.action.openSettings', 'Should open settings'); + assert.strictEqual(commandCall.args[1], 'python.terminal.useEnvFile', 'Should open useEnvFile setting'); + // State should NOT be set when clicking "Open Settings" + assert.ok(!mockState.set.called, 'state.set should not be called when opening settings'); + }); + + test('should not show notification when useEnvFile is true', async () => { + // Arrange - useEnvFile is enabled + mockState.get.resolves(false); + + // Setup environment variable change handler to capture it + envVarManager.reset(); + envVarManager + .setup((m) => m.onDidChangeEnvironmentVariables) + .returns((handler) => { + envVarChangeHandler = handler; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + return { dispose: () => {} } as any; + }); + + const mockConfig = { + get: sinon.stub(), + }; + mockConfig.get.withArgs('terminal.useEnvFile', false).returns(true); // enabled + mockConfig.get.withArgs('envFile').returns('.env'); + mockGetConfiguration.returns(mockConfig); + + const testUri = Uri.file('/workspace'); + mockGetWorkspaceFolder.returns({ uri: testUri }); + + // Act - create injector and trigger env var change + injector = new TerminalEnvVarInjector(envVarCollection.object, envVarManager.object); + await new Promise((resolve) => setTimeout(resolve, 10)); // Allow async initialization + + envVarChangeHandler({ uri: testUri }); + await new Promise((resolve) => setTimeout(resolve, 10)); // Allow async notification + + // Assert - notification should NOT be shown + assert.ok(!mockShowInformationMessage.called, 'showInformationMessage should not be called'); + }); + + test('should not show notification when envFile is not configured', async () => { + // Arrange - no envFile configured + mockState.get.resolves(false); + + // Setup environment variable change handler to capture it + envVarManager.reset(); + envVarManager + .setup((m) => m.onDidChangeEnvironmentVariables) + .returns((handler) => { + envVarChangeHandler = handler; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + return { dispose: () => {} } as any; + }); + + const mockConfig = { + get: sinon.stub(), + }; + mockConfig.get.withArgs('terminal.useEnvFile', false).returns(false); + mockConfig.get.withArgs('envFile').returns(undefined); // no env file + mockGetConfiguration.returns(mockConfig); + + const testUri = Uri.file('/workspace'); + mockGetWorkspaceFolder.returns({ uri: testUri }); + + // Act - create injector and trigger env var change + injector = new TerminalEnvVarInjector(envVarCollection.object, envVarManager.object); + await new Promise((resolve) => setTimeout(resolve, 10)); // Allow async initialization + + envVarChangeHandler({ uri: testUri }); + await new Promise((resolve) => setTimeout(resolve, 10)); // Allow async notification + + // Assert - notification should NOT be shown + assert.ok(!mockShowInformationMessage.called, 'showInformationMessage should not be called'); + }); +});