diff --git a/src/client/apiTypes.ts b/src/client/apiTypes.ts index 847b215bce49..6361a75edb48 100644 --- a/src/client/apiTypes.ts +++ b/src/client/apiTypes.ts @@ -121,10 +121,6 @@ export interface ActiveEnvironmentChangedParams { resource?: Uri; } -export interface RefreshEnvironmentsOptions { - clearCache?: boolean; -} - export interface IProposedExtensionAPI { environment: { /** @@ -203,7 +199,7 @@ export interface IProposedExtensionAPI { * * clearCache : When true, this will clear the cache before environment refresh * is triggered. */ - refreshEnvironment(options?: RefreshEnvironmentsOptions): Promise; + refreshEnvironment(): Promise; /** * Tracks discovery progress for current list of known environments, i.e when it starts, finishes or any other relevant * stage. Note the progress for a particular query is currently not tracked or reported, this only indicates progress of diff --git a/src/client/common/constants.ts b/src/client/common/constants.ts index 30ebb88c36a9..a5570b28e5da 100644 --- a/src/client/common/constants.ts +++ b/src/client/common/constants.ts @@ -85,7 +85,6 @@ export namespace Octicons { * to change the icons. */ export namespace ThemeIcons { - export const ClearAll = 'clear-all'; export const Refresh = 'refresh'; export const SpinningLoader = 'loading~spin'; } diff --git a/src/client/common/utils/localize.ts b/src/client/common/utils/localize.ts index a45c93270c5f..2928f640d1be 100644 --- a/src/client/common/utils/localize.ts +++ b/src/client/common/utils/localize.ts @@ -317,10 +317,6 @@ export namespace InterpreterQuickPickList { 'InterpreterQuickPickList.refreshInterpreterList', 'Refresh Interpreter list', ); - export const clearAllAndRefreshInterpreterList = localize( - 'InterpreterQuickPickList.clearAllAndRefreshInterpreterList', - 'Clear all and Refresh Interpreter list', - ); export const refreshingInterpreterList = localize( 'InterpreterQuickPickList.refreshingInterpreterList', 'Refreshing Interpreter list...', diff --git a/src/client/interpreter/configuration/interpreterSelector/commands/setInterpreter.ts b/src/client/interpreter/configuration/interpreterSelector/commands/setInterpreter.ts index c918e99bffbe..1d6bd31edf29 100644 --- a/src/client/interpreter/configuration/interpreterSelector/commands/setInterpreter.ts +++ b/src/client/interpreter/configuration/interpreterSelector/commands/setInterpreter.ts @@ -80,11 +80,6 @@ export class SetInterpreterCommand extends BaseInterpreterSelectorCommand { tooltip: InterpreterQuickPickList.refreshInterpreterList, }; - private readonly hardRefreshButton = { - iconPath: new ThemeIcon(ThemeIcons.ClearAll), - tooltip: InterpreterQuickPickList.clearAllAndRefreshInterpreterList, - }; - private readonly noPythonInstalled: ISpecialQuickPickItem = { label: `${Octicons.Error} ${InterpreterQuickPickList.noPythonInstalled}`, detail: InterpreterQuickPickList.clickForInstructions, @@ -155,16 +150,10 @@ export class SetInterpreterCommand extends BaseInterpreterSelectorCommand { matchOnDescription: true, title: InterpreterQuickPickList.browsePath.openButtonLabel, customButtonSetups: [ - { - button: this.hardRefreshButton, - callback: (quickpickInput) => { - this.refreshButtonCallback(quickpickInput, true); - }, - }, { button: this.refreshButton, callback: (quickpickInput) => { - this.refreshButtonCallback(quickpickInput, false); + this.refreshButtonCallback(quickpickInput); }, }, ], @@ -418,7 +407,7 @@ export class SetInterpreterCommand extends BaseInterpreterSelectorCommand { } } - private refreshButtonCallback(input: QuickPick, clearCache: boolean) { + private refreshButtonCallback(input: QuickPick) { input.buttons = [ { iconPath: new ThemeIcon(ThemeIcons.SpinningLoader), @@ -426,9 +415,9 @@ export class SetInterpreterCommand extends BaseInterpreterSelectorCommand { }, ]; this.interpreterService - .triggerRefresh(undefined, { clearCache }) + .triggerRefresh() .finally(() => { - input.buttons = [this.hardRefreshButton, this.refreshButton]; + input.buttons = [this.refreshButton]; }) .ignoreErrors(); } diff --git a/src/client/interpreter/interpreterService.ts b/src/client/interpreter/interpreterService.ts index 1f563125162d..cc4bf786dd6d 100644 --- a/src/client/interpreter/interpreterService.ts +++ b/src/client/interpreter/interpreterService.ts @@ -244,7 +244,7 @@ export class InterpreterService implements Disposable, IInterpreterService { traceLog('Conda envs without Python are known to not work well; fixing conda environment...'); const promise = installer.install(Product.python, await this.getInterpreterDetails(pythonPath)); shell.withProgress(progressOptions, () => promise); - promise.then(() => this.triggerRefresh(undefined, { clearCache: true }).ignoreErrors()); + promise.then(() => this.triggerRefresh().ignoreErrors()); } } } diff --git a/src/client/proposedApi.ts b/src/client/proposedApi.ts index e4ac6fd83caa..fc432efeb821 100644 --- a/src/client/proposedApi.ts +++ b/src/client/proposedApi.ts @@ -8,7 +8,6 @@ import { EnvironmentDetailsOptions, EnvironmentsChangedParams, IProposedExtensionAPI, - RefreshEnvironmentsOptions, } from './apiTypes'; import { arePathsSame } from './common/platform/fs-paths'; import { IInterpreterPathService, Resource } from './common/types'; @@ -101,8 +100,8 @@ export function buildProposedApi( setActiveEnvironment(path: string, resource?: Resource): Promise { return interpreterPathService.update(resource, ConfigurationTarget.WorkspaceFolder, path); }, - async refreshEnvironment(options?: RefreshEnvironmentsOptions) { - await discoveryApi.triggerRefresh(undefined, options ? { clearCache: options.clearCache } : undefined); + async refreshEnvironment() { + await discoveryApi.triggerRefresh(); const paths = discoveryApi.getEnvs().map((e) => getEnvPath(e.executable.filename, e.location)); return Promise.resolve(paths); }, diff --git a/src/client/pythonEnvironments/base/locator.ts b/src/client/pythonEnvironments/base/locator.ts index 609010501d63..c0d1cd23991c 100644 --- a/src/client/pythonEnvironments/base/locator.ts +++ b/src/client/pythonEnvironments/base/locator.ts @@ -194,10 +194,6 @@ export interface GetRefreshEnvironmentsOptions { } export type TriggerRefreshOptions = { - /** - * Trigger a fresh refresh. - */ - clearCache?: boolean; /** * Only trigger a refresh if it hasn't already been triggered for this session. */ diff --git a/src/client/pythonEnvironments/base/locators/composite/envsCollectionCache.ts b/src/client/pythonEnvironments/base/locators/composite/envsCollectionCache.ts index 7c12faf524c4..14663e2d117d 100644 --- a/src/client/pythonEnvironments/base/locators/composite/envsCollectionCache.ts +++ b/src/client/pythonEnvironments/base/locators/composite/envsCollectionCache.ts @@ -2,6 +2,7 @@ // Licensed under the MIT License. import { Event } from 'vscode'; +import { isTestExecution } from '../../../../common/constants'; import { traceInfo } from '../../../../logging'; import { reportInterpretersChanged } from '../../../../proposedApi'; import { arePathsSame, getFileInfo, pathExists } from '../../../common/externalDependencies'; @@ -52,13 +53,9 @@ export interface IEnvsCollectionCache { /** * Removes invalid envs from cache. Note this does not check for outdated info when * validating cache. + * @param latestListOfEnvs Carries list of latest envs for this workspace session if known. */ - validateCache(): Promise; - - /** - * Clears the in-memory cache. This can be used for hard refresh. - */ - clearCache(): Promise; + validateCache(latestListOfEnvs?: PythonEnvInfo[]): Promise; } export type PythonEnvLatestInfo = { hasLatestInfo?: boolean } & PythonEnvInfo; @@ -79,15 +76,35 @@ export class PythonEnvInfoCache extends PythonEnvsWatcher { + public async validateCache(latestListOfEnvs?: PythonEnvInfo[]): Promise { /** * We do check if an env has updated as we already run discovery in background * which means env cache will have up-to-date envs eventually. This also means - * we avoid the cost of running lstat. So simply remove envs which no longer - * exist. + * we avoid the cost of running lstat. So simply remove envs which are no longer + * valid. */ const areEnvsValid = await Promise.all( - this.envs.map((e) => (e.executable.filename === 'python' ? true : pathExists(e.executable.filename))), + this.envs.map(async (cachedEnv) => { + const { path } = getEnvPath(cachedEnv.executable.filename, cachedEnv.location); + if (await pathExists(path)) { + if (latestListOfEnvs) { + /** + * Only consider a cached env to be valid if it's relevant. That means: + * * It is either reported in the latest complete refresh for this session. + * * Or it is relevant for some other workspace folder which is not opened currently. + */ + if (cachedEnv.searchLocation) { + return true; + } + if (latestListOfEnvs.some((env) => cachedEnv.id === env.id)) { + return true; + } + } else { + return true; + } + } + return false; + }), ); const invalidIndexes = areEnvsValid .map((isValid, index) => (isValid ? -1 : index)) @@ -194,6 +211,15 @@ async function validateInfo(env: PythonEnvInfo) { export async function createCollectionCache(storage: IPersistentStorage): Promise { const cache = new PythonEnvInfoCache(storage); await cache.clearAndReloadFromStorage(); - await cache.validateCache(); + await validateCache(cache); return cache; } + +async function validateCache(cache: PythonEnvInfoCache) { + if (isTestExecution()) { + // For purposes for test execution, block on validation so that we can determinally know when it finishes. + return cache.validateCache(); + } + // Validate in background so it doesn't block on returning the API object. + return cache.validateCache().ignoreErrors(); +} diff --git a/src/client/pythonEnvironments/base/locators/composite/envsCollectionService.ts b/src/client/pythonEnvironments/base/locators/composite/envsCollectionService.ts index f652b420ecf8..0e1466bc385d 100644 --- a/src/client/pythonEnvironments/base/locators/composite/envsCollectionService.ts +++ b/src/client/pythonEnvironments/base/locators/composite/envsCollectionService.ts @@ -108,15 +108,12 @@ export class EnvsCollectionService extends PythonEnvsWatcher this.sendTelemetry(query, stopWatch)); } - private startRefresh(query: PythonLocatorQuery | undefined, options?: TriggerRefreshOptions): Promise { - if (options?.clearCache) { - this.cache.clearCache(); - } + private startRefresh(query: PythonLocatorQuery | undefined): Promise { this.createProgressStates(query); const promise = this.addEnvsToCacheForQuery(query); return promise @@ -176,7 +173,8 @@ export class EnvsCollectionService extends PythonEnvsWatcher { workspace = TypeMoq.Mock.ofType(); interpreterService = mock(); when(interpreterService.refreshPromise).thenReturn(undefined); + when(interpreterService.triggerRefresh()).thenResolve(); when(interpreterService.triggerRefresh(anything(), anything())).thenResolve(); workspace.setup((w) => w.rootPath).returns(() => 'rootPath'); @@ -568,17 +569,11 @@ suite('Set Interpreter Command', () => { expect(actualParameters).to.not.equal(undefined, 'Parameters not set'); const refreshButtons = actualParameters!.customButtonSetups; expect(refreshButtons).to.not.equal(undefined, 'Callback not set'); + expect(refreshButtons?.length).to.equal(1); - expect(refreshButtons?.length).to.equal(2); - let arg; - when(interpreterService.triggerRefresh(undefined, anything())).thenCall((_, _arg) => { - arg = _arg; - return Promise.resolve(); - }); await refreshButtons![0].callback!({} as QuickPick); // Invoke callback, meaning that the refresh button is clicked. - expect(arg).to.deep.equal({ clearCache: true }); - await refreshButtons![1].callback!({} as QuickPick); // Invoke callback, meaning that the refresh button is clicked. - expect(arg).to.deep.equal({ clearCache: false }); + + verify(interpreterService.triggerRefresh()).once(); }); test('Events to update quickpick updates the quickpick accordingly', async () => { diff --git a/src/test/pythonEnvironments/base/locators/composite/envsCollectionService.unit.test.ts b/src/test/pythonEnvironments/base/locators/composite/envsCollectionService.unit.test.ts index 1dd31c4424e5..7bb70bead0a9 100644 --- a/src/test/pythonEnvironments/base/locators/composite/envsCollectionService.unit.test.ts +++ b/src/test/pythonEnvironments/base/locators/composite/envsCollectionService.unit.test.ts @@ -54,8 +54,8 @@ suite('Python envs locator - Environments Collection', async () => { return envs; } - function createEnv(executable: string, searchLocation?: Uri, name?: string) { - return buildEnvInfo({ executable, searchLocation, name }); + function createEnv(executable: string, searchLocation?: Uri, name?: string, location?: string) { + return buildEnvInfo({ executable, searchLocation, name, location }); } function getLocatorEnvs() { @@ -72,14 +72,23 @@ suite('Python envs locator - Environments Collection', async () => { } function getValidCachedEnvs() { + const cachedEnvForWorkspace = createEnv( + path.join(TEST_LAYOUT_ROOT, 'workspace', 'folder1', 'win1', 'python.exe'), + Uri.file(path.join(TEST_LAYOUT_ROOT, 'workspace', 'folder1')), + ); const fakeLocalAppDataPath = path.join(TEST_LAYOUT_ROOT, 'storeApps'); const envCached1 = createEnv(path.join(fakeLocalAppDataPath, 'Microsoft', 'WindowsApps', 'python.exe')); const envCached2 = createEnv( path.join(TEST_LAYOUT_ROOT, 'pipenv', 'project1', '.venv', 'Scripts', 'python.exe'), Uri.file(TEST_LAYOUT_ROOT), ); - const envCached3 = createEnv('python'); - return [envCached1, envCached2, envCached3]; + const envCached3 = createEnv( + 'python', + undefined, + undefined, + path.join(TEST_LAYOUT_ROOT, 'envsWithoutPython', 'condaLackingPython'), + ); + return [cachedEnvForWorkspace, envCached1, envCached2, envCached3]; } function getCachedEnvs() { @@ -87,10 +96,11 @@ suite('Python envs locator - Environments Collection', async () => { return [...getValidCachedEnvs(), envCached3]; } - function getExpectedEnvs(doNotIncludeCached?: boolean) { - const fakeLocalAppDataPath = path.join(TEST_LAYOUT_ROOT, 'storeApps'); - const envCached1 = createEnv(path.join(fakeLocalAppDataPath, 'Microsoft', 'WindowsApps', 'python.exe')); - const envCached2 = createEnv('python'); + function getExpectedEnvs() { + const cachedEnvForWorkspace = createEnv( + path.join(TEST_LAYOUT_ROOT, 'workspace', 'folder1', 'win1', 'python.exe'), + Uri.file(path.join(TEST_LAYOUT_ROOT, 'workspace', 'folder1')), + ); const env1 = createEnv(path.join(TEST_LAYOUT_ROOT, 'conda1', 'python.exe'), undefined, updatedName); const env2 = createEnv( path.join(TEST_LAYOUT_ROOT, 'pipenv', 'project1', '.venv', 'Scripts', 'python.exe'), @@ -102,13 +112,8 @@ suite('Python envs locator - Environments Collection', async () => { undefined, updatedName, ); - if (doNotIncludeCached) { - return [env1, env2, env3].map((e: PythonEnvLatestInfo) => { - e.hasLatestInfo = true; - return e; - }); - } - return [envCached1, envCached2, env1, env2, env3].map((e: PythonEnvLatestInfo) => { + // Do not include cached envs which were not yielded by the locator, unless it belongs to some workspace. + return [cachedEnvForWorkspace, env1, env2, env3].map((e: PythonEnvLatestInfo) => { e.hasLatestInfo = true; return e; }); @@ -145,99 +150,6 @@ suite('Python envs locator - Environments Collection', async () => { ); }); - test('triggerRefresh() refreshes the collection and storage with any new environments', async () => { - const onUpdated = new EventEmitter(); - const locatedEnvs = getLocatorEnvs(); - const parentLocator = new SimpleLocator(locatedEnvs, { - onUpdated: onUpdated.event, - after: async () => { - locatedEnvs.forEach((env, index) => { - const update = cloneDeep(env); - update.name = updatedName; - onUpdated.fire({ index, update }); - }); - onUpdated.fire({ index: locatedEnvs.length - 1, update: undefined }); - // It turns out the last env is invalid, ensure it does not appear in the final result. - onUpdated.fire({ stage: ProgressReportStage.discoveryFinished }); - }, - }); - const cache = await createCollectionCache({ - load: async () => getCachedEnvs(), - store: async (e) => { - storage = e; - }, - }); - collectionService = new EnvsCollectionService(cache, parentLocator); - - await collectionService.triggerRefresh(); - const envs = collectionService.getEnvs(); - - const expected = getExpectedEnvs(); - assertEnvsEqual(envs, expected); - assertEnvsEqual(storage, expected); - - const eventData = [ - { - path: path.join(TEST_LAYOUT_ROOT, 'doesNotExist'), - type: 'remove', - }, - { - path: path.join(TEST_LAYOUT_ROOT, 'conda1', 'python.exe'), - type: 'add', - }, - { - path: path.join( - TEST_LAYOUT_ROOT, - 'pyenv2', - '.pyenv', - 'pyenv-win', - 'versions', - '3.6.9', - 'bin', - 'python.exe', - ), - type: 'add', - }, - { - path: path.join(TEST_LAYOUT_ROOT, 'virtualhome', '.venvs', 'win1', 'python.exe'), - type: 'add', - }, - { - path: path.join(TEST_LAYOUT_ROOT, 'conda1', 'python.exe'), - type: 'update', - }, - { - path: path.join(TEST_LAYOUT_ROOT, 'pipenv', 'project1', '.venv', 'Scripts', 'python.exe'), - type: 'update', - }, - { - path: path.join( - TEST_LAYOUT_ROOT, - 'pyenv2', - '.pyenv', - 'pyenv-win', - 'versions', - '3.6.9', - 'bin', - 'python.exe', - ), - type: 'update', - }, - { - path: path.join(TEST_LAYOUT_ROOT, 'virtualhome', '.venvs', 'win1', 'python.exe'), - type: 'update', - }, - { - path: path.join(TEST_LAYOUT_ROOT, 'virtualhome', '.venvs', 'win1', 'python.exe'), - type: 'remove', - }, - ]; - eventData.forEach((d) => { - sinon.assert.calledWithExactly(reportInterpretersChangedStub, [d]); - }); - sinon.assert.callCount(reportInterpretersChangedStub, eventData.length); - }); - test('If `ifNotTriggerredAlready` option is set and a refresh for query is already triggered, triggerRefresh() does not trigger a refresh', async () => { const onUpdated = new EventEmitter(); const locatedEnvs = getLocatorEnvs(); @@ -312,70 +224,9 @@ suite('Python envs locator - Environments Collection', async () => { }); const expected = getExpectedEnvs(); assertEnvsEqual(envs, expected); - - const eventData = [ - { - path: path.join(TEST_LAYOUT_ROOT, 'doesNotExist'), - type: 'remove', - }, - { - path: path.join(TEST_LAYOUT_ROOT, 'conda1', 'python.exe'), - type: 'add', - }, - { - path: path.join( - TEST_LAYOUT_ROOT, - 'pyenv2', - '.pyenv', - 'pyenv-win', - 'versions', - '3.6.9', - 'bin', - 'python.exe', - ), - type: 'add', - }, - { - path: path.join(TEST_LAYOUT_ROOT, 'virtualhome', '.venvs', 'win1', 'python.exe'), - type: 'add', - }, - { - path: path.join(TEST_LAYOUT_ROOT, 'conda1', 'python.exe'), - type: 'update', - }, - { - path: path.join(TEST_LAYOUT_ROOT, 'pipenv', 'project1', '.venv', 'Scripts', 'python.exe'), - type: 'update', - }, - { - path: path.join( - TEST_LAYOUT_ROOT, - 'pyenv2', - '.pyenv', - 'pyenv-win', - 'versions', - '3.6.9', - 'bin', - 'python.exe', - ), - type: 'update', - }, - { - path: path.join(TEST_LAYOUT_ROOT, 'virtualhome', '.venvs', 'win1', 'python.exe'), - type: 'update', - }, - { - path: path.join(TEST_LAYOUT_ROOT, 'virtualhome', '.venvs', 'win1', 'python.exe'), - type: 'remove', - }, - ]; - eventData.forEach((d) => { - sinon.assert.calledWithExactly(reportInterpretersChangedStub, [d]); - }); - sinon.assert.callCount(reportInterpretersChangedStub, eventData.length); }); - test('If `clearCache` option is set triggerRefresh() clears the cache before refreshing and fires expected events', async () => { + test('triggerRefresh() refreshes the collection with any new envs & removes cached envs if not relevant', async () => { const onUpdated = new EventEmitter(); const locatedEnvs = getLocatorEnvs(); const cachedEnvs = getCachedEnvs(); @@ -405,17 +256,18 @@ suite('Python envs locator - Environments Collection', async () => { events.push(e); }); - await collectionService.triggerRefresh(undefined, { clearCache: true }); + await collectionService.triggerRefresh(); let envs = cachedEnvs; // Ensure when all the events are applied to the original list in sequence, the final list is as expected. events.forEach((e) => { envs = applyChangeEventToEnvList(envs, e); }); - const expected = getExpectedEnvs(true); + const expected = getExpectedEnvs(); assertEnvsEqual(envs, expected); const queriedEnvs = collectionService.getEnvs(); assertEnvsEqual(queriedEnvs, expected); + assertEnvsEqual(storage, expected); }); test('Ensure progress stage updates are emitted correctly and refresh promises correct track promise for each stage', async () => { @@ -502,49 +354,6 @@ suite('Python envs locator - Environments Collection', async () => { expect(refreshPromise).to.equal(undefined, 'All paths discovered stage not applicable if a query is provided'); }); - test('refreshPromise() correctly indicates the status of the refresh', async () => { - const parentLocator = new SimpleLocator(getLocatorEnvs()); - const cache = await createCollectionCache({ - load: async () => getCachedEnvs(), - store: async () => noop(), - }); - collectionService = new EnvsCollectionService(cache, parentLocator); - - await collectionService.triggerRefresh(); - - const eventData = [ - { - path: path.join(TEST_LAYOUT_ROOT, 'doesNotExist'), - type: 'remove', - }, - { - path: path.join(TEST_LAYOUT_ROOT, 'conda1', 'python.exe'), - type: 'add', - }, - { - path: path.join( - TEST_LAYOUT_ROOT, - 'pyenv2', - '.pyenv', - 'pyenv-win', - 'versions', - '3.6.9', - 'bin', - 'python.exe', - ), - type: 'add', - }, - { - path: path.join(TEST_LAYOUT_ROOT, 'virtualhome', '.venvs', 'win1', 'python.exe'), - type: 'add', - }, - ]; - eventData.forEach((d) => { - sinon.assert.calledWithExactly(reportInterpretersChangedStub, [d]); - }); - sinon.assert.callCount(reportInterpretersChangedStub, eventData.length); - }); - test('resolveEnv() uses cache if complete and up to date info is available', async () => { const resolvedViaLocator = buildEnvInfo({ executable: 'Resolved via locator' }); const cachedEnvs = getCachedEnvs();