-
Notifications
You must be signed in to change notification settings - Fork 54
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
When an alert handler fails to run, log useful information #230
When an alert handler fails to run, log useful information #230
Conversation
@@ -157,8 +157,10 @@ + (void)parseEventType:(SISLLogEventType *)type subtype:(SISLLogEventSubtype *)s | |||
|
|||
// message format: "SLKeyboardTest.m:62: ..." | |||
NSArray *messageComponents = [message componentsSeparatedByString:@":"]; | |||
infoValue[@"fileName"] = messageComponents[0]; | |||
infoValue[@"lineNumber"] = @([messageComponents[1] integerValue]); | |||
if (messageComponents.count > 2) { |
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.
Ah good catch. I guess some of instruments
' exceptions begin with "Error:" too. Can you amend the comment at 158 to say as much?
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.
Actually I think a more apt description would be: "In rare circumstances, Subliminal may log an error with no call site." (the error's not coming from instruments
--it's our use of UIALogger.logError
above).
A few notes, back to you @justinseanmartin. |
All comments have been addressed, and I also added a 2 second UIA timeout around interacting with Alert buttons. |
@@ -393,7 +399,12 @@ - (SLAlertDismissHandler *)dismissWithButtonTitled:(NSString *)buttonTitle { | |||
NSString *UIAAlertHandler = [NSString stringWithFormat:@"\ | |||
var buttonElement = (%@)('%@');\ | |||
if (buttonElement && buttonElement.isValid()) {\ | |||
buttonElement.tap();\ | |||
UIATarget.localTarget().pushTimeout(2);\ |
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.
What do you think about the onAlert
function pushing/popping the timeout? Then that would cover any handler, not just -dismissWithButtonTitled:
.
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.
Also let's make the timeout 5 seconds--that's the default (used by UIAutomation absent Subliminal; and by all of Subliminal's APIs).
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.
The problem with this is that we're going to wait 5 seconds for each registered alert that doesn't match on the title. I think I can fix it by modifying the dismissWithButton logic though.
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.
I'm wrong, thanks for correcting me.
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.
No problem, thanks for double-checking!
Looks good, thanks! |
…p-exceptions When an alert handler fails to run, log useful information
When an alert handler fails to run, log some useful information. When I added this, it appears subliminal-instrument would crash with an index out of bounds exception.
Thoughts?