2010-10-14 Dirk Pranke <dpranke@chromium.org>
authordpranke@chromium.org <dpranke@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 14 Oct 2010 23:22:53 +0000 (23:22 +0000)
committerdpranke@chromium.org <dpranke@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 14 Oct 2010 23:22:53 +0000 (23:22 +0000)
        Reviewed by Eric Seidel.

        new-run-webkit-tests will now handle missing Ruby installs (or
        missing PrettyPatch scripts) more cleanly - previously this
        would be detected when we actually tried to create the diff, and
        the error message was obscure. Now we'll log a warning up front
        and otherwise be silent.

        This change also refactors some global variables to be class or
        instance variables to be slightly more testable and more
        modular. There are no cases where we create lots of port objects
        and can't afford to test for configurations repeatedly, so
        there's no performance concern here.

        https://bugs.webkit.org/show_bug.cgi?id=47466

        * Scripts/webkitpy/layout_tests/port/base.py:
        * Scripts/webkitpy/layout_tests/port/base_unittest.py:
        * Scripts/webkitpy/layout_tests/port/chromium.py:

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

WebKitTools/ChangeLog
WebKitTools/Scripts/webkitpy/layout_tests/port/base.py
WebKitTools/Scripts/webkitpy/layout_tests/port/base_unittest.py
WebKitTools/Scripts/webkitpy/layout_tests/port/chromium.py

index 1f42c86330f466953218bc131821552a7493a421..03b8e6a1e161f1e76263dd903c2102b6fc2062df 100644 (file)
@@ -1,3 +1,25 @@
+2010-10-14  Dirk Pranke  <dpranke@chromium.org>
+
+        Reviewed by Eric Seidel.
+
+        new-run-webkit-tests will now handle missing Ruby installs (or
+        missing PrettyPatch scripts) more cleanly - previously this
+        would be detected when we actually tried to create the diff, and
+        the error message was obscure. Now we'll log a warning up front
+        and otherwise be silent.
+
+        This change also refactors some global variables to be class or
+        instance variables to be slightly more testable and more
+        modular. There are no cases where we create lots of port objects
+        and can't afford to test for configurations repeatedly, so
+        there's no performance concern here.
+
+        https://bugs.webkit.org/show_bug.cgi?id=47466
+
+        * Scripts/webkitpy/layout_tests/port/base.py:
+        * Scripts/webkitpy/layout_tests/port/base_unittest.py:
+        * Scripts/webkitpy/layout_tests/port/chromium.py:
+
 2010-10-08  Martin Robinson  <mrobinson@igalia.com>
 
         Reviewed by Xan Lopez.
index 38b982b551d82076015043f8990126f126a48c9a..b15edb246468bf82dc95520e43d49db881bbe8ae 100644 (file)
@@ -55,22 +55,6 @@ from webkitpy.common.system.user import User
 _log = logutils.get_logger(__file__)
 
 
-# Python's Popen has a bug that causes any pipes opened to a
-# process that can't be executed to be leaked.  Since this
-# code is specifically designed to tolerate exec failures
-# to gracefully handle cases where wdiff is not installed,
-# the bug results in a massive file descriptor leak. As a
-# workaround, if an exec failure is ever experienced for
-# wdiff, assume it's not available.  This will leak one
-# file descriptor but that's better than leaking each time
-# wdiff would be run.
-#
-# http://mail.python.org/pipermail/python-list/
-#    2008-August/505753.html
-# http://bugs.python.org/issue3210
-_wdiff_available = True
-_pretty_patch_available = True
-
 # FIXME: This class should merge with webkitpy.webkit_port at some point.
 class Port(object):
     """Abstract class for Port-specific hooks for the layout_test package.
@@ -95,6 +79,25 @@ class Port(object):
         self._websocket_server = None
         self._http_lock = None
 
+        # Python's Popen has a bug that causes any pipes opened to a
+        # process that can't be executed to be leaked.  Since this
+        # code is specifically designed to tolerate exec failures
+        # to gracefully handle cases where wdiff is not installed,
+        # the bug results in a massive file descriptor leak. As a
+        # workaround, if an exec failure is ever experienced for
+        # wdiff, assume it's not available.  This will leak one
+        # file descriptor but that's better than leaking each time
+        # wdiff would be run.
+        #
+        # http://mail.python.org/pipermail/python-list/
+        #    2008-August/505753.html
+        # http://bugs.python.org/issue3210
+        self._wdiff_available = True
+
+        self._pretty_patch_path = self.path_from_webkit_base("BugsSite",
+              "PrettyPatch", "prettify.rb")
+        self._pretty_patch_available = True
+
     def default_child_processes(self):
         """Return the number of DumpRenderTree instances to use for this
         port."""
@@ -127,6 +130,27 @@ class Port(object):
         """This routine is used to check whether image_diff binary exists."""
         raise NotImplementedError('Port.check_image_diff')
 
+    def check_pretty_patch(self):
+        """Checks whether we can use the PrettyPatch ruby script."""
+
+        # check if Ruby is installed
+        try:
+            result = self._executive.run_command(['ruby', '--version'])
+        except OSError, e:
+            if e.errno in [errno.ENOENT, errno.EACCES, errno.ECHILD]:
+                _log.error("Ruby is not installed; "
+                           "can't generate pretty patches.")
+                _log.error('')
+                return False
+
+        if not self.path_exists(self._pretty_patch_path):
+            _log.error('Unable to find %s .' % self._pretty_patch_path)
+            _log.error("Can't generate pretty patches.")
+            _log.error('')
+            return False
+
+        return True
+
     def compare_text(self, expected_text, actual_text):
         """Return whether or not the two strings are *not* equal. This
         routine is used to diff text output.
@@ -638,8 +662,7 @@ class Port(object):
         """Returns a string of HTML indicating the word-level diff of the
         contents of the two filenames. Returns an empty string if word-level
         diffing isn't available."""
-        global _wdiff_available  # See explaination at top of file.
-        if not _wdiff_available:
+        if not self._wdiff_available:
             return ""
         try:
             # It's possible to raise a ScriptError we pass wdiff invalid paths.
@@ -647,33 +670,33 @@ class Port(object):
         except OSError, e:
             if e.errno in [errno.ENOENT, errno.EACCES, errno.ECHILD]:
                 # Silently ignore cases where wdiff is missing.
-                _wdiff_available = False
+                self._wdiff_available = False
                 return ""
             raise
 
-    _pretty_patch_error_html = "Failed to run PrettyPatch, see error console."
+    # 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):
-        # FIXME: Much of this function could move to prettypatch.rb
-        global _pretty_patch_available
-        if not _pretty_patch_available:
+        if not self._pretty_patch_available:
             return self._pretty_patch_error_html
-        pretty_patch_path = self.path_from_webkit_base("BugsSite", "PrettyPatch")
-        prettify_path = os.path.join(pretty_patch_path, "prettify.rb")
-        command = ["ruby", "-I", pretty_patch_path, prettify_path, diff_path]
+        command = ("ruby", "-I", os.path.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.
-            _pretty_patch_available = False
+            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.
-            _pretty_patch_available = False
-            _log.error("Failed to run PrettyPatch (%s):\n%s" % (command, e.message_with_output()))
+            # 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 _webkit_build_directory(self, args):
index e66c64dbe87dd47eb86117dac28de43e644518a3..40eb984cb914a59509699020809a3b0d6022a10c 100644 (file)
@@ -139,11 +139,11 @@ class PortTest(unittest.TestCase):
             expected_wdiff = "<head><style>.del { background: #faa; } .add { background: #afa; }</style></head><pre><span class=del>foo</span><span class=add>bar</span></pre>"
             self.assertEqual(wdiff, expected_wdiff)
             # Running the full wdiff_text method should give the same result.
-            base._wdiff_available = True  # In case it's somehow already disabled.
+            port._wdiff_available = True  # In case it's somehow already disabled.
             wdiff = port.wdiff_text(actual.name, expected.name)
             self.assertEqual(wdiff, expected_wdiff)
             # wdiff should still be available after running wdiff_text with a valid diff.
-            self.assertTrue(base._wdiff_available)
+            self.assertTrue(port._wdiff_available)
             actual.close()
             expected.close()
 
@@ -151,7 +151,7 @@ class PortTest(unittest.TestCase):
             self.assertRaises(ScriptError, port._run_wdiff, "/does/not/exist", "/does/not/exist2")
             self.assertRaises(ScriptError, port.wdiff_text, "/does/not/exist", "/does/not/exist2")
             # wdiff will still be available after running wdiff_text with invalid paths.
-            self.assertTrue(base._wdiff_available)
+            self.assertTrue(port._wdiff_available)
             base._wdiff_available = True
 
         # If wdiff does not exist _run_wdiff should throw an OSError.
@@ -161,8 +161,7 @@ class PortTest(unittest.TestCase):
         # wdiff_text should not throw an error if wdiff does not exist.
         self.assertEqual(port.wdiff_text("foo", "bar"), "")
         # However wdiff should not be available after running wdiff_text if wdiff is missing.
-        self.assertFalse(base._wdiff_available)
-        base._wdiff_available = True
+        self.assertFalse(port._wdiff_available)
 
     def test_diff_text(self):
         port = base.Port()
index 301d4b189b09c83d2706e360a73e8823fc1b0808..0fae62f3a231e60dd664992e75d0d18d35a3ac26 100644 (file)
@@ -115,6 +115,10 @@ class ChromiumPort(base.Port):
             result = self.check_image_diff(
                 'To override, invoke with --no-pixel-tests') and result
 
+        # It's okay if pretty patch isn't available, but we will at
+        # least log a message.
+        self.check_pretty_patch()
+
         return result
 
     def check_sys_deps(self, needs_http):