Experiments should not block on telemetry. (#10940)

* Experiments should not block on telemetry. * Fix tests * Fix linter issue
microsoft · Apr 3, 2020 · dfbfed3 · dfbfed3
1 parent 8afe46f
commit dfbfed3
Show file tree

Hide file tree

Showing 6 changed files with 13 additions and 90 deletions.
diff --git a/news/2 Fixes/10008.md b/news/2 Fixes/10008.md
@@ -0,0 +1 @@
+Experiments no longer block on telemetry.
diff --git a/src/client/common/experiments.ts b/src/client/common/experiments.ts
@@ -9,9 +9,9 @@ import { inject, injectable, named, optional } from 'inversify';
 import { parse } from 'jsonc-parser';
 import * as path from 'path';
 import { IConfigurationService, IHttpClient } from '../common/types';
-import { isTelemetryDisabled, sendTelemetryEvent } from '../telemetry';
+import { sendTelemetryEvent } from '../telemetry';
 import { EventName } from '../telemetry/constants';
-import { IApplicationEnvironment, IWorkspaceService } from './application/types';
+import { IApplicationEnvironment } from './application/types';
 import { EXTENSION_ROOT_DIR, STANDARD_OUTPUT_CHANNEL } from './constants';
 import { traceDecorators, traceError } from './logger';
 import { IFileSystem } from './platform/types';
@@ -87,7 +87,6 @@ export class ExperimentsManager implements IExperimentsManager {
     private activatedOnce: boolean = false;
     constructor(
         @inject(IPersistentStateFactory) private readonly persistentStateFactory: IPersistentStateFactory,
-        @inject(IWorkspaceService) private readonly workspaceService: IWorkspaceService,
         @inject(IHttpClient) private readonly httpClient: IHttpClient,
         @inject(ICryptoUtils) private readonly crypto: ICryptoUtils,
         @inject(IApplicationEnvironment) private readonly appEnvironment: IApplicationEnvironment,
@@ -116,7 +115,7 @@ export class ExperimentsManager implements IExperimentsManager {
 
     @swallowExceptions('Failed to activate experiments')
     public async activate(): Promise<void> {
-        if (this.activatedOnce || isTelemetryDisabled(this.workspaceService)) {
+        if (this.activatedOnce) {
             return;
         }
         this.activatedOnce = true;
@@ -319,6 +318,10 @@ export class ExperimentsManager implements IExperimentsManager {
         }
     }
 
+    public _activated(): boolean {
+        return this.activatedOnce;
+    }
+
     /**
      * You can only opt in or out of experiment groups, not control groups. So remove requests for control groups.
      */

diff --git a/src/test/common/experiments.unit.test.ts b/src/test/common/experiments.unit.test.ts
@@ -7,12 +7,10 @@
 
 import { assert, expect } from 'chai';
 import * as sinon from 'sinon';
-import { anything, instance, mock, resetCalls, verify, when } from 'ts-mockito';
+import { anything, instance, mock, verify, when } from 'ts-mockito';
 import * as TypeMoq from 'typemoq';
-import { WorkspaceConfiguration } from 'vscode';
 import { ApplicationEnvironment } from '../../client/common/application/applicationEnvironment';
-import { IApplicationEnvironment, IWorkspaceService } from '../../client/common/application/types';
-import { WorkspaceService } from '../../client/common/application/workspace';
+import { IApplicationEnvironment } from '../../client/common/application/types';
 import { PythonSettings } from '../../client/common/configSettings';
 import { ConfigurationService } from '../../client/common/configuration/service';
 import { CryptoUtils } from '../../client/common/crypto';
@@ -43,7 +41,6 @@ import { noop } from '../core';
 // tslint:disable: max-func-body-length
 
 suite('A/B experiments', () => {
-    let workspaceService: IWorkspaceService;
     let httpClient: IHttpClient;
     let crypto: ICryptoUtils;
     let appEnvironment: IApplicationEnvironment;
@@ -57,7 +54,6 @@ suite('A/B experiments', () => {
     let configurationService: ConfigurationService;
     let experiments: TypeMoq.IMock<IExperiments>;
     setup(() => {
-        workspaceService = mock(WorkspaceService);
         httpClient = mock(HttpClient);
         crypto = mock(CryptoUtils);
         appEnvironment = mock(ApplicationEnvironment);
@@ -85,7 +81,6 @@ suite('A/B experiments', () => {
         ).thenReturn(downloadedExperimentsStorage.object);
         expManager = new ExperimentsManager(
             instance(persistentStateFactory),
-            instance(workspaceService),
             instance(httpClient),
             instance(crypto),
             instance(appEnvironment),
@@ -188,50 +183,20 @@ suite('A/B experiments', () => {
         verify(httpClient.getJSON(configUri, false)).once();
     });
 
-    test('If the users have opted out of telemetry, then they are opted out of AB testing ', async () => {
-        const workspaceConfig = TypeMoq.Mock.ofType<WorkspaceConfiguration>();
-        const settings = { globalValue: false };
-
-        when(workspaceService.getConfiguration('telemetry')).thenReturn(workspaceConfig.object);
-        workspaceConfig
-            .setup((c) => c.inspect<boolean>('enableTelemetry'))
-            .returns(() => settings as any)
-            .verifiable(TypeMoq.Times.once());
-        downloadedExperimentsStorage
-            .setup((n) => n.value)
-            .returns(() => undefined)
-            .verifiable(TypeMoq.Times.never());
-
-        await expManager.activate();
-
-        verify(workspaceService.getConfiguration('telemetry')).once();
-        workspaceConfig.verifyAll();
-        downloadedExperimentsStorage.verifyAll();
-    });
-
     async function testEnablingExperiments(enabled: boolean) {
         const updateExperimentStorage = sinon.stub(ExperimentsManager.prototype, 'updateExperimentStorage');
         updateExperimentStorage.callsFake(() => Promise.resolve());
         const populateUserExperiments = sinon.stub(ExperimentsManager.prototype, 'populateUserExperiments');
         populateUserExperiments.callsFake(() => Promise.resolve());
         const initializeInBackground = sinon.stub(ExperimentsManager.prototype, 'initializeInBackground');
         initializeInBackground.callsFake(() => Promise.resolve());
-        const workspaceConfig = TypeMoq.Mock.ofType<WorkspaceConfiguration>();
-        const settings = {};
         experiments
             .setup((e) => e.enabled)
             .returns(() => enabled)
             .verifiable(TypeMoq.Times.atLeastOnce());
 
-        when(workspaceService.getConfiguration('telemetry')).thenReturn(workspaceConfig.object);
-        workspaceConfig
-            .setup((c) => c.inspect<boolean>('enableTelemetry'))
-            .returns(() => settings as any)
-            .verifiable(TypeMoq.Times.once());
-
         expManager = new ExperimentsManager(
             instance(persistentStateFactory),
-            instance(workspaceService),
             instance(httpClient),
             instance(crypto),
             instance(appEnvironment),
@@ -246,7 +211,6 @@ suite('A/B experiments', () => {
         assert.equal(populateUserExperiments.callCount, enabled ? 1 : 0);
         assert.equal(initializeInBackground.callCount, enabled ? 1 : 0);
 
-        workspaceConfig.verifyAll();
         experiments.verifyAll();
     }
     test('Ensure experiments are not initialized when it is disabled', async () => testEnablingExperiments(false));
@@ -263,7 +227,6 @@ suite('A/B experiments', () => {
 
         expManager = new ExperimentsManager(
             instance(persistentStateFactory),
-            instance(workspaceService),
             instance(httpClient),
             instance(crypto),
             instance(appEnvironment),
@@ -295,41 +258,23 @@ suite('A/B experiments', () => {
         initializeInBackground.callsFake(() => Promise.resolve());
         expManager = new ExperimentsManager(
             instance(persistentStateFactory),
-            instance(workspaceService),
             instance(httpClient),
             instance(crypto),
             instance(appEnvironment),
             output.object,
             instance(fs),
             instance(configurationService)
         );
-        // Activate it twice and check
-        const workspaceConfig = TypeMoq.Mock.ofType<WorkspaceConfiguration>();
-        const settings = {};
-
-        when(workspaceService.getConfiguration('telemetry')).thenReturn(workspaceConfig.object);
-        workspaceConfig
-            .setup((c) => c.inspect<boolean>('enableTelemetry'))
-            .returns(() => settings as any)
-            .verifiable(TypeMoq.Times.once());
-
-        // First activation
-        await expManager.activate();
-
-        resetCalls(workspaceService);
 
-        // Second activation
+        assert.isFalse(expManager._activated());
         await expManager.activate();
 
-        verify(workspaceService.getConfiguration(anything())).never();
-
-        workspaceConfig.verifyAll();
+        // Ensure activated flag is set
+        assert.isTrue(expManager._activated());
     });
 
     test('Ensure experiments are reliably downloaded in the background', async () => {
         const experimentsDeferred = createDeferred<void>();
-        const workspaceConfig = TypeMoq.Mock.ofType<WorkspaceConfiguration>();
-        const settings = {};
         const updateExperimentStorage = sinon.stub(ExperimentsManager.prototype, 'updateExperimentStorage');
         updateExperimentStorage.callsFake(() => Promise.resolve());
         const populateUserExperiments = sinon.stub(ExperimentsManager.prototype, 'populateUserExperiments');
@@ -338,7 +283,6 @@ suite('A/B experiments', () => {
         initializeInBackground.callsFake(() => experimentsDeferred.promise);
         expManager = new ExperimentsManager(
             instance(persistentStateFactory),
-            instance(workspaceService),
             instance(httpClient),
             instance(crypto),
             instance(appEnvironment),
@@ -347,12 +291,6 @@ suite('A/B experiments', () => {
             instance(configurationService)
         );
 
-        when(workspaceService.getConfiguration('telemetry')).thenReturn(workspaceConfig.object);
-        workspaceConfig
-            .setup((c) => c.inspect<boolean>('enableTelemetry'))
-            .returns(() => settings as any)
-            .verifiable(TypeMoq.Times.once());
-
         const promise = expManager.activate();
         const deferred = createDeferredFromPromise(promise);
         await sleep(1);
@@ -363,8 +301,6 @@ suite('A/B experiments', () => {
         experimentsDeferred.resolve();
         await sleep(1);
 
-        verify(workspaceService.getConfiguration('telemetry')).once();
-        workspaceConfig.verifyAll();
         assert.ok(initializeInBackground.calledOnce);
     });
 
@@ -396,7 +332,6 @@ suite('A/B experiments', () => {
         doBestEffortToPopulateExperiments.callsFake(() => Promise.resolve(false));
         expManager = new ExperimentsManager(
             instance(persistentStateFactory),
-            instance(workspaceService),
             instance(httpClient),
             instance(crypto),
             instance(appEnvironment),
@@ -438,7 +373,6 @@ suite('A/B experiments', () => {
         doBestEffortToPopulateExperiments.callsFake(() => Promise.resolve(true));
         expManager = new ExperimentsManager(
             instance(persistentStateFactory),
-            instance(workspaceService),
             instance(httpClient),
             instance(crypto),
             instance(appEnvironment),
@@ -476,7 +410,6 @@ suite('A/B experiments', () => {
         doBestEffortToPopulateExperiments.callsFake(() => Promise.resolve(false));
         expManager = new ExperimentsManager(
             instance(persistentStateFactory),
-            instance(workspaceService),
             instance(httpClient),
             instance(crypto),
             instance(appEnvironment),
@@ -528,7 +461,6 @@ suite('A/B experiments', () => {
         doBestEffortToPopulateExperiments.callsFake(() => Promise.resolve(false));
         expManager = new ExperimentsManager(
             instance(persistentStateFactory),
-            instance(workspaceService),
             instance(httpClient),
             instance(crypto),
             instance(appEnvironment),
@@ -583,7 +515,6 @@ suite('A/B experiments', () => {
                 doBestEffortToPopulateExperiments.callsFake(() => Promise.resolve(false));
                 expManager = new ExperimentsManager(
                     instance(persistentStateFactory),
-                    instance(workspaceService),
                     instance(httpClient),
                     instance(crypto),
                     instance(appEnvironment),
@@ -1022,7 +953,6 @@ suite('A/B experiments', () => {
             downloadAndStoreExperiments.callsFake(() => downloadExperimentsDeferred.promise);
             expManager = new ExperimentsManager(
                 instance(persistentStateFactory),
-                instance(workspaceService),
                 instance(httpClient),
                 instance(crypto),
                 instance(appEnvironment),
@@ -1047,7 +977,6 @@ suite('A/B experiments', () => {
             downloadAndStoreExperiments.callsFake(() => downloadExperimentsDeferred.promise);
             expManager = new ExperimentsManager(
                 instance(persistentStateFactory),
-                instance(workspaceService),
                 instance(httpClient),
                 instance(crypto),
                 instance(appEnvironment),
@@ -1070,7 +999,6 @@ suite('A/B experiments', () => {
             downloadAndStoreExperiments.callsFake(() => Promise.reject('Kaboom'));
             expManager = new ExperimentsManager(
                 instance(persistentStateFactory),
-                instance(workspaceService),
                 instance(httpClient),
                 instance(crypto),
                 instance(appEnvironment),

diff --git a/src/test/debugger/extension/adapter/factory.unit.test.ts b/src/test/debugger/extension/adapter/factory.unit.test.ts
@@ -15,7 +15,6 @@ import { DebugAdapterExecutable, DebugAdapterServer, DebugConfiguration, DebugSe
 import { ApplicationEnvironment } from '../../../../client/common/application/applicationEnvironment';
 import { ApplicationShell } from '../../../../client/common/application/applicationShell';
 import { IApplicationShell } from '../../../../client/common/application/types';
-import { WorkspaceService } from '../../../../client/common/application/workspace';
 import { ConfigurationService } from '../../../../client/common/configuration/service';
 import { CryptoUtils } from '../../../../client/common/crypto';
 import { DebugAdapterNewPtvsd } from '../../../../client/common/experimentGroups';
@@ -103,7 +102,6 @@ suite('Debugging - Adapter Factory', () => {
         rewiremock.enable();
         rewiremock('vscode-extension-telemetry').with({ default: Reporter });
 
-        const workspaceService = mock(WorkspaceService);
         const httpClient = mock(HttpClient);
         const crypto = mock(CryptoUtils);
         const appEnvironment = mock(ApplicationEnvironment);
@@ -117,7 +115,6 @@ suite('Debugging - Adapter Factory', () => {
         } as any) as IPythonSettings);
         experimentsManager = new ExperimentsManager(
             instance(persistentStateFactory),
-            instance(workspaceService),
             instance(httpClient),
             instance(crypto),
             instance(appEnvironment),

diff --git a/src/test/debugger/extension/adapter/outdatedDebuggerPrompt.unit.test.ts b/src/test/debugger/extension/adapter/outdatedDebuggerPrompt.unit.test.ts
@@ -10,7 +10,6 @@ import { DebugProtocol } from 'vscode-debugprotocol';
 import { ApplicationEnvironment } from '../../../../client/common/application/applicationEnvironment';
 import { ApplicationShell } from '../../../../client/common/application/applicationShell';
 import { IApplicationShell } from '../../../../client/common/application/types';
-import { WorkspaceService } from '../../../../client/common/application/workspace';
 import { ConfigurationService } from '../../../../client/common/configuration/service';
 import { CryptoUtils } from '../../../../client/common/crypto';
 import { DebugAdapterNewPtvsd } from '../../../../client/common/experimentGroups';
@@ -49,7 +48,6 @@ suite('Debugging - Outdated Debugger Prompt tests.', () => {
     };
 
     setup(() => {
-        const workspaceService = mock(WorkspaceService);
         const httpClient = mock(HttpClient);
         const crypto = mock(CryptoUtils);
         const appEnvironment = mock(ApplicationEnvironment);
@@ -63,7 +61,6 @@ suite('Debugging - Outdated Debugger Prompt tests.', () => {
         } as any) as IPythonSettings);
         experimentsManager = new ExperimentsManager(
             instance(persistentStateFactory),
-            instance(workspaceService),
             instance(httpClient),
             instance(crypto),
             instance(appEnvironment),

diff --git a/src/test/debugger/extension/configuration/resolvers/launchConfigExperiments.unit.test.ts b/src/test/debugger/extension/configuration/resolvers/launchConfigExperiments.unit.test.ts
@@ -8,7 +8,6 @@ import * as assert from 'assert';
 import rewiremock from 'rewiremock';
 import { instance, mock, spy, when } from 'ts-mockito';
 import { ApplicationEnvironment } from '../../../../../client/common/application/applicationEnvironment';
-import { WorkspaceService } from '../../../../../client/common/application/workspace';
 import { ConfigurationService } from '../../../../../client/common/configuration/service';
 import { CryptoUtils } from '../../../../../client/common/crypto';
 import { DebugAdapterNewPtvsd, WebAppReload } from '../../../../../client/common/experimentGroups';
@@ -60,7 +59,6 @@ suite('Debugging - Config Resolver Launch Experiments', () => {
         rewiremock.enable();
         rewiremock('vscode-extension-telemetry').with({ default: Reporter });
 
-        const workspaceService = mock(WorkspaceService);
         const httpClient = mock(HttpClient);
         const crypto = mock(CryptoUtils);
         const appEnvironment = mock(ApplicationEnvironment);
@@ -75,7 +73,6 @@ suite('Debugging - Config Resolver Launch Experiments', () => {
         } as any) as IPythonSettings);
         experimentsManager = new ExperimentsManager(
             instance(persistentStateFactory),
-            instance(workspaceService),
             instance(httpClient),
             instance(crypto),
             instance(appEnvironment),