Plamen5kov/refactoring #918

Plamen5kov · 2017-03-24T09:09:09Z

Moved functionality from tns emulate to tns run command.

Main changes:

--device flag starts emulator/simulator if necessary
--available-devices lists all available images to start along with all connected devices (shows Device Identifier)
can start device by using the Device Identifier for both android and ios emulators/simulators
can run application on device using it's: Device Identifier and index.

related issue: NativeScript/nativescript-cli#2349

justcodebuilduser · 2017-03-24T09:11:23Z

❤️

petekanev · 2017-03-24T09:34:32Z

mobile/ios/simulator/ios-emulator-services.ts

@@ -12,6 +12,11 @@ class IosEmulatorServices implements Mobile.IiOSSimulatorService {
 		return "";
 	}

+	public async getRunningEmulatorId(image: string): Promise<string> {
+		//todo: plamen5kov: fix later if necessary
+		return "";


You should return a Promise here - return Promise.resolve("")

I think the async decorator does that for me.

We can verify this by looking at the .js code, right? Can you please share it so that we all know how it looks?

async generates a method that returns Promise - you can safely return the "".

petekanev · 2017-03-24T09:37:31Z

mobile/wp8/wp8-emulator-services.ts

@@ -20,6 +20,11 @@ class Wp8EmulatorServices implements Mobile.IEmulatorPlatformServices {
 		return "";
 	}

+	public async getRunningEmulatorId(image: string): Promise<string> {
+		//todo: plamen5kov: fix later if necessary
+		return "";


You should return a Promise here - return Promise.resolve("")

Could you also remove the 'todo' and instead add a comment that it's an empty stub?

I prefer leaving the todo untill all the tests in master pass.

yyosifov · 2017-03-24T11:47:08Z

mobile/android/android-emulator-services.ts

 		let result: string[] = [];
 		if (this.$fs.exists(this.avdDir)) {
 			let entries = this.$fs.readDirectory(this.avdDir);
 			result = _.filter(entries, (e: string) => e.match(AndroidEmulatorServices.INI_FILES_MASK) !== null)
 				.map((e) => e.match(AndroidEmulatorServices.INI_FILES_MASK)[1]);
 		}
-		return result;
+		if (suggestedImage && !_.includes(result, suggestedImage)) {


I'd suggest we simplify and refactor this a little bit. It took me a lot of time to understand the flow here.

Why not getAvds to return just the AVDs and then we can filter them out when we have to?

You got a point. Will fix.

yyosifov · 2017-03-24T11:47:33Z

mobile/android/android-emulator-services.ts

 		let result: string[] = [];
 		if (this.$fs.exists(this.avdDir)) {
 			let entries = this.$fs.readDirectory(this.avdDir);
 			result = _.filter(entries, (e: string) => e.match(AndroidEmulatorServices.INI_FILES_MASK) !== null)
 				.map((e) => e.match(AndroidEmulatorServices.INI_FILES_MASK)[1]);
 		}
-		return result;
+		if (suggestedImage && !_.includes(result, suggestedImage)) {
+			return null;


When we return null, I guess null.map()... would throw an exception below in getBestFit(). One more reason for refactoring

yyosifov · 2017-03-24T12:26:10Z

mobile/mobile-core/devices-service.ts

+		let foundDevices = this.getDeviceInstances();
+
+		//if no --device is passed and no devices are found, the default emulator is started
+		if (data && data.platform && !data.deviceId) {


why not fail with an error in the beginning if !data || !data.platform? we move this check everywhere below and it's something I think this method cannot operate correctly without anyway?

To improve readability I would suggest something like: if(!data.deviceId && _.isEmpty(foundDevices)) { }

why not fail with an error in the beginning if !data || !data.platform?

Because there's common code that will use this functionality and won't need platform specified

yyosifov · 2017-03-24T12:30:33Z

mobile/mobile-core/devices-service.ts

+
+		// are there any running devices
+		await this.detectCurrentlyAttachedDevices();
+		let foundDevices = this.getDeviceInstances();


I would rename this variable to deviceInstances to be consistent with the method - getDeviceInstances

yyosifov · 2017-03-24T12:31:45Z

mobile/mobile-core/devices-service.ts

+
+		//check if --device(value) is running, if it's not or it's not the same as is specified, start with name from --device(value)
+		if (data && data.platform && data.deviceId) {
+			if (!helpers.isNumber(data.deviceId))  {


what if the deviceId is a number? Is it incorrect for deviceId? What's the format of the device Id and do we need to validate this here?

In this context, we only care if the deviceId is an image that we need to start the emulator from. If it's a number, we don't need to do anything

yyosifov · 2017-03-24T12:32:38Z

mobile/mobile-core/devices-service.ts

+		//check if --device(value) is running, if it's not or it's not the same as is specified, start with name from --device(value)
+		if (data && data.platform && data.deviceId) {
+			if (!helpers.isNumber(data.deviceId))  {
+				let searchedDevice = _.find(this.getDeviceInstances(), (device: Mobile.IDevice) => { return device.deviceInfo.identifier === data.deviceId; });


I would rename this to just "activeDeviceInstance" then it would be meaningful to have if(!activeDeviceInstance) { start one for me}

yyosifov · 2017-03-24T12:34:29Z

mobile/mobile-core/devices-service.ts

+
+		// if only emulator flag is passed and no other emulators are running, start default emulator
+		if (data && data.platform && data.emulator && foundDevices.length) {
+			let atLeastOneEmulator = _.some(foundDevices, (value) => {return value.isEmulator; });


this would be "runningDeviceInstance" I guess?

yyosifov · 2017-03-24T12:34:52Z

mobile/mobile-core/devices-service.ts

+		if (data && data.platform && data.emulator && foundDevices.length) {
+			let atLeastOneEmulator = _.some(foundDevices, (value) => {return value.isEmulator; });
+			if (!atLeastOneEmulator) {
+				return await this.startEmulator(data.platform);


does the --emulator flag support --emulatorId or a like for it?

--emulator flag is boolean, if you want to specify the emulator --device flag should be used.

yyosifov · 2017-03-24T12:37:00Z

mobile/mobile-core/devices-service.ts

+	 * Returns true if there's a running device with specified identifier.
+	 * @param identifier parameter passed by the user to --device flag
+	 */
+	private async hasDevice(identifier: string): Promise<boolean> {


As per the comment of the method I would make it more self-descriptive as "hasRunningDevice"

justcodebuilduser · 2017-03-27T11:34:01Z

💔

petekanev · 2017-03-24T09:46:03Z

mobile/mobile-core/devices-service.ts

+	 * Returns true if there's a running device with specified identifier.
+	 * @param identifier parameter passed by the user to --device flag
+	 */
+	private async hasDevice(identifier: string): Promise<boolean> {


I don't think any async operations are ran in the method.

petekanev · 2017-03-24T09:47:15Z

mobile/mobile-core/devices-service.ts

+			emulatorIdentifier = await emulatorService.getRunningEmulatorId(deviceOption);
+		}
+
+		if (await this.hasDevice(emulatorIdentifier)) {


Remove the await if this.hasDevice becomes synchronous.

petekanev · 2017-03-24T12:07:43Z

mobile/android/android-emulator-services.ts

@@ -117,7 +117,7 @@ class AndroidEmulatorServices implements Mobile.IAndroidEmulatorServices {
 			// unlock screen
 			await this.unlockScreen(emulatorId);
 		} else {
-			this.$errors.fail("Could not find an emulator image to run your project.");
+			this.$errors.fail(`Could not find an emulator image or identifier to run your project. Please run: "tns run <platform> --available-devices"`);


Make sure to change this one if tns devices will be used in favor of run --available devices

petekanev · 2017-03-24T12:35:13Z

mobile/mobile-core/devices-service.ts

@@ -217,6 +220,11 @@ export class DevicesService implements Mobile.IDevicesService {
 		await this.getDeviceDetectionIntervalFuture();
 	}

+	/**
+	 * Returns device that maches an identifier.


[minor] maches -> maTches typo

justcodebuilduser · 2017-03-27T12:02:11Z

💔

yyosifov · 2017-03-27T12:08:13Z

bootstrap.ts

-$injector.requireCommand("device|android", "./commands/device/list-devices");
-$injector.requireCommand("device|ios", "./commands/device/list-devices");
+$injector.requireCommand(["device|android", "devices|android"], "./commands/device/list-devices");
+$injector.requireCommand(["device|ios", "devics|ios"], "./commands/device/list-devices");


typo: devics|

yyosifov · 2017-03-27T12:08:41Z

commands/device/list-devices.ts

@@ -65,4 +72,4 @@ class ListiOSDevicesCommand implements ICommand {
 	}
 }

-$injector.registerCommand("device|ios", ListiOSDevicesCommand);
+$injector.registerCommand(["device|ios", "devics|ios"], ListiOSDevicesCommand);


typo: devics

yyosifov · 2017-03-27T12:15:36Z

mobile/android/android-emulator-services.ts

 		let minVersion = this.$emulatorSettingsService.minVersion;

-		let best = _(this.getAvds())
+		let avdResults = this.getAvds();
+		if (suggestedImage && !_.includes(avdResults, suggestedImage)) {


The whole if/else can be simplified even further to just one filter:

if(suggestedImage) {
avgResults = avgResults.filter(avd => avd === suggestedImage);
}

yyosifov · 2017-03-27T12:19:51Z

mobile/mobile-core/devices-service.ts

+		}
+
+		// if only emulator flag is passed and no other emulators are running, start default emulator
+		if (data && data.platform && data.emulator && deviceInstances.length) {


We can validate the data && data.platform in the beginning of the method - it's included in all If statements in this method

justcodebuilduser · 2017-03-27T12:39:45Z

💔

justcodebuilduser · 2017-03-27T12:58:54Z

💔

justcodebuilduser · 2017-03-27T14:20:20Z

💔

rosen-vladimirov · 2017-03-27T22:10:34Z

mobile/android/android-emulator-services.ts

@@ -117,7 +117,7 @@ class AndroidEmulatorServices implements Mobile.IAndroidEmulatorServices {
 			// unlock screen
 			await this.unlockScreen(emulatorId);
 		} else {
-			this.$errors.fail("Could not find an emulator image to run your project.");
+			this.$errors.fail(`Could not find an emulator image or identifier to run your project. Please run: "tns device <platform> --available-devices"`);


this code is used in different CLIs. In case you want to specify command that the user should execute, you can use something like:

his.$errors.fail(`Could not find an emulator image or identifier to run your project. Please run: "${this.$staticConfig.CLIENT_NAME} device <platform> --available-devices"`);

rosen-vladimirov · 2017-03-27T22:19:04Z

mobile/mobile-core/devices-service.ts

+			emulatorIdentifier = await emulatorService.getRunningEmulatorId(deviceOption);
+		}
+
+		if (this.hasRunningDevice(emulatorIdentifier)) {


I'm not sure this code is correct. In case I have running emulator and device attached, if I use any command with --device <my connected real device identifier>, the current code will still return the emulator instance, which is not what I want.

We pass the emulatorIdentifier from `emulatorService.getRunningEmulatorId(). Try it out: if the case is, connected emulator and a connected device and you want to run on the emulator you can do the following:
tns run android --device <emulator_index>
tns run android --device <emulator-5554/6 ...>
tns run android --device <emulator_image>
This will start the app only on the specified emulator.

If it's a device you want:
tns run android --device <device_identifier>
tns run android --device <device_index>

Please excuse me, totally my mistake - emulatorIdentifier will be falsey value in case I've specified the identifier of the real device

rosen-vladimirov · 2017-03-27T22:21:25Z

mobile/mobile-core/devices-service.ts

+	public async startEmulatorIfNecessary(data?: Mobile.IDevicesServicesInitializationOptions): Promise<void> {
+		if (data && data.deviceId && data.emulator) {
+			this.$errors.failWithoutHelp(`--device and –emulator are incompatible options.
+If you are trying to run on specific emulator, use tns run –device <DeviceID>.`);


`--device and --emulator are incompatible options. If you are trying to run on specific emulator, use "${this.$staticConfig.CLIENT_NAME} run --device <DeviceID>.`

rosen-vladimirov · 2017-03-27T22:23:45Z

mobile/mobile-core/devices-service.ts

+
+			// if only emulator flag is passed and no other emulators are running, start default emulator
+			if (data.emulator && deviceInstances.length) {
+				let runningDeviceInstance = _.some(deviceInstances, (value) => {return value.isEmulator; });


could be shorten:

const runningDeviceInstance = _.some(deviceInstances, value => value.isEmulator);

rosen-vladimirov · 2017-03-27T22:27:06Z

test/unit-tests/mobile/devices-service.ts

@@ -437,6 +440,8 @@ describe("devicesService", () => {
 			it("does not fail when androidDeviceDiscovery startLookingForDevices fails", async () => {
 				(<any>androidDeviceDiscovery).startLookingForDevices = (): Promise<void> => { throw new Error("my error"); };
 				iOSDeviceDiscovery.emit("deviceFound", iOSDevice);
+				let hostInfo = testInjector.resolve("hostInfo");
+				hostInfo.isDarwin = true;


why is this code added?

The purpose of the test is to check if startLookingForDevices fails, which is in no way compromised by adding hostInfo.isDarwin. It's used to resolve an emulator service, and by that time the test has either failed or not.

rosen-vladimirov · 2017-03-27T22:31:04Z

services/image-service.ts

+import { createTable } from "../helpers";
+
+//todo: plamen5kov: moved most of emulator-service here as a temporary solution untill after 3.0-RC
+export class ImageService implements Mobile.IImageService {


hm, why is this called ImageService? This way I would think it works with images, for example in App_Resources dir.

I could rename it to EmulatorImageService if it would be more readable?

rosen-vladimirov · 2017-03-27T22:41:13Z

test/unit-tests/mobile/devices-service.ts

@@ -469,6 +474,8 @@ describe("devicesService", () => {
 			assert.isFalse(devicesService.hasDevices, "Initially devicesService hasDevices must be false.");
 			iOSDeviceDiscovery.emit("deviceFound", iOSDevice);
 			androidDeviceDiscovery.emit("deviceFound", androidDevice);
+			let hostInfo = testInjector.resolve("hostInfo");
+			hostInfo.isDarwin = true;


same here - the test should be platform independent - why should we set the OS to be macOS ?

I planed to change this test. Will update it with correct message, in appliance with refactoring.

justcodebuilduser · 2017-03-28T05:28:49Z

❤️

add comments to devices-service updated tests after logical changes * removed startEmulator functionality from initialize, so we dont expect emulator to be started by initialize in any case anymore * in initialize we first check the platform, because other functionality depends on it * getDevice method should work even withrout platform specified (at least the test say so) fixed running emulator by index Pass the device id to the startEmulator. removed method we do not want to support made lint happy added available-devices option to mobile-cli-lib added alias for tns device: devices updates after review updated tests to check if emulators are started in specific cases updated getAvds logic and getbestFit to improve readability updates after review typo, field and method renamings fixed typos simplify code refactoring after review

justcodebuilduser · 2017-03-28T05:55:28Z

❤️

justcodebuilduser · 2017-03-28T08:06:19Z

❤️

Plamen5kov requested review from yyosifov, rosen-vladimirov and TsvetanMilanov March 24, 2017 09:09

Plamen5kov mentioned this pull request Mar 24, 2017

Plamen5kov/move emulate to run NativeScript/nativescript-cli#2647

Merged

Plamen5kov requested a review from petekanev March 24, 2017 09:13

Plamen5kov self-assigned this Mar 24, 2017

petekanev reviewed Mar 24, 2017

View reviewed changes

yyosifov reviewed Mar 24, 2017

View reviewed changes

petekanev reviewed Mar 27, 2017

View reviewed changes

yyosifov reviewed Mar 27, 2017

View reviewed changes

Plamen5kov force-pushed the plamen5kov/refactoring branch from 252c027 to 2fe8654 Compare March 27, 2017 12:57

rosen-vladimirov suggested changes Mar 27, 2017

View reviewed changes

Plamen5kov force-pushed the plamen5kov/refactoring branch from 4950d06 to 24af78c Compare March 28, 2017 05:25

Plamen5kov added 2 commits March 28, 2017 08:37

moved emulator-service to common as aa temporary solution

363a40e

Plamen5kov force-pushed the plamen5kov/refactoring branch from 24af78c to 363a40e Compare March 28, 2017 05:53

updates after review

33a7fdf

rosen-vladimirov approved these changes Mar 28, 2017

View reviewed changes

Plamen5kov merged commit 11415a5 into master Mar 28, 2017

Plamen5kov mentioned this pull request Mar 28, 2017

added unit tests for startEmulatorIfNecessary method in devicesService #924

Merged

Plamen5kov deleted the plamen5kov/refactoring branch March 28, 2017 16:25

Plamen5kov/refactoring #918

Plamen5kov/refactoring #918

Conversation

Plamen5kov commented Mar 24, 2017 • edited Loading

justcodebuilduser commented Mar 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justcodebuilduser commented Mar 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justcodebuilduser commented Mar 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justcodebuilduser commented Mar 27, 2017

justcodebuilduser commented Mar 27, 2017

justcodebuilduser commented Mar 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Plamen5kov Mar 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Plamen5kov Mar 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justcodebuilduser commented Mar 28, 2017

justcodebuilduser commented Mar 28, 2017

justcodebuilduser commented Mar 28, 2017

Plamen5kov commented Mar 24, 2017 •

edited

Loading

Plamen5kov Mar 28, 2017 •

edited

Loading

Plamen5kov Mar 28, 2017 •

edited

Loading