use am start instead of monkey to start application #995

Plamen5kov · 2017-07-31T08:43:52Z

problem
monkey is a tool used for testing, not for production use and we shouldn't use it since it's a tool used and built for stress testing. It has random behavior and it's not recommended for production use and therefore we are replacing monkey with the recommended am tool.
Fix Issue: NativeScript/NativeScript#4591

solution
Use am tool instead to start the application by it's activity identifier which is a combination of the package name and the default activity.
Example for such an identifier would be: org.nativescript.appname/com.tns.MainActivity

TsvetanMilanov

The monkey tool is also used in https://github.com/telerik/mobile-cli-lib/blob/master/mobile/android/android-emulator-services.ts#L181. I think you should change it there as well.

TsvetanMilanov · 2017-08-02T07:45:18Z

mobile/android/android-application-manager.ts

-			"-c", "android.intent.category.LAUNCHER",
-			"1"]);
+		const pmDumpOutput = await this.adb.executeShellCommand(["pm", "dump", appIdentifier, "|", "grep", "-A", "1", "MAIN"]);
+		const fullActivityNameRegExp = /([A-Za-z]{1}[A-Za-z\d_]*\.)*[A-Za-z][A-Za-z\d_]*\/([a-z][a-z_0-9]*\.)*[A-Z_]($[A-Z_]|[\w_])*/;


Can you provide some example output of pm dump?

Also this regex is really complicated. Is it from official documentation? In any case we need tests for it :)

One regex is for verified Android application package and the second part is for a fully qualified java class name. Will write the tests!

Here's an example of the pm dump | grep -A 1 MAIN:

android.intent.action.MAIN: 3b2df03 org.nativescript.cliapp/com.tns.NativeScriptActivity filter 50dd82e Action: "android.intent.action.MAIN" Category: "android.intent.category.LAUNCHER" -- intent={act=android.intent.action.MAIN cat=[android.intent.category.LAUNCHER] flg=0x10200000 cmp=org.nativescript.cliapp/com.tns.NativeScriptActivity} realActivity=org.nativescript.cliapp/com.tns.NativeScriptActivity -- Intent { act=android.intent.action.MAIN cat=[android.intent.category.LAUNCHER] flg=0x10200000 cmp=org.nativescript.cliapp/com.tns.NativeScriptActivity } frontOfTask=true task=TaskRecord{fe592ac #449 A=org.nativescript.cliapp U=0 StackId=1 sz=1}

and the regex finds the full path to the activity name, which in this case is: org.nativescript.cliapp/com.tns.NativeScriptActivity

TsvetanMilanov · 2017-08-02T07:50:42Z

mobile/android/android-application-manager.ts

+		const pmDumpOutput = await this.adb.executeShellCommand(["pm", "dump", appIdentifier, "|", "grep", "-A", "1", "MAIN"]);
+		const fullActivityNameRegExp = /([A-Za-z]{1}[A-Za-z\d_]*\.)*[A-Za-z][A-Za-z\d_]*\/([a-z][a-z_0-9]*\.)*[A-Z_]($[A-Z_]|[\w_])*/;
+		const activityMatch = new RegExp(fullActivityNameRegExp, "m");
+		const possibleIdentifier = activityMatch.exec(pmDumpOutput)[0];


activityMatch.exec(pmDumpOutput) will return null if nothing matches the RegExp. I think it will be better to check the result before we try to get the first match from it.

TsvetanMilanov · 2017-08-02T07:54:31Z

mobile/android/android-application-manager.ts

+		const pmDumpOutput = await this.adb.executeShellCommand(["pm", "dump", appIdentifier, "|", "grep", "-A", "1", "MAIN"]);
+		const fullActivityNameRegExp = /([A-Za-z]{1}[A-Za-z\d_]*\.)*[A-Za-z][A-Za-z\d_]*\/([a-z][a-z_0-9]*\.)*[A-Z_]($[A-Z_]|[\w_])*/;
+		const activityMatch = new RegExp(fullActivityNameRegExp, "m");
+		const possibleIdentifier = activityMatch.exec(pmDumpOutput)[0];


Also I think if the result is null we should try to start the default NativeScript/AppBuilder activity. This way if the output from pm dump is changed we will be able to start applications which use the default activity.

I'd prefer we don't start Activities that are not classified with the "MAIN" tag. The activity may be in the manifest, but not as a "MAIN" activity, and it will be an unexpected behavior to start an activity not meant as main.

Yes, it will be unexpected behavior. My point is that if we start the default main activity, after we have failed to get it from pm dump, we will not break the CLI for the users who use the default main activity.
When we use the monkey tool we know that it will start the main activity, no matter how it's named. With the new logic we search for the main activity name with grep -A 1 and I think it is possible that on different Android versions the output can be different. Also if we hit other problems with the new approach we will not be able to start any application if we don't use the default main activity.

I understand your point, but we can't start a hardcoded activity and hope it's the main one in the manifest.

TsvetanMilanov · 2017-08-02T07:56:36Z

mobile/android/android-application-manager.ts

+		if (possibleIdentifier) {
+			await this.adb.executeShellCommand(["am", "start", "-n", possibleIdentifier]);
+		} else {
+			this.$logger.trace(`Tried starting activity: ${possibleIdentifier}, but failed`);


We will get here if we don't have possibleIdentifier. This means that we will log null/undefined/"". Maybe we should log the appIdentifier.

good catch will fix to show app identifier

Plamen5kov · 2017-08-03T11:48:23Z

@TsvetanMilanov the method you're referring to runApplicationOnEmulatorCore in the android-emulator-services is not used since we removed the emulate command. We can remove the whole emulator service, but it let it be in another PR.

TsvetanMilanov · 2017-08-04T12:06:43Z

mobile/android/android-application-manager.ts

@@ -102,4 +114,8 @@ export class AndroidApplicationManager extends ApplicationManagerBase {

 		return applicationViews;
 	}
+
+	public getFullyQualifiedActivityRegex(): RegExp {


This method is used only in this class. It should be private. You should test it through the public method startApplication.

TsvetanMilanov · 2017-08-04T12:07:47Z

mobile/android/android-application-manager.ts

-			"1"]);
+		const pmDumpOutput = await this.adb.executeShellCommand(["pm", "dump", appIdentifier, "|", "grep", "-A", "1", "MAIN"]);
+		const fullActivityNameRegExp = this.getFullyQualifiedActivityRegex();
+		const activityMatch = new RegExp(fullActivityNameRegExp, "m");


fullActivityNameRegExp is RegExp. You don't need to create new RegExp from RegExp. Just add the m flag in getFullyQualifiedActivityRegex.
Also I don't think you need the m flag.

I do need the "m" flag because we are searching for "MAIN", but the actual string we're matching is on the next line.

TsvetanMilanov · 2017-08-04T12:11:17Z

mobile/android/android-application-manager.ts

-			"-p", appIdentifier,
-			"-c", "android.intent.category.LAUNCHER",
-			"1"]);
+		const pmDumpOutput = await this.adb.executeShellCommand(["pm", "dump", appIdentifier, "|", "grep", "-A", "1", "MAIN"]);


You should add the example output from pm dump from #995 (comment) as comment here.

TsvetanMilanov · 2017-08-04T13:54:54Z

mobile/android/android-application-manager.ts

@@ -102,4 +114,8 @@ export class AndroidApplicationManager extends ApplicationManagerBase {

 		return applicationViews;
 	}
+
+	public getFullyQualifiedActivityRegex(): RegExp {
+		return /([A-Za-z]{1}[A-Za-z\d_]*\.)*[A-Za-z][A-Za-z\d_]*\/([a-z][a-z_0-9]*\.)*[A-Z_$]($[A-Z_$]|[$_\w_])*/;


If you want to make this long regular expression easier to read/understand you can make this method like this one -> https://github.com/telerik/mobile-cli-lib/blob/master/mobile/mobile-core/android-process-service.ts#L18.

I didn't want to split the regex because of the escaping, but if it's done like so in other places, I can also do it here.

TsvetanMilanov · 2017-08-04T13:56:18Z

test/unit-tests/android-application-manager.ts

+				assert.isArray(match);
+				assert.isTrue(expectedElement === match[0]);
+			}
+			assert.isTrue(true);


Why we need this assert.isTrue(true);?

must have forgotten it when testing, will remove it

TsvetanMilanov · 2017-08-04T14:24:29Z

test/unit-tests/android-application-manager.ts

+			"org.nativescript.testApp/com.tns.$_TestClass",
+			"org.nativescript.testApp/com.tns._$TestClass"
+		],
+		validTestInput = [


You should add more test cases which include some real output from the pm dump command.

TsvetanMilanov · 2017-08-04T14:29:54Z

test/unit-tests/android-application-manager.ts

+		testInjector.register("deviceLogProvider", {});
+		testInjector.register("hooksService", {});
+	});
+	describe("tries to get fully qualified activity class name", () => {


This describe should be the name of the method it tests. This way it will be consistent with the other test files. Example -> https://github.com/telerik/mobile-cli-lib/blob/master/test/unit-tests/analytics-service.ts#L173

TsvetanMilanov · 2017-08-04T14:35:15Z

test/unit-tests/android-application-manager.ts

+describe("android-application-manager", () => {
+
+	let testInjector: IInjector,
+		validTestInput: Array<string>,


I think this variable should be in the test which uses it.
Also it's type should be string[]

TsvetanMilanov · 2017-08-04T14:36:18Z

test/unit-tests/android-application-manager.ts

+
+	let testInjector: IInjector,
+		validTestInput: Array<string>,
+		expectedValidTestInput: Array<string>;


I think this variable should be in the test which uses it.
Also it's type should be string[]

why does it matter if they are string[] or Array?

There is no difference between Array<string> and string[]. I prefer to use string[] for consistency with the other code.

TsvetanMilanov · 2017-08-04T14:49:12Z

test/unit-tests/android-application-manager.ts

+		testInjector.register("hooksService", {});
+	});
+	describe("tries to get fully qualified activity class name", () => {
+		it("and succeeds finding the right name", async () => {


Please add test which verifies that we default to monkey if we can't get the main activity name.

TsvetanMilanov · 2017-08-04T15:25:39Z

@Plamen5kov Can you check if androidEmulatorServices is used in the AppBuilder CLI before you remove it?

Plamen5kov · 2017-08-07T06:36:16Z

Sure, when I make another PR about removing the androidEmulatorService, I'll check.

rosen-vladimirov

I have some concerns regarding usage of pm dump. I'll need at least clarification of some cases before approving this PR.

I like the tests.

rosen-vladimirov · 2017-08-07T21:56:20Z

mobile/android/android-application-manager.ts

+		const packageActivitySeparator = "\\/";
+		const fullJavaClassName = "([a-z][a-z_0-9]*\\.)*[A-Z_$]($[A-Z_$]|[$_\\w_])*";
+
+		return new RegExp(`${androidPackageName}${packageActivitySeparator}${fullJavaClassName}`, `m`);


Creating RegExp every time is slow operation. You can add @cache decorator at the top of the method.
However, I'm wondering why can't we apply specific regular expression for each application identifier that we are using. At the moment, the regular expression is generic one, but wouldn't it be easier (and safer) to use the applicationIdentifier in the regular expression. In fact we are searching for <application identifier>/<fullJavaClassName>

Will put @cache as you suggested, thank you!
I thought about using the app identifier for the first part of the regex, but I prefer to keep the general implementation since the app identifier is sometimes overridden by the user in app.gradle.

rosen-vladimirov · 2017-08-07T21:57:08Z

mobile/android/android-application-manager.ts

+			Intent { act=android.intent.action.MAIN cat=[android.intent.category.LAUNCHER] flg=0x10200000 cmp=org.nativescript.cliapp/com.tns.NativeScriptActivity }
+			frontOfTask=true task=TaskRecord{fe592ac #449 A=org.nativescript.cliapp U=0 StackId=1 sz=1}
+		*/
+		const pmDumpOutput = await this.adb.executeShellCommand(["pm", "dump", appIdentifier, "|", "grep", "-A", "1", "MAIN"]);


So what's the expected behavior when the application has more than one MAIN activity, for example:

DUMP OF SERVICE package: Activity Resolver Table: Non-Data Actions: android.intent.action.MAIN: 17f3e44f org.nativescript.appwithuniquename/com.tns.NativeScriptActivity 268277dc org.nativescript.appwithuniquename/com.tns.NativeScriptActivity1

Currently we'll get only the first one, are we fine with this?

Also, shouldn't we find the activity with Launcher category? For example in case I have the following in my manifest:

<activity android:name="com.tns.NativeScriptActivity" android:label="@string/title_activity_kimera" android:configChanges="keyboardHidden|orientation|screenSize" android:theme="@style/LaunchScreenTheme"> <meta-data android:name="SET_THEME_ON_LAUNCH" android:resource="@style/AppTheme" /> <intent-filter> <action android:name="android.intent.action.MAIN" /> <category android:name="android.intent.category.LAUNCHER" /> </intent-filter> </activity> <activity android:name="com.tns.NativeScriptActivity1"> <meta-data android:name="Test" android:resource="@style/AppTheme" /> <intent-filter> <action android:name="android.intent.action.MAIN" /> <category android:name="android.intent.category.IOT_LAUNCHER"/> <category android:name="android.intent.category.DEFAULT"/> </intent-filter> </activity>

The com.tns.NativeScriptActivity should be started, will this happen?

Isn't it faster to call adb shell dumpsys package <appIdentifier>. On my machine it is twice faster, but maybe it's only here.

[Minor] Do we really need this grep? First of all, is grep -A available on all Android devices? Also we are getting part of the output of pm dump here and later we are searching for something via regular expression. I'm wondering, is it possible to make the regular expression smart enough to parse the full result.

So what's the expected behavior when the application has more than one MAIN activity
The first main activity will be taken. I've not seen a user case where there's more than one MAIN activity, plus the user can specify a DEFAULT main activity, but I haven't seen that either, so I didn't want to complicate the implementation if there's no need.

Also, shouldn't we find the activity with Launcher category?
I also thought about this, but then I found out that some part of the usual Android applications don't specify a launcher category. Read more here. Keeping this in mind I thought best not to discriminate against such apps.
Regarding the example of the two MAIN activities you provided, I haven't thought about it, because up until now, I hadn't seen this scenario, but I'm open to discussing all possible scenarios.

Isn't it faster to call adb shell dumpsys package <appIdentifier>
To be honest I used pm dump instead of dumpsys because when I saw the content of pm it seemed more reliable because it was part of the framework.jar. dumpsys command is just a binary that is somewhere in the /system/bin folder, and I couldn't find any information on its support and reliability.

generic_x86:/ # cat /system/bin/pm # Script to start "pm" on the device, which has a very rudimentary # shell. # base=/system export CLASSPATH=$base/framework/pm.jar exec app_process $base/bin com.android.commands.pm.Pm "$@" generic_x86:/ # cat /system/bin/am #!/system/bin/sh # # Script to start "am" on the device, which has a very rudimentary # shell. #

Do we really need this grep
I've tested this and the regex will work with the full result of the pm dump in case -A 1 isn't available, but the regex works faster in a shorter amount of text so I left it with the -A 1 option.

@Plamen5kov , thanks for the detailed explanations. I'm not familiar with Android apps with more than one MAIN activities, there are such samples in Android Studio. It looks like the only way to understand if the current approach is fine is to merge it and see if such case will ever arise.

rosen-vladimirov · 2017-08-07T22:04:47Z

mobile/android/android-application-manager.ts

+		const pmDumpOutput = await this.adb.executeShellCommand(["pm", "dump", appIdentifier, "|", "grep", "-A", "1", "MAIN"]);
+		const activityMatch = this.getFullyQualifiedActivityRegex();
+		const match = activityMatch.exec(pmDumpOutput);
+		let possibleIdentifier = "";


this can be:

const possibleIdentifier = match && match[0];

rosen-vladimirov · 2017-08-07T22:07:21Z

test/unit-tests/android-application-manager.ts

+			}
+		}
+		AndroidDebugBridgeStub.methodCallCount++;
+		Promise.resolve();


you do not need this

rosen-vladimirov · 2017-08-07T22:10:51Z

test/unit-tests/android-application-manager.ts

+		if (args && args.length) {
+			const possibleIdentifier = args[args.length - 1];
+			let validTestString = this.expectedValidTestInput[AndroidDebugBridgeStub.methodCallCount];
+			if (possibleIdentifier === validTestString) {


return possibleIdentifier === validTestString;

rosen-vladimirov · 2017-08-07T22:45:32Z

test/unit-tests/android-application-manager.ts

+	private checkIfValidIdentifierPassed(args: string[]): Boolean {
+		if (args && args.length) {
+			const possibleIdentifier = args[args.length - 1];
+			let validTestString = this.expectedValidTestInput[AndroidDebugBridgeStub.methodCallCount];


rosen-vladimirov · 2017-08-07T22:45:52Z

test/unit-tests/android-application-manager.ts

+	});
+	describe("startApplication", () => {
+		it("fires up the right application", async () => {
+			for (let i = 0; i < androidDebugBridge.getInputLength(); i += 1) {


i +=1 -> i++

i += 1 is faster, but if you insist I have no problem.

Well, that's not entirely correct. Consider the following example.

const iterations = 100*1000*1000; console.time("i+=1"); for(let i = 0; i < iterations; i+=1) { } console.timeEnd("i+=1"); console.time("i++"); for(let i = 0; i < iterations; i++) { } console.timeEnd("i++");

And the result is:

$ node b.js i+=1: 1118.667ms i++: 887.539ms $ node b.js i+=1: 1116.609ms i++: 877.461ms $ node b.js i+=1: 1106.636ms i++: 882.537ms

But anyway, the codebase contains many lines with <variable name>++ , so I prefer consistency here.

Ha, cool they fixed it. Will change it.

rosen-vladimirov

After fixing comments and green build.

rosen-vladimirov · 2017-08-10T05:38:47Z

mobile/android/android-application-manager.ts

+			Intent { act=android.intent.action.MAIN cat=[android.intent.category.LAUNCHER] flg=0x10200000 cmp=org.nativescript.cliapp/com.tns.NativeScriptActivity }
+			frontOfTask=true task=TaskRecord{fe592ac #449 A=org.nativescript.cliapp U=0 StackId=1 sz=1}
+		*/
+		const pmDumpOutput = await this.adb.executeShellCommand(["pm", "dump", appIdentifier, "|", "grep", "-A", "1", "MAIN"]);


@Plamen5kov , thanks for the detailed explanations. I'm not familiar with Android apps with more than one MAIN activities, there are such samples in Android Studio. It looks like the only way to understand if the current approach is fine is to merge it and see if such case will ever arise.

rosen-vladimirov · 2017-08-10T05:40:45Z

test/unit-tests/android-application-manager.ts

+			if (args[0] === "pm") {
+				const passedIdentifier = args[2];
+				if (passedIdentifier === invalidIdentifier) {
+					return Promise.resolve("invalid output string");


No need for Promise.resolve here - just return "invalid output string";

rosen-vladimirov · 2017-08-10T05:40:54Z

test/unit-tests/android-application-manager.ts

+					return Promise.resolve("invalid output string");
+				} else {
+					const testString = this.validTestInput[AndroidDebugBridgeStub.methodCallCount];
+					return Promise.resolve(testString);


No need for Promise.resolve here - just return testString;

Plamen5kov · 2017-08-11T05:17:00Z

ping @TsvetanMilanov please tell me if you have any further suggestions

split regex added fallback to monkey test refactored tests

Plamen5kov requested a review from TsvetanMilanov July 31, 2017 08:43

Plamen5kov force-pushed the plamen5kov/use-am-instead-of-monkey branch from 4431195 to 74a0ec2 Compare July 31, 2017 08:55

Plamen5kov self-assigned this Jul 31, 2017

TsvetanMilanov suggested changes Aug 2, 2017

View reviewed changes

Plamen5kov force-pushed the plamen5kov/use-am-instead-of-monkey branch from eea4ae4 to c00d54a Compare August 3, 2017 12:07

TsvetanMilanov suggested changes Aug 4, 2017

View reviewed changes

rosen-vladimirov suggested changes Aug 7, 2017

View reviewed changes

rosen-vladimirov approved these changes Aug 10, 2017

View reviewed changes

Plamen5kov force-pushed the plamen5kov/use-am-instead-of-monkey branch from bcaaef0 to c5557e0 Compare August 11, 2017 05:16

TsvetanMilanov approved these changes Aug 11, 2017

View reviewed changes

Plamen5kov added 7 commits August 11, 2017 17:02

use am start instead of monkey to start application

fcb12ba

update after review and added tests

7cd02d5

added monkey in case am fails and made lint happy

991db6e

added tests through start application

4738607

split regex added fallback to monkey test refactored tests

remove only so all tests run

bc034c7

updates after review

8b4b8fa

updates after review

e92d53e

Plamen5kov force-pushed the plamen5kov/use-am-instead-of-monkey branch from c5557e0 to e92d53e Compare August 11, 2017 14:02

Plamen5kov merged commit 99ae8cc into master Aug 11, 2017

Plamen5kov deleted the plamen5kov/use-am-instead-of-monkey branch August 11, 2017 14:09

use am start instead of monkey to start application #995

use am start instead of monkey to start application #995

Uh oh!

Conversation

Plamen5kov commented Jul 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TsvetanMilanov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Plamen5kov Aug 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Plamen5kov commented Aug 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TsvetanMilanov commented Aug 4, 2017

Uh oh!

Plamen5kov commented Aug 7, 2017

Uh oh!

rosen-vladimirov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Plamen5kov commented Jul 31, 2017 •

edited

Loading

Plamen5kov Aug 2, 2017 •

edited

Loading

Plamen5kov commented Aug 3, 2017 •

edited

Loading