Move ShowExtensionSurveyPrompt experiment to ExP (#15693)

kimadeline · web-flow · commit 573dd9860882 · 2021-03-18T12:29:49.000-07:00
* Move ShowExtensionSurveyPrompt experiment to ExP
* Allow opt-in/out of the new experiment
* Use experiment name in test name
diff --git a/experiments.json b/experiments.json
@@ -1,16 +1,4 @@
 [
-    {
-        "name": "ShowExtensionSurveyPrompt - enabled",
-        "salt": "ShowExtensionSurveyPrompt",
-        "max": 100,
-        "min": 60
-    },
-    {
-        "name": "ShowExtensionSurveyPrompt - control",
-        "salt": "ShowExtensionSurveyPrompt",
-        "min": 0,
-        "max": 40
-    },
     {
         "name": "CollectLSRequestTiming - experiment",
         "salt": "CollectLSRequestTiming",
diff --git a/package.json b/package.json
@@ -1054,12 +1054,12 @@
                     "default": [],
                     "items": {
                         "enum": [
-                            "ShowExtensionSurveyPrompt - enabled",
                             "DeprecatePythonPath - experiment",
                             "tryPylance",
                             "pythonTensorboardExperiment",
                             "pythonDiscoveryModule",
                             "pythonJediLSP",
+                            "pythonSurveyNotification",
                             "All"
                         ]
                     },
@@ -1071,12 +1071,12 @@
                     "default": [],
                     "items": {
                         "enum": [
-                            "ShowExtensionSurveyPrompt - enabled",
                             "DeprecatePythonPath - experiment",
                             "tryPylance",
                             "pythonTensorboardExperiment",
                             "pythonDiscoveryModule",
                             "pythonJediLSP",
+                            "pythonSurveyNotification",
                             "All"
                         ]
                     },
diff --git a/src/client/activation/extensionSurvey.ts b/src/client/activation/extensionSurvey.ts
@@ -11,7 +11,7 @@ import { ShowExtensionSurveyPrompt } from '../common/experiments/groups';
 import '../common/extensions';
 import { traceDecorators } from '../common/logger';
 import { IPlatformService } from '../common/platform/types';
-import { IBrowserService, IExperimentsManager, IPersistentStateFactory, IRandom } from '../common/types';
+import { IBrowserService, IExperimentService, IPersistentStateFactory, IRandom } from '../common/types';
 import { Common, ExtensionSurveyBanner } from '../common/utils/localize';
 import { sendTelemetryEvent } from '../telemetry';
 import { EventName } from '../telemetry/constants';
@@ -33,16 +33,15 @@ export class ExtensionSurveyPrompt implements IExtensionSingleActivationService
         @inject(IBrowserService) private browserService: IBrowserService,
         @inject(IPersistentStateFactory) private persistentState: IPersistentStateFactory,
         @inject(IRandom) private random: IRandom,
-        @inject(IExperimentsManager) private experiments: IExperimentsManager,
+        @inject(IExperimentService) private experiments: IExperimentService,
         @inject(IApplicationEnvironment) private appEnvironment: IApplicationEnvironment,
         @inject(IPlatformService) private platformService: IPlatformService,
         @optional() private sampleSizePerOneHundredUsers: number = 10,
         @optional() private waitTimeToShowSurvey: number = WAIT_TIME_TO_SHOW_SURVEY,
     ) {}
 
     public async activate(): Promise<void> {
-        if (!this.experiments.inExperiment(ShowExtensionSurveyPrompt.enabled)) {
-            this.experiments.sendTelemetryIfInExperiment(ShowExtensionSurveyPrompt.control);
+        if (!(await this.experiments.inExperiment(ShowExtensionSurveyPrompt.experiment))) {
             return;
         }
         const show = this.shouldShowBanner();
diff --git a/src/client/common/experiments/groups.ts b/src/client/common/experiments/groups.ts
@@ -1,7 +1,6 @@
 // Experiment to check whether to show "Extension Survey prompt" or not.
 export enum ShowExtensionSurveyPrompt {
-    control = 'ShowExtensionSurveyPrompt - control',
-    enabled = 'ShowExtensionSurveyPrompt - enabled',
+    experiment = 'pythonSurveyNotification',
 }
 
 /*
diff --git a/src/client/common/experiments/manager.ts b/src/client/common/experiments/manager.ts
@@ -33,7 +33,7 @@ export const experimentStorageKey = 'EXPERIMENT_STORAGE_KEY';
  * as about 40% of the users never come back for the second session.
  */
 const configFile = path.join(EXTENSION_ROOT_DIR, 'experiments.json');
-export const oldExperimentSalts = ['ShowExtensionSurveyPrompt', 'LS'];
+export const oldExperimentSalts = ['LS'];
 
 /**
  * <DEPRECATED> Manages and stores experiments, implements the AB testing functionality
diff --git a/src/test/activation/extensionSurvey.unit.test.ts b/src/test/activation/extensionSurvey.unit.test.ts
@@ -14,7 +14,7 @@ import { PersistentStateFactory } from '../../client/common/persistentState';
 import { IPlatformService } from '../../client/common/platform/types';
 import {
     IBrowserService,
-    IExperimentsManager,
+    IExperimentService,
     IPersistentState,
     IPersistentStateFactory,
     IRandom,
@@ -29,14 +29,14 @@ suite('Extension survey prompt - shouldShowBanner()', () => {
     let browserService: TypeMoq.IMock<IBrowserService>;
     let random: TypeMoq.IMock<IRandom>;
     let persistentStateFactory: IPersistentStateFactory;
-    let experiments: TypeMoq.IMock<IExperimentsManager>;
+    let experiments: TypeMoq.IMock<IExperimentService>;
     let platformService: TypeMoq.IMock<IPlatformService>;
     let appEnvironment: TypeMoq.IMock<IApplicationEnvironment>;
     let disableSurveyForTime: TypeMoq.IMock<IPersistentState<any>>;
     let doNotShowAgain: TypeMoq.IMock<IPersistentState<any>>;
     let extensionSurveyPrompt: ExtensionSurveyPrompt;
     setup(() => {
-        experiments = TypeMoq.Mock.ofType<IExperimentsManager>();
+        experiments = TypeMoq.Mock.ofType<IExperimentService>();
         appShell = TypeMoq.Mock.ofType<IApplicationShell>();
         browserService = TypeMoq.Mock.ofType<IBrowserService>();
         random = TypeMoq.Mock.ofType<IRandom>();
@@ -176,7 +176,7 @@ suite('Extension survey prompt - shouldShowBanner()', () => {
 });
 
 suite('Extension survey prompt - showSurvey()', () => {
-    let experiments: TypeMoq.IMock<IExperimentsManager>;
+    let experiments: TypeMoq.IMock<IExperimentService>;
     let appShell: TypeMoq.IMock<IApplicationShell>;
     let browserService: TypeMoq.IMock<IBrowserService>;
     let random: TypeMoq.IMock<IRandom>;
@@ -205,7 +205,7 @@ suite('Extension survey prompt - showSurvey()', () => {
         when(
             persistentStateFactory.createGlobalPersistentState(extensionSurveyStateKeys.doNotShowAgain, false),
         ).thenReturn(doNotShowAgain.object);
-        experiments = TypeMoq.Mock.ofType<IExperimentsManager>();
+        experiments = TypeMoq.Mock.ofType<IExperimentService>();
         extensionSurveyPrompt = new ExtensionSurveyPrompt(
             appShell.object,
             browserService.object,
@@ -418,7 +418,7 @@ suite('Extension survey prompt - activate()', () => {
     let persistentStateFactory: IPersistentStateFactory;
     let shouldShowBanner: sinon.SinonStub<any>;
     let showSurvey: sinon.SinonStub<any>;
-    let experiments: TypeMoq.IMock<IExperimentsManager>;
+    let experiments: TypeMoq.IMock<IExperimentService>;
     let extensionSurveyPrompt: ExtensionSurveyPrompt;
     let platformService: TypeMoq.IMock<IPlatformService>;
     let appEnvironment: TypeMoq.IMock<IApplicationEnvironment>;
@@ -427,7 +427,7 @@ suite('Extension survey prompt - activate()', () => {
         browserService = TypeMoq.Mock.ofType<IBrowserService>();
         random = TypeMoq.Mock.ofType<IRandom>();
         persistentStateFactory = mock(PersistentStateFactory);
-        experiments = TypeMoq.Mock.ofType<IExperimentsManager>();
+        experiments = TypeMoq.Mock.ofType<IExperimentService>();
         platformService = TypeMoq.Mock.ofType<IPlatformService>();
         appEnvironment = TypeMoq.Mock.ofType<IApplicationEnvironment>();
     });
@@ -436,10 +436,9 @@ suite('Extension survey prompt - activate()', () => {
         sinon.restore();
     });
 
-    test("If user is not in 'ShowExtensionPrompt' experiment, send telemetry if in control group & return", async () => {
+    test("If user is not in 'ShowExtensionSurveyPrompt' experiment, return immediately", async () => {
         shouldShowBanner = sinon.stub(ExtensionSurveyPrompt.prototype, 'shouldShowBanner');
         shouldShowBanner.callsFake(() => false);
-        showSurvey = sinon.stub(ExtensionSurveyPrompt.prototype, 'showSurvey');
         extensionSurveyPrompt = new ExtensionSurveyPrompt(
             appShell.object,
             browserService.object,
@@ -451,19 +450,15 @@ suite('Extension survey prompt - activate()', () => {
             10,
         );
         experiments
-            .setup((exp) => exp.inExperiment(ShowExtensionSurveyPrompt.enabled))
-            .returns(() => false)
-            .verifiable(TypeMoq.Times.once());
-        experiments
-            .setup((exp) => exp.sendTelemetryIfInExperiment(ShowExtensionSurveyPrompt.control))
-            .returns(() => undefined)
+            .setup((exp) => exp.inExperiment(ShowExtensionSurveyPrompt.experiment))
+            .returns(() => Promise.resolve(false))
             .verifiable(TypeMoq.Times.once());
         await extensionSurveyPrompt.activate();
         assert.ok(shouldShowBanner.notCalled);
         experiments.verifyAll();
     });
 
-    test("No survey is shown if shouldShowBanner() returns false and user is in 'ShowExtensionPrompt' experiment", async () => {
+    test("No survey is shown if shouldShowBanner() returns false and user is in 'ShowExtensionSurveyPrompt' experiment", async () => {
         const deferred = createDeferred<true>();
         shouldShowBanner = sinon.stub(ExtensionSurveyPrompt.prototype, 'shouldShowBanner');
         shouldShowBanner.callsFake(() => false);
@@ -485,13 +480,9 @@ suite('Extension survey prompt - activate()', () => {
             50,
         );
         experiments
-            .setup((exp) => exp.inExperiment(ShowExtensionSurveyPrompt.enabled))
-            .returns(() => true)
+            .setup((exp) => exp.inExperiment(ShowExtensionSurveyPrompt.experiment))
+            .returns(() => Promise.resolve(true))
             .verifiable(TypeMoq.Times.once());
-        experiments
-            .setup((exp) => exp.sendTelemetryIfInExperiment(TypeMoq.It.isAny()))
-            .returns(() => undefined)
-            .verifiable(TypeMoq.Times.never());
         await extensionSurveyPrompt.activate();
         assert.ok(shouldShowBanner.calledOnce);
 
@@ -501,7 +492,7 @@ suite('Extension survey prompt - activate()', () => {
         experiments.verifyAll();
     });
 
-    test("Survey is shown after waitTimeToShowSurvey if shouldShowBanner() returns true and user is in 'ShowExtensionPrompt' experiment", async () => {
+    test("Survey is shown after waitTimeToShowSurvey if shouldShowBanner() returns true and user is in 'ShowExtensionSurveyPrompt' experiment", async () => {
         const deferred = createDeferred<true>();
         shouldShowBanner = sinon.stub(ExtensionSurveyPrompt.prototype, 'shouldShowBanner');
         shouldShowBanner.callsFake(() => true);
@@ -523,13 +514,9 @@ suite('Extension survey prompt - activate()', () => {
             50,
         );
         experiments
-            .setup((exp) => exp.inExperiment(ShowExtensionSurveyPrompt.enabled))
-            .returns(() => true)
+            .setup((exp) => exp.inExperiment(ShowExtensionSurveyPrompt.experiment))
+            .returns(() => Promise.resolve(true))
             .verifiable(TypeMoq.Times.once());
-        experiments
-            .setup((exp) => exp.sendTelemetryIfInExperiment(TypeMoq.It.isAny()))
-            .returns(() => undefined)
-            .verifiable(TypeMoq.Times.never());
         await extensionSurveyPrompt.activate();
         assert.ok(shouldShowBanner.calledOnce);
 
diff --git a/src/test/common/experiments/manager.unit.test.ts b/src/test/common/experiments/manager.unit.test.ts
@@ -251,7 +251,7 @@ suite('A/B experiments', () => {
             verify(crypto.createHash(anything(), 'number', 'FNV')).once();
         });
         test('Use the expected list of old experiments', async () => {
-            const expectedOldExperimentSalts = ['ShowExtensionSurveyPrompt', 'LS'];
+            const expectedOldExperimentSalts = ['LS'];
             assert.deepEqual(expectedOldExperimentSalts, oldExperimentSalts);
         });
     });

Original file line number	Diff line number	Diff line change
`@@ -1,7 +1,6 @@`
`1`	`1`	`// Experiment to check whether to show "Extension Survey prompt" or not.`
`2`	`2`	`export enum ShowExtensionSurveyPrompt {`
`3`		`- control = 'ShowExtensionSurveyPrompt - control',`
`4`		`- enabled = 'ShowExtensionSurveyPrompt - enabled',`
	`3`	`+ experiment = 'pythonSurveyNotification',`
`5`	`4`	`}`
`6`	`5`
`7`	`6`	`/*`