fix: logcat process is not restarted in some cases

In case you have `keepSingleProcess` passed to logcat Stream (this option is passed when using CLI as a library - in this case we keep a long living logcat process alive and change only the filter for it by specifying different PIDs), in case the logcat process dies (for example device is disconnected), the process is not cleaned from CLI's internal cache.
This way, in case you try to start the logs again (in the example above - just attach the device again), they are not started as CLI thinks it has a process for this case. So, in case the logcat stream process dies, ensure we clean all the resources related to it.
Add unit tests for this case and remove calls to `type`/`cat` from the tests - there's no need to spawn actual child process in the tests.

Author: rosen-vladimirov

lib/common/mobile/android/logcat-helper.ts

@@ -39,7 +39,8 @@ export class LogcatHelper implements Mobile.ILogcatHelper {
 
 			logcatStream.on("close", (code: number) => {
 				try {
-					this.stop(deviceIdentifier);
+					this.forceStop(deviceIdentifier);
+
 					if (code !== 0) {
 						this.$logger.trace("ADB process exited with code " + code.toString());
 					}
@@ -76,13 +77,17 @@ export class LogcatHelper implements Mobile.ILogcatHelper {
 	 */
 	public stop(deviceIdentifier: string): void {
 		if (this.mapDevicesLoggingData[deviceIdentifier] && !this.mapDevicesLoggingData[deviceIdentifier].keepSingleProcess) {
-			this.mapDevicesLoggingData[deviceIdentifier].loggingProcess.removeAllListeners();
-			this.mapDevicesLoggingData[deviceIdentifier].loggingProcess.kill("SIGINT");
-			this.mapDevicesLoggingData[deviceIdentifier].lineStream.removeAllListeners();
-			delete this.mapDevicesLoggingData[deviceIdentifier];
+			this.forceStop(deviceIdentifier);
 		}
 	}
 
+	private forceStop(deviceIdentifier: string): void {
+		this.mapDevicesLoggingData[deviceIdentifier].loggingProcess.removeAllListeners();
+		this.mapDevicesLoggingData[deviceIdentifier].loggingProcess.kill("SIGINT");
+		this.mapDevicesLoggingData[deviceIdentifier].lineStream.removeAllListeners();
+		delete this.mapDevicesLoggingData[deviceIdentifier];
+	}
+
 	private async getLogcatStream(deviceIdentifier: string, pid?: string) {
 		const device = await this.$devicesService.getDevice(deviceIdentifier);
 		const minAndroidWithLogcatPidSupport = "7.0.0";

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

@@ -3,34 +3,39 @@ import { Yok } from "../../../../yok";
 import { assert } from "chai";
 import * as path from "path";
 import * as childProcess from "child_process";
+import * as fileSystem from "fs";
+import { EventEmitter } from "events";
+import stream = require("stream");
+
+class ChildProcessMockInstance extends EventEmitter {
+	public stdout: stream.Readable;
+	public stderr: any;
+	public processKillCallCount = 0;
+
+	constructor(pathToSample: string) {
+		super();
+		this.stdout = fileSystem.createReadStream(pathToSample);
+		this.stderr = new EventEmitter();
+	}
+
+	public kill(): void {
+		this.processKillCallCount++;
+	}
+}
 
 class ChildProcessStub {
 	public processSpawnCallCount = 0;
-	public processKillCallCount = 0;
 	public adbProcessArgs: string[] = [];
-	private isWin = /^win/.test(process.platform);
+	public lastSpawnedProcess: ChildProcessMockInstance = null;
 
 	public spawn(command: string, args?: string[], options?: any): childProcess.ChildProcess {
 		this.adbProcessArgs = args;
 		this.processSpawnCallCount++;
-		let pathToExecutable = "";
-		let shell = "";
-		if (this.isWin) {
-			pathToExecutable = "type";
-			shell = "cmd";
-		} else {
-			pathToExecutable = "cat";
-		}
-		pathToExecutable = path.join(pathToExecutable);
 		const pathToSample = path.join(__dirname, "valid-sample.txt");
-		const spawnedProcess = childProcess.spawn(pathToExecutable, [pathToSample], { shell });
-		const spawnedProcessKill = spawnedProcess.kill;
-		spawnedProcess.kill = (signal: string) => {
-			this.processKillCallCount++;
-			spawnedProcessKill.call(spawnedProcessKill, signal);
-		};
-
-		return spawnedProcess;
+
+		this.lastSpawnedProcess = new ChildProcessMockInstance(pathToSample);
+
+		return <any>this.lastSpawnedProcess;
 	}
 }
 
@@ -207,7 +212,7 @@ describe("logcat-helper", () => {
 			await logcatHelper.stop(validIdentifier);
 			await logcatHelper.stop(validIdentifier);
 
-			assert.equal(childProcessStub.processKillCallCount, 1);
+			assert.equal(childProcessStub.lastSpawnedProcess.processKillCallCount, 1);
 		});
 
 		it("should not kill the process if started with keepSingleProcess", async () => {
@@ -220,7 +225,7 @@ describe("logcat-helper", () => {
 
 			await logcatHelper.stop(validIdentifier);
 			await logcatHelper.stop(validIdentifier);
-			assert.equal(childProcessStub.processKillCallCount, 0);
+			assert.equal(childProcessStub.lastSpawnedProcess.processKillCallCount, 0);
 		});
 
 		it("should do nothing if called without start", async () => {
@@ -229,7 +234,27 @@ describe("logcat-helper", () => {
 			await logcatHelper.stop(validIdentifier);
 
 			assert.equal(childProcessStub.processSpawnCallCount, 0);
-			assert.equal(childProcessStub.processKillCallCount, 0);
 		});
+
+		for (const keepSingleProcess of [false, true]) {
+			it(`should clear the device identifier cache when the logcat process is killed manually and keepSingleProcess is ${keepSingleProcess}`, async () => {
+				const logcatHelper = injector.resolve<LogcatHelper>("logcatHelper");
+
+				await logcatHelper.start({
+					deviceIdentifier: validIdentifier,
+					keepSingleProcess
+				});
+
+				assert.equal(childProcessStub.processSpawnCallCount, 1);
+
+				childProcessStub.lastSpawnedProcess.emit("close");
+
+				await logcatHelper.start({
+					deviceIdentifier: validIdentifier
+				});
+
+				assert.equal(childProcessStub.processSpawnCallCount, 2);
+			});
+		}
 	});
 });