Bug 204707 - Add a unit-test to EWS for scenario when there is flakiness with patch and also on clean tree
Summary: Add a unit-test to EWS for scenario when there is flakiness with patch and al...
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:
 
Reported: 2019-11-29 10:06 PST by Aakash Jain
Modified: 2019-11-29 22:06 PST (History)
6 users (show)

See Also:


Attachments
Unit-test for old EWS (1.79 KB, patch)
2019-11-29 10:08 PST, Aakash Jain
no flags Details | Formatted Diff | Diff
Similar unit-test for new EWS (1.98 KB, patch)
2019-11-29 10:16 PST, 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 2019-11-29 10:06:01 PST
Add a unit-test to old EWS for scenario when there is flakiness with patch and also on clean tree.
Comment 1 Aakash Jain 2019-11-29 10:08:16 PST
Created attachment 384518 [details]
Unit-test for old EWS
Comment 2 Aakash Jain 2019-11-29 10:09:51 PST
Committed r252943: <https://trac.webkit.org/changeset/252943>
Comment 3 Radar WebKit Bug Importer 2019-11-29 10:10:18 PST
<rdar://problem/57533998>
Comment 4 Aakash Jain 2019-11-29 10:16:59 PST
Created attachment 384519 [details]
Similar unit-test for new EWS
Comment 5 Aakash Jain 2019-11-29 10:18:13 PST
.
Comment 6 Aakash Jain 2019-11-29 10:20:48 PST
Committed r252946: <https://trac.webkit.org/changeset/252946>
Comment 7 Alexey Proskuryakov 2019-11-29 22:06:53 PST
I think that I know why I didn't believe that this was the old EWS behavior.

Looking at the "if self._results_failed_different_tests" branch in PatchAnalysisTask._retry_layout_tests, I see that it calls self._should_defer_patch_or_throw, which says that the result should be a PASS. But then we ignore the result, and make it a DEFER anyway!

            if tests_that_consistently_failed:
                if self._should_defer_patch_or_throw(tests_that_consistently_failed, first_results_archive,
                                                     first_script_error, first_failure_status_id):
                    return False  # Defer patch

            # At this point we know that at least one test flaked, but no consistent failures
            # were introduced. This is a bit of a grey-zone.
            return False  # Defer patch


What's the point of the first "return False" here if we are going to defer either way?