-
-
Notifications
You must be signed in to change notification settings - Fork 454
add skipLogCapture #878
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add skipLogCapture #878
Conversation
mykola-mokhnach
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not so easy with iOS. Please check all getLog occurrences in the code, because this will most likely throw an error if called with logging disabled. Also, it is necessary to check how such change would influence Simulator startup, because previously the fact that Simulator has started was checked via logs scanning. Which means that Simulator startup is going to always throw an error if the capability is set to true
|
Okay. thanks. Will check quick look: Simulator launched as below from no-simulator process. |
|
There's also getAllLogs. Are they used? I didn't find any usages for them. |
|
|
I grepped with some wordings and took a look at them. |
|
I don't think simulator startup should be a problem. Logs were only used for pre-iOS 8, and even then it uses |
| screenshotQuality: { | ||
| isNumber: true | ||
| }, | ||
| skipLogCapture: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not necessary if the desired capability is specified in appium-ios-driver.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will remove here after appium-boneyard/appium-ios-driver#355 and publish new version.
(In this PR or as another PR)
lib/driver.js
Outdated
| const isLogCaptureStarted = await startLogCapture(); | ||
|
|
||
| if (this.opts.skipLogCapture) { | ||
| log.info("'skipLogCapture' is set. Skipping starting logs such as crash, system, safari console and safari network."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This log line is repeated twice. Perhaps it would be better to move the check into the startLogCapture function on line 337?
const startLogCapture = async () => {
if (this.opts.skipLogCapture) {
log.info("'skipLogCapture' is set. Skipping starting logs such as crash, system, safari console and safari network.");
return false;
}
const result = await this.startLogCapture();
if (result) {
this.logEvent('logCaptureStarted');
}
return result;
};There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make sense than mine 👍
|
extensions.setContext will throw an error if |
lib/driver.js
Outdated
|
|
||
| await this.runReset(); | ||
|
|
||
| const memoizedSkipLogCaptureLog = _.memoize(function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
memoizedLogInfo
|
Please add the new capability into readme |
* add skipLogCapture * fix reviews * use memoize to prevent duplicated log * append skipLogCapture in caps table, tweak method name
appium/appium#12203 (comment)
cc @jumois
I ensured below:
showIOSLogappears on appium console at the same time