Added tests for decorator-like implementation in enviroment provider

microsoft · Mar 6, 2020 · e60d5da · e60d5da
1 parent fbcb93c
commit e60d5da
Show file tree

Hide file tree

Showing 7 changed files with 136 additions and 149 deletions.
diff --git a/src/client/common/utils/cacheUtils.ts b/src/client/common/utils/cacheUtils.ts
@@ -27,19 +27,27 @@ const resourceSpecificCacheStores = new Map<string, Map<string, CacheData>>();
  * @param {VSCodeType} [vscode=require('vscode')]
  * @returns
  */
-function getCacheKey(resource: Resource, vscode: VSCodeType = require('vscode'), serviceContainer: IServiceContainer) {
+function getCacheKey(
+    resource: Resource,
+    vscode: VSCodeType = require('vscode'),
+    serviceContainer: IServiceContainer | undefined
+) {
     const section = vscode.workspace.getConfiguration('python', vscode.Uri.file(__filename));
     if (!section) {
         return 'python';
     }
+    let interpreterPathService: IInterpreterPathService | undefined;
     let inExperiment: boolean | undefined;
-    const interpreterPathService = serviceContainer.get<IInterpreterPathService>(IInterpreterPathService);
-    const abExperiments = serviceContainer.get<IExperimentsManager>(IExperimentsManager);
-    inExperiment = abExperiments.inExperiment(DeprecatePythonPath.experiment);
-    abExperiments.sendTelemetryIfInExperiment(DeprecatePythonPath.control);
-    const globalPythonPath = inExperiment
-        ? interpreterPathService.inspectInterpreterPath(vscode.Uri.file(__filename)).globalValue || 'python'
-        : section.inspect<string>('pythonPath')!.globalValue || 'python';
+    if (serviceContainer) {
+        interpreterPathService = serviceContainer.get<IInterpreterPathService>(IInterpreterPathService);
+        const abExperiments = serviceContainer.get<IExperimentsManager>(IExperimentsManager);
+        inExperiment = abExperiments.inExperiment(DeprecatePythonPath.experiment);
+        abExperiments.sendTelemetryIfInExperiment(DeprecatePythonPath.control);
+    }
+    const globalPythonPath =
+        inExperiment && interpreterPathService
+            ? interpreterPathService.inspectInterpreterPath(vscode.Uri.file(__filename)).globalValue || 'python'
+            : section.inspect<string>('pythonPath')!.globalValue || 'python';
     // Get the workspace related to this resource.
     if (
         !resource ||
@@ -52,9 +60,10 @@ function getCacheKey(resource: Resource, vscode: VSCodeType = require('vscode'),
     if (!folder) {
         return globalPythonPath;
     }
-    const workspacePythonPath = inExperiment
-        ? interpreterPathService.getInterpreterPath(resource)
-        : vscode.workspace.getConfiguration('python', resource).get<string>('pythonPath') || 'python';
+    const workspacePythonPath =
+        inExperiment && interpreterPathService
+            ? interpreterPathService.getInterpreterPath(resource)
+            : vscode.workspace.getConfiguration('python', resource).get<string>('pythonPath') || 'python';
     return `${folder.uri.fsPath}-${workspacePythonPath}`;
 }
 /**
@@ -67,7 +76,7 @@ function getCacheKey(resource: Resource, vscode: VSCodeType = require('vscode'),
 function getCacheStore(
     resource: Resource,
     vscode: VSCodeType = require('vscode'),
-    serviceContainer: IServiceContainer
+    serviceContainer: IServiceContainer | undefined
 ) {
     const key = getCacheKey(resource, vscode, serviceContainer);
     if (!resourceSpecificCacheStores.has(key)) {
@@ -165,7 +174,7 @@ export class InMemoryInterpreterSpecificCache<T> extends InMemoryCache<T> {
         keyPrefix: string,
         expiryDurationMs: number,
         args: [Uri | undefined, ...any[]],
-        private readonly serviceContainer: IServiceContainer,
+        private readonly serviceContainer: IServiceContainer | undefined,
         private readonly vscode: VSCodeType = require('vscode')
     ) {
         super(expiryDurationMs, getCacheKeyFromFunctionArgs(keyPrefix, args.slice(1)));

diff --git a/src/client/common/utils/decorators.ts b/src/client/common/utils/decorators.ts
@@ -1,6 +1,6 @@
 // tslint:disable:no-any no-require-imports no-function-expression no-invalid-this
 
-import { ProgressLocation, ProgressOptions, Uri, window } from 'vscode';
+import { ProgressLocation, ProgressOptions, window } from 'vscode';
 import '../../common/extensions';
 import { IServiceContainer } from '../../ioc/types';
 import { isTestExecution } from '../constants';
@@ -127,7 +127,6 @@ export function makeDebounceAsyncDecorator(wait?: number) {
 }
 
 type VSCodeType = typeof import('vscode');
-type PromiseFunctionWithFirstArgOfResource = (...any: [Uri | undefined, ...any[]]) => Promise<any>;
 
 export function clearCachedResourceSpecificIngterpreterData(
     key: string,
@@ -139,27 +138,6 @@ export function clearCachedResourceSpecificIngterpreterData(
     cacheStore.clear();
 }
 
-/**
- * This is intended to be a replacement to a decorator. `serviceContainer` object can't
- * be passed into a normal decorator as decorators are not applied after class is constructed
- */
-export function cacheResourceSpecificInterpreterData(
-    key: string,
-    expiryDurationMs: number,
-    serviceContainer: IServiceContainer,
-    originalMethod: PromiseFunctionWithFirstArgOfResource,
-    ...args: [Uri | undefined, ...any[]]
-) {
-    const cacheStore = new InMemoryInterpreterSpecificCache(key, expiryDurationMs, args, serviceContainer);
-    if (cacheStore.hasData) {
-        traceVerbose(`Cached data exists ${key}, ${args[0] ? args[0].fsPath : '<No Resource>'}`);
-        return Promise.resolve(cacheStore.data);
-    }
-    const promise = originalMethod(...args) as Promise<any>;
-    promise.then(result => (cacheStore.data = result)).ignoreErrors();
-    return promise;
-}
-
 type PromiseFunctionWithAnyArgs = (...any: any) => Promise<any>;
 const cacheStoreForMethods = getGlobalCacheStore();
 export function cache(expiryDurationMs: number) {

diff --git a/src/client/common/variables/environmentVariablesProvider.ts b/src/client/common/variables/environmentVariablesProvider.ts
@@ -1,16 +1,18 @@
 // Copyright (c) Microsoft Corporation. All rights reserved.
 // Licensed under the MIT License.
 
-import { inject, injectable } from 'inversify';
+import { inject, injectable, optional } from 'inversify';
 import { ConfigurationChangeEvent, Disposable, Event, EventEmitter, FileSystemWatcher, Uri } from 'vscode';
 import { IServiceContainer } from '../../ioc/types';
 import { IWorkspaceService } from '../application/types';
+import { traceVerbose } from '../logger';
 import { IPlatformService } from '../platform/types';
 import { IConfigurationService, ICurrentProcess, IDisposableRegistry } from '../types';
-import { cacheResourceSpecificInterpreterData, clearCachedResourceSpecificIngterpreterData } from '../utils/decorators';
+import { InMemoryInterpreterSpecificCache } from '../utils/cacheUtils';
+import { clearCachedResourceSpecificIngterpreterData } from '../utils/decorators';
 import { EnvironmentVariables, IEnvironmentVariablesProvider, IEnvironmentVariablesService } from './types';
 
-const cacheDuration = 60 * 60 * 1000;
+const CACHE_DURATION = 60 * 60 * 1000;
 @injectable()
 export class EnvironmentVariablesProvider implements IEnvironmentVariablesProvider, Disposable {
     public trackedWorkspaceFolders = new Set<string>();
@@ -24,7 +26,8 @@ export class EnvironmentVariablesProvider implements IEnvironmentVariablesProvid
         @inject(IWorkspaceService) private workspaceService: IWorkspaceService,
         @inject(IConfigurationService) private readonly configurationService: IConfigurationService,
         @inject(ICurrentProcess) private process: ICurrentProcess,
-        @inject(IServiceContainer) private serviceContainer: IServiceContainer
+        @inject(IServiceContainer) private serviceContainer: IServiceContainer,
+        @optional() private cacheDuration: number = CACHE_DURATION
     ) {
         disposableRegistry.push(this);
         this.changeEventEmitter = new EventEmitter();
@@ -46,13 +49,20 @@ export class EnvironmentVariablesProvider implements IEnvironmentVariablesProvid
     }
 
     public async getEnvironmentVariables(resource?: Uri): Promise<EnvironmentVariables> {
-        return cacheResourceSpecificInterpreterData(
+        // Cache resource specific interpreter data
+        const cacheStore = new InMemoryInterpreterSpecificCache(
             'getEnvironmentVariables',
-            cacheDuration,
-            this.serviceContainer,
-            this._getEnvironmentVariables,
-            resource
+            this.cacheDuration,
+            [resource],
+            this.serviceContainer
         );
+        if (cacheStore.hasData) {
+            traceVerbose(`Cached data exists getEnvironmentVariables, ${resource ? resource.fsPath : '<No Resource>'}`);
+            return Promise.resolve(cacheStore.data) as Promise<EnvironmentVariables>;
+        }
+        const promise = this._getEnvironmentVariables(resource);
+        promise.then(result => (cacheStore.data = result)).ignoreErrors();
+        return promise;
     }
     public async _getEnvironmentVariables(resource?: Uri): Promise<EnvironmentVariables> {
         let mergedVars = await this.getCustomEnvironmentVariables(resource);

diff --git a/src/test/common/utils/cacheUtils.unit.test.ts b/src/test/common/utils/cacheUtils.unit.test.ts
@@ -123,7 +123,7 @@ suite('Common Utils - CacheUtils', () => {
                 const pythonPath = 'Some Python Path';
                 const vsc = createMockVSC(pythonPath);
                 const resource = Uri.parse('a');
-                const cache = new InMemoryInterpreterSpecificCache('Something', 10000, [resource], vsc);
+                const cache = new InMemoryInterpreterSpecificCache('Something', 10000, [resource], undefined, vsc);
 
                 expect(cache.hasData).to.be.equal(false, 'Must not have any data');
 
@@ -136,7 +136,7 @@ suite('Common Utils - CacheUtils', () => {
                 const pythonPath = 'Some Python Path';
                 const vsc = createMockVSC(pythonPath);
                 const resource = Uri.parse('a');
-                const cache = new InMemoryInterpreterSpecificCache('Something', 10000, [resource], vsc);
+                const cache = new InMemoryInterpreterSpecificCache('Something', 10000, [resource], undefined, vsc);
 
                 expect(cache.hasData).to.be.equal(false, 'Must not have any data');
 
@@ -153,7 +153,7 @@ suite('Common Utils - CacheUtils', () => {
                 const pythonPath = 'Some Python Path';
                 const vsc = createMockVSC(pythonPath);
                 const resource = Uri.parse('a');
-                const cache = new InMemoryInterpreterSpecificCache('Something', 10000, [resource], vsc);
+                const cache = new InMemoryInterpreterSpecificCache('Something', 10000, [resource], undefined, vsc);
 
                 expect(cache.hasData).to.be.equal(false, 'Must not have any data');
 
@@ -170,7 +170,7 @@ suite('Common Utils - CacheUtils', () => {
                 const pythonPath = 'Some Python Path';
                 const vsc = createMockVSC(pythonPath);
                 const resource = Uri.parse('a');
-                const cache = new TestInMemoryInterpreterSpecificCache('Something', 100, [resource], vsc);
+                const cache = new TestInMemoryInterpreterSpecificCache('Something', 100, [resource], undefined, vsc);
 
                 expect(cache.hasData).to.be.equal(false, 'Must not have any data before caching.');
                 cache.data = scenario.dataToStore;
@@ -204,7 +204,7 @@ suite('Common Utils - CacheUtils', () => {
                 const resource = Uri.parse('a');
                 (vsc.workspace as any).workspaceFolders = [{ index: 0, name: '1', uri: Uri.parse('wkfolder') }];
                 vsc.workspace.getWorkspaceFolder = () => vsc.workspace.workspaceFolders![0];
-                const cache = new InMemoryInterpreterSpecificCache('Something', 10000, [resource], vsc);
+                const cache = new InMemoryInterpreterSpecificCache('Something', 10000, [resource], undefined, vsc);
 
                 expect(cache.hasData).to.be.equal(false, 'Must not have any data');
 
@@ -218,8 +218,14 @@ suite('Common Utils - CacheUtils', () => {
                 const vsc = createMockVSC(pythonPath);
                 const resource = Uri.parse('a');
                 const anotherResource = Uri.parse('b');
-                const cache = new InMemoryInterpreterSpecificCache('Something', 10000, [resource], vsc);
-                const cache2 = new InMemoryInterpreterSpecificCache('Something', 10000, [anotherResource], vsc);
+                const cache = new InMemoryInterpreterSpecificCache('Something', 10000, [resource], undefined, vsc);
+                const cache2 = new InMemoryInterpreterSpecificCache(
+                    'Something',
+                    10000,
+                    [anotherResource],
+                    undefined,
+                    vsc
+                );
 
                 expect(cache.hasData).to.be.equal(false, 'Must not have any data');
                 expect(cache2.hasData).to.be.equal(false, 'Must not have any data');
@@ -238,8 +244,14 @@ suite('Common Utils - CacheUtils', () => {
                 const anotherResource = Uri.parse('b');
                 (vsc.workspace as any).workspaceFolders = [{ index: 0, name: '1', uri: Uri.parse('wkfolder') }];
                 vsc.workspace.getWorkspaceFolder = () => vsc.workspace.workspaceFolders![0];
-                const cache = new InMemoryInterpreterSpecificCache('Something', 10000, [resource], vsc);
-                const cache2 = new InMemoryInterpreterSpecificCache('Something', 10000, [anotherResource], vsc);
+                const cache = new InMemoryInterpreterSpecificCache('Something', 10000, [resource], undefined, vsc);
+                const cache2 = new InMemoryInterpreterSpecificCache(
+                    'Something',
+                    10000,
+                    [anotherResource],
+                    undefined,
+                    vsc
+                );
 
                 expect(cache.hasData).to.be.equal(false, 'Must not have any data');
                 expect(cache2.hasData).to.be.equal(false, 'Must not have any data');
@@ -264,8 +276,14 @@ suite('Common Utils - CacheUtils', () => {
                     const index = res.fsPath === resource.fsPath ? 0 : 1;
                     return vsc.workspace.workspaceFolders![index];
                 };
-                const cache = new InMemoryInterpreterSpecificCache('Something', 10000, [resource], vsc);
-                const cache2 = new InMemoryInterpreterSpecificCache('Something', 10000, [anotherResource], vsc);
+                const cache = new InMemoryInterpreterSpecificCache('Something', 10000, [resource], undefined, vsc);
+                const cache2 = new InMemoryInterpreterSpecificCache(
+                    'Something',
+                    10000,
+                    [anotherResource],
+                    undefined,
+                    vsc
+                );
 
                 expect(cache.hasData).to.be.equal(false, 'Must not have any data');
                 expect(cache2.hasData).to.be.equal(false, 'Must not have any data');

diff --git a/src/test/common/utils/decorators.unit.test.ts b/src/test/common/utils/decorators.unit.test.ts
@@ -5,15 +5,8 @@
 
 import { expect, use } from 'chai';
 import * as chaiPromise from 'chai-as-promised';
-import { Uri } from 'vscode';
-import { Resource } from '../../../client/common/types';
 import { clearCache } from '../../../client/common/utils/cacheUtils';
-import {
-    cache,
-    cacheResourceSpecificInterpreterData,
-    makeDebounceAsyncDecorator,
-    makeDebounceDecorator
-} from '../../../client/common/utils/decorators';
+import { cache, makeDebounceAsyncDecorator, makeDebounceDecorator } from '../../../client/common/utils/decorators';
 import { sleep } from '../../core';
 use(chaiPromise);
 
@@ -27,89 +20,6 @@ suite('Common Utils - Decorators', function() {
     suite('Cache', () => {
         setup(clearCache);
         teardown(clearCache);
-        function createMockVSC(pythonPath: string): typeof import('vscode') {
-            return {
-                workspace: {
-                    getConfiguration: () => {
-                        return {
-                            get: () => {
-                                return pythonPath;
-                            },
-                            inspect: () => {
-                                return { globalValue: pythonPath };
-                            }
-                        };
-                    },
-                    getWorkspaceFolder: () => {
-                        return;
-                    }
-                },
-                Uri: Uri
-            } as any;
-        }
-        test('Result must be cached when using cache decorator', async () => {
-            const vsc = createMockVSC('');
-            class TestClass {
-                public invoked = false;
-                @cacheResourceSpecificInterpreterData('Something', 100000, vsc)
-                public async doSomething(_resource: Resource, a: number, b: number): Promise<number> {
-                    this.invoked = true;
-                    return a + b;
-                }
-            }
-
-            const cls = new TestClass();
-            const uri = Uri.parse('a');
-            const uri2 = Uri.parse('b');
-
-            let result = await cls.doSomething(uri, 1, 2);
-            expect(result).to.equal(3);
-            expect(cls.invoked).to.equal(true, 'Must be invoked');
-
-            cls.invoked = false;
-            let result2 = await cls.doSomething(uri2, 2, 3);
-            expect(result2).to.equal(5);
-            expect(cls.invoked).to.equal(true, 'Must be invoked');
-
-            cls.invoked = false;
-            result = await cls.doSomething(uri, 1, 2);
-            result2 = await cls.doSomething(uri2, 2, 3);
-            expect(result).to.equal(3);
-            expect(result2).to.equal(5);
-            expect(cls.invoked).to.equal(false, 'Must not be invoked');
-        });
-        test('Cache result must be cleared when cache expires', async () => {
-            const vsc = createMockVSC('');
-            class TestClass {
-                public invoked = false;
-                @cacheResourceSpecificInterpreterData('Something', 100, vsc)
-                public async doSomething(_resource: Resource, a: number, b: number): Promise<number> {
-                    this.invoked = true;
-                    return a + b;
-                }
-            }
-
-            const cls = new TestClass();
-            const uri = Uri.parse('a');
-            let result = await cls.doSomething(uri, 1, 2);
-
-            expect(result).to.equal(3);
-            expect(cls.invoked).to.equal(true, 'Must be invoked');
-
-            cls.invoked = false;
-            result = await cls.doSomething(uri, 1, 2);
-
-            expect(result).to.equal(3);
-            expect(cls.invoked).to.equal(false, 'Must not be invoked');
-
-            await sleep(110);
-
-            cls.invoked = false;
-            result = await cls.doSomething(uri, 1, 2);
-
-            expect(result).to.equal(3);
-            expect(cls.invoked).to.equal(true, 'Must be invoked');
-        });
     });
     suite('Cache Decorator', () => {
         const oldValueOfVSC_PYTHON_UNIT_TEST = process.env.VSC_PYTHON_UNIT_TEST;

diff --git a/src/test/common/variables/envVarsProvider.multiroot.test.ts b/src/test/common/variables/envVarsProvider.multiroot.test.ts
@@ -91,7 +91,8 @@ suite('Multiroot Environment Variables Provider', () => {
             new PlatformService(),
             workspaceService,
             cfgService,
-            mockProcess
+            mockProcess,
+            ioc.serviceContainer
         );
     }