extension/src/goTelemetry: do our JSON enc/dec for telemetry start time

We noticed the vscode Memento API doesn't behave as we expected. That
made our recent attempt to increase the telemetry prompt rate
stopped by this bug.

Instead of relying on the Memento API's JSON stringify for Date object
encoding, do the enc/dec work on ourside and let Memento work with
string types.

The existing changes cover the json encoding, decoding cases.

Now goMain.ts activate() returns ExtensionTestAPI in testing mode.

In telemetry testing, we want to check if the value recorded with
vscode's real Memento API can be still usable. The real implementation
is accessible only by capturing the ExtensionContext passed to the
activate() invocation. Allow our test to access the extension's
globalState using the new ExtensionTestAPI.

Fixes #3312

Change-Id: I4540f83201f315624b077d113d6bfe2b3d608719
Reviewed-on: https://go-review.googlesource.com/c/vscode-go/+/576775
Reviewed-by: Robert Findley <rfindley@google.com>
Auto-Submit: Hyang-Ah Hana Kim <hyangah@gmail.com>
kokoro-CI: kokoro <noreply+kokoro@google.com>
Commit-Queue: Hyang-Ah Hana Kim <hyangah@gmail.com>

Author: hyangah

extension/src/goMain.ts

@@ -76,12 +76,24 @@ import { telemetryReporter } from './goTelemetry';
 
 const goCtx: GoExtensionContext = {};
 
-export async function activate(ctx: vscode.ExtensionContext): Promise<ExtensionAPI | undefined> {
+// Allow tests to access the extension context utilities.
+interface ExtensionTestAPI {
+	globalState: vscode.Memento;
+}
+
+export async function activate(ctx: vscode.ExtensionContext): Promise<ExtensionAPI | ExtensionTestAPI | undefined> {
 	if (process.env['VSCODE_GO_IN_TEST'] === '1') {
-		// Make sure this does not run when running in test.
-		return;
+		// TODO: VSCODE_GO_IN_TEST was introduced long before we learned about
+		// ctx.extensionMode, and used in multiple places.
+		// Investigate if use of VSCODE_GO_IN_TEST can be removed
+		// in favor of ctx.extensionMode and clean up.
+		if (ctx.extensionMode === vscode.ExtensionMode.Test) {
+			return { globalState: ctx.globalState };
+		}
+		// We shouldn't expose the memento in production mode even when VSCODE_GO_IN_TEST
+		// environment variable is set.
+		return; // Skip the remaining activation work.
 	}
-
 	const start = Date.now();
 	setGlobalState(ctx.globalState);
 	setWorkspaceState(ctx.workspaceState);

extension/src/goTelemetry.ts

@@ -21,6 +21,25 @@ export const GOPLS_MAYBE_PROMPT_FOR_TELEMETRY = 'gopls.maybe_prompt_for_telemetr
 // Exported for testing.
 export const TELEMETRY_START_TIME_KEY = 'telemetryStartTime';
 
+// Run our encode/decode function for the Date object, to be defensive
+// from vscode Memento API behavior change.
+// Exported for testing.
+export function recordTelemetryStartTime(storage: vscode.Memento, date: Date) {
+	storage.update(TELEMETRY_START_TIME_KEY, date.toJSON());
+}
+
+function readTelemetryStartTime(storage: vscode.Memento): Date | null {
+	const value = storage.get<string | number | Date>(TELEMETRY_START_TIME_KEY);
+	if (!value) {
+		return null;
+	}
+	const telemetryStartTime = new Date(value);
+	if (telemetryStartTime.toString() === 'Invalid Date') {
+		return null;
+	}
+	return telemetryStartTime;
+}
+
 enum ReporterState {
 	NOT_INITIALIZED,
 	IDLE,
@@ -153,9 +172,9 @@ export class TelemetryService {
 		this.active = true;
 		// record the first time we see the gopls with telemetry support.
 		// The timestamp will be used to avoid prompting too early.
-		const telemetryStartTime = globalState.get<Date>(TELEMETRY_START_TIME_KEY);
+		const telemetryStartTime = readTelemetryStartTime(globalState);
 		if (!telemetryStartTime) {
-			globalState.update(TELEMETRY_START_TIME_KEY, new Date());
+			recordTelemetryStartTime(globalState, new Date());
 		}
 	}
 
@@ -172,9 +191,11 @@ export class TelemetryService {
 		if (!isVSCodeTelemetryEnabled) return;
 
 		// Allow at least 7days for gopls to collect some data.
-		const now = new Date();
-		const telemetryStartTime = this.globalState.get<Date>(TELEMETRY_START_TIME_KEY, now);
-		if (daysBetween(telemetryStartTime, now) < 7) {
+		const telemetryStartTime = readTelemetryStartTime(this.globalState);
+		if (!telemetryStartTime) {
+			return;
+		}
+		if (daysBetween(telemetryStartTime, new Date()) < 7) {
 			return;
 		}
 

extension/test/gopls/extension.test.ts

@@ -10,7 +10,7 @@ import * as vscode from 'vscode';
 import { getGoConfig } from '../../src/config';
 import sinon = require('sinon');
 import { getGoVersion, GoVersion } from '../../src/util';
-import { GOPLS_MAYBE_PROMPT_FOR_TELEMETRY, TELEMETRY_START_TIME_KEY, TelemetryService } from '../../src/goTelemetry';
+import { GOPLS_MAYBE_PROMPT_FOR_TELEMETRY, recordTelemetryStartTime, TelemetryService } from '../../src/goTelemetry';
 import { MockMemento } from '../mocks/MockMemento';
 import { Env } from './goplsTestEnv.utils';
 
@@ -205,8 +205,7 @@ suite('Go Extension Tests With Gopls', function () {
 		const workspaceDir = path.resolve(testdataDir, 'gogetdocTestData');
 		await env.startGopls(path.join(workspaceDir, 'test.go'), undefined, workspaceDir);
 		const memento = new MockMemento();
-		memento.update(TELEMETRY_START_TIME_KEY, new Date('2000-01-01'));
-
+		recordTelemetryStartTime(memento, new Date('2000-01-01'));
 		const sut = new TelemetryService(env.languageClient, memento, [GOPLS_MAYBE_PROMPT_FOR_TELEMETRY]);
 		try {
 			await Promise.all([

extension/test/gopls/telemetry.test.ts

@@ -10,7 +10,8 @@ import {
 	GOPLS_MAYBE_PROMPT_FOR_TELEMETRY,
 	TELEMETRY_START_TIME_KEY,
 	TelemetryReporter,
-	TelemetryService
+	TelemetryService,
+	recordTelemetryStartTime
 } from '../../src/goTelemetry';
 import { MockMemento } from '../mocks/MockMemento';
 import { maybeInstallVSCGO } from '../../src/goInstallTools';
@@ -21,9 +22,12 @@ import os = require('os');
 import { rmdirRecursive } from '../../src/util';
 import { extensionId } from '../../src/const';
 import { executableFileExists, fileExists } from '../../src/utils/pathUtils';
-import { ExtensionMode } from 'vscode';
+import { ExtensionMode, Memento, extensions } from 'vscode';
+
+describe('# prompt for telemetry', async () => {
+	const extension = extensions.getExtension(extensionId);
+	assert(extension);
 
-describe('# prompt for telemetry', () => {
 	it(
 		'do not prompt if language client is not used',
 		testTelemetryPrompt(
@@ -131,6 +135,22 @@ describe('# prompt for telemetry', () => {
 			false
 		)
 	);
+	// testExtensionAPI.globalState is a real memento instance passed by ExtensionHost.
+	// This instance is active throughout the integration test.
+	// When you add more test cases that interact with the globalState,
+	// be aware that multiple test cases may access and mutate it asynchronously.
+	const testExtensionAPI = await extension.activate();
+	it('check we can salvage the value in the real memento', async () => {
+		// write Date with Memento.update - old way. Now we always use string for TELEMETRY_START_TIME_KEY value.
+		testExtensionAPI.globalState.update(TELEMETRY_START_TIME_KEY, new Date(Date.now() - 7 * 24 * 60 * 60 * 1000));
+		await testTelemetryPrompt(
+			{
+				samplingInterval: 1000,
+				mementoInstance: testExtensionAPI.globalState
+			},
+			true
+		)();
+	});
 });
 
 interface testCase {
@@ -141,6 +161,7 @@ interface testCase {
 	vsTelemetryDisabled?: boolean; // assume the user disabled vscode general telemetry.
 	samplingInterval: number; // N where N out of 1000 are sampled.
 	hashMachineID?: number; // stub the machine id hash computation function.
+	mementoInstance?: Memento; // if set, use this instead of mock memento.
 }
 
 function testTelemetryPrompt(tc: testCase, wantPrompt: boolean) {
@@ -153,9 +174,9 @@ function testTelemetryPrompt(tc: testCase, wantPrompt: boolean) {
 		const spy = sinon.spy(languageClient, 'sendRequest');
 		const lc = tc.noLangClient ? undefined : languageClient;
 
-		const memento = new MockMemento();
+		const memento = tc.mementoInstance ?? new MockMemento();
 		if (tc.firstDate) {
-			memento.update(TELEMETRY_START_TIME_KEY, tc.firstDate);
+			recordTelemetryStartTime(memento, tc.firstDate);
 		}
 		const commands = tc.goplsWithoutTelemetry ? [] : [GOPLS_MAYBE_PROMPT_FOR_TELEMETRY];