Bug 155000 – webkitpy should verify timestamp from CrashLogs while collecting all crash logs

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Aakash Jain]

Sometimes "Other crashes" section in the layout-tests results contains crashes that actually happened hours before. It might be OS regression. We should verify timestamps in crash logs for robustness.
see <rdar://problem/24860219>

[Aakash Jain]

Created attachment 272815 [details]
Proposed patch

[Alexey Proskuryakov]

Comment on attachment 272815 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=272815&action=review

> Tools/ChangeLog:10
> +        (CrashLogs._is_new_crash_log): Check if crash logs are newer than required.

I would say: "Make sure that crash log timestamp is within expected time range, because file modification time is not always accurate".

> Tools/Scripts/webkitpy/common/system/crashlogs.py:165
> +    def _is_new_crash_log(self, log_contents, newer_than=None):

Do we ever want to call this without a newer_than argument? It's confusing to have it optional when the function makes no sense without it.

Just seeing the function name, someone could imagine that this is about crash log format (new vs. old). Maybe something like "_is_timestamp_newer_than" would be better?

Another idea: instead of _is_new_crash_log, add a function that returns internal timestamp (possibly as datetime), and then the caller can compare it to "newer_than". There is no need to combine extracting the timestamp with a policy decision of what it should be.

[Aakash Jain]

Created attachment 273123 [details]
Updated Patch

Updated patch with review comments incorporated.

[Alexey Proskuryakov]

Comment on attachment 273123 [details]
Updated Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=273123&action=review

> Tools/Scripts/webkitpy/common/system/crashlogs.py:170
> +    def get_timestamp_from_logs(self, log_contents):

Should this be "get_timestamp_from_logs" (singular)?

[Aakash Jain]

Created attachment 273201 [details]
Updated patch

[WebKit Commit Bot]

Comment on attachment 273201 [details]
Updated patch

Rejecting attachment 273201 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 273201, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Tools/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/937825

[WebKit Commit Bot]

Comment on attachment 273201 [details]
Updated patch

Clearing flags on attachment: 273201

Committed r197704: <http://trac.webkit.org/changeset/197704>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.