Bug 203411 – [EWS] status bubble tooltip should display failing layout tests names rather than generic 'x failures'

• Status: ASSIGNED
• Component: Tools / Tests

[Aakash Jain]

Status bubble tooltip message should display failing layout tests names rather than generic 'x failures'.

For e.g.: in https://ews-build.webkit.org/#/builders/24/builds/2573, the layout-tests step message simply says: '4 failures 7 flakes', similarly the re-run-layout-tests step message says: '5 failures 4 flakes'.  Similarly run-layout-tests-without-patch step message says: '5 failures 7 flakes'.

These messages are also displayed in status-bubble tooltip, and aren't very informative. They don't tell the patch author which tests failed. Even after clicking the status-bubble and navigating to the buildbot page, the failures names aren't displayed. One more click is required on 'view layout test results' to look at the failures. 

If the names of the failing test are displayed in the tooltip itself, patch author might be able to make an estimate if the failing tests might be related to his/her patch or completely unrelated flaky failures. Therefore, we should display the exact test names of the failing tests rather than generic 'x failures' message.

Also flakes information might be useful in build.webkit.org, but not useful for EWS, patch author doesn't really care about flakiness unrelated to his/her patch in EWS.

Also, layout-test step shouldn't be marked as WARNING due to flakes. EWS purpose is to tell whether the patch is good or bad, displaying WARNING status due to flakiness completely unrelated to the tests isn't the right UX for EWS.

[Aakash Jain]

This change will also help bot-watchers to easily notice the flaky tests in a given build, by just looking at the buildbot build page.

[Aakash Jain]

Created attachment 381916 [details]
Patch

[Aakash Jain]

Created attachment 381921 [details]
Screenshot - tooltip before this change

[Aakash Jain]

Created attachment 381922 [details]
Screenshot - tooltip after this change

Screenshot of tooltip from another build after this change.

[Jonathan Bedard]

Comment on attachment 381916 [details]
Patch

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

> Tools/BuildSlaveSupport/ews-build/steps.py:-1030
> -

This seems like a pretty big change, actually....I'm not necessarily opposed to it, but the idea of removing flakiness warnings from EWS might be unpopular, have we had any feedback about those warnings from engineers?

> Tools/BuildSlaveSupport/ews-build/steps_unittest.py:1167
> +        self.expectOutcome(result=SUCCESS, state_string='Passed layout tests')

Does this mean that when a test 'flakes', we say it passed now?

[Aakash Jain]

Sample runs:

1 pre-existing test failure: https://ews-build.webkit-uat.org/#/builders/6/builds/33
5+ pre-existing test failures: https://ews-build.webkit-uat.org/#/builders/6/builds/34

Failures introduced by patch: https://ews-build.webkit-uat.org/#/builders/6/builds/27

Passing build: https://ews-build.webkit-uat.org/#/builders/6/builds/43, https://ews-build.webkit-uat.org/#/builders/6/builds/45

Currently ongoing build: https://ews-build.webkit-uat.org/#/builders/6/builds/47  (https://ews.webkit-uat.org/status-bubble/381876/)

[Aakash Jain]

Comment on attachment 381916 [details]
Patch

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

>> Tools/BuildSlaveSupport/ews-build/steps_unittest.py:1167
>> +        self.expectOutcome(result=SUCCESS, state_string='Passed layout tests')
> 
> Does this mean that when a test 'flakes', we say it passed now?

Yes. Note that even currently when layout-test run find a flaky failure, the status-bubble is still green, the Buildbot page says: "Passed layout tests". It is just the build-step which we are changing, not the overall status.

e.g.: In https://ews-build.webkit.org/#/builders/17/builds/4699 layout-tests says '4 flakes', but the build and corresponding status-bubble in https://bugs.webkit.org/show_bug.cgi?id=202838 is green.

Also, note that some flakes are found in almost every layout-test run, e.g.: https://ews-build.webkit.org/#/builders/17/builds/4604, https://ews-build.webkit.org/#/builders/17/builds/4602, https://ews-build.webkit.org/#/builders/24/builds/24, https://ews-build.webkit.org/#/builders/14/builds/1603, https://ews-build.webkit.org/#/builders/15/builds/598

So until, we have less flakiness, this isn't actionable, and seems like just noise.

[Jonathan Bedard]

I don't like that we're separating test names with commas, that is not very readable.

Is there a way to insert newlines?