Skip to content

Commit ee031ad

Browse files
authored
overhaul interpreter selection prioritization code to align with desired UX (#1155)
## Summary Implements a robust priority chain for interpreter selection that respects user-configured settings, fixes multi-root workspace issues, and prevents unwanted settings.json modifications on startup. ## Changes New Priority Chain (in order): 1. `pythonProjects[] `- Project-specific settings (highest priority) 2. `defaultEnvManager` - User-configured only (not fallback values) 3. `python.defaultInterpreterPath `- Legacy setting support 4. Auto-discovery - Local venv → System Python (lowest priority) ### Key Fixes: - Settings now take priority over cached environments - `python.defaultInterpreterPath` is now respected when no explicit `defaultEnvManager` is configured - No automatic `settings.json` writes on workspace open (only on explicit user actions) - User notification when configured settings can't be applied (with "Open Settings" action) ### How Cache Fits In The cache stores previously resolved environments to avoid re-running discovery on every request. **Important: User-configured settings always take priority over cache. The cache is only used as a fallback when:** No user explicitly configured any of the above settings, AND An environment was previously resolved for that scope On workspace open, the full priority chain runs fresh for each folder. Cache is populated but not written to settings.json unless the user explicitly selects an interpreter. Fixes #1145 Fixes #833 Fixes #1124 ### Testing 29 unit tests covering priority chain, config change listener, multi-root workspaces, and caching behavio
1 parent 5c10ad3 commit ee031ad

File tree

12 files changed

+2158
-380
lines changed

12 files changed

+2158
-380
lines changed

.github/instructions/generic.instructions.md

Lines changed: 22 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -6,27 +6,40 @@ Provide project context and coding guidelines that AI should follow when generat
66

77
## Localization
88

9-
- Localize all user-facing messages using VS Code’s `l10n` API.
10-
- Internal log messages do not require localization.
9+
- Localize all user-facing messages using VS Code’s `l10n` API.
10+
- Internal log messages do not require localization.
1111

1212
## Logging
1313

14-
- Use the extension’s logging utilities (`traceLog`, `traceVerbose`) for internal logs.
15-
- Do not use `console.log` or `console.warn` for logging.
14+
- Use the extension’s logging utilities (`traceLog`, `traceVerbose`) for internal logs.
15+
- Do not use `console.log` or `console.warn` for logging.
1616

1717
## Settings Precedence
1818

19-
- Always consider VS Code settings precedence:
19+
- Always consider VS Code settings precedence:
2020
1. Workspace folder
2121
2. Workspace
2222
3. User/global
23-
- Remove or update settings from the highest precedence scope first.
23+
- Remove or update settings from the highest precedence scope first.
2424

2525
## Error Handling & User Notifications
2626

27-
- Avoid showing the same error message multiple times in a session; track state with a module-level variable.
28-
- Use clear, actionable error messages and offer relevant buttons (e.g., "Open settings", "Close").
27+
- Avoid showing the same error message multiple times in a session; track state with a module-level variable.
28+
- Use clear, actionable error messages and offer relevant buttons (e.g., "Open settings", "Close").
2929

3030
## Documentation
3131

32-
- Add clear docstrings to public functions, describing their purpose, parameters, and behavior.
32+
- Add clear docstrings to public functions, describing their purpose, parameters, and behavior.
33+
34+
## Cross-Platform Path Handling
35+
36+
**CRITICAL**: This extension runs on Windows, macOS, and Linux. NEVER hardcode POSIX-style paths.
37+
38+
- Use `path.join()` or `path.resolve()` instead of string concatenation with `/`
39+
- Use `Uri.file(path).fsPath` when comparing paths (Windows uses `\`, POSIX uses `/`)
40+
- When asserting path equality, compare `fsPath` to `fsPath`, not raw strings
41+
42+
## Learnings
43+
44+
- When using `getConfiguration().inspect()`, always pass a scope/Uri to `getConfiguration(section, scope)` — otherwise `workspaceFolderValue` will be `undefined` because VS Code doesn't know which folder to inspect (1)
45+
- **path.normalize() vs path.resolve()**: On Windows, `path.normalize('\test')` keeps it as `\test`, but `path.resolve('\test')` adds the current drive → `C:\test`. When comparing paths, use `path.resolve()` on BOTH sides or they won't match (2)

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

Lines changed: 102 additions & 81 deletions
Original file line numberDiff line numberDiff line change
@@ -20,36 +20,53 @@ This guide covers the full testing lifecycle:
2020

2121
**User Requests Testing:**
2222

23-
- "Write tests for this function"
24-
- "Run the tests"
25-
- "Fix the failing tests"
26-
- "Test this code"
27-
- "Add test coverage"
23+
- "Write tests for this function"
24+
- "Run the tests"
25+
- "Fix the failing tests"
26+
- "Test this code"
27+
- "Add test coverage"
2828

2929
**File Context Triggers:**
3030

31-
- Working in `**/test/**` directories
32-
- Files ending in `.test.ts` or `.unit.test.ts`
33-
- Test failures or compilation errors
34-
- Coverage reports or test output analysis
31+
- Working in `**/test/**` directories
32+
- Files ending in `.test.ts` or `.unit.test.ts`
33+
- Test failures or compilation errors
34+
- Coverage reports or test output analysis
35+
36+
## 🚨 CRITICAL: Common Mistakes (Read First!)
37+
38+
These mistakes have occurred REPEATEDLY. Check this list BEFORE writing any test code:
39+
40+
| Mistake | Fix |
41+
| ---------------------------------------------- | ------------------------------------------------------------------ |
42+
| Hardcoded POSIX paths like `'/test/workspace'` | Use `'.'` for relative paths, `Uri.file(x).fsPath` for comparisons |
43+
| Stubbing `workspace.getConfiguration` directly | Stub the wrapper `workspaceApis.getConfiguration` instead |
44+
| Stubbing `workspace.workspaceFolders` property | Stub wrapper function `workspaceApis.getWorkspaceFolders()` |
45+
| Comparing `fsPath` to raw string | Compare `fsPath` to `Uri.file(expected).fsPath` |
46+
47+
**Pre-flight checklist before completing test work:**
48+
49+
- [ ] All paths use `Uri.file().fsPath` (no hardcoded `/path/to/x`)
50+
- [ ] All VS Code API stubs use wrapper modules, not `vscode.*` directly
51+
- [ ] Tests pass on both Windows and POSIX
3552

3653
## Test Types
3754

3855
When implementing tests as an AI agent, choose between two main types:
3956

4057
### Unit Tests (`*.unit.test.ts`)
4158

42-
- **Fast isolated testing** - Mock all external dependencies
43-
- **Use for**: Pure functions, business logic, data transformations
44-
- **Execute with**: `runTests` tool with specific file patterns
45-
- **Mock everything** - VS Code APIs automatically mocked via `/src/test/unittests.ts`
59+
- **Fast isolated testing** - Mock all external dependencies
60+
- **Use for**: Pure functions, business logic, data transformations
61+
- **Execute with**: `runTests` tool with specific file patterns
62+
- **Mock everything** - VS Code APIs automatically mocked via `/src/test/unittests.ts`
4663

4764
### Extension Tests (`*.test.ts`)
4865

49-
- **Full VS Code integration** - Real environment with actual APIs
50-
- **Use for**: Command registration, UI interactions, extension lifecycle
51-
- **Execute with**: VS Code launch configurations or `runTests` tool
52-
- **Slower but comprehensive** - Tests complete user workflows
66+
- **Full VS Code integration** - Real environment with actual APIs
67+
- **Use for**: Command registration, UI interactions, extension lifecycle
68+
- **Execute with**: VS Code launch configurations or `runTests` tool
69+
- **Slower but comprehensive** - Tests complete user workflows
5370

5471
## 🤖 Agent Tool Usage for Test Execution
5572

@@ -172,17 +189,17 @@ function analyzeFailure(failure: TestFailure): TestFailureAnalysis {
172189

173190
**Choose Unit Tests (`*.unit.test.ts`) when analyzing:**
174191

175-
- Functions with clear inputs/outputs and no VS Code API dependencies
176-
- Data transformation, parsing, or utility functions
177-
- Business logic that can be isolated with mocks
178-
- Error handling scenarios with predictable inputs
192+
- Functions with clear inputs/outputs and no VS Code API dependencies
193+
- Data transformation, parsing, or utility functions
194+
- Business logic that can be isolated with mocks
195+
- Error handling scenarios with predictable inputs
179196

180197
**Choose Extension Tests (`*.test.ts`) when analyzing:**
181198

182-
- Functions that register VS Code commands or use `vscode.*` APIs
183-
- UI components, tree views, or command palette interactions
184-
- File system operations requiring workspace context
185-
- Extension lifecycle events (activation, deactivation)
199+
- Functions that register VS Code commands or use `vscode.*` APIs
200+
- UI components, tree views, or command palette interactions
201+
- File system operations requiring workspace context
202+
- Extension lifecycle events (activation, deactivation)
186203

187204
**Agent Implementation Pattern:**
188205

@@ -300,22 +317,22 @@ function generateTestScenarios(analysis: FunctionAnalysis): TestScenario[] {
300317

301318
#### Main Flows
302319

303-
- **Happy path scenarios** - normal expected usage
304-
- **Alternative paths** - different configuration combinations
305-
- **Integration scenarios** - multiple features working together
320+
-**Happy path scenarios** - normal expected usage
321+
-**Alternative paths** - different configuration combinations
322+
-**Integration scenarios** - multiple features working together
306323

307324
#### Edge Cases
308325

309-
- 🔸 **Boundary conditions** - empty inputs, missing data
310-
- 🔸 **Error scenarios** - network failures, permission errors
311-
- 🔸 **Data validation** - invalid inputs, type mismatches
326+
- 🔸 **Boundary conditions** - empty inputs, missing data
327+
- 🔸 **Error scenarios** - network failures, permission errors
328+
- 🔸 **Data validation** - invalid inputs, type mismatches
312329

313330
#### Real-World Scenarios
314331

315-
- **Fresh install** - clean slate
316-
- **Existing user** - migration scenarios
317-
- **Power user** - complex configurations
318-
- 🔸 **Error recovery** - graceful degradation
332+
-**Fresh install** - clean slate
333+
-**Existing user** - migration scenarios
334+
-**Power user** - complex configurations
335+
- 🔸 **Error recovery** - graceful degradation
319336

320337
### Example Test Plan Structure
321338

@@ -324,30 +341,30 @@ function generateTestScenarios(analysis: FunctionAnalysis): TestScenario[] {
324341

325342
### 1. Configuration Migration Tests
326343

327-
- No legacy settings exist
328-
- Legacy settings already migrated
329-
- Fresh migration needed
330-
- Partial migration required
331-
- Migration failures
344+
- No legacy settings exist
345+
- Legacy settings already migrated
346+
- Fresh migration needed
347+
- Partial migration required
348+
- Migration failures
332349

333350
### 2. Configuration Source Tests
334351

335-
- Global search paths
336-
- Workspace search paths
337-
- Settings precedence
338-
- Configuration errors
352+
- Global search paths
353+
- Workspace search paths
354+
- Settings precedence
355+
- Configuration errors
339356

340357
### 3. Path Resolution Tests
341358

342-
- Absolute vs relative paths
343-
- Workspace folder resolution
344-
- Path validation and filtering
359+
- Absolute vs relative paths
360+
- Workspace folder resolution
361+
- Path validation and filtering
345362

346363
### 4. Integration Scenarios
347364

348-
- Combined configurations
349-
- Deduplication logic
350-
- Error handling flows
365+
- Combined configurations
366+
- Deduplication logic
367+
- Error handling flows
351368
```
352369

353370
## 🔧 Step 4: Set Up Your Test Infrastructure
@@ -514,47 +531,47 @@ envConfig.inspect
514531

515532
### Configuration Tests
516533

517-
- Test different setting combinations
518-
- Test setting precedence (workspace > user > default)
519-
- Test configuration errors and recovery
520-
- Always use dynamic path construction with Node.js `path` module when testing functions that resolve paths against workspace folders to ensure cross-platform compatibility
534+
- Test different setting combinations
535+
- Test setting precedence (workspace > user > default)
536+
- Test configuration errors and recovery
537+
- Always use dynamic path construction with Node.js `path` module when testing functions that resolve paths against workspace folders to ensure cross-platform compatibility
521538

522539
### Data Flow Tests
523540

524-
- Test how data moves through the system
525-
- Test transformations (path resolution, filtering)
526-
- Test state changes (migrations, updates)
541+
- Test how data moves through the system
542+
- Test transformations (path resolution, filtering)
543+
- Test state changes (migrations, updates)
527544

528545
### Error Handling Tests
529546

530-
- Test graceful degradation
531-
- Test error logging
532-
- Test fallback behaviors
547+
- Test graceful degradation
548+
- Test error logging
549+
- Test fallback behaviors
533550

534551
### Integration Tests
535552

536-
- Test multiple features together
537-
- Test real-world scenarios
538-
- Test edge case combinations
553+
- Test multiple features together
554+
- Test real-world scenarios
555+
- Test edge case combinations
539556

540557
## 📊 Step 8: Review and Refine
541558

542559
### Test Quality Checklist
543560

544-
- [ ] **Clear naming** - test names describe the scenario and expected outcome
545-
- [ ] **Good coverage** - main flows, edge cases, error scenarios
546-
- [ ] **Resilient assertions** - won't break due to minor changes
547-
- [ ] **Readable structure** - follows Mock → Run → Assert pattern
548-
- [ ] **Isolated tests** - each test is independent
549-
- [ ] **Fast execution** - tests run quickly with proper mocking
561+
- [ ] **Clear naming** - test names describe the scenario and expected outcome
562+
- [ ] **Good coverage** - main flows, edge cases, error scenarios
563+
- [ ] **Resilient assertions** - won't break due to minor changes
564+
- [ ] **Readable structure** - follows Mock → Run → Assert pattern
565+
- [ ] **Isolated tests** - each test is independent
566+
- [ ] **Fast execution** - tests run quickly with proper mocking
550567

551568
### Common Anti-Patterns to Avoid
552569

553-
- ❌ Testing implementation details instead of behavior
554-
- ❌ Brittle assertions that break on cosmetic changes
555-
- ❌ Order-dependent tests that fail due to processing changes
556-
- ❌ Tests that don't clean up mocks properly
557-
- ❌ Overly complex test setup that's hard to understand
570+
- ❌ Testing implementation details instead of behavior
571+
- ❌ Brittle assertions that break on cosmetic changes
572+
- ❌ Order-dependent tests that fail due to processing changes
573+
- ❌ Tests that don't clean up mocks properly
574+
- ❌ Overly complex test setup that's hard to understand
558575

559576
## 🔄 Reviewing and Improving Existing Tests
560577

@@ -567,13 +584,17 @@ envConfig.inspect
567584

568585
### Common Fixes
569586

570-
- Over-complex mocks → Minimal mocks with only needed methods
571-
- Brittle assertions → Behavior-focused with error messages
572-
- Vague test names → Clear scenario descriptions (transform "should return X when Y" into "should [expected behavior] when [scenario context]")
573-
- Missing structure → Mock → Run → Assert pattern
574-
- Untestable Node.js APIs → Create proxy abstraction functions (use function overloads to preserve intelligent typing while making functions mockable)
587+
- Over-complex mocks → Minimal mocks with only needed methods
588+
- Brittle assertions → Behavior-focused with error messages
589+
- Vague test names → Clear scenario descriptions (transform "should return X when Y" into "should [expected behavior] when [scenario context]")
590+
- Missing structure → Mock → Run → Assert pattern
591+
- Untestable Node.js APIs → Create proxy abstraction functions (use function overloads to preserve intelligent typing while making functions mockable)
575592

576593
## 🧠 Agent Learnings
577-
- Avoid testing exact error messages or log output - assert only that errors are thrown or rejection occurs to prevent brittle tests (1)
578-
- Create shared mock helpers (e.g., `createMockLogOutputChannel()`) instead of duplicating mock setup across multiple test files (1)
579594

595+
- Avoid testing exact error messages or log output - assert only that errors are thrown or rejection occurs to prevent brittle tests (1)
596+
- Create shared mock helpers (e.g., `createMockLogOutputChannel()`) instead of duplicating mock setup across multiple test files (1)
597+
- Always compile tests (`npm run compile-tests`) before running them after adding new test cases - test counts will be wrong if running against stale compiled output (1)
598+
- Never create "documentation tests" that just `assert.ok(true)` — if mocking limitations prevent testing, either test a different layer that IS mockable, or skip the test entirely with a clear explanation (1)
599+
- **REPEATED**: When stubbing vscode APIs in tests via wrapper modules (e.g., `workspaceApis`), the production code must also use those wrappers — sinon cannot stub properties directly on the vscode namespace like `workspace.workspaceFolders`, so both production and test code must reference the same stubbable wrapper functions (3)
600+
- **REPEATED**: Use OS-agnostic path handling in tests: use `'.'` for relative paths in configs (NOT `/test/workspace`), compare `fsPath` to `Uri.file(expected).fsPath` (NOT raw strings). This breaks on Windows every time! (5)

src/common/constants.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@ export const EXTENSION_ROOT_DIR = path.dirname(__dirname);
77

88
export const DEFAULT_PACKAGE_MANAGER_ID = 'ms-python.python:pip';
99
export const DEFAULT_ENV_MANAGER_ID = 'ms-python.python:venv';
10+
export const VENV_MANAGER_ID = 'ms-python.python:venv';
11+
export const SYSTEM_MANAGER_ID = 'ms-python.python:system';
1012

1113
export const KNOWN_FILES = [
1214
'requirements.txt',

src/extension.ts

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,10 @@ import {
4545
} from './features/envCommands';
4646
import { PythonEnvironmentManagers } from './features/envManagers';
4747
import { EnvVarManager, PythonEnvVariableManager } from './features/execution/envVariableManager';
48+
import {
49+
applyInitialEnvironmentSelection,
50+
registerInterpreterSettingsChangeListener,
51+
} from './features/interpreterSelection';
4852
import { PythonProjectManagerImpl } from './features/projectManager';
4953
import { getPythonApi, setPythonApi } from './features/pythonApi';
5054
import { registerCompletionProvider } from './features/settings/settingCompletions';
@@ -67,12 +71,7 @@ import { PythonStatusBarImpl } from './features/views/pythonStatusBar';
6771
import { updateViewsAndStatus } from './features/views/revealHandler';
6872
import { TemporaryStateManager } from './features/views/temporaryStateManager';
6973
import { ProjectItem, PythonEnvTreeItem } from './features/views/treeViewItems';
70-
import {
71-
collectEnvironmentInfo,
72-
getEnvManagerAndPackageManagerConfigLevels,
73-
resolveDefaultInterpreter,
74-
runPetInTerminalImpl,
75-
} from './helpers';
74+
import { collectEnvironmentInfo, getEnvManagerAndPackageManagerConfigLevels, runPetInTerminalImpl } from './helpers';
7675
import { EnvironmentManagers, ProjectCreators, PythonProjectManager } from './internal.api';
7776
import { registerSystemPythonFeatures } from './managers/builtin/main';
7877
import { SysPythonManager } from './managers/builtin/sysPythonManager';
@@ -460,7 +459,12 @@ export async function activate(context: ExtensionContext): Promise<PythonEnviron
460459
shellStartupVarsMgr.initialize(),
461460
]);
462461

463-
await resolveDefaultInterpreter(nativeFinder, envManagers, api);
462+
await applyInitialEnvironmentSelection(envManagers, projectManager, nativeFinder, api);
463+
464+
// Register listener for interpreter settings changes for interpreter re-selection
465+
context.subscriptions.push(
466+
registerInterpreterSettingsChangeListener(envManagers, projectManager, nativeFinder, api),
467+
);
464468

465469
sendTelemetryEvent(EventNames.EXTENSION_MANAGER_REGISTRATION_DURATION, start.elapsedTime);
466470
await terminalManager.initialize(api);

src/features/creators/newPackageProject.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -167,7 +167,7 @@ export class NewPackageProject implements PythonProjectCreator {
167167
}
168168

169169
// 6. Set the Python environment for the package
170-
this.envManagers.setEnvironment(createdPackage?.uri, createdEnv);
170+
await this.envManagers.setEnvironment(createdPackage?.uri, createdEnv);
171171

172172
// 7. add custom github copilot instructions
173173
if (createCopilotInstructions) {

0 commit comments

Comments
 (0)