Fix error message for empty stacktrace files #375
Conversation
| HockeyLog.warn("The crash data is invalid"); | ||
| deleteStackTrace(weakContext, filename); | ||
| if (listener != null) { | ||
| listener.onCrashesSent(); |
There was a problem hiding this comment.
Why is that not onNoCrashesFound since we are not sending anything?
There was a problem hiding this comment.
Ah because we already onCrashesFound earlier. Well it's weird then, maybe we should just call onCrashesNotSent then?
There was a problem hiding this comment.
Also there is no unit test for that, we should probably add some.
| HockeyLog.warn("The crash data is invalid"); | ||
| deleteStackTrace(weakContext, filename); | ||
| if (listener != null) { | ||
| listener.onCrashesNotSent(); |
There was a problem hiding this comment.
One thing I don't understand is that we can have multiple stack traces.
This method is called in a loop, yet at first invalid stack trace you are calling listener. Isn't that the wrong thing to do? I thought the listener would be called at the end of all the sending operations for all crashes.
There was a problem hiding this comment.
Edit: actually we call it for each crashes on success but no parameter is passed, I guess design issue and I was confused as in AppCenter we pass the report as a parameter for those.
guperrot
dismissed
their stale review
Canceling requested change, need to re-review now with latest pushes.
| try { | ||
| stacktrace = contentsOfFile(weakContext, filename); | ||
| } catch (Exception e) { | ||
| HockeyLog.error("Failed to read crash data", e); |
There was a problem hiding this comment.
If this catch block is executed, then we go to line 412 and check length on a null stacktrace object. Shouldn't that crash? According to me the new unit test covers only empty stack trace case. I would suggest calling listener and cleaning retry counter too for null case.
MatkovIvan
force-pushed
the
fix/message-for-empty-stacktrace
branch
from
4e27b64 to
2874014
Compare
|
Anything blocking us from merging this? |
|
@TroubleMakerBen It's ready to go |
Ignore whitespace diff view