Simplify ExpectedFailures
authorabarth@webkit.org <abarth@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 27 Jul 2012 07:48:04 +0000 (07:48 +0000)
committerabarth@webkit.org <abarth@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 27 Jul 2012 07:48:04 +0000 (07:48 +0000)
https://bugs.webkit.org/show_bug.cgi?id=92216

Reviewed by Eric Seidel.

This patch simplifies the ExpectedFailures class we use to remember
which tests are currently failing on the bots. When we wrote this code
originally, we weren't entirely sure how it would work. Now that we
understand it more clearly, we can write the code more clearly.

* Scripts/webkitpy/tool/bot/expectedfailures.py:
(_has_failures):
(_is_trustworthy):
(ExpectedFailures.__init__):
(ExpectedFailures.failures_were_expected):
(ExpectedFailures.unexpected_failures_observed):
(ExpectedFailures.update):
* Scripts/webkitpy/tool/bot/expectedfailures_unittest.py:
(ExpectedFailuresTest._assert_can_trust):
(ExpectedFailuresTest.test_failures_were_expected):
(ExpectedFailuresTest.test_unexpected_failures_observed):
(ExpectedFailuresTest.test_unexpected_failures_observed_when_tree_is_hosed):
* Scripts/webkitpy/tool/bot/patchanalysistask.py:
(PatchAnalysisTask._test):
(PatchAnalysisTask._build_and_test_without_patch):
(PatchAnalysisTask._test_patch):

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

Tools/ChangeLog
Tools/Scripts/webkitpy/tool/bot/expectedfailures.py
Tools/Scripts/webkitpy/tool/bot/expectedfailures_unittest.py
Tools/Scripts/webkitpy/tool/bot/patchanalysistask.py

index 728de8a..3ea3f52 100644 (file)
@@ -1,3 +1,32 @@
+2012-07-27  Adam Barth  <abarth@webkit.org>
+
+        Simplify ExpectedFailures
+        https://bugs.webkit.org/show_bug.cgi?id=92216
+
+        Reviewed by Eric Seidel.
+
+        This patch simplifies the ExpectedFailures class we use to remember
+        which tests are currently failing on the bots. When we wrote this code
+        originally, we weren't entirely sure how it would work. Now that we
+        understand it more clearly, we can write the code more clearly.
+
+        * Scripts/webkitpy/tool/bot/expectedfailures.py:
+        (_has_failures):
+        (_is_trustworthy):
+        (ExpectedFailures.__init__):
+        (ExpectedFailures.failures_were_expected):
+        (ExpectedFailures.unexpected_failures_observed):
+        (ExpectedFailures.update):
+        * Scripts/webkitpy/tool/bot/expectedfailures_unittest.py:
+        (ExpectedFailuresTest._assert_can_trust):
+        (ExpectedFailuresTest.test_failures_were_expected):
+        (ExpectedFailuresTest.test_unexpected_failures_observed):
+        (ExpectedFailuresTest.test_unexpected_failures_observed_when_tree_is_hosed):
+        * Scripts/webkitpy/tool/bot/patchanalysistask.py:
+        (PatchAnalysisTask._test):
+        (PatchAnalysisTask._build_and_test_without_patch):
+        (PatchAnalysisTask._test_patch):
+
 2012-07-27  Csaba Osztrogon√°c  <ossy@webkit.org>
 
         [Qt][WK2] REGRESSION(r119127): resetting window.internals settings between tests doesn't work properly
index adbd795..c0cfe21 100644 (file)
 class ExpectedFailures(object):
     def __init__(self):
         self._failures = set()
-        # If the set of failures is unbounded, self._failures isn't very
-        # meaningful because we can't store an unbounded set in memory.
-        self._failures_are_bounded = True
+        self._is_trustworthy = True
 
-    def _has_failures(self, results):
-        return bool(results and len(results.failing_tests()) != 0)
+    @classmethod
+    def _has_failures(cls, results):
+        return bool(results and results.failing_tests())
 
-    def has_bounded_failures(self, results):
-        assert(results)  # You probably want to call _has_failures first!
-        return bool(results.failure_limit_count() and len(results.failing_tests()) < results.failure_limit_count())
-
-    def _can_trust_results(self, results):
-        return self._has_failures(results) and self.has_bounded_failures(results)
+    @classmethod
+    def _should_trust(cls, results):
+        return bool(cls._has_failures(results) and results.failure_limit_count() and len(results.failing_tests()) < results.failure_limit_count())
 
     def failures_were_expected(self, results):
-        if not self._can_trust_results(results):
+        if not self._is_trustworthy:
+            return False
+        if not self._should_trust(results):
             return False
         return set(results.failing_tests()) <= self._failures
 
     def unexpected_failures_observed(self, results):
-        if not self._has_failures(results):
+        if not self._is_trustworthy:
             return None
-        if not self._failures_are_bounded:
+        if not self._has_failures(results):
             return None
         return set(results.failing_tests()) - self._failures
 
-    def shrink_expected_failures(self, results, run_success):
-        if run_success:
-            self._failures = set()
-            self._failures_are_bounded = True
-        elif self._can_trust_results(results):
-            # Remove all expected failures which are not in the new failing results.
-            self._failures.intersection_update(set(results.failing_tests()))
-            self._failures_are_bounded = True
-
-    def grow_expected_failures(self, results):
-        if not self._can_trust_results(results):
-            self._failures_are_bounded = False
-            return
-        self._failures.update(results.failing_tests())
-        self._failures_are_bounded = True
-        # FIXME: Should we assert() here that expected_failures never crosses a certain size?
+    def update(self, results):
+        if results:
+            self._failures = set(results.failing_tests())
+            self._is_trustworthy = self._should_trust(results)
index 0668746..4c1c3d9 100644 (file)
@@ -45,7 +45,7 @@ class MockResults(object):
 
 class ExpectedFailuresTest(unittest.TestCase):
     def _assert_can_trust(self, results, can_trust):
-        self.assertEquals(ExpectedFailures()._can_trust_results(results), can_trust)
+        self.assertEquals(ExpectedFailures._should_trust(results), can_trust)
 
     def test_can_trust_results(self):
         self._assert_can_trust(None, False)
@@ -61,21 +61,22 @@ class ExpectedFailuresTest(unittest.TestCase):
 
     def test_failures_were_expected(self):
         failures = ExpectedFailures()
-        failures.grow_expected_failures(MockResults(['foo.html']))
+        failures.update(MockResults(['foo.html']))
         self._assert_expected(failures, ['foo.html'], True)
         self._assert_expected(failures, ['bar.html'], False)
-        failures.shrink_expected_failures(MockResults(['baz.html']), False)
-        self._assert_expected(failures, ['foo.html'], False)
-        self._assert_expected(failures, ['baz.html'], False)
+        self._assert_expected(failures, ['bar.html', 'foo.html'], False)
 
-        failures.grow_expected_failures(MockResults(['baz.html']))
+        failures.update(MockResults(['baz.html']))
         self._assert_expected(failures, ['baz.html'], True)
-        failures.shrink_expected_failures(MockResults(), True)
+        self._assert_expected(failures, ['foo.html'], False)
+
+        failures.update(MockResults([]))
         self._assert_expected(failures, ['baz.html'], False)
+        self._assert_expected(failures, ['foo.html'], False)
 
     def test_unexpected_failures_observed(self):
         failures = ExpectedFailures()
-        failures.grow_expected_failures(MockResults(['foo.html']))
+        failures.update(MockResults(['foo.html']))
         self.assertEquals(failures.unexpected_failures_observed(MockResults(['foo.html', 'bar.html'])), set(['bar.html']))
         self.assertEquals(failures.unexpected_failures_observed(MockResults(['baz.html'])), set(['baz.html']))
         unbounded_results = MockResults(['baz.html', 'qux.html', 'taco.html'], failure_limit=3)
@@ -85,7 +86,7 @@ class ExpectedFailuresTest(unittest.TestCase):
 
     def test_unexpected_failures_observed_when_tree_is_hosed(self):
         failures = ExpectedFailures()
-        failures.grow_expected_failures(MockResults(['foo.html', 'banana.html'], failure_limit=2))
+        failures.update(MockResults(['foo.html', 'banana.html'], failure_limit=2))
         self.assertEquals(failures.unexpected_failures_observed(MockResults(['foo.html', 'bar.html'])), None)
         self.assertEquals(failures.unexpected_failures_observed(MockResults(['baz.html'])), None)
         unbounded_results = MockResults(['baz.html', 'qux.html', 'taco.html'], failure_limit=3)
index 96518c6..05ba737 100644 (file)
@@ -134,7 +134,7 @@ class PatchAnalysisTask(object):
         "Unable to build without patch")
 
     def _test(self):
-        success = self._run_command([
+        return self._run_command([
             "build-and-test",
             "--no-clean",
             "--no-update",
@@ -145,11 +145,8 @@ class PatchAnalysisTask(object):
         "Passed tests",
         "Patch does not pass tests")
 
-        self._expected_failures.shrink_expected_failures(self._delegate.test_results(), success)
-        return success
-
     def _build_and_test_without_patch(self):
-        success = self._run_command([
+        return self._run_command([
             "build-and-test",
             "--force-clean",
             "--no-update",
@@ -160,9 +157,6 @@ class PatchAnalysisTask(object):
         "Able to pass tests without patch",
         "Unable to pass tests without patch (tree is red?)")
 
-        self._expected_failures.shrink_expected_failures(self._delegate.test_results(), success)
-        return success
-
     def _land(self):
         # Unclear if this should pass --quiet or not.  If --parent-command always does the reporting, then it should.
         return self._run_command([
@@ -220,7 +214,7 @@ class PatchAnalysisTask(object):
             return self.report_failure(first_results_archive, first_results, first_script_error)
 
         clean_tree_results = self._delegate.test_results()
-        self._expected_failures.grow_expected_failures(clean_tree_results)
+        self._expected_failures.update(clean_tree_results)
 
         # Re-check if the original results are now to be expected to avoid a full re-try.
         if self._expected_failures.failures_were_expected(first_results):