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
View All Add an attachment (proposed patch, testcase, etc.)

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?