Unreviewed, rolling out r159839.
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 28 Nov 2013 16:55:28 +0000 (16:55 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 28 Nov 2013 16:55:28 +0000 (16:55 +0000)
http://trac.webkit.org/changeset/159839
https://bugs.webkit.org/show_bug.cgi?id=124974

run-webkit-tests doesn't generate pretty diff (Requested by
Ossy on #webkit).

* Scripts/webkitpy/common/prettypatch.py:
(PrettyPatch.__init__):
(PrettyPatch.pretty_diff):
* Scripts/webkitpy/layout_tests/controllers/test_result_writer.py:
(TestResultWriter.create_text_diff_and_write_result):
* Scripts/webkitpy/layout_tests/models/test_run_results.py:
(summarize_results):
* Scripts/webkitpy/port/base.py:
(Port.__init__):
(Port.pretty_patch_available):
(Port.check_pretty_patch):
(Port.variable):
(Port.variable.pretty_patch_text):
* Scripts/webkitpy/port/base_unittest.py:
(PortTest.test_pretty_patch_os_error):
(PortTest.test_pretty_patch_script_error):

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

Tools/ChangeLog
Tools/Scripts/webkitpy/common/prettypatch.py
Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer.py
Tools/Scripts/webkitpy/layout_tests/models/test_run_results.py
Tools/Scripts/webkitpy/port/base.py
Tools/Scripts/webkitpy/port/base_unittest.py

index cdb808a..cef0032 100644 (file)
@@ -1,3 +1,29 @@
+2013-11-28  Commit Queue  <commit-queue@webkit.org>
+
+        Unreviewed, rolling out r159839.
+        http://trac.webkit.org/changeset/159839
+        https://bugs.webkit.org/show_bug.cgi?id=124974
+
+        run-webkit-tests doesn't generate pretty diff (Requested by
+        Ossy on #webkit).
+
+        * Scripts/webkitpy/common/prettypatch.py:
+        (PrettyPatch.__init__):
+        (PrettyPatch.pretty_diff):
+        * Scripts/webkitpy/layout_tests/controllers/test_result_writer.py:
+        (TestResultWriter.create_text_diff_and_write_result):
+        * Scripts/webkitpy/layout_tests/models/test_run_results.py:
+        (summarize_results):
+        * Scripts/webkitpy/port/base.py:
+        (Port.__init__):
+        (Port.pretty_patch_available):
+        (Port.check_pretty_patch):
+        (Port.variable):
+        (Port.variable.pretty_patch_text):
+        * Scripts/webkitpy/port/base_unittest.py:
+        (PortTest.test_pretty_patch_os_error):
+        (PortTest.test_pretty_patch_script_error):
+
 2013-11-28  Antti Koivisto  <antti@apple.com>
 
         Remove feature: CSS variables
index 7868a5f..e8a913a 100644 (file)
 # (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
 # OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
-import errno
-import logging
 import os
 import tempfile
-from webkitpy.common.system.executive import ScriptError
-
-_log = logging.getLogger(__name__)
 
 
 class PrettyPatch(object):
     # FIXME: PrettyPatch should not require checkout_root.
-    def __init__(self, executive, checkout_root, filesystem=None):
+    def __init__(self, executive, checkout_root):
         self._executive = executive
-        self._filesystem = filesystem
         self._checkout_root = checkout_root
-        self._pretty_patch_path = os.path.join(self._checkout_root, "Websites", "bugs.webkit.org", "PrettyPatch")
-        self._prettify_path = os.path.join(self._pretty_patch_path, "prettify.rb")
-        self.pretty_patch_error_html = "Failed to run PrettyPatch, see error log."
-        self.ppatch_available = None
 
     def pretty_diff_file(self, diff):
         # Diffs can contain multiple text files of different encodings
@@ -62,59 +52,16 @@ class PrettyPatch(object):
         if not diff:
             return ""
 
+        pretty_patch_path = os.path.join(self._checkout_root,
+                                         "Websites", "bugs.webkit.org",
+                                         "PrettyPatch")
+        prettify_path = os.path.join(pretty_patch_path, "prettify.rb")
         args = [
             "ruby",
             "-I",
-            self._pretty_patch_path,
-            self._prettify_path,
+            pretty_patch_path,
+            prettify_path,
         ]
         # PrettyPatch does not modify the encoding of the diff output
         # so we can't expect it to be utf-8.
         return self._executive.run_command(args, input=diff, decode_output=False)
-
-    def pretty_patch_available(self):
-        if self.ppatch_available is None:
-            self.ppatch_available = self.check_pretty_patch(logging=False)
-        return self.ppatch_available
-
-    def check_pretty_patch(self, logging=True):
-        """Checks whether we can use the PrettyPatch ruby script."""
-        try:
-            _ = self._executive.run_command(['ruby', '--version'])
-        except OSError, e:
-            if e.errno in [errno.ENOENT, errno.EACCES, errno.ECHILD]:
-                if logging:
-                    _log.warning("Ruby is not installed; can't generate pretty patches.")
-                    _log.warning('')
-                return False
-
-        if not self._filesystem.exists(self._pretty_patch_path):
-            if logging:
-                _log.warning("Unable to find %s; can't generate pretty patches." % self._pretty_patch_path)
-                _log.warning('')
-            return False
-
-        return True
-
-    def pretty_patch_text(self, diff_path):
-        if self.ppatch_available is None:
-            self.ppatch_available = self.check_pretty_patch(logging=False)
-        if not self.ppatch_available:
-            return self.pretty_patch_error_html
-        command = ("ruby", "-I", self._filesystem.dirname(self._pretty_patch_path),
-                   self._pretty_patch_path, diff_path)
-        try:
-            # Diffs are treated as binary (we pass decode_output=False) as they
-            # may contain multiple files of conflicting encodings.
-            return self._executive.run_command(command, decode_output=False)
-        except OSError, e:
-            # If the system is missing ruby log the error and stop trying.
-            self.ppatch_available = False
-            _log.error("Failed to run PrettyPatch (%s): %s" % (command, e))
-            return self.pretty_patch_error_html
-        except ScriptError, e:
-            # If ruby failed to run for some reason, log the command
-            # output and stop trying.
-            self.ppatch_available = False
-            _log.error("Failed to run PrettyPatch (%s):\n%s" % (command, e.message_with_output()))
-            return self.pretty_patch_error_html
index 005c702..23e44d5 100644 (file)
@@ -196,8 +196,8 @@ class TestResultWriter(object):
             self._write_binary_file(wdiff_filename, wdiff)
 
         # Use WebKit's PrettyPatch.rb to get an HTML diff.
-        if self._port.pretty_patch.pretty_patch_available():
-            pretty_patch = self._port.pretty_patch.pretty_patch_text(diff_filename)
+        if self._port.pretty_patch_available():
+            pretty_patch = self._port.pretty_patch_text(diff_filename)
             pretty_patch_filename = self.output_filename(self.FILENAME_SUFFIX_PRETTY_PATCH)
             self._write_binary_file(pretty_patch_filename, pretty_patch)
 
index c460f5b..7632364 100644 (file)
@@ -243,7 +243,7 @@ def summarize_results(port_obj, expectations, initial_results, retry_results, en
     results['interrupted'] = initial_results.interrupted  # Does results.html have enough information to compute this itself? (by checking total number of results vs. total number of tests?)
     results['layout_tests_dir'] = port_obj.layout_tests_dir()
     results['has_wdiff'] = port_obj.wdiff_available()
-    results['has_pretty_patch'] = port_obj.pretty_patch.pretty_patch_available()
+    results['has_pretty_patch'] = port_obj.pretty_patch_available()
     results['pixel_tests_enabled'] = port_obj.get_option('pixel_tests')
 
     try:
index fb6726e..3118b33 100644 (file)
@@ -51,7 +51,6 @@ except ImportError:
 from webkitpy.common import find_files
 from webkitpy.common import read_checksum_from_png
 from webkitpy.common.memoized import memoized
-from webkitpy.common.prettypatch import PrettyPatch
 from webkitpy.common.system import path
 from webkitpy.common.system.executive import ScriptError
 from webkitpy.common.system.systemhost import SystemHost
@@ -117,7 +116,6 @@ class Port(object):
         self._filesystem = host.filesystem
         self._webkit_finder = WebKitFinder(host.filesystem)
         self._config = port_config.Config(self._executive, self._filesystem, self.port_name)
-        self.pretty_patch = PrettyPatch(self._executive, self.path_from_webkit_base(), self._filesystem)
 
         self._helper = None
         self._http_server = None
@@ -141,6 +139,10 @@ class Port(object):
         # http://bugs.python.org/issue3210
         self._wdiff_available = None
 
+        # FIXME: prettypatch.py knows this path, why is it copied here?
+        self._pretty_patch_path = self.path_from_webkit_base("Websites", "bugs.webkit.org", "PrettyPatch", "prettify.rb")
+        self._pretty_patch_available = None
+
         if not hasattr(options, 'configuration') or not options.configuration:
             self.set_option_default('configuration', self.default_configuration())
         self._test_configuration = None
@@ -176,6 +178,11 @@ class Port(object):
             self._wdiff_available = self.check_wdiff(logging=False)
         return self._wdiff_available
 
+    def pretty_patch_available(self):
+        if self._pretty_patch_available is None:
+            self._pretty_patch_available = self.check_pretty_patch(logging=False)
+        return self._pretty_patch_available
+
     def should_retry_crashes(self):
         return False
 
@@ -274,6 +281,25 @@ class Port(object):
             return False
         return True
 
+    def check_pretty_patch(self, logging=True):
+        """Checks whether we can use the PrettyPatch ruby script."""
+        try:
+            _ = self._executive.run_command(['ruby', '--version'])
+        except OSError, e:
+            if e.errno in [errno.ENOENT, errno.EACCES, errno.ECHILD]:
+                if logging:
+                    _log.warning("Ruby is not installed; can't generate pretty patches.")
+                    _log.warning('')
+                return False
+
+        if not self._filesystem.exists(self._pretty_patch_path):
+            if logging:
+                _log.warning("Unable to find %s; can't generate pretty patches." % self._pretty_patch_path)
+                _log.warning('')
+            return False
+
+        return True
+
     def check_wdiff(self, logging=True):
         if not self._path_to_wdiff():
             # Don't need to log here since this is the port choosing not to use wdiff.
@@ -1128,6 +1154,32 @@ class Port(object):
                 return ""
             raise
 
+    # This is a class variable so we can test error output easily.
+    _pretty_patch_error_html = "Failed to run PrettyPatch, see error log."
+
+    def pretty_patch_text(self, diff_path):
+        if self._pretty_patch_available is None:
+            self._pretty_patch_available = self.check_pretty_patch(logging=False)
+        if not self._pretty_patch_available:
+            return self._pretty_patch_error_html
+        command = ("ruby", "-I", self._filesystem.dirname(self._pretty_patch_path),
+                   self._pretty_patch_path, diff_path)
+        try:
+            # Diffs are treated as binary (we pass decode_output=False) as they
+            # may contain multiple files of conflicting encodings.
+            return self._executive.run_command(command, decode_output=False)
+        except OSError, e:
+            # If the system is missing ruby log the error and stop trying.
+            self._pretty_patch_available = False
+            _log.error("Failed to run PrettyPatch (%s): %s" % (command, e))
+            return self._pretty_patch_error_html
+        except ScriptError, e:
+            # If ruby failed to run for some reason, log the command
+            # output and stop trying.
+            self._pretty_patch_available = False
+            _log.error("Failed to run PrettyPatch (%s):\n%s" % (command, e.message_with_output()))
+            return self._pretty_patch_error_html
+
     def default_configuration(self):
         return self._config.default_configuration()
 
index e2da5c6..c04cae0 100644 (file)
@@ -90,24 +90,24 @@ class PortTest(unittest.TestCase):
         port = self.make_port(executive=executive_mock.MockExecutive2(exception=OSError))
         oc = OutputCapture()
         oc.capture_output()
-        self.assertEqual(port.pretty_patch.pretty_patch_text("patch.txt"),
-                         port.pretty_patch.pretty_patch_error_html)
+        self.assertEqual(port.pretty_patch_text("patch.txt"),
+                         port._pretty_patch_error_html)
 
         # This tests repeated calls to make sure we cache the result.
-        self.assertEqual(port.pretty_patch.pretty_patch_text("patch.txt"),
-                         port.pretty_patch.pretty_patch_error_html)
+        self.assertEqual(port.pretty_patch_text("patch.txt"),
+                         port._pretty_patch_error_html)
         oc.restore_output()
 
     def test_pretty_patch_script_error(self):
         # FIXME: This is some ugly white-box test hacking ...
         port = self.make_port(executive=executive_mock.MockExecutive2(exception=ScriptError))
-        port.pretty_patch.ppatch_available = True
-        self.assertEqual(port.pretty_patch.pretty_patch_text("patch.txt"),
-                         port.pretty_patch.pretty_patch_error_html)
+        port._pretty_patch_available = True
+        self.assertEqual(port.pretty_patch_text("patch.txt"),
+                         port._pretty_patch_error_html)
 
         # This tests repeated calls to make sure we cache the result.
-        self.assertEqual(port.pretty_patch.pretty_patch_text("patch.txt"),
-                         port.pretty_patch.pretty_patch_error_html)
+        self.assertEqual(port.pretty_patch_text("patch.txt"),
+                         port._pretty_patch_error_html)
 
     def integration_test_run_wdiff(self):
         executive = Executive()