From f3bc7b11178aef6950c668647e037490b9b8ebfb Mon Sep 17 00:00:00 2001 From: Kim-Adeline Miguel Date: Tue, 10 Aug 2021 14:55:51 -0700 Subject: [PATCH 1/4] package.json formatting rip --- package.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/package.json b/package.json index 4c67a2b648e7..1a4fbae7a14f 100644 --- a/package.json +++ b/package.json @@ -197,7 +197,7 @@ "light": "resources/walkthrough/open-folder-light.png", "dark": "resources/walkthrough/open-folder-dark.png", "hc": "resources/walkthrough/open-folder-hc.png" - }, + }, "altText": "Open a folder" }, "completionEvents": [ @@ -214,7 +214,7 @@ "light": "resources/walkthrough/open-folder-light.png", "dark": "resources/walkthrough/open-folder-dark.png", "hc": "resources/walkthrough/open-folder-hc.png" - }, + }, "altText": "Open a folder" }, "completionEvents": [ From 0ee26d95b10072f7f6f7f6d1c9b218ec771ee033 Mon Sep 17 00:00:00 2001 From: Kim-Adeline Miguel Date: Tue, 10 Aug 2021 16:40:31 -0700 Subject: [PATCH 2/4] Add flag to run full autoselection only once --- news/3 Code Health/16735.md | 1 + src/client/interpreter/autoSelection/index.ts | 14 +- .../autoSelection/index.unit.test.ts | 144 ++++++++++++++++++ 3 files changed, 155 insertions(+), 4 deletions(-) create mode 100644 news/3 Code Health/16735.md diff --git a/news/3 Code Health/16735.md b/news/3 Code Health/16735.md new file mode 100644 index 000000000000..2a356067bc0e --- /dev/null +++ b/news/3 Code Health/16735.md @@ -0,0 +1 @@ +Run auto-selection only once, and return the cached value for subsequent calls. diff --git a/src/client/interpreter/autoSelection/index.ts b/src/client/interpreter/autoSelection/index.ts index 59acef376eea..d110c4510ad7 100644 --- a/src/client/interpreter/autoSelection/index.ts +++ b/src/client/interpreter/autoSelection/index.ts @@ -45,6 +45,8 @@ export class InterpreterAutoSelectionService implements IInterpreterAutoSelectio private readonly rules: IInterpreterAutoSelectionRule[] = []; + private didCallInterpreterAutoselection = false; + constructor( @inject(IWorkspaceService) private readonly workspaceService: IWorkspaceService, @inject(IPersistentStateFactory) private readonly stateFactory: IPersistentStateFactory, @@ -127,10 +129,14 @@ export class InterpreterAutoSelectionService implements IInterpreterAutoSelectio await this.initializeStore(resource); await this.clearWorkspaceStoreIfInvalid(resource); - if (await this.experimentService.inExperiment(EnvironmentSorting.experiment)) { - await this.autoselectInterpreterWithLocators(resource); - } else { - await this.autoselectInterpreterWithRules(resource); + if (!this.didCallInterpreterAutoselection) { + if (await this.experimentService.inExperiment(EnvironmentSorting.experiment)) { + await this.autoselectInterpreterWithLocators(resource); + } else { + await this.autoselectInterpreterWithRules(resource); + } + + this.didCallInterpreterAutoselection = true; } deferred.resolve(); diff --git a/src/test/interpreters/autoSelection/index.unit.test.ts b/src/test/interpreters/autoSelection/index.unit.test.ts index ac54badba8a5..5bb42fbe4dec 100644 --- a/src/test/interpreters/autoSelection/index.unit.test.ts +++ b/src/test/interpreters/autoSelection/index.unit.test.ts @@ -167,6 +167,64 @@ suite('Interpreters - Auto Selection', () => { expect(eventFired).to.deep.equal(true, 'event not fired'); verify(userDefinedInterpreter.autoSelectInterpreter(undefined, autoSelectionService)).once(); }); + + test('Rules are called if it is the first time auto-selection is called', async () => { + const resource = Uri.parse('resource'); + const interpreterComparer = new EnvironmentTypeComparer(instance(helper)); + + autoSelectionService = new InterpreterAutoSelectionServiceTest( + instance(workspaceService), + instance(stateFactory), + instance(fs), + instance(experiments), + instance(interpreterService), + interpreterComparer, + instance(systemInterpreter), + instance(currentPathInterpreter), + instance(winRegInterpreter), + instance(cachedPaths), + instance(userDefinedInterpreter), + instance(workspaceInterpreter), + instance(proxy), + instance(helper), + ); + + autoSelectionService.initializeStore = () => Promise.resolve(); + + await autoSelectionService.autoSelectInterpreter(resource); + + verify(userDefinedInterpreter.autoSelectInterpreter(resource, autoSelectionService)).once(); + }); + + test('Rules are only called once if auto-selection is called multiple times', async () => { + const resource = Uri.parse('resource'); + const interpreterComparer = new EnvironmentTypeComparer(instance(helper)); + + autoSelectionService = new InterpreterAutoSelectionServiceTest( + instance(workspaceService), + instance(stateFactory), + instance(fs), + instance(experiments), + instance(interpreterService), + interpreterComparer, + instance(systemInterpreter), + instance(currentPathInterpreter), + instance(winRegInterpreter), + instance(cachedPaths), + instance(userDefinedInterpreter), + instance(workspaceInterpreter), + instance(proxy), + instance(helper), + ); + + autoSelectionService.initializeStore = () => Promise.resolve(); + + await autoSelectionService.autoSelectInterpreter(resource); + + await autoSelectionService.autoSelectInterpreter(resource); + + verify(userDefinedInterpreter.autoSelectInterpreter(resource, autoSelectionService)).once(); + }); }); suite('When using locator-based auto-selection', () => { @@ -267,6 +325,92 @@ suite('Interpreters - Auto Selection', () => { verify(interpreterService.getInterpreters(resource, anything())).once(); verify(state.updateValue(systemEnv)).once(); }); + + test('Locators are called if it is the first time auto-selection is called', async () => { + const interpreterComparer = new EnvironmentTypeComparer(instance(helper)); + + when(interpreterService.getInterpreters(resource, anything())).thenCall(() => + Promise.resolve([ + { + envType: EnvironmentType.Conda, + envPath: path.join('some', 'conda', 'env'), + version: { major: 3, minor: 7, patch: 2 }, + } as PythonEnvironment, + { + envType: EnvironmentType.Pipenv, + envPath: path.join('some', 'pipenv', 'env'), + version: { major: 3, minor: 10, patch: 0 }, + } as PythonEnvironment, + ]), + ); + + autoSelectionService = new InterpreterAutoSelectionServiceTest( + instance(workspaceService), + instance(stateFactory), + instance(fs), + instance(experiments), + instance(interpreterService), + interpreterComparer, + instance(systemInterpreter), + instance(currentPathInterpreter), + instance(winRegInterpreter), + instance(cachedPaths), + instance(userDefinedInterpreter), + instance(workspaceInterpreter), + instance(proxy), + instance(helper), + ); + + autoSelectionService.initializeStore = () => Promise.resolve(); + + await autoSelectionService.autoSelectInterpreter(resource); + + verify(interpreterService.getInterpreters(resource, anything())).once(); + }); + + test('Locators are only called once if auto-selection is called multiple times', async () => { + const interpreterComparer = new EnvironmentTypeComparer(instance(helper)); + + when(interpreterService.getInterpreters(resource, anything())).thenCall(() => + Promise.resolve([ + { + envType: EnvironmentType.Conda, + envPath: path.join('some', 'conda', 'env'), + version: { major: 3, minor: 7, patch: 2 }, + } as PythonEnvironment, + { + envType: EnvironmentType.Pipenv, + envPath: path.join('some', 'pipenv', 'env'), + version: { major: 3, minor: 10, patch: 0 }, + } as PythonEnvironment, + ]), + ); + + autoSelectionService = new InterpreterAutoSelectionServiceTest( + instance(workspaceService), + instance(stateFactory), + instance(fs), + instance(experiments), + instance(interpreterService), + interpreterComparer, + instance(systemInterpreter), + instance(currentPathInterpreter), + instance(winRegInterpreter), + instance(cachedPaths), + instance(userDefinedInterpreter), + instance(workspaceInterpreter), + instance(proxy), + instance(helper), + ); + + autoSelectionService.initializeStore = () => Promise.resolve(); + + await autoSelectionService.autoSelectInterpreter(resource); + + await autoSelectionService.autoSelectInterpreter(resource); + + verify(interpreterService.getInterpreters(resource, anything())).once(); + }); }); test('Initialize the store', async () => { From 869caba73fd8966ad43c31440fbc1e058aa9c810 Mon Sep 17 00:00:00 2001 From: Kim-Adeline Miguel Date: Wed, 11 Aug 2021 13:15:00 -0700 Subject: [PATCH 3/4] Add workspace persistent state key/value --- src/client/interpreter/autoSelection/index.ts | 28 +++--- .../autoSelection/index.unit.test.ts | 98 ++++++------------- 2 files changed, 45 insertions(+), 81 deletions(-) diff --git a/src/client/interpreter/autoSelection/index.ts b/src/client/interpreter/autoSelection/index.ts index d110c4510ad7..2893571296bf 100644 --- a/src/client/interpreter/autoSelection/index.ts +++ b/src/client/interpreter/autoSelection/index.ts @@ -45,8 +45,6 @@ export class InterpreterAutoSelectionService implements IInterpreterAutoSelectio private readonly rules: IInterpreterAutoSelectionRule[] = []; - private didCallInterpreterAutoselection = false; - constructor( @inject(IWorkspaceService) private readonly workspaceService: IWorkspaceService, @inject(IPersistentStateFactory) private readonly stateFactory: IPersistentStateFactory, @@ -129,14 +127,10 @@ export class InterpreterAutoSelectionService implements IInterpreterAutoSelectio await this.initializeStore(resource); await this.clearWorkspaceStoreIfInvalid(resource); - if (!this.didCallInterpreterAutoselection) { - if (await this.experimentService.inExperiment(EnvironmentSorting.experiment)) { - await this.autoselectInterpreterWithLocators(resource); - } else { - await this.autoselectInterpreterWithRules(resource); - } - - this.didCallInterpreterAutoselection = true; + if (await this.experimentService.inExperiment(EnvironmentSorting.experiment)) { + await this.autoselectInterpreterWithLocators(resource); + } else { + await this.autoselectInterpreterWithRules(resource); } deferred.resolve(); @@ -247,6 +241,12 @@ export class InterpreterAutoSelectionService implements IInterpreterAutoSelectio return undefined; } + private getAutoSelectionInterpretersQueryState(): IPersistentState { + const key = `autoSelectionInterpretersQueried`; + + return this.stateFactory.createWorkspacePersistentState(key, undefined); + } + private async autoselectInterpreterWithRules(resource: Resource): Promise { await this.userDefinedInterpreter.autoSelectInterpreter(resource, this); @@ -266,7 +266,11 @@ export class InterpreterAutoSelectionService implements IInterpreterAutoSelectio * As such, we can sort interpreters based on what it returns. */ private async autoselectInterpreterWithLocators(resource: Resource): Promise { - const interpreters = await this.interpreterService.getInterpreters(resource, { ignoreCache: true }); + // Do not perform a full interpreter search if we already have cached interpreters for this workspace. + const queriedState = this.getAutoSelectionInterpretersQueryState(); + const interpreters = await this.interpreterService.getInterpreters(resource, { + ignoreCache: queriedState.value !== true, + }); const workspaceUri = this.interpreterHelper.getActiveWorkspaceUri(resource); // When auto-selecting an intepreter for a workspace, we either want to return a local one @@ -284,6 +288,8 @@ export class InterpreterAutoSelectionService implements IInterpreterAutoSelectio this.setGlobalInterpreter(filteredInterpreters[0]); } + queriedState.updateValue(true); + this.didAutoSelectedInterpreterEmitter.fire(); } } diff --git a/src/test/interpreters/autoSelection/index.unit.test.ts b/src/test/interpreters/autoSelection/index.unit.test.ts index 5bb42fbe4dec..68c8a0253307 100644 --- a/src/test/interpreters/autoSelection/index.unit.test.ts +++ b/src/test/interpreters/autoSelection/index.unit.test.ts @@ -167,64 +167,6 @@ suite('Interpreters - Auto Selection', () => { expect(eventFired).to.deep.equal(true, 'event not fired'); verify(userDefinedInterpreter.autoSelectInterpreter(undefined, autoSelectionService)).once(); }); - - test('Rules are called if it is the first time auto-selection is called', async () => { - const resource = Uri.parse('resource'); - const interpreterComparer = new EnvironmentTypeComparer(instance(helper)); - - autoSelectionService = new InterpreterAutoSelectionServiceTest( - instance(workspaceService), - instance(stateFactory), - instance(fs), - instance(experiments), - instance(interpreterService), - interpreterComparer, - instance(systemInterpreter), - instance(currentPathInterpreter), - instance(winRegInterpreter), - instance(cachedPaths), - instance(userDefinedInterpreter), - instance(workspaceInterpreter), - instance(proxy), - instance(helper), - ); - - autoSelectionService.initializeStore = () => Promise.resolve(); - - await autoSelectionService.autoSelectInterpreter(resource); - - verify(userDefinedInterpreter.autoSelectInterpreter(resource, autoSelectionService)).once(); - }); - - test('Rules are only called once if auto-selection is called multiple times', async () => { - const resource = Uri.parse('resource'); - const interpreterComparer = new EnvironmentTypeComparer(instance(helper)); - - autoSelectionService = new InterpreterAutoSelectionServiceTest( - instance(workspaceService), - instance(stateFactory), - instance(fs), - instance(experiments), - instance(interpreterService), - interpreterComparer, - instance(systemInterpreter), - instance(currentPathInterpreter), - instance(winRegInterpreter), - instance(cachedPaths), - instance(userDefinedInterpreter), - instance(workspaceInterpreter), - instance(proxy), - instance(helper), - ); - - autoSelectionService.initializeStore = () => Promise.resolve(); - - await autoSelectionService.autoSelectInterpreter(resource); - - await autoSelectionService.autoSelectInterpreter(resource); - - verify(userDefinedInterpreter.autoSelectInterpreter(resource, autoSelectionService)).once(); - }); }); suite('When using locator-based auto-selection', () => { @@ -326,11 +268,19 @@ suite('Interpreters - Auto Selection', () => { verify(state.updateValue(systemEnv)).once(); }); - test('Locators are called if it is the first time auto-selection is called', async () => { + test('getInterpreters is called with ignoreCache at true if there is no value set in the workspace persistent state', async () => { const interpreterComparer = new EnvironmentTypeComparer(instance(helper)); + const options: { ignoreCache: boolean }[] = []; + const queryState = mock(PersistentState) as PersistentState; - when(interpreterService.getInterpreters(resource, anything())).thenCall(() => - Promise.resolve([ + when(queryState.value).thenReturn(undefined); + when(stateFactory.createWorkspacePersistentState(anyString(), undefined)).thenReturn( + instance(queryState), + ); + when(interpreterService.getInterpreters(resource, anything())).thenCall((_, opts) => { + options.push(opts); + + return Promise.resolve([ { envType: EnvironmentType.Conda, envPath: path.join('some', 'conda', 'env'), @@ -341,8 +291,8 @@ suite('Interpreters - Auto Selection', () => { envPath: path.join('some', 'pipenv', 'env'), version: { major: 3, minor: 10, patch: 0 }, } as PythonEnvironment, - ]), - ); + ]); + }); autoSelectionService = new InterpreterAutoSelectionServiceTest( instance(workspaceService), @@ -366,13 +316,22 @@ suite('Interpreters - Auto Selection', () => { await autoSelectionService.autoSelectInterpreter(resource); verify(interpreterService.getInterpreters(resource, anything())).once(); + expect(options).to.deep.equal([{ ignoreCache: true }], 'getInterpreters options are different'); }); - test('Locators are only called once if auto-selection is called multiple times', async () => { + test('getInterpreters is called with ignoreCache at false if there is a value set in the workspace persistent state', async () => { const interpreterComparer = new EnvironmentTypeComparer(instance(helper)); + const options: { ignoreCache: boolean }[] = []; + const queryState = mock(PersistentState) as PersistentState; - when(interpreterService.getInterpreters(resource, anything())).thenCall(() => - Promise.resolve([ + when(queryState.value).thenReturn(true); + when(stateFactory.createWorkspacePersistentState(anyString(), undefined)).thenReturn( + instance(queryState), + ); + when(interpreterService.getInterpreters(resource, anything())).thenCall((_, opts) => { + options.push(opts); + + return Promise.resolve([ { envType: EnvironmentType.Conda, envPath: path.join('some', 'conda', 'env'), @@ -383,8 +342,8 @@ suite('Interpreters - Auto Selection', () => { envPath: path.join('some', 'pipenv', 'env'), version: { major: 3, minor: 10, patch: 0 }, } as PythonEnvironment, - ]), - ); + ]); + }); autoSelectionService = new InterpreterAutoSelectionServiceTest( instance(workspaceService), @@ -407,9 +366,8 @@ suite('Interpreters - Auto Selection', () => { await autoSelectionService.autoSelectInterpreter(resource); - await autoSelectionService.autoSelectInterpreter(resource); - verify(interpreterService.getInterpreters(resource, anything())).once(); + expect(options).to.deep.equal([{ ignoreCache: false }], 'getInterpreters options are different'); }); }); From ee5ecdd7ea7940d21a0638f0e94d23bba9d4db7a Mon Sep 17 00:00:00 2001 From: Kim-Adeline Miguel Date: Wed, 11 Aug 2021 15:45:53 -0700 Subject: [PATCH 4/4] Make key folder-specific --- src/client/interpreter/autoSelection/index.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/client/interpreter/autoSelection/index.ts b/src/client/interpreter/autoSelection/index.ts index 2893571296bf..bb50c30f3992 100644 --- a/src/client/interpreter/autoSelection/index.ts +++ b/src/client/interpreter/autoSelection/index.ts @@ -241,8 +241,9 @@ export class InterpreterAutoSelectionService implements IInterpreterAutoSelectio return undefined; } - private getAutoSelectionInterpretersQueryState(): IPersistentState { - const key = `autoSelectionInterpretersQueried`; + private getAutoSelectionInterpretersQueryState(resource: Resource): IPersistentState { + const workspaceUri = this.interpreterHelper.getActiveWorkspaceUri(resource); + const key = `autoSelectionInterpretersQueried-${workspaceUri?.folderUri.fsPath || 'global'}`; return this.stateFactory.createWorkspacePersistentState(key, undefined); } @@ -267,7 +268,7 @@ export class InterpreterAutoSelectionService implements IInterpreterAutoSelectio */ private async autoselectInterpreterWithLocators(resource: Resource): Promise { // Do not perform a full interpreter search if we already have cached interpreters for this workspace. - const queriedState = this.getAutoSelectionInterpretersQueryState(); + const queriedState = this.getAutoSelectionInterpretersQueryState(resource); const interpreters = await this.interpreterService.getInterpreters(resource, { ignoreCache: queriedState.value !== true, });