[webkit-reviews] review granted: [Bug 210202] Buildbot: Force crash log submission after each test run : [Attachment 395868] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Jonathan Bedard
<jbedard at apple.com>'s request for review:
Bug 210202: Buildbot: Force crash log submission after each test run
https://bugs.webkit.org/show_bug.cgi?id=210202

Attachment 395868: Patch

https://bugs.webkit.org/attachment.cgi?id=395868&action=review




--- Comment #7 from Alexey Proskuryakov <ap at webkit.org> ---
Comment on attachment 395868
  --> https://bugs.webkit.org/attachment.cgi?id=395868
Patch

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

> Tools/ChangeLog:18
> +	   * BuildSlaveSupport/trigger-crash-collection: Added.
> +	   * BuildSlaveSupport/wait-for-crash-collection: Added.

Maybe trigger-crash-log-submission? "Crash collection" doesn't parse without
context.

> Tools/BuildSlaveSupport/trigger-crash-collection:35
> +    # Work around for <rdar://problem/60507877>

As a noun, "workaround" doesn't have a space. Please also add a period at the
end.

> Tools/BuildSlaveSupport/trigger-crash-collection:37
> +	   print('Failed to kill diagnostic agent')

Please verify that this process is named like this in all supported macOS
versions.

Nit: Better to have the exact name in the log, diagnostics_agent.

> Tools/BuildSlaveSupport/trigger-crash-collection:39
> +    print('Killed diagnostic agent')

Better to have the exact name in the log.

> Tools/BuildSlaveSupport/trigger-crash-collection:43
> +	   print('Failed to request crashreporter submission')

Nit: using different words in logs and in tool name suggests that one of those
is not ideal. Is it "request" or "trigger"?

> Tools/BuildSlaveSupport/wait-for-crash-collection:29
> +SUBMIT_DIAG_INFO = '/System/Library/CoreServices/SubmitDiagInfo'

Have you confirmed that the name is the same on all supported macOS versions?

> Tools/BuildSlaveSupport/wait-for-crash-collection:33
> +    process = subprocess.Popen(['/bin/ps', '-eo', 'pid,comm'],
stdout=subprocess.PIPE, stderr=subprocess.PIPE)

Surprised that there isn't a way for this in Python that doesn't involve
launching a separate tool.

> Tools/BuildSlaveSupport/wait-for-crash-collection:51
> +def main():

I think that this should have a timeout, either built in, or an argument. It's
better to run tests anyway if SubmitDiagInfo doesn't quit.

> Tools/BuildSlaveSupport/wait-for-crash-collection:59
> +    if not pid:
> +	   print('Failed to find {}'.format(SUBMIT_DIAG_INFO))
> +	   return 1

This seems like a success case, not failure.

> Tools/BuildSlaveSupport/wait-for-crash-collection:61
> +    print('Waiting on process {} to quiesce'.format(pid))

"on" or "for"?

> Tools/BuildSlaveSupport/wait-for-crash-collection:64
> +    while cpu_percentage(pid) > 5:
> +	   pass

Should we sleep for a bit here, to not spin a whole CPU core.

> Tools/BuildSlaveSupport/build.webkit.org-config/steps.py:152
> +    description = ["waiting for crash collectio to quiesce"]

Typo: collectio