diff --git a/news/2 Fixes/10008.md b/news/2 Fixes/10008.md new file mode 100644 index 000000000000..6082d291476a --- /dev/null +++ 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 index 462fb6818a0f..74072703612f 100644 --- 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 { - 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 index cae02e8a2d70..ee95c98c3265 100644 --- 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; 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,27 +183,6 @@ 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(); - const settings = { globalValue: false }; - - when(workspaceService.getConfiguration('telemetry')).thenReturn(workspaceConfig.object); - workspaceConfig - .setup((c) => c.inspect('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()); @@ -216,22 +190,13 @@ suite('A/B experiments', () => { populateUserExperiments.callsFake(() => Promise.resolve()); const initializeInBackground = sinon.stub(ExperimentsManager.prototype, 'initializeInBackground'); initializeInBackground.callsFake(() => Promise.resolve()); - const workspaceConfig = TypeMoq.Mock.ofType(); - 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('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,7 +258,6 @@ suite('A/B experiments', () => { initializeInBackground.callsFake(() => Promise.resolve()); expManager = new ExperimentsManager( instance(persistentStateFactory), - instance(workspaceService), instance(httpClient), instance(crypto), instance(appEnvironment), @@ -303,33 +265,16 @@ suite('A/B experiments', () => { instance(fs), instance(configurationService) ); - // Activate it twice and check - const workspaceConfig = TypeMoq.Mock.ofType(); - const settings = {}; - - when(workspaceService.getConfiguration('telemetry')).thenReturn(workspaceConfig.object); - workspaceConfig - .setup((c) => c.inspect('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(); - const workspaceConfig = TypeMoq.Mock.ofType(); - 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('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 index 903a072c0cba..d94825ed72fc 100644 --- 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 index 1895c3d9c41e..a924be25a2db 100644 --- 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 index f361640a5198..339f3bd68ba1 100644 --- 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),