RESOLVED FIXED 199877
[ews-build] Add build step to AnalyzeLayoutTestsResults 
https://bugs.webkit.org/show_bug.cgi?id=199877
Summary [ews-build] Add build step to AnalyzeLayoutTestsResults
Aakash Jain
Reported 2019-07-17 12:27:43 PDT
Add build step to analyze layout-tests results, including re-run and clean-tree-run. Determine if the failures were introduced by the patch, were pre-existing, or the build need to be RETRied in case of flakiness or excessive test failures.
Attachments
WIP (10.43 KB, patch)
2019-07-17 12:30 PDT, Aakash Jain 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from ews112 for mac-highsierra (3.03 MB, application/zip)
2019-07-17 14:17 PDT, EWS Watchlist 		no flags
Details
Patch (15.10 KB, patch)
2019-07-18 09:25 PDT, Aakash Jain 		no flags
DetailsFormatted DiffDiff
Patch for landing (15.19 KB, patch)
2019-07-18 12:01 PDT, Aakash Jain 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Aakash Jain
Comment 1 2019-07-17 12:30:05 PDT
Created attachment 374314 [details] WIP
EWS Watchlist
Comment 2 2019-07-17 12:31:55 PDT Comment hidden (obsolete)
EWS Watchlist
Comment 3 2019-07-17 14:17:51 PDT
Comment on attachment 374314 [details] WIP Attachment 374314 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12760460 New failing tests: jquery/offset.html
EWS Watchlist
Comment 4 2019-07-17 14:17:53 PDT
Created attachment 374328 [details] Archive of layout-test-results from ews112 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-highsierra Platform: Mac OS X 10.13.6
Jonathan Bedard
Comment 5 2019-07-17 16:27:55 PDT
Comment on attachment 374314 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=374314&action=review > Tools/BuildSlaveSupport/ews-build/steps.py:1010 > +class AnalyzeLayoutTestsResults(buildstep.BuildStep): This looks like it has infinite retry on failure, is that deliberate? > Tools/BuildSlaveSupport/ews-build/steps.py:1041 > + def _results_failed_different_tests(self, first_results_failing_tests, second_results_failing_tests): Confused why we need this function > Tools/BuildSlaveSupport/ews-build/steps.py:1049 > + first_results_did_exceed_test_failure_limit = self.getProperty('first_results_exceed_failure_limit') I'm not entirely convinced that this would clarify the code, I wouldn't even call this a suggestion, more of a potential alternative: ... first_results = dict(did_exceed_test_failure_limit=self.getProperty('first_results_exceed_failure_limit'), failing_tests=set(self.getProperty('first_run_failures', [])) second_results = dict(...) clean_results = dict(...) ... You might be able to de-dupe your if-only-one-exceeded-limit if statements by doing something like this: ... for one_over, one_under in [(first_results, second_results ), (second_results, first_results)]: if not one_over['did_exceed_test_failure_limit']: continue ... ... This abstraction may be more complexity that it's worth, though, so it's up to you if you think this makes things more clear. It prevents duplication of code, but also is a bit more confusing, so it's a trade-off. > Tools/BuildSlaveSupport/ews-build/steps.py:1056 > + if second_results_did_exceed_test_failure_limit and first_results_did_exceed_test_failure_limit: Why second then first instead of first then second? > Tools/BuildSlaveSupport/ews-build/steps.py:1057 > + if (len(first_results_failing_tests) - len(clean_tree_results_failing_tests)) <= 5: This seems pretty arbitrary....why <= 5? Should that be <= 0? > Tools/BuildSlaveSupport/ews-build/steps.py:1061 > + if second_results_did_exceed_test_failure_limit: So if one of the two attempts fails, we force a retry if the other doesn't have failures? > Tools/BuildSlaveSupport/ews-build/steps.py:1064 > + failures_introduced_by_patch = first_results_failing_tests - clean_tree_results_failing_tests Are these sets? Or lists? I don't think this would work with lists.
Aakash Jain
Comment 6 2019-07-18 09:15:49 PDT
Comment on attachment 374314 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=374314&action=review >> Tools/BuildSlaveSupport/ews-build/steps.py:1010 >> +class AnalyzeLayoutTestsResults(buildstep.BuildStep): > > This looks like it has infinite retry on failure, is that deliberate? Yes, that's deliberate, to match with old EWS behavior. >> Tools/BuildSlaveSupport/ews-build/steps.py:1056 >> + if second_results_did_exceed_test_failure_limit and first_results_did_exceed_test_failure_limit: > > Why second then first instead of first then second? was trying to be consistent with webkitpy/tool/bot/patchanalysistask.py, re-ordered in updated patch. >> Tools/BuildSlaveSupport/ews-build/steps.py:1057 >> + if (len(first_results_failing_tests) - len(clean_tree_results_failing_tests)) <= 5: > > This seems pretty arbitrary....why <= 5? Should that be <= 0? It was added in http://trac.webkit.org/changeset/175063/webkit with a comment. Added that comment in my patch as well. >> Tools/BuildSlaveSupport/ews-build/steps.py:1061 >> + if second_results_did_exceed_test_failure_limit: > > So if one of the two attempts fails, we force a retry if the other doesn't have failures? Yeah, because we can't be sure that patch is good. As it failed lot of tests in one run and didn't have any failures in other run. We should retry to confirm. Logic copied from webkitpy/tool/bot/patchanalysistask.py >> Tools/BuildSlaveSupport/ews-build/steps.py:1064 >> + failures_introduced_by_patch = first_results_failing_tests - clean_tree_results_failing_tests > > Are these sets? Or lists? I don't think this would work with lists. sets
Aakash Jain
Comment 7 2019-07-18 09:25:32 PDT
Created attachment 374391 [details] Patch
EWS Watchlist
Comment 8 2019-07-18 09:28:44 PDT Comment hidden (obsolete)
Jonathan Bedard
Comment 9 2019-07-18 10:33:10 PDT
Comment on attachment 374391 [details] Patch I would mention where we are taking this code from in the changelog.
Aakash Jain
Comment 10 2019-07-18 12:01:19 PDT
Created attachment 374403 [details] Patch for landing
Aakash Jain
Comment 11 2019-07-18 12:03:20 PDT
Committed r247569: <https://trac.webkit.org/changeset/247569>
Radar WebKit Bug Importer
Comment 12 2019-07-18 12:04:24 PDT
<rdar://problem/53266131>
Note You need to log in before you can comment on or make changes to this bug.