Remove duplicate code from PatchAnalysisTask._test_patch and fix bug
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 7 Nov 2014 02:27:49 +0000 (02:27 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 7 Nov 2014 02:27:49 +0000 (02:27 +0000)
regarding incorrect call to PatchAnalysisTask.report_failure
https://bugs.webkit.org/show_bug.cgi?id=138229

Patch by Jake Nielsen <jacob_nielsen@apple.com> on 2014-11-06
Reviewed by Daniel Bates.

* Scripts/webkitpy/common/net/layouttestresults.py:
Makes sure test_results returns a list.
(LayoutTestResults.test_results):
* Scripts/webkitpy/tool/bot/commitqueuetask_unittest.py:
Makes unit tests also check to make sure
task.results_from_patch_test_run() and
task.results_from_test_run_without_patch return instances of
LayoutTestResults.
(CommitQueueTaskTest._run_and_expect_patch_analysis_result):
* Scripts/webkitpy/tool/bot/patchanalysistask.py:
Condenses duplicate code into _should_defer_patch_or_throw, and
removes the now-unused _clean_tree_results member.
(PatchAnalysisTask.__init__):
(PatchAnalysisTask._continue_testing_patch_that_exceeded_failure_limit_on_first_or_second_try):
(PatchAnalysisTask._should_defer_patch_or_throw):
(PatchAnalysisTask._test_patch):
(PatchAnalysisTask.results_from_patch_test_run):
(PatchAnalysisTask.results_from_test_run_without_patch): Deleted.
* Scripts/webkitpy/tool/commands/earlywarningsystem.py:
Removes needless call to results_from_test_run_without_patch
(AbstractEarlyWarningSystem._failing_tests_message):
* Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py:
Changes order of test failure messages to be in the order that they
appear.
(AbstractEarlyWarningSystemTest.test_failing_tests_message):
* Scripts/webkitpy/tool/commands/queues.py:
Removes needless call to results_from_test_run_without_patch.
(CommitQueue._failing_tests_message):
* Scripts/webkitpy/tool/commands/queues_unittest.py:
(MockCommitQueueTask.results_from_test_run_without_patch): Deleted.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@175735 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Tools/ChangeLog
Tools/Scripts/webkitpy/common/net/layouttestresults.py
Tools/Scripts/webkitpy/tool/bot/commitqueuetask_unittest.py
Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py
Tools/Scripts/webkitpy/tool/commands/earlywarningsystem.py
Tools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py
Tools/Scripts/webkitpy/tool/commands/queues.py
Tools/Scripts/webkitpy/tool/commands/queues_unittest.py

index f7dda68..c20d277 100644 (file)
@@ -1,3 +1,42 @@
+2014-11-06  Jake Nielsen  <jacob_nielsen@apple.com>
+
+        Remove duplicate code from PatchAnalysisTask._test_patch and fix bug
+        regarding incorrect call to PatchAnalysisTask.report_failure
+        https://bugs.webkit.org/show_bug.cgi?id=138229
+
+        Reviewed by Daniel Bates.
+
+        * Scripts/webkitpy/common/net/layouttestresults.py:
+        Makes sure test_results returns a list.
+        (LayoutTestResults.test_results):
+        * Scripts/webkitpy/tool/bot/commitqueuetask_unittest.py:
+        Makes unit tests also check to make sure
+        task.results_from_patch_test_run() and
+        task.results_from_test_run_without_patch return instances of
+        LayoutTestResults.
+        (CommitQueueTaskTest._run_and_expect_patch_analysis_result):
+        * Scripts/webkitpy/tool/bot/patchanalysistask.py:
+        Condenses duplicate code into _should_defer_patch_or_throw, and
+        removes the now-unused _clean_tree_results member.
+        (PatchAnalysisTask.__init__):
+        (PatchAnalysisTask._continue_testing_patch_that_exceeded_failure_limit_on_first_or_second_try):
+        (PatchAnalysisTask._should_defer_patch_or_throw):
+        (PatchAnalysisTask._test_patch):
+        (PatchAnalysisTask.results_from_patch_test_run):
+        (PatchAnalysisTask.results_from_test_run_without_patch): Deleted.
+        * Scripts/webkitpy/tool/commands/earlywarningsystem.py:
+        Removes needless call to results_from_test_run_without_patch
+        (AbstractEarlyWarningSystem._failing_tests_message):
+        * Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py:
+        Changes order of test failure messages to be in the order that they
+        appear.
+        (AbstractEarlyWarningSystemTest.test_failing_tests_message):
+        * Scripts/webkitpy/tool/commands/queues.py:
+        Removes needless call to results_from_test_run_without_patch.
+        (CommitQueue._failing_tests_message):
+        * Scripts/webkitpy/tool/commands/queues_unittest.py:
+        (MockCommitQueueTask.results_from_test_run_without_patch): Deleted.
+
 2014-11-06  Ryuan Choi  <ryuan.choi@navercorp.com>
 
         Unreviewed. Reorder my e-mail addresses in contributors.json
index e74ca78..7fb67c1 100644 (file)
@@ -62,7 +62,7 @@ class LayoutTestResults(object):
         return self._did_exceed_test_failure_limit
 
     def test_results(self):
-        return self._test_results
+        return [result for result in self._test_results]
 
     def results_matching_failure_types(self, failure_types):
         return [result for result in self._test_results if result.has_failure_matching_types(*failure_types)]
index dc139c8..302a536 100644 (file)
@@ -181,6 +181,7 @@ class CommitQueueTaskTest(unittest.TestCase):
         # The failure status only means anything if we actually failed.
         if expected_analysis_result == PatchAnalysisResult.FAIL:
             self.assertEqual(task.failure_status_id, expected_failure_status_id)
+            self.assertIsInstance(task.results_from_patch_test_run(patch), LayoutTestResults)
 
     def _run_through_task(self, commit_queue, expected_logs, expected_exception=None, expect_retry=False):
         self.maxDiff = None
index f5ead4c..6dc083f 100644 (file)
@@ -82,7 +82,6 @@ class PatchAnalysisTask(object):
         self._script_error = None
         self._results_archive_from_patch_test_run = None
         self._results_from_patch_test_run = None
-        self._clean_tree_results = None
 
     def _run_command(self, command, success_message, failure_message):
         try:
@@ -185,17 +184,34 @@ class PatchAnalysisTask(object):
 
     def _continue_testing_patch_that_exceeded_failure_limit_on_first_or_second_try(self, results, results_archive, script_error):
         self._build_and_test_without_patch()
-        self._clean_tree_results = self._delegate.test_results()
 
         # If we've made it here, then many (500) tests are failing with the patch applied, but
         # if the clean tree is also failing many tests, even if it's not quite as many (495),
         # then we can't be certain that the discrepancy isn't due to flakiness, and hence we must
         # defer judgement.
-        if (len(results.failing_tests()) - len(self._clean_tree_results.failing_tests())) <= 5:
+        if (len(results.failing_tests()) - len(self._delegate.test_results().failing_tests())) <= 5:
             return False
 
         return self.report_failure(results_archive, results, script_error)
 
+    def _should_defer_patch_or_throw(self, failures_with_patch, results_archive_for_failures_with_patch, script_error, failure_id):
+        self._build_and_test_without_patch()
+        clean_tree_results = self._delegate.test_results()
+
+        if clean_tree_results.did_exceed_test_failure_limit():
+            # We cannot know whether the failures we saw in the test runs with the patch are expected.
+            return True
+
+        failures_introduced_by_patch = frozenset(failures_with_patch) - frozenset(clean_tree_results.failing_test_results())
+        if failures_introduced_by_patch:
+            self.failure_status_id = failure_id
+            # report_failure will either throw or return false.
+            return not self.report_failure(results_archive_for_failures_with_patch, LayoutTestResults(failures_introduced_by_patch, did_exceed_test_failure_limit=False), script_error)
+
+        # In this case, we know that all of the failures that we saw with the patch were
+        # also present without the patch, so we don't need to defer.
+        return False
+
     def _test_patch(self):
         if self._test():
             return True
@@ -236,32 +252,15 @@ class PatchAnalysisTask(object):
 
             tests_that_consistently_failed = first_failing_results_set.intersection(second_failing_results_set)
             if tests_that_consistently_failed:
-                self._build_and_test_without_patch()
-                self._clean_tree_results = self._delegate.test_results()
-                tests_that_failed_on_tree = self._clean_tree_results.failing_test_results()
-
-                new_failures_introduced_by_patch = tests_that_consistently_failed.difference(set(tests_that_failed_on_tree))
-                if new_failures_introduced_by_patch:
-                    self.failure_status_id = first_failure_status_id
-                    return self.report_failure(first_results_archive, new_failures_introduced_by_patch, first_script_error)
+                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 there is flakyness with the patch applied, but no consistent failures
+            # 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
-
-        if self._build_and_test_without_patch():
-            # The error from the previous ._test() run is real, report it.
-            self.failure_status_id = first_failure_status_id
-            return self.report_failure(first_results_archive, first_results, first_script_error)
+            return False  # Defer patch
 
-        self._clean_tree_results = self._delegate.test_results()
-
-        if self._clean_tree_results.did_exceed_test_failure_limit():
-            return False
-
-        if set(first_results.failing_tests()) - set(self._clean_tree_results.failing_tests()):
-            self.failure_status_id = first_failure_status_id
-            return self.report_failure(first_results_archive, first_results, first_script_error)
+        if self._should_defer_patch_or_throw(first_results.failing_test_results(), first_results_archive, first_script_error, first_failure_status_id):
+            return False  # Defer patch
 
         # At this point, we know that the first and second runs had the exact same failures,
         # and that those failures are all present on the clean tree, so we can say with certainty
@@ -276,10 +275,6 @@ class PatchAnalysisTask(object):
         assert(self._patch.id() == patch.id())  # PatchAnalysisTask is not currently re-useable.
         return self._results_from_patch_test_run
 
-    def results_from_test_run_without_patch(self, patch):
-        assert(self._patch.id() == patch.id())  # PatchAnalysisTask is not currently re-useable.
-        return self._clean_tree_results
-
     def report_failure(self, results_archive=None, results=None, script_error=None):
         if not self.validate():
             return False
index 6cfde3b..34b67c7 100644 (file)
@@ -58,16 +58,13 @@ class AbstractEarlyWarningSystem(AbstractReviewQueue, EarlyWarningSystemTaskDele
 
     def _failing_tests_message(self, task, patch):
         results = task.results_from_patch_test_run(patch)
-        clean_results = task.results_from_test_run_without_patch(patch)
 
-        unexpected_failures = None
-        if results and clean_results:
-            unexpected_failures = list(set(results.failing_tests()) - set(clean_results.failing_tests()))
-        if not unexpected_failures:
+        if not results:
             return None
-        if results and results.did_exceed_test_failure_limit():
+
+        if results.did_exceed_test_failure_limit():
             return "Number of test failures exceeded the failure limit."
-        return "New failing tests:\n%s" % "\n".join(unexpected_failures)
+        return "New failing tests:\n%s" % "\n".join(results.failing_tests())
 
     def _post_reject_message_on_bug(self, tool, patch, status_id, extra_message_text=None):
         if not extra_message_text:
index 01c9628..c6de13a 100644 (file)
@@ -51,9 +51,8 @@ class AbstractEarlyWarningSystemTest(QueuesTest):
         task.results_from_patch_test_run = lambda a: LayoutTestResults([test_results.TestResult("foo.html", failures=[test_failures.FailureTextMismatch()]),
                                                                           test_results.TestResult("bar.html", failures=[test_failures.FailureTextMismatch()])],
                                                                           did_exceed_test_failure_limit=False)
-        task.results_from_test_run_without_patch = lambda a: LayoutTestResults([], did_exceed_test_failure_limit=False)
         patch = ews._tool.bugs.fetch_attachment(10000)
-        self.assertMultiLineEqual(ews._failing_tests_message(task, patch), "New failing tests:\nbar.html\nfoo.html")
+        self.assertMultiLineEqual(ews._failing_tests_message(task, patch), "New failing tests:\nfoo.html\nbar.html")
 
 
 class EarlyWarningSystemTest(QueuesTest):
index 23b7859..6be853f 100644 (file)
@@ -338,17 +338,13 @@ class CommitQueue(PatchProcessingQueue, StepSequenceErrorHandler, CommitQueueTas
 
     def _failing_tests_message(self, task, patch):
         results = task.results_from_patch_test_run(patch)
-        clean_results = task.results_from_test_run_without_patch(patch)
 
-        unexpected_failures = None
-        if results and clean_results:
-            unexpected_failures = list(set(results.failing_tests()) - set(clean_results.failing_tests()))
-        # FIXME: Can this ever happen?
-        if not unexpected_failures:
+        if not results:
             return None
-        if results and results.did_exceed_test_failure_limit():
+
+        if results.did_exceed_test_failure_limit():
             return "Number of test failures exceeded the failure limit."
-        return "New failing tests:\n%s" % "\n".join(unexpected_failures)
+        return "New failing tests:\n%s" % "\n".join(results.failing_tests())
 
     def _error_message_for_bug(self, task, patch, script_error):
         message = self._failing_tests_message(task, patch)
index e2a4880..b9f718d 100644 (file)
@@ -308,9 +308,6 @@ MOCK: release_work_item: commit-queue 10000
             def results_from_patch_test_run(self, patch):
                 return LayoutTestResults([test_results.TestResult("mock_test_name.html", failures=[test_failures.FailureTextMismatch()])], did_exceed_test_failure_limit=False)
 
-            def results_from_test_run_without_patch(self, patch):
-                return LayoutTestResults([], did_exceed_test_failure_limit=False)
-
         queue = CommitQueue(MockCommitQueueTask)
 
         def mock_run_webkit_patch(command):