Bug 204368

Summary: EWS should use results database instead of doing clean-tree runs
Product: WebKit Reporter: Aakash Jain <aakash_jain>
Component: Tools / TestsAssignee: Aakash Jain <aakash_jain>
Status: RESOLVED FIXED    
Severity: Normal CC: aakash_jain, ahmad.saleem792, ap, jbedard, ryanhaddad, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=224289
https://bugs.webkit.org/show_bug.cgi?id=248466
https://bugs.webkit.org/show_bug.cgi?id=248970
https://bugs.webkit.org/show_bug.cgi?id=251388
Bug Depends on:    
Bug Blocks: 248037    
Attachments:
Description Flags
Patch none

Description Aakash Jain 2019-11-19 12:09:11 PST
EWS bots do clean-tree run when the test fails, in order to confirm if the test-failure is pre-existing or introduced by the patch. A lot of time, test-failure information is already available in results database (through build.webkit.org bots). We should try to get the clean-tree failure info from results database using its API, and avoid doing clean-tree runs. This would save a lot of time on EWS bots.
Comment 1 Alexey Proskuryakov 2019-11-19 17:43:25 PST
How can we confirm that this will be sufficient? It feels like EWS-only failures are somewhat common.

I think that doing this in addition to retries is better than "instead" as the bug suggests. We can still retry with a clean tree if the failures are unknown.
Comment 2 Jonathan Bedard 2019-11-20 09:45:44 PST
(In reply to Alexey Proskuryakov from comment #1)
> How can we confirm that this will be sufficient? It feels like EWS-only
> failures are somewhat common.
> 
> I think that doing this in addition to retries is better than "instead" as
> the bug suggests. We can still retry with a clean tree if the failures are
> unknown.

We could do the retry if the failures are unknown, yes. And we might start that way.

I think the retry is generally a bad idea, though. It means that in the case where an engineer did introduce a regression, we are going to be taking significantly longer to actually report that regression than in the successful case. Given the longer turn around for layout tests, I think this pretty significantly reduces the usefulness of EWS. We're essentially delaying results in the exact circumstances those results would be most impactful.

I suppose we make the bubble orange on retry, but I feel like we might be training folks to ignore orange bubbles given how flakey things are at the moment.
Comment 3 Alexey Proskuryakov 2019-11-20 10:31:45 PST
If we don't retry, then we will have a lot of false positives, training engineers to ignore EWS altogether, not just in orange state.
Comment 4 Jonathan Bedard 2019-11-20 11:08:15 PST Comment hidden (obsolete)
Comment 5 Jonathan Bedard 2019-11-20 11:14:39 PST
Comment on attachment 383973 [details]
Patch

This patch should have been uploaded to https://bugs.webkit.org/show_bug.cgi?id=204409.
Comment 6 Jonathan Bedard 2019-11-20 11:25:44 PST
(In reply to Alexey Proskuryakov from comment #3)
> If we don't retry, then we will have a lot of false positives, training
> engineers to ignore EWS altogether, not just in orange state.

I'm not entirely convinced that we need retries to get rid of false positives. There isn't a good reason that clean tree runs in EWS should differ from trunk results at all, and if we're cross-referencing trunk results, we could even expand the set of commits we're looking at to find flakey tests.

This approach hinges on us quickly fixing things like https://bugs.webkit.org/show_bug.cgi?id=204409, but I really do think that things like that are bugs, not expected behavior.
Comment 7 Alexey Proskuryakov 2019-11-20 15:23:45 PST
One big reason is that EWS is different hardware. We definitely have quite a few tests that are only flaky or failing on EWS.
Comment 8 Aakash Jain 2019-12-05 08:24:24 PST
Seems like one thing we agree on is that, if the test(s) which fails in EWS is also marked as failing in results database, then we can skip the retry/clean-tree-run.

If the test failure is new (not visible in results database), then we can continue with existing retry/clean-tree-run logic. 

This would still be a significant efficiency improvement, given that most of the times test failures seen in EWS are also seen in build.webkit.org
Comment 9 Alexey Proskuryakov 2019-12-05 08:58:33 PST
Correct. I think that we should use a fairly strict definition of "failing in results database', something like the test having failed within 24 hours, and maybe even require the same OS/flavor.
Comment 10 Radar WebKit Bug Importer 2021-09-10 09:27:43 PDT
<rdar://problem/82975679>
Comment 11 Ryan Haddad 2022-11-09 14:45:47 PST
*** Bug 247700 has been marked as a duplicate of this bug. ***
Comment 12 Aakash Jain 2022-11-24 13:44:14 PST
This was earlier blocked by results database not supporting git-hash or commit-identifier, and then due to not having appropriate api endpoint which ews could talk to. Both these issues were resolved.

Implementing this as discussed above.
Comment 13 Aakash Jain 2022-11-24 13:56:56 PST Comment hidden (obsolete)
Comment 14 Aakash Jain 2022-11-24 13:59:31 PST
Pull request: https://github.com/WebKit/WebKit/pull/6792
Comment 15 EWS 2022-11-28 14:44:34 PST
Committed 257090@main (b9824fc290ac): <https://commits.webkit.org/257090@main>

Reviewed commits have been landed. Closing PR #6792 and removing active labels.