diff --git a/news/2 Fixes/17370.md b/news/2 Fixes/17370.md new file mode 100644 index 000000000000..e3e57aba0c01 --- /dev/null +++ b/news/2 Fixes/17370.md @@ -0,0 +1 @@ +Ensure we block getting active interpreter on auto-selection. diff --git a/src/client/common/process/pythonExecutionFactory.ts b/src/client/common/process/pythonExecutionFactory.ts index a3c74891ef59..fb66ef4f7d09 100644 --- a/src/client/common/process/pythonExecutionFactory.ts +++ b/src/client/common/process/pythonExecutionFactory.ts @@ -12,7 +12,7 @@ import { inDiscoveryExperiment } from '../experiments/helpers'; import { sendTelemetryEvent } from '../../telemetry'; import { EventName } from '../../telemetry/constants'; import { IFileSystem } from '../platform/types'; -import { IConfigurationService, IDisposableRegistry, IExperimentService } from '../types'; +import { IConfigurationService, IDisposableRegistry, IExperimentService, IInterpreterPathProxyService } from '../types'; import { ProcessService } from './proc'; import { createCondaEnv, createPythonEnv, createWindowsStoreEnv } from './pythonEnvironment'; import { createPythonProcessService } from './pythonProcess'; @@ -27,6 +27,7 @@ import { IPythonExecutionService, } from './types'; import { isWindowsStoreInterpreter } from '../../pythonEnvironments/discovery/locators/services/windowsStoreInterpreter'; +import { IInterpreterAutoSelectionService } from '../../interpreter/autoSelection/types'; // Minimum version number of conda required to be able to use 'conda run' export const CONDA_RUN_VERSION = '4.6.0'; @@ -48,6 +49,8 @@ export class PythonExecutionFactory implements IPythonExecutionFactory { @inject(IBufferDecoder) private readonly decoder: IBufferDecoder, @inject(IComponentAdapter) private readonly pyenvs: IComponentAdapter, @inject(IExperimentService) private readonly experimentService: IExperimentService, + @inject(IInterpreterAutoSelectionService) private readonly autoSelection: IInterpreterAutoSelectionService, + @inject(IInterpreterPathProxyService) private readonly interpreterPathExpHelper: IInterpreterPathProxyService, ) { // Acquire other objects here so that if we are called during dispose they are available. this.disposables = this.serviceContainer.get(IDisposableRegistry); @@ -56,6 +59,10 @@ export class PythonExecutionFactory implements IPythonExecutionFactory { } public async create(options: ExecutionFactoryCreationOptions): Promise { + const interpreterPath = this.interpreterPathExpHelper.get(options.resource); + if (!interpreterPath || interpreterPath === 'python') { + await this.autoSelection.autoSelectInterpreter(options.resource); // Block on this only if no interpreter selected. + } const pythonPath = options.pythonPath ? options.pythonPath : this.configService.getSettings(options.resource).pythonPath; diff --git a/src/client/interpreter/display/index.ts b/src/client/interpreter/display/index.ts index 21aa292ffbf0..c7a725887684 100644 --- a/src/client/interpreter/display/index.ts +++ b/src/client/interpreter/display/index.ts @@ -3,17 +3,10 @@ import { Disposable, OutputChannel, StatusBarAlignment, StatusBarItem, Uri } fro import { IApplicationShell, IWorkspaceService } from '../../common/application/types'; import { STANDARD_OUTPUT_CHANNEL } from '../../common/constants'; import '../../common/extensions'; -import { - IDisposableRegistry, - IInterpreterPathProxyService, - IOutputChannel, - IPathUtils, - Resource, -} from '../../common/types'; +import { IDisposableRegistry, IOutputChannel, IPathUtils, Resource } from '../../common/types'; import { Interpreters } from '../../common/utils/localize'; import { IServiceContainer } from '../../ioc/types'; import { PythonEnvironment } from '../../pythonEnvironments/info'; -import { IInterpreterAutoSelectionService } from '../autoSelection/types'; import { IInterpreterDisplay, IInterpreterHelper, @@ -29,10 +22,8 @@ export class InterpreterDisplay implements IInterpreterDisplay { private readonly workspaceService: IWorkspaceService; private readonly pathUtils: IPathUtils; private readonly interpreterService: IInterpreterService; - private readonly interpreterPathExpHelper: IInterpreterPathProxyService; private currentlySelectedInterpreterPath?: string; private currentlySelectedWorkspaceFolder: Resource; - private readonly autoSelection: IInterpreterAutoSelectionService; private interpreterPath: string | undefined; private statusBarCanBeDisplayed?: boolean; private visibilityFilters: IInterpreterStatusbarVisibilityFilter[] = []; @@ -43,14 +34,10 @@ export class InterpreterDisplay implements IInterpreterDisplay { this.workspaceService = serviceContainer.get(IWorkspaceService); this.pathUtils = serviceContainer.get(IPathUtils); this.interpreterService = serviceContainer.get(IInterpreterService); - this.autoSelection = serviceContainer.get(IInterpreterAutoSelectionService); this.python27SupportPrompt = serviceContainer.get(IPython27SupportPrompt); const application = serviceContainer.get(IApplicationShell); const disposableRegistry = serviceContainer.get(IDisposableRegistry); - this.interpreterPathExpHelper = serviceContainer.get( - IInterpreterPathProxyService, - ); this.statusBar = application.createStatusBarItem(StatusBarAlignment.Left, 100); this.statusBar.command = 'python.setInterpreter'; @@ -86,10 +73,6 @@ export class InterpreterDisplay implements IInterpreterDisplay { } } private async updateDisplay(workspaceFolder?: Uri) { - const interpreterPath = this.interpreterPathExpHelper.get(workspaceFolder); - if (!interpreterPath || interpreterPath === 'python') { - await this.autoSelection.autoSelectInterpreter(workspaceFolder); // Block on this only if no interpreter selected. - } const interpreter = await this.interpreterService.getActiveInterpreter(workspaceFolder); this.currentlySelectedWorkspaceFolder = workspaceFolder; if (interpreter) { diff --git a/src/test/common/installer.test.ts b/src/test/common/installer.test.ts index c8f46a1aca4c..93d498fbc594 100644 --- a/src/test/common/installer.test.ts +++ b/src/test/common/installer.test.ts @@ -103,6 +103,7 @@ import { IFileDownloader, IHttpClient, IInstaller, + IInterpreterPathProxyService, IInterpreterPathService, IPathUtils, IPersistentStateFactory, @@ -122,6 +123,7 @@ import { MockModuleInstaller } from '../mocks/moduleInstaller'; import { MockProcessService } from '../mocks/proc'; import { UnitTestIocContainer } from '../testing/serviceRegistry'; import { closeActiveWindows, initializeTest, IS_MULTI_ROOT_TEST } from '../initialize'; +import { InterpreterPathProxyService } from '../../client/common/interpreterPathProxyService'; suite('Installer', () => { let ioc: UnitTestIocContainer; @@ -203,6 +205,10 @@ suite('Installer', () => { ); ioc.serviceManager.addSingleton(IActiveResourceService, ActiveResourceService); ioc.serviceManager.addSingleton(IInterpreterPathService, InterpreterPathService); + ioc.serviceManager.addSingleton( + IInterpreterPathProxyService, + InterpreterPathProxyService, + ); ioc.serviceManager.addSingleton(IExtensions, Extensions); ioc.serviceManager.addSingleton(IRandom, Random); ioc.serviceManager.addSingleton(ITerminalServiceFactory, TerminalServiceFactory); diff --git a/src/test/common/moduleInstaller.test.ts b/src/test/common/moduleInstaller.test.ts index 832003031c8c..91c0afd32997 100644 --- a/src/test/common/moduleInstaller.test.ts +++ b/src/test/common/moduleInstaller.test.ts @@ -102,6 +102,7 @@ import { IFileDownloader, IHttpClient, IInstaller, + IInterpreterPathProxyService, IInterpreterPathService, IPathUtils, IPersistentStateFactory, @@ -131,6 +132,7 @@ import { MockModuleInstaller } from '../mocks/moduleInstaller'; import { MockProcessService } from '../mocks/proc'; import { UnitTestIocContainer } from '../testing/serviceRegistry'; import { closeActiveWindows, initializeTest } from '../initialize'; +import { InterpreterPathProxyService } from '../../client/common/interpreterPathProxyService'; chaiUse(chaiAsPromised); @@ -228,6 +230,10 @@ suite('Module Installer', () => { ioc.serviceManager.addSingleton(IActiveResourceService, ActiveResourceService); ioc.serviceManager.addSingleton(IInterpreterPathService, InterpreterPathService); + ioc.serviceManager.addSingleton( + IInterpreterPathProxyService, + InterpreterPathProxyService, + ); ioc.serviceManager.addSingleton(IExtensions, Extensions); ioc.serviceManager.addSingleton(IRandom, Random); ioc.serviceManager.addSingleton(IApplicationShell, ApplicationShell); diff --git a/src/test/common/process/pythonExecutionFactory.unit.test.ts b/src/test/common/process/pythonExecutionFactory.unit.test.ts index cbf2a0dddbb2..bccda1f8c724 100644 --- a/src/test/common/process/pythonExecutionFactory.unit.test.ts +++ b/src/test/common/process/pythonExecutionFactory.unit.test.ts @@ -7,7 +7,7 @@ import * as assert from 'assert'; import { expect } from 'chai'; import { SemVer } from 'semver'; import * as sinon from 'sinon'; -import { anyString, anything, instance, mock, verify, when } from 'ts-mockito'; +import { anyString, anything, instance, mock, reset, verify, when } from 'ts-mockito'; import * as typemoq from 'typemoq'; import { Uri } from 'vscode'; @@ -24,7 +24,12 @@ import { IProcessServiceFactory, IPythonExecutionService, } from '../../../client/common/process/types'; -import { IConfigurationService, IDisposableRegistry, IExperimentService } from '../../../client/common/types'; +import { + IConfigurationService, + IDisposableRegistry, + IExperimentService, + IInterpreterPathProxyService, +} from '../../../client/common/types'; import { Architecture } from '../../../client/common/utils/platform'; import { EnvironmentActivationService } from '../../../client/interpreter/activation/service'; import { IEnvironmentActivationService } from '../../../client/interpreter/activation/types'; @@ -42,6 +47,7 @@ import * as ExperimentHelpers from '../../../client/common/experiments/helpers'; import * as WindowsStoreInterpreter from '../../../client/pythonEnvironments/discovery/locators/services/windowsStoreInterpreter'; import { ExperimentService } from '../../../client/common/experiments/service'; import { DiscoveryVariants } from '../../../client/common/experiments/groups'; +import { IInterpreterAutoSelectionService } from '../../../client/interpreter/autoSelection/types'; const pythonInterpreter: PythonEnvironment = { path: '/foo/bar/python.exe', @@ -96,7 +102,8 @@ suite('Process - PythonExecutionFactory', () => { let executionService: typemoq.IMock; let isWindowsStoreInterpreterStub: sinon.SinonStub; let inDiscoveryExperimentStub: sinon.SinonStub; - + let autoSelection: IInterpreterAutoSelectionService; + let interpreterPathExpHelper: IInterpreterPathProxyService; setup(() => { bufferDecoder = mock(BufferDecoder); activationHelper = mock(EnvironmentActivationService); @@ -105,6 +112,9 @@ suite('Process - PythonExecutionFactory', () => { condaService = mock(CondaService); condaLocatorService = mock(); processLogger = mock(ProcessLogger); + autoSelection = mock(); + interpreterPathExpHelper = mock(); + when(interpreterPathExpHelper.get(anything())).thenReturn('selected interpreter path'); experimentService = mock(ExperimentService); when(experimentService.inExperiment(DiscoveryVariants.discoverWithFileWatching)).thenResolve(false); when(experimentService.inExperiment(DiscoveryVariants.discoveryWithoutFileWatching)).thenResolve(false); @@ -152,6 +162,8 @@ suite('Process - PythonExecutionFactory', () => { instance(bufferDecoder), instance(pyenvs), instance(experimentService), + instance(autoSelection), + instance(interpreterPathExpHelper), ); isWindowsStoreInterpreterStub = sinon.stub(WindowsStoreInterpreter, 'isWindowsStoreInterpreter'); @@ -175,6 +187,25 @@ suite('Process - PythonExecutionFactory', () => { verify(processFactory.create(resource)).once(); verify(pythonSettings.pythonPath).once(); }); + + test('If no interpreter is explicitly set, ensure we autoselect before PythonExecutionService is created', async () => { + const pythonSettings = mock(PythonSettings); + when(processFactory.create(resource)).thenResolve(processService.object); + when(activationHelper.getActivatedEnvironmentVariables(resource)).thenResolve({ x: '1' }); + when(pythonSettings.pythonPath).thenReturn('HELLO'); + reset(interpreterPathExpHelper); + when(interpreterPathExpHelper.get(anything())).thenReturn('python'); + when(autoSelection.autoSelectInterpreter(anything())).thenResolve(); + when(configService.getSettings(resource)).thenReturn(instance(pythonSettings)); + + const service = await factory.create({ resource }); + + expect(service).to.not.equal(undefined); + verify(autoSelection.autoSelectInterpreter(anything())).once(); + verify(processFactory.create(resource)).once(); + verify(pythonSettings.pythonPath).once(); + }); + test('Ensure we use an existing `create` method if there are no environment variables for the activated env', async () => { const pythonPath = 'path/to/python'; const pythonSettings = mock(PythonSettings); diff --git a/src/test/interpreters/display.unit.test.ts b/src/test/interpreters/display.unit.test.ts index c13f49eff3f3..3aeeb000d11c 100644 --- a/src/test/interpreters/display.unit.test.ts +++ b/src/test/interpreters/display.unit.test.ts @@ -1,7 +1,6 @@ import { expect } from 'chai'; import * as path from 'path'; import { SemVer } from 'semver'; -import { anything, instance, mock, verify, when } from 'ts-mockito'; import * as TypeMoq from 'typemoq'; import { ConfigurationTarget, @@ -15,17 +14,9 @@ import { import { IApplicationShell, IWorkspaceService } from '../../client/common/application/types'; import { STANDARD_OUTPUT_CHANNEL } from '../../client/common/constants'; import { IFileSystem } from '../../client/common/platform/types'; -import { - IInterpreterPathProxyService, - IDisposableRegistry, - IOutputChannel, - IPathUtils, - ReadWrite, -} from '../../client/common/types'; +import { IDisposableRegistry, IOutputChannel, IPathUtils, ReadWrite } from '../../client/common/types'; import { Interpreters } from '../../client/common/utils/localize'; import { Architecture } from '../../client/common/utils/platform'; -import { InterpreterAutoSelectionService } from '../../client/interpreter/autoSelection'; -import { IInterpreterAutoSelectionService } from '../../client/interpreter/autoSelection/types'; import { IInterpreterDisplay, IInterpreterHelper, @@ -57,12 +48,10 @@ suite('Interpreters Display', () => { let fileSystem: TypeMoq.IMock; let disposableRegistry: Disposable[]; let statusBar: TypeMoq.IMock; - let interpreterPathProxyService: TypeMoq.IMock; let interpreterDisplay: IInterpreterDisplay; let interpreterHelper: TypeMoq.IMock; let pathUtils: TypeMoq.IMock; let output: TypeMoq.IMock; - let autoSelection: IInterpreterAutoSelectionService; let python27SupportPrompt: TypeMoq.IMock; setup(() => { @@ -74,11 +63,9 @@ suite('Interpreters Display', () => { interpreterHelper = TypeMoq.Mock.ofType(); disposableRegistry = []; statusBar = TypeMoq.Mock.ofType(); - interpreterPathProxyService = TypeMoq.Mock.ofType(); pathUtils = TypeMoq.Mock.ofType(); output = TypeMoq.Mock.ofType(); python27SupportPrompt = TypeMoq.Mock.ofType(); - autoSelection = mock(InterpreterAutoSelectionService); serviceContainer .setup((c) => c.get(TypeMoq.It.isValue(IOutputChannel), STANDARD_OUTPUT_CHANNEL)) @@ -94,16 +81,10 @@ suite('Interpreters Display', () => { .returns(() => interpreterService.object); serviceContainer.setup((c) => c.get(TypeMoq.It.isValue(IFileSystem))).returns(() => fileSystem.object); serviceContainer.setup((c) => c.get(TypeMoq.It.isValue(IDisposableRegistry))).returns(() => disposableRegistry); - serviceContainer - .setup((c) => c.get(TypeMoq.It.isValue(IInterpreterPathProxyService))) - .returns(() => interpreterPathProxyService.object); serviceContainer .setup((c) => c.get(TypeMoq.It.isValue(IInterpreterHelper))) .returns(() => interpreterHelper.object); serviceContainer.setup((c) => c.get(TypeMoq.It.isValue(IPathUtils))).returns(() => pathUtils.object); - serviceContainer - .setup((c) => c.get(TypeMoq.It.isValue(IInterpreterAutoSelectionService))) - .returns(() => instance(autoSelection)); serviceContainer .setup((c) => c.get(TypeMoq.It.isValue(IPython27SupportPrompt))) .returns(() => python27SupportPrompt.object); @@ -148,7 +129,6 @@ suite('Interpreters Display', () => { path: path.join('user', 'development', 'env', 'bin', 'python'), }; setupWorkspaceFolder(resource, workspaceFolder); - when(autoSelection.autoSelectInterpreter(anything())).thenResolve(); interpreterService .setup((i) => i.getInterpreters(TypeMoq.It.isValue(workspaceFolder))) .returns(() => Promise.resolve([])); @@ -158,7 +138,6 @@ suite('Interpreters Display', () => { await interpreterDisplay.refresh(resource); - verify(autoSelection.autoSelectInterpreter(anything())).once(); statusBar.verify((s) => (s.text = TypeMoq.It.isValue(activeInterpreter.displayName)!), TypeMoq.Times.once()); statusBar.verify((s) => (s.tooltip = TypeMoq.It.isValue(activeInterpreter.path)!), TypeMoq.Times.atLeastOnce()); }); @@ -175,7 +154,6 @@ suite('Interpreters Display', () => { .setup((p) => p.getDisplayName(TypeMoq.It.isAny(), TypeMoq.It.isAny())) .returns(() => activeInterpreter.path); setupWorkspaceFolder(resource, workspaceFolder); - when(autoSelection.autoSelectInterpreter(anything())).thenResolve(); interpreterService .setup((i) => i.getInterpreters(TypeMoq.It.isValue(workspaceFolder))) .returns(() => Promise.resolve([])); @@ -223,7 +201,6 @@ suite('Interpreters Display', () => { interpreterService .setup((i) => i.getActiveInterpreter(TypeMoq.It.isValue(workspaceFolder))) .returns(() => Promise.resolve(undefined)); - interpreterPathProxyService.setup((c) => c.get(TypeMoq.It.isAny())).returns(() => pythonPath); fileSystem.setup((f) => f.fileExists(TypeMoq.It.isValue(pythonPath))).returns(() => Promise.resolve(false)); interpreterHelper .setup((v) => v.getInterpreterInformation(TypeMoq.It.isValue(pythonPath))) @@ -278,7 +255,6 @@ suite('Interpreters Display', () => { path: path.join('user', 'development', 'env', 'bin', 'python'), }; setupWorkspaceFolder(resource, workspaceFolder); - when(autoSelection.autoSelectInterpreter(anything())).thenResolve(); interpreterService .setup((i) => i.getInterpreters(TypeMoq.It.isValue(workspaceFolder))) .returns(() => Promise.resolve([])); diff --git a/src/test/linters/lint.functional.test.ts b/src/test/linters/lint.functional.test.ts index 659a723d8662..0bd5e9ba1a50 100644 --- a/src/test/linters/lint.functional.test.ts +++ b/src/test/linters/lint.functional.test.ts @@ -9,7 +9,7 @@ import * as fs from 'fs-extra'; import * as os from 'os'; import * as path from 'path'; import * as sinon from 'sinon'; -import { instance, mock } from 'ts-mockito'; +import { anything, instance, mock, when } from 'ts-mockito'; import * as TypeMoq from 'typemoq'; import { CancellationTokenSource, TextDocument, TextLine, Uri } from 'vscode'; import { Product } from '../../client/common/installer/productInstaller'; @@ -26,7 +26,12 @@ import { IPythonExecutionFactory, IPythonToolExecutionService, } from '../../client/common/process/types'; -import { IConfigurationService, IDisposableRegistry, IExperimentService } from '../../client/common/types'; +import { + IConfigurationService, + IDisposableRegistry, + IExperimentService, + IInterpreterPathProxyService, +} from '../../client/common/types'; import { IEnvironmentVariablesProvider } from '../../client/common/variables/types'; import { IEnvironmentActivationService } from '../../client/interpreter/activation/types'; import { @@ -42,6 +47,7 @@ import { deleteFile, PYTHON_PATH } from '../common'; import { BaseTestFixture, getLinterID, getProductName, newMockDocument, throwUnknownProduct } from './common'; import * as ExperimentHelpers from '../../client/common/experiments/helpers'; import { DiscoveryVariants } from '../../client/common/experiments/groups'; +import { IInterpreterAutoSelectionService } from '../../client/interpreter/autoSelection/types'; const workspaceDir = path.join(__dirname, '..', '..', '..', 'src', 'test'); const workspaceUri = Uri.file(workspaceDir); @@ -735,6 +741,10 @@ class TestFixture extends BaseTestFixture { const inDiscoveryExperimentStub = sinon.stub(ExperimentHelpers, 'inDiscoveryExperiment'); inDiscoveryExperimentStub.resolves(false); + const autoSelection = mock(); + const interpreterPathExpHelper = mock(); + when(interpreterPathExpHelper.get(anything())).thenReturn('selected interpreter path'); + return new PythonExecutionFactory( serviceContainer.object, envActivationService.object, @@ -744,6 +754,8 @@ class TestFixture extends BaseTestFixture { decoder, instance(pyenvs), experimentService.object, + instance(autoSelection), + instance(interpreterPathExpHelper), ); } diff --git a/src/test/refactor/rename.test.ts b/src/test/refactor/rename.test.ts index 6599e0e36de9..fd91b6e4dea8 100644 --- a/src/test/refactor/rename.test.ts +++ b/src/test/refactor/rename.test.ts @@ -6,7 +6,7 @@ import { expect } from 'chai'; import { EOL } from 'os'; import * as path from 'path'; -import { instance, mock } from 'ts-mockito'; +import { anything, instance, mock, when } from 'ts-mockito'; import * as typeMoq from 'typemoq'; import { Range, @@ -29,8 +29,14 @@ import { IPythonExecutionFactory, IPythonExecutionService, } from '../../client/common/process/types'; -import { IConfigurationService, IExperimentService, IPythonSettings } from '../../client/common/types'; +import { + IConfigurationService, + IExperimentService, + IInterpreterPathProxyService, + IPythonSettings, +} from '../../client/common/types'; import { IEnvironmentActivationService } from '../../client/interpreter/activation/types'; +import { IInterpreterAutoSelectionService } from '../../client/interpreter/autoSelection/types'; import { IComponentAdapter, ICondaService, IInterpreterService } from '../../client/interpreter/contracts'; import { IServiceContainer } from '../../client/ioc/types'; import { RefactorProxy } from '../../client/refactor/proxy'; @@ -96,6 +102,10 @@ suite('Refactor Rename', () => { .setup((e) => e.inExperiment(DiscoveryVariants.discoverWithFileWatching)) .returns(() => Promise.resolve(false)); + const autoSelection = mock(); + const interpreterPathExpHelper = mock(); + when(interpreterPathExpHelper.get(anything())).thenReturn('selected interpreter path'); + serviceContainer .setup((s) => s.get(typeMoq.It.isValue(IPythonExecutionFactory), typeMoq.It.isAny())) .returns( @@ -111,6 +121,8 @@ suite('Refactor Rename', () => { undefined as any, instance(pyenvs), experimentService.object, + instance(autoSelection), + instance(interpreterPathExpHelper), ), ); const processLogger = typeMoq.Mock.ofType();