Do not break CLI process in case analytics fail (#3197)

In case we are unable to start the Analytics Broker process, CLI will
fail. But analytics errors should not break user's workflow, so catch the
error and ensure the actions will continue. Add tests to verify the
behavior.

Author: rosen-vladimirov

lib/services/analytics/analytics-service.ts

@@ -6,7 +6,7 @@ import { isInteractive } from '../../common/helpers';
 import { DeviceTypes, AnalyticsClients } from "../../common/constants";
 
 export class AnalyticsService extends AnalyticsServiceBase {
-	private static ANALYTICS_BROKER_START_TIMEOUT = 30 * 1000;
+	private static ANALYTICS_BROKER_START_TIMEOUT = 10 * 1000;
 	private brokerProcess: ChildProcess;
 
 	constructor(protected $logger: ILogger,
@@ -182,7 +182,14 @@ export class AnalyticsService extends AnalyticsServiceBase {
 	}
 
 	private async sendMessageToBroker(message: ITrackingInformation): Promise<void> {
-		const broker = await this.getAnalyticsBroker();
+		let broker: ChildProcess;
+		try {
+			broker = await this.getAnalyticsBroker();
+		} catch (err) {
+			this.$logger.trace("Unable to get broker instance due to error: ", err);
+			return;
+		}
+
 		return new Promise<void>((resolve, reject) => {
 			if (broker && broker.connected) {
 				try {

test/services/analytics/analytics-service.ts

@@ -0,0 +1,295 @@
+import { AnalyticsService } from "../../../lib/services/analytics/analytics-service";
+import { Yok } from "../../../lib/common/yok";
+import * as stubs from "../../stubs";
+import { assert } from "chai";
+import { EventEmitter } from "events";
+import { AnalyticsClients } from "../../../lib/common/constants";
+
+const helpers = require("../../../lib/common/helpers");
+const originalIsInteractive = helpers.isInteractive;
+
+const trackFeatureUsage = "TrackFeatureUsage";
+const createTestInjector = (): IInjector => {
+	const testInjector = new Yok();
+	testInjector.register("options", {});
+	testInjector.register("logger", stubs.LoggerStub);
+
+	testInjector.register("staticConfig", {
+		disableAnalytics: false,
+		TRACK_FEATURE_USAGE_SETTING_NAME: trackFeatureUsage,
+		PATH_TO_BOOTSTRAP: "pathToBootstrap.js"
+	});
+
+	testInjector.register("prompter", {
+
+	});
+
+	testInjector.register("userSettingsService", {
+		getSettingValue: async (settingName: string): Promise<any> => {
+			return "true";
+		}
+	});
+	testInjector.register("analyticsSettingsService", {
+		canDoRequest: (): Promise<boolean> => Promise.resolve(true)
+	});
+	testInjector.register("osInfo", {});
+	testInjector.register("childProcess", {});
+	testInjector.register("processService", {
+		attachToProcessExitSignals: (context: any, callback: () => void): void => undefined
+	});
+	testInjector.register("projectDataService", {});
+	testInjector.register("mobileHelper", {});
+
+	return testInjector;
+};
+
+describe("analyticsService", () => {
+	afterEach(() => {
+		helpers.isInteractive = originalIsInteractive;
+	});
+
+	describe("trackInGoogleAnalytics", () => {
+		describe("does not track", () => {
+			const testScenario = async (configuration: {
+				disableAnalytics: boolean,
+				assertMessage: string,
+				userSettingsServiceOpts?: { trackFeatureUsageValue: string, defaultValue: string }
+			}) => {
+				const testInjector = createTestInjector();
+				const staticConfig = testInjector.resolve<Config.IStaticConfig>("staticConfig");
+				staticConfig.disableAnalytics = configuration.disableAnalytics;
+
+				configuration.userSettingsServiceOpts = configuration.userSettingsServiceOpts || { trackFeatureUsageValue: "false", defaultValue: "true" };
+				const userSettingsService = testInjector.resolve<any>("userSettingsService");
+				userSettingsService.getSettingValue = async (settingName: string): Promise<string> => {
+					if (settingName === trackFeatureUsage) {
+						return configuration.userSettingsServiceOpts.trackFeatureUsageValue;
+					}
+
+					return configuration.userSettingsServiceOpts.defaultValue;
+				};
+
+				let isChildProcessSpawned = false;
+				const childProcess = testInjector.resolve<IChildProcess>("childProcess");
+				childProcess.spawn = (command: string, args?: string[], options?: any): any => {
+					isChildProcessSpawned = true;
+				};
+
+				const analyticsService = testInjector.resolve<IAnalyticsService>(AnalyticsService);
+				await analyticsService.trackInGoogleAnalytics({
+					googleAnalyticsDataType: GoogleAnalyticsDataType.Page,
+					customDimensions: {
+						customDimension1: "value1"
+					}
+				});
+
+				assert.isFalse(isChildProcessSpawned, configuration.assertMessage);
+			};
+
+			it("does not track when staticConfig's disableAnalytics is true", () => {
+				return testScenario({
+					disableAnalytics: true,
+					assertMessage: "When staticConfig.disableAnalytics is true, no child process should be started, i.e. we should not track anything."
+				});
+			});
+
+			it(`does not track when ${trackFeatureUsage} is not true`, async () => {
+				await testScenario({
+					disableAnalytics: false,
+					assertMessage: `When ${trackFeatureUsage} is false, no child process should be started, i.e. we should not track anything.`,
+					userSettingsServiceOpts: {
+						trackFeatureUsageValue: "false", defaultValue: "true"
+					}
+				});
+
+				await testScenario({
+					disableAnalytics: false,
+					assertMessage: `When ${trackFeatureUsage} is undefined, no child process should be started, i.e. we should not track anything.`,
+					userSettingsServiceOpts: {
+						trackFeatureUsageValue: undefined, defaultValue: "true"
+					}
+				});
+			});
+
+		});
+
+		const getSpawnedProcess = (): any => {
+			const spawnedProcess: any = new EventEmitter();
+			spawnedProcess.stdout = new EventEmitter();
+			spawnedProcess.stderr = new EventEmitter();
+			spawnedProcess.unref = (): void => undefined;
+			return spawnedProcess;
+		};
+
+		describe("does not fail", () => {
+			const assertExpectedError = async (testInjector: IInjector, opts: { isChildProcessSpawned: boolean, expectedErrorMessage: string }) => {
+				const analyticsService = testInjector.resolve<IAnalyticsService>(AnalyticsService);
+				await analyticsService.trackInGoogleAnalytics({
+					googleAnalyticsDataType: GoogleAnalyticsDataType.Page,
+					customDimensions: {
+						customDimension1: "value1"
+					}
+				});
+
+				assert.isTrue(opts.isChildProcessSpawned);
+				const logger = testInjector.resolve<stubs.LoggerStub>("logger");
+				assert.isTrue(logger.traceOutput.indexOf(opts.expectedErrorMessage) !== -1);
+			};
+
+			const setupTest = (expectedErrorMessage: string): any => {
+				const testInjector = createTestInjector();
+				const opts = {
+					isChildProcessSpawned: false,
+					expectedErrorMessage
+				};
+
+				const childProcess = testInjector.resolve<IChildProcess>("childProcess");
+				return {
+					testInjector,
+					opts,
+					childProcess
+				};
+			};
+
+			it("when unable to start broker process", async () => {
+				const { testInjector, childProcess, opts } = setupTest("Unable to get broker instance due to error:  Error: custom error");
+				childProcess.spawn = (command: string, args?: string[], options?: any): any => {
+					opts.isChildProcessSpawned = true;
+					throw new Error("custom error");
+				};
+
+				await assertExpectedError(testInjector, opts);
+			});
+
+			it("when broker cannot start for required timeout", async () => {
+				const { testInjector, childProcess, opts } = setupTest("Unable to get broker instance due to error:  Error: Unable to start Analytics Broker process.");
+				const originalSetTimeout = setTimeout;
+				childProcess.spawn = (command: string, args?: string[], options?: any): any => {
+					opts.isChildProcessSpawned = true;
+					global.setTimeout = (callback: (...args: any[]) => void, ms: number, ...otherArgs: any[]) => originalSetTimeout(callback, 1);
+					return getSpawnedProcess();
+				};
+
+				await assertExpectedError(testInjector, opts);
+
+				global.setTimeout = originalSetTimeout;
+			});
+
+			it("when broker is not connected", async () => {
+				const { testInjector, childProcess, opts } = setupTest("Broker not found or not connected.");
+
+				childProcess.spawn = (command: string, args?: string[], options?: any): any => {
+					opts.isChildProcessSpawned = true;
+					const spawnedProcess: any = getSpawnedProcess();
+
+					spawnedProcess.connected = false;
+					spawnedProcess.send = (): void => undefined;
+					setTimeout(() => spawnedProcess.emit("message", AnalyticsMessages.BrokerReadyToReceive), 1);
+					return spawnedProcess;
+				};
+
+				await assertExpectedError(testInjector, opts);
+			});
+
+			it("when sending message fails", async () => {
+				const { testInjector, childProcess, opts } = setupTest("Error while trying to send message to broker: Error: Failed to sent data.");
+
+				childProcess.spawn = (command: string, args?: string[], options?: any): any => {
+					opts.isChildProcessSpawned = true;
+					const spawnedProcess: any = getSpawnedProcess();
+
+					spawnedProcess.connected = true;
+					spawnedProcess.send = (): void => {
+						throw new Error("Failed to sent data.");
+					};
+
+					setTimeout(() => spawnedProcess.emit("message", AnalyticsMessages.BrokerReadyToReceive), 1);
+					return spawnedProcess;
+				};
+
+				await assertExpectedError(testInjector, opts);
+			});
+		});
+
+		describe("sends correct message to broker", () => {
+			const setupTest = (expectedResult: any, dataToSend: any, terminalOpts?: { isInteractive: boolean }): { testInjector: IInjector, opts: any } => {
+				helpers.isInteractive = () => terminalOpts ? terminalOpts.isInteractive : true;
+
+				const testInjector = createTestInjector();
+				const opts = {
+					isChildProcessSpawned: false,
+					expectedResult,
+					dataToSend,
+					messageSent: <any>null
+				};
+
+				const childProcess = testInjector.resolve<IChildProcess>("childProcess");
+				childProcess.spawn = (command: string, args?: string[], options?: any): any => {
+					opts.isChildProcessSpawned = true;
+					const spawnedProcess: any = getSpawnedProcess();
+
+					spawnedProcess.connected = true;
+					spawnedProcess.send = (msg: any, action: () => void): void => {
+						opts.messageSent = msg;
+						action();
+					};
+
+					setTimeout(() => spawnedProcess.emit("message", AnalyticsMessages.BrokerReadyToReceive), 1);
+
+					return spawnedProcess;
+				};
+
+				return {
+					testInjector,
+					opts
+				};
+			};
+
+			const assertExpectedResult = async (testInjector: IInjector, opts: { isChildProcessSpawned: boolean, expectedResult: any, messageSent: any, dataToSend: any }) => {
+				const analyticsService = testInjector.resolve<IAnalyticsService>(AnalyticsService);
+				await analyticsService.trackInGoogleAnalytics(opts.dataToSend);
+
+				assert.isTrue(opts.isChildProcessSpawned);
+				assert.deepEqual(opts.messageSent, opts.expectedResult);
+			};
+
+			const getDataToSend = (gaDataType: string): any => ({
+				googleAnalyticsDataType: gaDataType,
+				customDimensions: {
+					customDimension1: "value1"
+				}
+			});
+
+			const getExpectedResult = (gaDataType: string, analyticsClient?: string): any => ({
+				type: "googleAnalyticsData",
+				category: "CLI",
+				googleAnalyticsDataType: gaDataType,
+				customDimensions: { customDimension1: "value1", cd5: analyticsClient || "CLI" }
+			});
+
+			_.each([GoogleAnalyticsDataType.Page, GoogleAnalyticsDataType.Event], (googleAnalyticsDataType: string) => {
+				it(`when data is ${googleAnalyticsDataType}`, async () => {
+					const { testInjector, opts } = setupTest(getExpectedResult(googleAnalyticsDataType), getDataToSend(googleAnalyticsDataType));
+					await assertExpectedResult(testInjector, opts);
+				});
+
+				it(`when data is ${googleAnalyticsDataType} and terminal is not interactive`, async () => {
+					const { testInjector, opts } = setupTest(getExpectedResult(googleAnalyticsDataType, AnalyticsClients.Unknown), getDataToSend(googleAnalyticsDataType), { isInteractive: false });
+					await assertExpectedResult(testInjector, opts);
+				});
+
+				_.each([true, false], (isInteractive) => {
+					it(`when data is ${googleAnalyticsDataType} terminal is ${isInteractive ? "" : "not "}interactive and --analyticsClient is passed`, async () => {
+						const analyticsClient = "AnalyticsClient";
+
+						const { testInjector, opts } = setupTest(getExpectedResult(googleAnalyticsDataType, analyticsClient), getDataToSend(googleAnalyticsDataType), { isInteractive });
+						const options = testInjector.resolve<IOptions>("options");
+						options.analyticsClient = analyticsClient;
+
+						await assertExpectedResult(testInjector, opts);
+					});
+				});
+			});
+		});
+	});
+});

test/stubs.ts

@@ -13,9 +13,12 @@ export class LoggerStub implements ILogger {
 	warnWithLabel(...args: string[]): void { }
 	info(...args: string[]): void { }
 	debug(...args: string[]): void { }
-	trace(...args: string[]): void { }
+	trace(...args: string[]): void {
+		this.traceOutput += util.format.apply(null, args) + "\n";
+	}
 
 	public output = "";
+	public traceOutput = "";
 
 	out(...args: string[]): void {
 		this.output += util.format.apply(null, args) + "\n";