diff --git a/news/2 Fixes/6270.md b/news/2 Fixes/6270.md new file mode 100644 index 000000000000..013d2b28bd23 --- /dev/null +++ b/news/2 Fixes/6270.md @@ -0,0 +1 @@ +Opting out of telemetry correctly opts out of A/B testing diff --git a/src/client/common/experiments.ts b/src/client/common/experiments.ts index 4960dc96ff41..9fcc87c5525e 100644 --- a/src/client/common/experiments.ts +++ b/src/client/common/experiments.ts @@ -78,7 +78,7 @@ export class ExperimentsManager implements IExperimentsManager { @swallowExceptions('Failed to activate experiments') public async activate(): Promise { - if (this.activatedOnce) { + if (this.activatedOnce || isTelemetryDisabled(this.workspaceService)) { return; } this.activatedOnce = true; @@ -123,13 +123,11 @@ export class ExperimentsManager implements IExperimentsManager { } /** - * Downloads experiments and updates storage given following conditions are met - * * Telemetry is not disabled - * * Previously downloaded experiments are no longer valid + * Downloads experiments and updates storage given previously downloaded experiments are no longer valid */ @traceDecorators.error('Failed to initialize experiments') public async initializeInBackground() { - if (isTelemetryDisabled(this.workspaceService) || this.isDownloadedStorageValid.value) { + if (this.isDownloadedStorageValid.value) { return; } const downloadedExperiments = await this.httpClient.getJSON(configUri, false); diff --git a/src/test/common/experiments.unit.test.ts b/src/test/common/experiments.unit.test.ts index 82cf97bce3e7..870f47f665fb 100644 --- a/src/test/common/experiments.unit.test.ts +++ b/src/test/common/experiments.unit.test.ts @@ -6,7 +6,7 @@ // tslint:disable:no-any import { assert, expect } from 'chai'; -import { anything, instance, mock, verify, when } from 'ts-mockito'; +import { anything, instance, mock, resetCalls, verify, when } from 'ts-mockito'; import * as TypeMoq from 'typemoq'; import { WorkspaceConfiguration } from 'vscode'; import { ApplicationEnvironment } from '../../client/common/application/applicationEnvironment'; @@ -53,15 +53,9 @@ suite('A/B experiments', () => { }); async function testInitialization( - settings: { globalValue?: boolean } = {}, downloadError: boolean = false, experimentsDownloaded?: any ) { - const workspaceConfig = TypeMoq.Mock.ofType(); - when(workspaceService.getConfiguration('telemetry')).thenReturn(workspaceConfig.object); - workspaceConfig.setup(c => c.inspect('enableTelemetry')) - .returns(() => settings as any) - .verifiable(TypeMoq.Times.once()); if (downloadError) { when(httpClient.getJSON(configUri, false)).thenReject(new Error('Kaboom')); } else { @@ -77,19 +71,10 @@ suite('A/B experiments', () => { // tslint:disable-next-line:no-empty } catch { } - verify(workspaceService.getConfiguration('telemetry')).once(); - workspaceConfig.verifyAll(); isDownloadedStorageValid.verifyAll(); experimentStorage.verifyAll(); } - test('If the users have opted out of telemetry, then they are opted out of AB testing ', async () => { - isDownloadedStorageValid.setup(n => n.value).returns(() => false).verifiable(TypeMoq.Times.never()); - - // settings = { globalValue: false } - await testInitialization({ globalValue: false }); - }); - test('Initializing experiments does not download experiments if storage is valid and contains experiments', async () => { isDownloadedStorageValid.setup(n => n.value).returns(() => true).verifiable(TypeMoq.Times.once()); @@ -113,8 +98,8 @@ suite('A/B experiments', () => { .returns(() => Promise.resolve(undefined)) .verifiable(TypeMoq.Times.never()); - // settings = {}, downloadError = false, experimentsDownloaded = experiments - await testInitialization({}, false, experiments); + // downloadError = false, experimentsDownloaded = experiments + await testInitialization(false, experiments); verify(httpClient.getJSON(configUri, false)).once(); }); @@ -134,49 +119,75 @@ suite('A/B experiments', () => { isDownloadedStorageValid.setup(n => n.updateValue(true)).returns(() => Promise.resolve(undefined)).verifiable(TypeMoq.Times.never()); downloadedExperimentsStorage.setup(n => n.updateValue(anything())).returns(() => Promise.resolve(undefined)).verifiable(TypeMoq.Times.never()); - // settings = {}, downloadError = true - await testInitialization({}, true); + // downloadError = true + await testInitialization(true); 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(); + }); + test('Ensure experiments can only be activated once', async () => { // 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()); + downloadedExperimentsStorage .setup(n => n.value) .returns(() => undefined) .verifiable(TypeMoq.Times.once()); - when(fs.fileExists(anything())).thenResolve(false); + when( + fs.fileExists(anything()) + ).thenResolve(false); experimentStorage.setup(n => n.value).returns(() => undefined) .verifiable(TypeMoq.Times.exactly(2)); isDownloadedStorageValid .setup(n => n.value) .returns(() => true) .verifiable(TypeMoq.Times.once()); - when(workspaceService.getConfiguration('telemetry')).thenReturn(workspaceConfig.object); - workspaceConfig.setup(c => c.inspect('enableTelemetry')) - .returns(() => settings as any); // First activation await expManager.activate(); - downloadedExperimentsStorage.reset(); - downloadedExperimentsStorage - .setup(n => n.value) - .returns(() => undefined) - .verifiable(TypeMoq.Times.never()); + resetCalls(workspaceService); // Second activation await expManager.activate(); - downloadedExperimentsStorage.verifyAll(); + verify(workspaceService.getConfiguration(anything())).never(); + workspaceConfig.verifyAll(); verify(fs.fileExists(anything())).once(); isDownloadedStorageValid.verifyAll(); experimentStorage.verifyAll(); + downloadedExperimentsStorage.verifyAll(); }); test('Ensure experiments are reliably initialized in the background', async () => {