This repository was archived by the owner on Feb 2, 2021. It is now read-only.
Fix getting ios simulator device logs #815
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
❤️ |
rosen-vladimirov
commented
Oct 3, 2016
| @@ -174,7 +201,7 @@ declare module Mobile { | |||
| * @param {string} logLevel Selected logging level. | |||
| * @return {string} The filtered result based on the input or null when the input data shouldn't be shown. | |||
| */ | |||
| filterData(data: string, logLevel: string): string; | |||
| filterData(data: string, logLevel: string, pid: string): string; | |||
There was a problem hiding this comment.
Add JSDocs for pid
| @@ -142,6 +142,32 @@ declare module Mobile { | |||
| interface IDeviceLogProvider { | |||
| logData(line: string, platform: string, deviceIdentifier: string): void; | |||
| setLogLevel(level: string, deviceIdentifier?: string): void; | |||
| setApplictionPidForDevice(deviceIdentifier: string, pid: string): void; | |||
There was a problem hiding this comment.
Add JSDocs
|
LGTM 👍 |
rosen-vladimirov
force-pushed
the
vladimirov/sim-logs
branch
from
a0252f5 to
da88661
Compare
|
❤️ |
1 similar comment
|
❤️ |
rosen-vladimirov
force-pushed
the
vladimirov/sim-logs
branch
from
0f89549 to
3e84715
Compare
|
❤️ |
rosen-vladimirov
force-pushed
the
vladimirov/sim-logs
branch
2 times, most recently
from
57280ed to
71fd9f9
Compare
|
❤️ |
|
💔 |
|
run ci |
|
❤️ |
rosen-vladimirov
force-pushed
the
vladimirov/sim-logs
branch
from
71fd9f9 to
9775b92
Compare
|
❤️ |
1 similar comment
|
❤️ |
rosen-vladimirov
force-pushed
the
vladimirov/sim-logs
branch
from
8c42566 to
c4e403f
Compare
|
❤️ |
iOS Simulator device logs are printed directly to process.stdout from the ios-sim-portable. However this makes it really difficult to filter the logs and provide them to different callers (for example AppBuilder CLI, NativeScript CLI, Proton). In order to fix this, use the ios-sim-portable just to start the process of getting device logs. Filter them in the CLI itself. Also change the logic in ios-log-filter to use process pid in case it's passed. For iOS Simulator we have filtered logs based on the PID of the application. With these changes, simulator logs are passed to ios-log-filter, so that's why we need process' PID there. When application is restarted (during livesync for example), we set the PID again and the filter will use the new PID. Add unit tests to verify passing PID to iOSLogFilter works correctly.
rosen-vladimirov
force-pushed
the
vladimirov/sim-logs
branch
from
c4e403f to
1cd1804
Compare
|
❤️ |
| if(deviceIdentifier) { | ||
| this.devicesLogLevel[deviceIdentifier] = logLevel.toUpperCase(); | ||
| if (deviceIdentifier) { | ||
| this.devicesLogOptions[deviceIdentifier] = this.devicesLogOptions[deviceIdentifier] || <Mobile.IDeviceLogOptions> { }; |
There was a problem hiding this comment.
Can you extract this check and the set of the device log option on the line below in a method for example setDeviceLogOption:
protected setDeviceLogOption(deviceId: string, optionName: string, value: string): void {
this.devicesLogOptions[deviceId] = this.devicesLogOptions[deviceId] || <Mobile.IDeviceLogOptions> { };
this.devicesLogOptions[deviceId][optionName]= value;
}| _.each(this.devicesLogLevel, (deviceLogLevel: string, deviceId: string) => { | ||
| this.devicesLogLevel[deviceId] = this.$logFilter.loggingLevel; | ||
|
|
||
| _.each(this.devicesLogOptions, (deviceLogLevel: string, deviceId: string) => { |
There was a problem hiding this comment.
You can use _.keys here instead of _.each because you don't use the deviceLogLevel: string.
|
@TsvetanMilanov nice catch, comments are handled in the code ;) |
rosen-vladimirov
force-pushed
the
vladimirov/sim-logs
branch
from
78da1b2 to
dd4b91c
Compare
|
❤️ |
1 similar comment
|
❤️ |
rosen-vladimirov
force-pushed
the
vladimirov/sim-logs
branch
from
dd4b91c to
e475807
Compare
|
❤️ |
TsvetanMilanov
approved these changes
Oct 4, 2016
rosen-vladimirov
commented
Oct 4, 2016
| logLevel = logLevel || this.$logFilter.loggingLevel; | ||
|
|
||
| if (deviceIdentifier) { | ||
| logLevel = (this.devicesLogOptions[deviceIdentifier] && this.devicesLogOptions[deviceIdentifier].logLevel) || this.$logFilter.loggingLevel; |
There was a problem hiding this comment.
this code seems really strange. setLogLevelForDevice method does not use the passed logLevel.
Consider either removing the logLevel parameter or using it here:
logLevel = (this.devicesLogOptions[deviceIdentifier] && this.devicesLogOptions[deviceIdentifier].logLevel) || logLevel;In many cases we need the propertyName and pass it to another function. However in case we use string and rename the property in the future, we almost always forget to update the string. Introduce helper function that accepts function as argument. The argument function must return the property which name we need. Add unit tests for the helper method. Use the new method to simplify setting of deviceLogOptions.
rosen-vladimirov
force-pushed
the
vladimirov/sim-logs
branch
from
e475807 to
fc329b6
Compare
|
❤️ |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
iOS Simulator device logs are printed directly to process.stdout from the ios-sim-portable. However this makes it really difficult to filter the logs and provide them to different callers (for example AppBuilder CLI, NativeScript CLI, Proton).
In order to fix this, use the ios-sim-portable just to start the process of getting device logs. Filter them in the CLI itself.
Also change the logic in ios-log-filter to use process pid in case it's passed. For iOS Simulator we have filtered logs based on the PID of the application. With these changes, simulator logs are passed to ios-log-filter, so that's why we need process' PID there.
When application is restarted (during livesync for example), we set the PID again and the filter will use the new PID.
Merge AFTER NativeScript/ios-sim-portable#79