Bug 215174 - EWS emails about build failure should include relevant error logs
Summary: EWS emails about build failure should include relevant error logs
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Aakash Jain
URL:
Keywords: InRadar
Depends on:
Blocks: 215220
  Show dependency treegraph
 
Reported: 2020-08-05 10:44 PDT by Aakash Jain
Modified: 2020-09-15 09:52 PDT (History)
5 users (show)

See Also:


Attachments
Patch (5.03 KB, patch)
2020-08-05 10:49 PDT, Aakash Jain
no flags Details | Formatted Diff | Diff
Patch (6.92 KB, patch)
2020-08-05 12:43 PDT, Aakash Jain
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Aakash Jain 2020-08-05 10:44:07 PDT
EWS emails about build failure should include relevant error logs. It would make it easier to quickly identify the issue from the email itself.

These logs will be filtered/shortened version of error logs which are displayed in Buildbot build (which were added in Bug 203418).

Currently these emails are enabled only for bot-watchers, but when we will enable them for engineers, having relevant error logs in email would be useful.
Comment 1 Aakash Jain 2020-08-05 10:49:27 PDT
Created attachment 406012 [details]
Patch
Comment 2 Aakash Jain 2020-08-05 11:02:55 PDT
Tested in https://ews-build.webkit-uat.org/#/builders/33/builds/274
Comment 3 Jonathan Bedard 2020-08-05 11:18:53 PDT
Comment on attachment 406012 [details]
Patch

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

> Tools/BuildSlaveSupport/ews-build/steps.py:-1398
> -        return defer.succeed(None)

Was this line removed intentionally?

> Tools/BuildSlaveSupport/ews-build/steps.py:1408
> +    def getResults(self, name):

What is the return value of this function? I ask because it looks like @defer.inlineCallbacks is overriding that.

> Tools/BuildSlaveSupport/ews-build/steps.py:1411
> +            defer.returnValue(None)

This looks like an early exit, but I'm not familiar with this 'defer' idiom, do we not need to return so we exit early?
Comment 4 Aakash Jain 2020-08-05 12:41:11 PDT
Comment on attachment 406012 [details]
Patch

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

>> Tools/BuildSlaveSupport/ews-build/steps.py:-1398
>> -        return defer.succeed(None)
> 
> Was this line removed intentionally?

Yes, it was moved. This line belonged to start() method earlier, and now also it belongs to start() method.

>> Tools/BuildSlaveSupport/ews-build/steps.py:1411
>> +            defer.returnValue(None)
> 
> This looks like an early exit, but I'm not familiar with this 'defer' idiom, do we not need to return so we exit early?

Yes. Also this method is mostly copied from AnalyzeAPITestsResults::getTestsResults()
Comment 5 Aakash Jain 2020-08-05 12:43:47 PDT
Created attachment 406026 [details]
Patch
Comment 6 EWS 2020-08-05 13:41:36 PDT
Committed r265302: <https://trac.webkit.org/changeset/265302>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 406026 [details].
Comment 7 Radar WebKit Bug Importer 2020-08-05 13:42:18 PDT
<rdar://problem/66591524>