Skip to content

Commit 3394294

Browse files
committed
Refactor command execution in TerminalEnvVarInjector to use command API and update related tests
1 parent f658e5e commit 3394294

File tree

3 files changed

+26
-8
lines changed

3 files changed

+26
-8
lines changed

.github/instructions/testing-workflow.instructions.md

Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -235,9 +235,25 @@ interface FunctionAnalysis {
235235
import * as sinon from 'sinon';
236236
import * as workspaceApis from '../../common/workspace.apis'; // Wrapper functions
237237

238-
// Stub wrapper functions, not VS Code APIs directly
239-
// Always mock wrapper functions (e.g., workspaceApis.getConfiguration()) instead of
240-
// VS Code APIs directly to avoid stubbing issues
238+
// ❌ WRONG - stubbing raw VS Code modules causes errors
239+
import { commands } from 'vscode';
240+
const mockExecuteCommand = sinon.stub(commands, 'executeCommand').resolves();
241+
// Error: TypeError: Cannot stub non-existent property executeCommand
242+
243+
// ✅ CORRECT - stub the wrapper function from common API layer
244+
import * as commandApi from '../../common/command.api';
245+
const mockExecuteCommand = sinon.stub(commandApi, 'executeCommand').resolves();
246+
247+
// CRITICAL: Always check the implementation's imports first
248+
// If the code uses wrapper functions like these, stub the wrapper:
249+
// - commandApi.executeCommand() → stub commandApi.executeCommand
250+
// - workspaceApis.getConfiguration() → stub workspaceApis.getConfiguration
251+
// - windowApis.showInformationMessage() → stub windowApis.showInformationMessage
252+
// - workspaceApis.getWorkspaceFolder() → stub workspaceApis.getWorkspaceFolder
253+
//
254+
// Why? Raw VS Code modules aren't exported in the test environment, causing
255+
// "Cannot stub non-existent property" errors when trying to stub them directly.
256+
241257
const mockGetConfiguration = sinon.stub(workspaceApis, 'getConfiguration');
242258
```
243259

@@ -574,6 +590,7 @@ envConfig.inspect
574590
- Untestable Node.js APIs → Create proxy abstraction functions (use function overloads to preserve intelligent typing while making functions mockable)
575591

576592
## 🧠 Agent Learnings
593+
577594
- Avoid testing exact error messages or log output - assert only that errors are thrown or rejection occurs to prevent brittle tests (1)
578595
- Create shared mock helpers (e.g., `createMockLogOutputChannel()`) instead of duplicating mock setup across multiple test files (1)
579-
596+
- 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)

src/features/terminal/terminalEnvVarInjector.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,13 +4,13 @@
44
import * as fse from 'fs-extra';
55
import * as path from 'path';
66
import {
7-
commands,
87
Disposable,
98
EnvironmentVariableScope,
109
GlobalEnvironmentVariableCollection,
1110
workspace,
1211
WorkspaceFolder,
1312
} from 'vscode';
13+
import { executeCommand } from '../../common/command.api';
1414
import { Common } from '../../common/localize';
1515
import { traceError, traceVerbose } from '../../common/logging';
1616
import { getGlobalPersistentState } from '../../common/persistentState';
@@ -210,7 +210,7 @@ export class TerminalEnvVarInjector implements Disposable {
210210
);
211211

212212
if (result === Common.openSettings) {
213-
await commands.executeCommand('workbench.action.openSettings', 'python.terminal.useEnvFile');
213+
await executeCommand('workbench.action.openSettings', 'python.terminal.useEnvFile');
214214
traceVerbose('TerminalEnvVarInjector: User opened settings for useEnvFile');
215215
} else if (result === Common.dontShowAgain) {
216216
await state.set(TerminalEnvVarInjector.DONT_SHOW_ENV_FILE_NOTIFICATION_KEY, true);

src/test/features/terminalEnvVarInjectorNotification.unit.test.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,8 @@
44
import * as assert from 'node:assert';
55
import * as sinon from 'sinon';
66
import * as typeMoq from 'typemoq';
7-
import { commands, GlobalEnvironmentVariableCollection, Uri, workspace } from 'vscode';
7+
import { GlobalEnvironmentVariableCollection, Uri, workspace } from 'vscode';
8+
import * as commandApi from '../../common/command.api';
89
import { Common } from '../../common/localize';
910
import * as persistentState from '../../common/persistentState';
1011
import * as windowApis from '../../common/window.apis';
@@ -74,7 +75,7 @@ suite('TerminalEnvVarInjector Notification Tests', () => {
7475
mockShowInformationMessage = sinon.stub(windowApis, 'showInformationMessage').resolves(undefined);
7576

7677
// Setup executeCommand mock
77-
mockExecuteCommand = sinon.stub(commands, 'executeCommand').resolves();
78+
mockExecuteCommand = sinon.stub(commandApi, 'executeCommand').resolves();
7879

7980
// Setup environment variable change event handler - will be overridden in tests
8081
envVarManager

0 commit comments

Comments
 (0)