Skip to content

Commit e16cb1e

Browse files
author
Kartik Raj
authored
Ensure we block getting active interpreter on auto-selection (#17370) (#17372)
* Ensure we block getting active interpreter on autoselection * News entry * Fix single workspace tests * Fix lint
1 parent c37dc54 commit e16cb1e

File tree

9 files changed

+85
-51
lines changed

9 files changed

+85
-51
lines changed

news/2 Fixes/17370.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Ensure we block getting active interpreter on auto-selection.

src/client/common/process/pythonExecutionFactory.ts

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ import { inDiscoveryExperiment } from '../experiments/helpers';
1212
import { sendTelemetryEvent } from '../../telemetry';
1313
import { EventName } from '../../telemetry/constants';
1414
import { IFileSystem } from '../platform/types';
15-
import { IConfigurationService, IDisposableRegistry, IExperimentService } from '../types';
15+
import { IConfigurationService, IDisposableRegistry, IExperimentService, IInterpreterPathProxyService } from '../types';
1616
import { ProcessService } from './proc';
1717
import { createCondaEnv, createPythonEnv, createWindowsStoreEnv } from './pythonEnvironment';
1818
import { createPythonProcessService } from './pythonProcess';
@@ -27,6 +27,7 @@ import {
2727
IPythonExecutionService,
2828
} from './types';
2929
import { isWindowsStoreInterpreter } from '../../pythonEnvironments/discovery/locators/services/windowsStoreInterpreter';
30+
import { IInterpreterAutoSelectionService } from '../../interpreter/autoSelection/types';
3031

3132
// Minimum version number of conda required to be able to use 'conda run'
3233
export const CONDA_RUN_VERSION = '4.6.0';
@@ -48,6 +49,8 @@ export class PythonExecutionFactory implements IPythonExecutionFactory {
4849
@inject(IBufferDecoder) private readonly decoder: IBufferDecoder,
4950
@inject(IComponentAdapter) private readonly pyenvs: IComponentAdapter,
5051
@inject(IExperimentService) private readonly experimentService: IExperimentService,
52+
@inject(IInterpreterAutoSelectionService) private readonly autoSelection: IInterpreterAutoSelectionService,
53+
@inject(IInterpreterPathProxyService) private readonly interpreterPathExpHelper: IInterpreterPathProxyService,
5154
) {
5255
// Acquire other objects here so that if we are called during dispose they are available.
5356
this.disposables = this.serviceContainer.get<IDisposableRegistry>(IDisposableRegistry);
@@ -56,6 +59,10 @@ export class PythonExecutionFactory implements IPythonExecutionFactory {
5659
}
5760

5861
public async create(options: ExecutionFactoryCreationOptions): Promise<IPythonExecutionService> {
62+
const interpreterPath = this.interpreterPathExpHelper.get(options.resource);
63+
if (!interpreterPath || interpreterPath === 'python') {
64+
await this.autoSelection.autoSelectInterpreter(options.resource); // Block on this only if no interpreter selected.
65+
}
5966
const pythonPath = options.pythonPath
6067
? options.pythonPath
6168
: this.configService.getSettings(options.resource).pythonPath;

src/client/interpreter/display/index.ts

Lines changed: 1 addition & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -3,17 +3,10 @@ import { Disposable, OutputChannel, StatusBarAlignment, StatusBarItem, Uri } fro
33
import { IApplicationShell, IWorkspaceService } from '../../common/application/types';
44
import { STANDARD_OUTPUT_CHANNEL } from '../../common/constants';
55
import '../../common/extensions';
6-
import {
7-
IDisposableRegistry,
8-
IInterpreterPathProxyService,
9-
IOutputChannel,
10-
IPathUtils,
11-
Resource,
12-
} from '../../common/types';
6+
import { IDisposableRegistry, IOutputChannel, IPathUtils, Resource } from '../../common/types';
137
import { Interpreters } from '../../common/utils/localize';
148
import { IServiceContainer } from '../../ioc/types';
159
import { PythonEnvironment } from '../../pythonEnvironments/info';
16-
import { IInterpreterAutoSelectionService } from '../autoSelection/types';
1710
import {
1811
IInterpreterDisplay,
1912
IInterpreterHelper,
@@ -29,10 +22,8 @@ export class InterpreterDisplay implements IInterpreterDisplay {
2922
private readonly workspaceService: IWorkspaceService;
3023
private readonly pathUtils: IPathUtils;
3124
private readonly interpreterService: IInterpreterService;
32-
private readonly interpreterPathExpHelper: IInterpreterPathProxyService;
3325
private currentlySelectedInterpreterPath?: string;
3426
private currentlySelectedWorkspaceFolder: Resource;
35-
private readonly autoSelection: IInterpreterAutoSelectionService;
3627
private interpreterPath: string | undefined;
3728
private statusBarCanBeDisplayed?: boolean;
3829
private visibilityFilters: IInterpreterStatusbarVisibilityFilter[] = [];
@@ -43,14 +34,10 @@ export class InterpreterDisplay implements IInterpreterDisplay {
4334
this.workspaceService = serviceContainer.get<IWorkspaceService>(IWorkspaceService);
4435
this.pathUtils = serviceContainer.get<IPathUtils>(IPathUtils);
4536
this.interpreterService = serviceContainer.get<IInterpreterService>(IInterpreterService);
46-
this.autoSelection = serviceContainer.get<IInterpreterAutoSelectionService>(IInterpreterAutoSelectionService);
4737
this.python27SupportPrompt = serviceContainer.get<IPython27SupportPrompt>(IPython27SupportPrompt);
4838

4939
const application = serviceContainer.get<IApplicationShell>(IApplicationShell);
5040
const disposableRegistry = serviceContainer.get<Disposable[]>(IDisposableRegistry);
51-
this.interpreterPathExpHelper = serviceContainer.get<IInterpreterPathProxyService>(
52-
IInterpreterPathProxyService,
53-
);
5441

5542
this.statusBar = application.createStatusBarItem(StatusBarAlignment.Left, 100);
5643
this.statusBar.command = 'python.setInterpreter';
@@ -86,10 +73,6 @@ export class InterpreterDisplay implements IInterpreterDisplay {
8673
}
8774
}
8875
private async updateDisplay(workspaceFolder?: Uri) {
89-
const interpreterPath = this.interpreterPathExpHelper.get(workspaceFolder);
90-
if (!interpreterPath || interpreterPath === 'python') {
91-
await this.autoSelection.autoSelectInterpreter(workspaceFolder); // Block on this only if no interpreter selected.
92-
}
9376
const interpreter = await this.interpreterService.getActiveInterpreter(workspaceFolder);
9477
this.currentlySelectedWorkspaceFolder = workspaceFolder;
9578
if (interpreter) {

src/test/common/installer.test.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,7 @@ import {
103103
IFileDownloader,
104104
IHttpClient,
105105
IInstaller,
106+
IInterpreterPathProxyService,
106107
IInterpreterPathService,
107108
IPathUtils,
108109
IPersistentStateFactory,
@@ -122,6 +123,7 @@ import { MockModuleInstaller } from '../mocks/moduleInstaller';
122123
import { MockProcessService } from '../mocks/proc';
123124
import { UnitTestIocContainer } from '../testing/serviceRegistry';
124125
import { closeActiveWindows, initializeTest, IS_MULTI_ROOT_TEST } from '../initialize';
126+
import { InterpreterPathProxyService } from '../../client/common/interpreterPathProxyService';
125127

126128
suite('Installer', () => {
127129
let ioc: UnitTestIocContainer;
@@ -203,6 +205,10 @@ suite('Installer', () => {
203205
);
204206
ioc.serviceManager.addSingleton<IActiveResourceService>(IActiveResourceService, ActiveResourceService);
205207
ioc.serviceManager.addSingleton<IInterpreterPathService>(IInterpreterPathService, InterpreterPathService);
208+
ioc.serviceManager.addSingleton<IInterpreterPathProxyService>(
209+
IInterpreterPathProxyService,
210+
InterpreterPathProxyService,
211+
);
206212
ioc.serviceManager.addSingleton<IExtensions>(IExtensions, Extensions);
207213
ioc.serviceManager.addSingleton<IRandom>(IRandom, Random);
208214
ioc.serviceManager.addSingleton<ITerminalServiceFactory>(ITerminalServiceFactory, TerminalServiceFactory);

src/test/common/moduleInstaller.test.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -102,6 +102,7 @@ import {
102102
IFileDownloader,
103103
IHttpClient,
104104
IInstaller,
105+
IInterpreterPathProxyService,
105106
IInterpreterPathService,
106107
IPathUtils,
107108
IPersistentStateFactory,
@@ -131,6 +132,7 @@ import { MockModuleInstaller } from '../mocks/moduleInstaller';
131132
import { MockProcessService } from '../mocks/proc';
132133
import { UnitTestIocContainer } from '../testing/serviceRegistry';
133134
import { closeActiveWindows, initializeTest } from '../initialize';
135+
import { InterpreterPathProxyService } from '../../client/common/interpreterPathProxyService';
134136

135137
chaiUse(chaiAsPromised);
136138

@@ -228,6 +230,10 @@ suite('Module Installer', () => {
228230

229231
ioc.serviceManager.addSingleton<IActiveResourceService>(IActiveResourceService, ActiveResourceService);
230232
ioc.serviceManager.addSingleton<IInterpreterPathService>(IInterpreterPathService, InterpreterPathService);
233+
ioc.serviceManager.addSingleton<IInterpreterPathProxyService>(
234+
IInterpreterPathProxyService,
235+
InterpreterPathProxyService,
236+
);
231237
ioc.serviceManager.addSingleton<IExtensions>(IExtensions, Extensions);
232238
ioc.serviceManager.addSingleton<IRandom>(IRandom, Random);
233239
ioc.serviceManager.addSingleton<IApplicationShell>(IApplicationShell, ApplicationShell);

src/test/common/process/pythonExecutionFactory.unit.test.ts

Lines changed: 34 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ import * as assert from 'assert';
77
import { expect } from 'chai';
88
import { SemVer } from 'semver';
99
import * as sinon from 'sinon';
10-
import { anyString, anything, instance, mock, verify, when } from 'ts-mockito';
10+
import { anyString, anything, instance, mock, reset, verify, when } from 'ts-mockito';
1111
import * as typemoq from 'typemoq';
1212
import { Uri } from 'vscode';
1313

@@ -24,7 +24,12 @@ import {
2424
IProcessServiceFactory,
2525
IPythonExecutionService,
2626
} from '../../../client/common/process/types';
27-
import { IConfigurationService, IDisposableRegistry, IExperimentService } from '../../../client/common/types';
27+
import {
28+
IConfigurationService,
29+
IDisposableRegistry,
30+
IExperimentService,
31+
IInterpreterPathProxyService,
32+
} from '../../../client/common/types';
2833
import { Architecture } from '../../../client/common/utils/platform';
2934
import { EnvironmentActivationService } from '../../../client/interpreter/activation/service';
3035
import { IEnvironmentActivationService } from '../../../client/interpreter/activation/types';
@@ -42,6 +47,7 @@ import * as ExperimentHelpers from '../../../client/common/experiments/helpers';
4247
import * as WindowsStoreInterpreter from '../../../client/pythonEnvironments/discovery/locators/services/windowsStoreInterpreter';
4348
import { ExperimentService } from '../../../client/common/experiments/service';
4449
import { DiscoveryVariants } from '../../../client/common/experiments/groups';
50+
import { IInterpreterAutoSelectionService } from '../../../client/interpreter/autoSelection/types';
4551

4652
const pythonInterpreter: PythonEnvironment = {
4753
path: '/foo/bar/python.exe',
@@ -96,7 +102,8 @@ suite('Process - PythonExecutionFactory', () => {
96102
let executionService: typemoq.IMock<IPythonExecutionService>;
97103
let isWindowsStoreInterpreterStub: sinon.SinonStub;
98104
let inDiscoveryExperimentStub: sinon.SinonStub;
99-
105+
let autoSelection: IInterpreterAutoSelectionService;
106+
let interpreterPathExpHelper: IInterpreterPathProxyService;
100107
setup(() => {
101108
bufferDecoder = mock(BufferDecoder);
102109
activationHelper = mock(EnvironmentActivationService);
@@ -105,6 +112,9 @@ suite('Process - PythonExecutionFactory', () => {
105112
condaService = mock(CondaService);
106113
condaLocatorService = mock<ICondaLocatorService>();
107114
processLogger = mock(ProcessLogger);
115+
autoSelection = mock<IInterpreterAutoSelectionService>();
116+
interpreterPathExpHelper = mock<IInterpreterPathProxyService>();
117+
when(interpreterPathExpHelper.get(anything())).thenReturn('selected interpreter path');
108118
experimentService = mock(ExperimentService);
109119
when(experimentService.inExperiment(DiscoveryVariants.discoverWithFileWatching)).thenResolve(false);
110120
when(experimentService.inExperiment(DiscoveryVariants.discoveryWithoutFileWatching)).thenResolve(false);
@@ -152,6 +162,8 @@ suite('Process - PythonExecutionFactory', () => {
152162
instance(bufferDecoder),
153163
instance(pyenvs),
154164
instance(experimentService),
165+
instance(autoSelection),
166+
instance(interpreterPathExpHelper),
155167
);
156168

157169
isWindowsStoreInterpreterStub = sinon.stub(WindowsStoreInterpreter, 'isWindowsStoreInterpreter');
@@ -175,6 +187,25 @@ suite('Process - PythonExecutionFactory', () => {
175187
verify(processFactory.create(resource)).once();
176188
verify(pythonSettings.pythonPath).once();
177189
});
190+
191+
test('If no interpreter is explicitly set, ensure we autoselect before PythonExecutionService is created', async () => {
192+
const pythonSettings = mock(PythonSettings);
193+
when(processFactory.create(resource)).thenResolve(processService.object);
194+
when(activationHelper.getActivatedEnvironmentVariables(resource)).thenResolve({ x: '1' });
195+
when(pythonSettings.pythonPath).thenReturn('HELLO');
196+
reset(interpreterPathExpHelper);
197+
when(interpreterPathExpHelper.get(anything())).thenReturn('python');
198+
when(autoSelection.autoSelectInterpreter(anything())).thenResolve();
199+
when(configService.getSettings(resource)).thenReturn(instance(pythonSettings));
200+
201+
const service = await factory.create({ resource });
202+
203+
expect(service).to.not.equal(undefined);
204+
verify(autoSelection.autoSelectInterpreter(anything())).once();
205+
verify(processFactory.create(resource)).once();
206+
verify(pythonSettings.pythonPath).once();
207+
});
208+
178209
test('Ensure we use an existing `create` method if there are no environment variables for the activated env', async () => {
179210
const pythonPath = 'path/to/python';
180211
const pythonSettings = mock(PythonSettings);

src/test/interpreters/display.unit.test.ts

Lines changed: 1 addition & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
import { expect } from 'chai';
22
import * as path from 'path';
33
import { SemVer } from 'semver';
4-
import { anything, instance, mock, verify, when } from 'ts-mockito';
54
import * as TypeMoq from 'typemoq';
65
import {
76
ConfigurationTarget,
@@ -15,17 +14,9 @@ import {
1514
import { IApplicationShell, IWorkspaceService } from '../../client/common/application/types';
1615
import { STANDARD_OUTPUT_CHANNEL } from '../../client/common/constants';
1716
import { IFileSystem } from '../../client/common/platform/types';
18-
import {
19-
IInterpreterPathProxyService,
20-
IDisposableRegistry,
21-
IOutputChannel,
22-
IPathUtils,
23-
ReadWrite,
24-
} from '../../client/common/types';
17+
import { IDisposableRegistry, IOutputChannel, IPathUtils, ReadWrite } from '../../client/common/types';
2518
import { Interpreters } from '../../client/common/utils/localize';
2619
import { Architecture } from '../../client/common/utils/platform';
27-
import { InterpreterAutoSelectionService } from '../../client/interpreter/autoSelection';
28-
import { IInterpreterAutoSelectionService } from '../../client/interpreter/autoSelection/types';
2920
import {
3021
IInterpreterDisplay,
3122
IInterpreterHelper,
@@ -57,12 +48,10 @@ suite('Interpreters Display', () => {
5748
let fileSystem: TypeMoq.IMock<IFileSystem>;
5849
let disposableRegistry: Disposable[];
5950
let statusBar: TypeMoq.IMock<StatusBarItem>;
60-
let interpreterPathProxyService: TypeMoq.IMock<IInterpreterPathProxyService>;
6151
let interpreterDisplay: IInterpreterDisplay;
6252
let interpreterHelper: TypeMoq.IMock<IInterpreterHelper>;
6353
let pathUtils: TypeMoq.IMock<IPathUtils>;
6454
let output: TypeMoq.IMock<IOutputChannel>;
65-
let autoSelection: IInterpreterAutoSelectionService;
6655
let python27SupportPrompt: TypeMoq.IMock<IPython27SupportPrompt>;
6756

6857
setup(() => {
@@ -74,11 +63,9 @@ suite('Interpreters Display', () => {
7463
interpreterHelper = TypeMoq.Mock.ofType<IInterpreterHelper>();
7564
disposableRegistry = [];
7665
statusBar = TypeMoq.Mock.ofType<StatusBarItem>();
77-
interpreterPathProxyService = TypeMoq.Mock.ofType<IInterpreterPathProxyService>();
7866
pathUtils = TypeMoq.Mock.ofType<IPathUtils>();
7967
output = TypeMoq.Mock.ofType<IOutputChannel>();
8068
python27SupportPrompt = TypeMoq.Mock.ofType<IPython27SupportPrompt>();
81-
autoSelection = mock(InterpreterAutoSelectionService);
8269

8370
serviceContainer
8471
.setup((c) => c.get(TypeMoq.It.isValue(IOutputChannel), STANDARD_OUTPUT_CHANNEL))
@@ -94,16 +81,10 @@ suite('Interpreters Display', () => {
9481
.returns(() => interpreterService.object);
9582
serviceContainer.setup((c) => c.get(TypeMoq.It.isValue(IFileSystem))).returns(() => fileSystem.object);
9683
serviceContainer.setup((c) => c.get(TypeMoq.It.isValue(IDisposableRegistry))).returns(() => disposableRegistry);
97-
serviceContainer
98-
.setup((c) => c.get(TypeMoq.It.isValue(IInterpreterPathProxyService)))
99-
.returns(() => interpreterPathProxyService.object);
10084
serviceContainer
10185
.setup((c) => c.get(TypeMoq.It.isValue(IInterpreterHelper)))
10286
.returns(() => interpreterHelper.object);
10387
serviceContainer.setup((c) => c.get(TypeMoq.It.isValue(IPathUtils))).returns(() => pathUtils.object);
104-
serviceContainer
105-
.setup((c) => c.get(TypeMoq.It.isValue(IInterpreterAutoSelectionService)))
106-
.returns(() => instance(autoSelection));
10788
serviceContainer
10889
.setup((c) => c.get(TypeMoq.It.isValue(IPython27SupportPrompt)))
10990
.returns(() => python27SupportPrompt.object);
@@ -148,7 +129,6 @@ suite('Interpreters Display', () => {
148129
path: path.join('user', 'development', 'env', 'bin', 'python'),
149130
};
150131
setupWorkspaceFolder(resource, workspaceFolder);
151-
when(autoSelection.autoSelectInterpreter(anything())).thenResolve();
152132
interpreterService
153133
.setup((i) => i.getInterpreters(TypeMoq.It.isValue(workspaceFolder)))
154134
.returns(() => Promise.resolve([]));
@@ -158,7 +138,6 @@ suite('Interpreters Display', () => {
158138

159139
await interpreterDisplay.refresh(resource);
160140

161-
verify(autoSelection.autoSelectInterpreter(anything())).once();
162141
statusBar.verify((s) => (s.text = TypeMoq.It.isValue(activeInterpreter.displayName)!), TypeMoq.Times.once());
163142
statusBar.verify((s) => (s.tooltip = TypeMoq.It.isValue(activeInterpreter.path)!), TypeMoq.Times.atLeastOnce());
164143
});
@@ -175,7 +154,6 @@ suite('Interpreters Display', () => {
175154
.setup((p) => p.getDisplayName(TypeMoq.It.isAny(), TypeMoq.It.isAny()))
176155
.returns(() => activeInterpreter.path);
177156
setupWorkspaceFolder(resource, workspaceFolder);
178-
when(autoSelection.autoSelectInterpreter(anything())).thenResolve();
179157
interpreterService
180158
.setup((i) => i.getInterpreters(TypeMoq.It.isValue(workspaceFolder)))
181159
.returns(() => Promise.resolve([]));
@@ -223,7 +201,6 @@ suite('Interpreters Display', () => {
223201
interpreterService
224202
.setup((i) => i.getActiveInterpreter(TypeMoq.It.isValue(workspaceFolder)))
225203
.returns(() => Promise.resolve(undefined));
226-
interpreterPathProxyService.setup((c) => c.get(TypeMoq.It.isAny())).returns(() => pythonPath);
227204
fileSystem.setup((f) => f.fileExists(TypeMoq.It.isValue(pythonPath))).returns(() => Promise.resolve(false));
228205
interpreterHelper
229206
.setup((v) => v.getInterpreterInformation(TypeMoq.It.isValue(pythonPath)))
@@ -278,7 +255,6 @@ suite('Interpreters Display', () => {
278255
path: path.join('user', 'development', 'env', 'bin', 'python'),
279256
};
280257
setupWorkspaceFolder(resource, workspaceFolder);
281-
when(autoSelection.autoSelectInterpreter(anything())).thenResolve();
282258
interpreterService
283259
.setup((i) => i.getInterpreters(TypeMoq.It.isValue(workspaceFolder)))
284260
.returns(() => Promise.resolve([]));

0 commit comments

Comments
 (0)