detach logcat helper on stop application + tests (#1011)

* detach logcat helper on stop application + tests

* fix getcontent bat script

fix logcat helper test

* enabled all tests

* update after review

* update logic

* removed unused private field

* made lint happy

Author: Plamen5kov

definitions/mobile.d.ts

@@ -138,6 +138,7 @@ declare module Mobile {
 
 	interface ILogcatHelper {
 		start(deviceIdentifier: string): Promise<void>;
+		stop(deviceIdentifier: string): void;
 	}
 
 	/**

mobile/android/android-application-manager.ts

@@ -83,6 +83,7 @@ export class AndroidApplicationManager extends ApplicationManagerBase {
 	}
 
 	public stopApplication(appIdentifier: string): Promise<void> {
+		this.$logcatHelper.stop(this.identifier);
 		return this.adb.executeShellCommand(["am", "force-stop", `${appIdentifier}`]);
 	}
 

mobile/android/logcat-helper.ts

@@ -1,34 +1,41 @@
 import byline = require("byline");
 import { DeviceAndroidDebugBridge } from "./device-android-debug-bridge";
+import { ChildProcess } from "child_process";
+
+interface IDeviceLoggingData {
+	loggingProcess: ChildProcess;
+	lineStream: any;
+}
 
 export class LogcatHelper implements Mobile.ILogcatHelper {
-	private mapDeviceToLoggingStarted: IDictionary<boolean>;
+	private mapDevicesLoggingData: IDictionary<IDeviceLoggingData>;
 
 	constructor(private $deviceLogProvider: Mobile.IDeviceLogProvider,
 		private $devicePlatformsConstants: Mobile.IDevicePlatformsConstants,
 		private $logger: ILogger,
 		private $injector: IInjector,
 		private $processService: IProcessService) {
-		this.mapDeviceToLoggingStarted = Object.create(null);
+		this.mapDevicesLoggingData = Object.create(null);
 	}
 
 	public async start(deviceIdentifier: string): Promise<void> {
-		if (deviceIdentifier && !this.mapDeviceToLoggingStarted[deviceIdentifier]) {
+		if (deviceIdentifier && !this.mapDevicesLoggingData[deviceIdentifier]) {
 			const adb: Mobile.IDeviceAndroidDebugBridge = this.$injector.resolve(DeviceAndroidDebugBridge, { identifier: deviceIdentifier });
 
-			// remove cached logs:
-			await adb.executeCommand(["logcat", "-c"]);
-
-			const adbLogcat = await adb.executeCommand(["logcat"], { returnChildProcess: true });
-			const lineStream = byline(adbLogcat.stdout);
+			const logcatStream = await adb.executeCommand(["logcat"], { returnChildProcess: true });
+			const lineStream = byline(logcatStream.stdout);
+			this.mapDevicesLoggingData[deviceIdentifier] = {
+				loggingProcess: logcatStream,
+				lineStream: lineStream
+			};
 
-			adbLogcat.stderr.on("data", (data: NodeBuffer) => {
+			logcatStream.stderr.on("data", (data: NodeBuffer) => {
 				this.$logger.trace("ADB logcat stderr: " + data.toString());
 			});
 
-			adbLogcat.on("close", (code: number) => {
+			logcatStream.on("close", (code: number) => {
 				try {
-					this.mapDeviceToLoggingStarted[deviceIdentifier] = false;
+					this.stop(deviceIdentifier);
 					if (code !== 0) {
 						this.$logger.trace("ADB process exited with code " + code.toString());
 					}
@@ -42,10 +49,16 @@ export class LogcatHelper implements Mobile.ILogcatHelper {
 				this.$deviceLogProvider.logData(lineText, this.$devicePlatformsConstants.Android, deviceIdentifier);
 			});
 
-			this.$processService.attachToProcessExitSignals(this, adbLogcat.kill);
+			this.$processService.attachToProcessExitSignals(this, logcatStream.kill);
+		}
+	}
 
-			this.mapDeviceToLoggingStarted[deviceIdentifier] = true;
+	public stop(deviceIdentifier: string): void {
+		if (this.mapDevicesLoggingData[deviceIdentifier]) {
+			this.mapDevicesLoggingData[deviceIdentifier].loggingProcess.removeAllListeners();
+			this.mapDevicesLoggingData[deviceIdentifier].lineStream.removeAllListeners();
 		}
+		delete this.mapDevicesLoggingData[deviceIdentifier];
 	}
 }
 

test/unit-tests/mobile/android/logcat-helper.ts

@@ -0,0 +1,125 @@
+import { LogcatHelper } from "../../../../mobile/android/logcat-helper";
+import { Yok } from "../../../../yok";
+import { assert } from "chai";
+import * as path from "path";
+import * as childProcess from "child_process";
+
+class ChildProcessStub {
+	public static methodCallCount = 0;
+	private isWin = /^win/.test(process.platform);
+
+	public spawn(command: string, args?: string[], options?: any): childProcess.ChildProcess {
+		ChildProcessStub.methodCallCount++;
+		let pathToExecutable = "";
+		if (this.isWin) {
+			pathToExecutable = "type";
+		} else {
+			pathToExecutable = "cat";
+		}
+		pathToExecutable = path.join(pathToExecutable);
+		const pathToSample = path.join(__dirname, "valid-sample.txt");
+		return childProcess.spawn(pathToExecutable, [pathToSample]);
+	}
+}
+
+function createTestInjector(): IInjector {
+	const injector = new Yok();
+	injector.register("injector", injector);
+	injector.register("logcatHelper", LogcatHelper);
+	injector.register("logger", {
+		debug(formatStr?: any, ...args: any[]): void {
+			//left blank intentionally because of lint
+		},
+		trace(formatStr?: any, ...args: any[]): void {
+			if (formatStr && formatStr.indexOf("ADB") !== -1) {
+				//loghelper failed or socket closed"
+				assert.isTrue(false);
+			}
+		}
+	});
+	injector.register("errors", {});
+	injector.register("devicePlatformsConstants", { Android: "Android" });
+	injector.register("processService", {
+		attachToProcessExitSignals(context: any, callback: () => void): void {
+			//left blank intentionally because of lint
+		},
+	});
+	injector.register("deviceLogProvider", {
+		logData(line: string, platform: string, deviceIdentifier: string): void {
+			//left blank intentionally because of lint
+		},
+	});
+	injector.register("childProcess", ChildProcessStub);
+	injector.register("staticConfig", {
+		async getAdbFilePath(): Promise<string> {
+			return "";
+		}
+	});
+	injector.register("androidDebugBridgeResultHandler", {});
+
+	return injector;
+}
+
+describe("logcat-helper", () => {
+	let injector: IInjector;
+	let adbLogcatCrashedOrClosed: boolean;
+	let loggedData: string[];
+
+	beforeEach(() => {
+		adbLogcatCrashedOrClosed = false;
+		injector = createTestInjector();
+		loggedData = [];
+		ChildProcessStub.methodCallCount = 0;
+	});
+
+	describe("start", () => {
+		it("whole logcat is read correctly", (done: mocha.Done) => {
+			injector.register("deviceLogProvider", {
+				logData(line: string, platform: string, deviceIdentifier: string): void {
+					loggedData.push(line);
+					if (line === "end") {
+						assert.isAbove(loggedData.length, 0);
+						done();
+					}
+				}
+			});
+
+			const logcatHelper = injector.resolve<LogcatHelper>("logcatHelper");
+			logcatHelper.start("valid-identifier");
+		});
+		it("if loghelper start is called multiple times with the same identifier it's a noop the second time", async () => {
+			const logcatHelper = injector.resolve<LogcatHelper>("logcatHelper");
+			await logcatHelper.start("valid-identifier");
+			assert.equal(ChildProcessStub.methodCallCount, 1);
+			await logcatHelper.start("valid-identifier");
+			assert.equal(ChildProcessStub.methodCallCount, 1);
+			await logcatHelper.start("valid-identifier");
+			assert.equal(ChildProcessStub.methodCallCount, 1);
+		});
+		it("if loghelper start works when it's called multiple times with different identifiers", async () => {
+			const logcatHelper = injector.resolve<LogcatHelper>("logcatHelper");
+			await logcatHelper.start("valid-identifier1");
+			assert.equal(ChildProcessStub.methodCallCount, 1);
+			await logcatHelper.start("valid-identifier2");
+			assert.equal(ChildProcessStub.methodCallCount, 2);
+			await logcatHelper.start("valid-identifier3");
+			assert.equal(ChildProcessStub.methodCallCount, 3);
+		});
+	});
+	describe("stop", () => {
+		it("device identifier is cleared on stop", async () => {
+			const logcatHelper = injector.resolve<LogcatHelper>("logcatHelper");
+			await logcatHelper.start("valid-identifier");
+			assert.equal(ChildProcessStub.methodCallCount, 1);
+			await logcatHelper.stop("valid-identifier");
+			await logcatHelper.start("valid-identifier");
+			assert.equal(ChildProcessStub.methodCallCount, 2);
+		});
+		it("stop doesn't blow up if called multiple times", async () => {
+			const logcatHelper = injector.resolve<LogcatHelper>("logcatHelper");
+			await logcatHelper.stop("valid-identifier");
+			await logcatHelper.stop("valid-identifier");
+			assert.isTrue(true);
+		});
+	});
+});