Fix iOS11 Simulator logs #3145

KristianDD · 2017-10-03T17:10:58Z

Log format for iOS 10.3: "Oct 3 19:07:55 machinename cliapp[15339]: CONSOLE LOG file:///app/main-view-model.js:18:20: Test Console fix..."

Log format for iOS 11: "2017-10-09 13:34:38.527844+0300 localhost cliapp[22235]: (NativeScript) CONSOLE LOG file:///app/main-view-model.js:18:20: Test Console fix.."

Needs NativeScript/ios-sim-portable#95 and telerik/mobile-cli-lib#1016

Plamen5kov · 2017-10-04T06:40:50Z

lib/services/ios-log-filter.ts

@@ -16,7 +16,7 @@ export class IOSLogFilter extends iOSLogFilterBase.IOSLogFilter implements Mobil

 	public filterData(data: string, logLevel: string, pid?: string): string {
 		data = super.filterData(data, logLevel, pid);
-		if (pid && data && data.indexOf(`[${pid}]`) === -1) {
+		if (pid && data && data.search(new RegExp(`[\\[\\s]${pid}[\\]\\s]`)) === -1) {


IMO either choose RegExp or indexOf for consistency in the code

I used regex to avoid nested or complex "if" statements:

if(data && pid){ if(data.indexOf(` ${pid} `) || data.indexOf(`[${pid}]`)) { } }

or

if(data && pid && (data.indexOf(` ${pid} `) || data.indexOf(`[${pid}]`)) { }

The statement (data.indexOf(` ${pid} `) || data.indexOf(`[${pid}]`) can be extracted into a separate method and called everywhere needed.
We'd avoid complexity and gain readability IMO

Plamen5kov · 2017-10-04T06:41:34Z

lib/services/ios-log-filter.ts

-						line = line.substring(pidIndex + searchString.length, line.length);
+					const pidIndex = line.indexOf(`[${pid}]`);
+					const ios11PidIndex = line.indexOf(` ${pid} `);
+					if (pidIndex > 0 || ios11PidIndex > 0) {


again choose either != 1 or > 0 for consistency in the code

Could change it to != -1

I chose to use indexOf because its a bit more fast in my opinion(didn't really compared performance tests though).

It is a bit faster, but I made the suggestions just for consistency sake because IMO it's more important

Plamen5kov

suggestions

Mitko-Kerezov · 2017-10-05T08:22:53Z

lib/services/ios-log-filter.ts

@@ -16,7 +16,7 @@ export class IOSLogFilter extends iOSLogFilterBase.IOSLogFilter implements Mobil

 	public filterData(data: string, logLevel: string, pid?: string): string {
 		data = super.filterData(data, logLevel, pid);
-		if (pid && data && data.indexOf(`[${pid}]`) === -1) {
+		if (pid && data && data.search(new RegExp(`[\\[\\s]${pid}[\\]\\s]`)) === -1) {


The statement (data.indexOf(` ${pid} `) || data.indexOf(`[${pid}]`) can be extracted into a separate method and called everywhere needed.
We'd avoid complexity and gain readability IMO

Mitko-Kerezov · 2017-10-05T08:41:38Z

lib/services/ios-log-filter.ts

+					const pidIndex = line.indexOf(`[${pid}]`);
+					const ios11PidIndex = line.indexOf(` ${pid} `);
+					if (pidIndex > 0 || ios11PidIndex > 0) {
+						const pidRegex = new RegExp(`.*[\\[\\s]{1}${pid}[\\s\\]]{1}.*?:(?:.*?\\(NativeScript\\) )*`);


Why do we need the part after : in this RegExp - I'm talking about the ignored capture group (?:.*?\$NativeScript\$ )*

If one should wish to console.log something like Loving (NativeScript) right now in their application the log would appear as
"Aug 22 10:59:20 MCSOFAPPBLD TestApp[52946]: CONSOLE LOG file:///app/home/home-view-model.js:6:20: Loving (NativeScript) right now"

and the RegExp would strip it to right now

You are right, will fix the regex.

Mitko-Kerezov · 2017-10-05T08:43:36Z

lib/services/ios-log-filter.ts

 					}
+
+					continue;


If we don't match with neither pidIndex nor ios11PidIndex I do not believe we'd want to continue.
I think this continue statement should be returned in the previous block

Taking continue outside the if statement prevents lines that don't contain the pid to be logged.

KristianDD · 2017-10-10T09:59:02Z

@NathanaelA we are passing the predicate here, but we are still supporting older iOS versions as you can see here. For the older iOS versions we still need the filtration code. The benefit of the predicate is that the code that filters messages will be invoked only for NS messages, Moreover the regex is used only if the line contains the "pid" of the application. We haven't noticed any performance issues so far.

The filtration of exception messages is not reproducible on Simulators, at least on my side. The real device logging and filtering is handled by different branch of the code.

Mitko-Kerezov · 2017-10-10T13:15:33Z

lib/services/ios-log-filter.ts

-					if (pidIndex > 0) {
-						line = line.substring(pidIndex + searchString.length, line.length);
+					if (line.indexOf(`[${pid}]: `) !== -1) {
+						const pidRegex = new RegExp(`.*[\\[]{1}${pid}[\\]]{1}(?::\\s(:?\\(NativeScript\\)\\s)*)`);


The RegExp can be shortened to:
const pidRegex = new RegExp(`.*[\\[]${pid}[\\]]:\\s(:?\$NativeScript\$\\s)*`);

How does this look as a final version:

const pidRegex = new RegExp(`^.*[\\[]${pid}[\\]]:\\s(?:\$NativeScript\$\\s)?`);

Mitko-Kerezov

Code looks okay to me
Some tests would be nice

Mitko-Kerezov · 2017-10-12T05:50:37Z

lib/services/ios-log-filter.ts

-					if (pidIndex > 0) {
-						line = line.substring(pidIndex + searchString.length, line.length);
+					if (line.indexOf(`[${pid}]: `) !== -1) {
+						const pidRegex = new RegExp(`^.*[\\[]${pid}[\\]]:\\s(?:\\(NativeScript\\)\\s)?`);


There is actually no need for [\\[] and [\\]] it can be further shortened to:
const pidRegex = new RegExp(`^.*\\[${pid}\\]:\\s(?:\$NativeScript\$\\s)?`);

dtopuzov · 2017-10-12T06:05:14Z

run ci

dtopuzov · 2017-10-12T08:26:10Z

run ci

KristianDD self-assigned this Oct 3, 2017

KristianDD added the bug label Oct 3, 2017

Plamen5kov reviewed Oct 4, 2017

View reviewed changes

This comment was marked as abuse.

Sign in to view

Mitko-Kerezov suggested changes Oct 5, 2017

View reviewed changes

This comment was marked as abuse.

Sign in to view

Mitko-Kerezov suggested changes Oct 10, 2017

View reviewed changes

KristianDD force-pushed the kddimitrov/ios-11-log branch from f51583e to 6e2de8a Compare October 11, 2017 12:55

Mitko-Kerezov suggested changes Oct 12, 2017

View reviewed changes

KristianDD force-pushed the kddimitrov/ios-11-log branch from da5db06 to 2413d82 Compare October 13, 2017 11:21

Mitko-Kerezov approved these changes Oct 13, 2017

View reviewed changes

Kristian D. Dimitrov added 3 commits October 13, 2017 18:08

Fix iOS11 Simulator logs

692faba

Fix comments for iOS11 Simulator log.

0e2ee17

Add tests and fix comments iOS11 Simulator log

0c4bd3e

KristianDD force-pushed the kddimitrov/ios-11-log branch from be0856a to 6f87058 Compare October 16, 2017 07:46

Update ios-sim-portable to 3.2.0

a9a73a4

KristianDD force-pushed the kddimitrov/ios-11-log branch from 6f87058 to a9a73a4 Compare October 16, 2017 07:58

KristianDD changed the title ~~DO NOT MERGE Fix iOS11 Simulator logs~~ Fix iOS11 Simulator logs Oct 16, 2017

KristianDD merged commit 124d236 into master Oct 16, 2017

KristianDD deleted the kddimitrov/ios-11-log branch October 16, 2017 09:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix iOS11 Simulator logs #3145

Fix iOS11 Simulator logs #3145

KristianDD commented Oct 3, 2017 •

edited

Loading

Plamen5kov Oct 4, 2017

KristianDD Oct 4, 2017

Mitko-Kerezov Oct 5, 2017

Plamen5kov Oct 4, 2017

KristianDD Oct 4, 2017

KristianDD Oct 4, 2017

Plamen5kov Oct 4, 2017

Plamen5kov left a comment

This comment was marked as abuse.

Mitko-Kerezov Oct 5, 2017

Mitko-Kerezov Oct 5, 2017

KristianDD Oct 9, 2017 •

edited

Loading

Mitko-Kerezov Oct 5, 2017

KristianDD Oct 9, 2017

This comment was marked as abuse.

KristianDD commented Oct 10, 2017

Mitko-Kerezov Oct 10, 2017

KristianDD Oct 11, 2017 •

edited

Loading

Mitko-Kerezov left a comment

Mitko-Kerezov Oct 12, 2017

dtopuzov commented Oct 12, 2017

dtopuzov commented Oct 12, 2017

Fix iOS11 Simulator logs #3145

Fix iOS11 Simulator logs #3145

Conversation

KristianDD commented Oct 3, 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 left a comment

Choose a reason for hiding this comment

This comment was marked as abuse.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KristianDD Oct 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as abuse.

KristianDD commented Oct 10, 2017

Choose a reason for hiding this comment

KristianDD Oct 11, 2017 • edited Loading

Choose a reason for hiding this comment

Mitko-Kerezov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtopuzov commented Oct 12, 2017

dtopuzov commented Oct 12, 2017

KristianDD commented Oct 3, 2017 •

edited

Loading

KristianDD Oct 9, 2017 •

edited

Loading

KristianDD Oct 11, 2017 •

edited

Loading