2010-04-28 Eric Seidel <eric@webkit.org>
authoreric@webkit.org <eric@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 28 Apr 2010 09:44:42 +0000 (09:44 +0000)
committereric@webkit.org <eric@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 28 Apr 2010 09:44:42 +0000 (09:44 +0000)
        Reviewed by Shinichiro Hamaji.

        wdiff_text throws ScriptError because wdiff returns non-zero when files differ
        https://bugs.webkit.org/show_bug.cgi?id=38246

        wdiff returns 0 when files are the same, 1 when they differ.
        run_command by default raises ScriptError if the return code is non-zero.
        Fixed this by adding a custom error handler which only raises if the
        return code is not 1.

        I broke up the huge wdiff_text() method into little pieces
        for easier unit testing.  There is only one functional change here
        and that is the addition of the custom error handler.

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

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

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

index 9533d39..b4dbecb 100644 (file)
@@ -1,3 +1,22 @@
+2010-04-28  Eric Seidel  <eric@webkit.org>
+
+        Reviewed by Shinichiro Hamaji.
+
+        wdiff_text throws ScriptError because wdiff returns non-zero when files differ
+        https://bugs.webkit.org/show_bug.cgi?id=38246
+
+        wdiff returns 0 when files are the same, 1 when they differ.
+        run_command by default raises ScriptError if the return code is non-zero.
+        Fixed this by adding a custom error handler which only raises if the
+        return code is not 1.
+
+        I broke up the huge wdiff_text() method into little pieces
+        for easier unit testing.  There is only one functional change here
+        and that is the addition of the custom error handler.
+
+        * Scripts/webkitpy/layout_tests/port/base.py:
+        * Scripts/webkitpy/layout_tests/port/base_unittest.py:
+
 2010-04-28  Fumitoshi Ukai  <ukai@chromium.org>
 
         Unreviewed build fix.
index 1ca465c..cac6722 100644 (file)
@@ -535,39 +535,63 @@ class Port(object):
         expectations, determining search paths, and logging information."""
         raise NotImplementedError('Port.version')
 
+    _WDIFF_DEL = '##WDIFF_DEL##'
+    _WDIFF_ADD = '##WDIFF_ADD##'
+    _WDIFF_END = '##WDIFF_END##'
+
+    def _format_wdiff_output_as_html(self, wdiff):
+        wdiff = cgi.escape(wdiff)
+        wdiff = wdiff.replace(self._WDIFF_DEL, "<span class=del>")
+        wdiff = wdiff.replace(self._WDIFF_ADD, "<span class=add>")
+        wdiff = wdiff.replace(self._WDIFF_END, "</span>")
+        html = "<head><style>.del { background: #faa; } "
+        html += ".add { background: #afa; }</style></head>"
+        html += "<pre>%s</pre>" % wdiff
+        return html
+
+    def _wdiff_command(self, actual_filename, expected_filename):
+        executable = self._path_to_wdiff()
+        return [executable,
+                "--start-delete=%s" % self._WDIFF_DEL,
+                "--end-delete=%s" % self._WDIFF_END,
+                "--start-insert=%s" % self._WDIFF_ADD,
+                "--end-insert=%s" % self._WDIFF_END,
+                actual_filename,
+                expected_filename]
+
+    @staticmethod
+    def _handle_wdiff_error(script_error):
+        # Exit 1 means the files differed, any other exit code is an error.
+        if script_error.exit_code != 1:
+            raise script_error
+
+    def _run_wdiff(self, actual_filename, expected_filename):
+        """Runs wdiff and may throw exceptions.
+        This is mostly a hook for unit testing."""
+        # Diffs are treated as binary as they may include multiple files
+        # with conflicting encodings.  Thus we do not decode the output.
+        command = self._wdiff_command(actual_filename, expected_filename)
+        wdiff = self._executive.run_command(command, decode_output=False,
+            error_handler=self._handle_wdiff_error)
+        return self._format_wdiff_output_as_html(wdiff)
+
     def wdiff_text(self, actual_filename, expected_filename):
         """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."""
-        executable = self._path_to_wdiff()
-        cmd = [executable,
-               '--start-delete=##WDIFF_DEL##',
-               '--end-delete=##WDIFF_END##',
-               '--start-insert=##WDIFF_ADD##',
-               '--end-insert=##WDIFF_END##',
-               actual_filename,
-               expected_filename]
         global _wdiff_available  # See explaination at top of file.
-        result = ''
+        if not _wdiff_available:
+            return ""
         try:
-            if _wdiff_available:
-                wdiff = self._executive.run_command(cmd, decode_output=False)
-                wdiff = cgi.escape(wdiff)
-                wdiff = wdiff.replace('##WDIFF_DEL##', '<span class=del>')
-                wdiff = wdiff.replace('##WDIFF_ADD##', '<span class=add>')
-                wdiff = wdiff.replace('##WDIFF_END##', '</span>')
-                result = '<head><style>.del { background: #faa; } '
-                result += '.add { background: #afa; }</style></head>'
-                result += '<pre>' + wdiff + '</pre>'
+            # It's possible to raise a ScriptError we pass wdiff invalid paths.
+            return self._run_wdiff(actual_filename, expected_filename)
         except OSError, e:
-            if (e.errno == errno.ENOENT or e.errno == errno.EACCES or
-                e.errno == errno.ECHILD):
+            if e.errno in [errno.ENOENT, errno.EACCES, errno.ECHILD]:
+                # Silently ignore cases where wdiff is missing.
                 _wdiff_available = False
-            else:
-                raise e
-        # Diffs are treated as binary as they may include multiple files
-        # with conflicting encodings.  Thus we do not decode the output here.
-        return result
+                return ""
+            raise
+        assert(False)  # Should never be reached.
 
     _pretty_patch_error_html = "Failed to run PrettyPatch, see error console."
 
index 8ea9165..f821353 100644 (file)
 
 import base
 import unittest
+import tempfile
 
+from webkitpy.common.system.executive import Executive, ScriptError
 from webkitpy.thirdparty.mock import Mock
 
 
+class PortTest(unittest.TestCase):
+
+    def test_format_wdiff_output_as_html(self):
+        output = "OUTPUT %s %s %s" % (base.Port._WDIFF_DEL, base.Port._WDIFF_ADD, base.Port._WDIFF_END)
+        html = base.Port()._format_wdiff_output_as_html(output)
+        expected_html = "<head><style>.del { background: #faa; } .add { background: #afa; }</style></head><pre>OUTPUT <span class=del> <span class=add> </span></pre>"
+        self.assertEqual(html, expected_html)
+
+    def test_wdiff_command(self):
+        port = base.Port()
+        port._path_to_wdiff = lambda: "/path/to/wdiff"
+        command = port._wdiff_command("/actual/path", "/expected/path")
+        expected_command = [
+            "/path/to/wdiff",
+            "--start-delete=##WDIFF_DEL##",
+            "--end-delete=##WDIFF_END##",
+            "--start-insert=##WDIFF_ADD##",
+            "--end-insert=##WDIFF_END##",
+            "/actual/path",
+            "/expected/path",
+        ]
+        self.assertEqual(command, expected_command)
+
+    def _file_with_contents(self, contents, encoding="utf-8"):
+        new_file = tempfile.NamedTemporaryFile()
+        new_file.write(contents.encode(encoding))
+        new_file.flush()
+        return new_file
+
+    def test_run_wdiff(self):
+        executive = Executive()
+        # This may fail on some systems.  We could ask the port
+        # object for the wdiff path, but since we don't know what
+        # port object to use, this is sufficient for now.
+        try:
+            wdiff_path = executive.run_command(["which", "wdiff"]).rstrip()
+        except Exception, e:
+            wdiff_path = None
+
+        port = base.Port()
+        port._path_to_wdiff = lambda: wdiff_path
+
+        if wdiff_path:
+            # "with tempfile.NamedTemporaryFile() as actual" does not seem to work in Python 2.5
+            actual = self._file_with_contents(u"foo")
+            expected = self._file_with_contents(u"bar")
+            wdiff = port._run_wdiff(actual.name, expected.name)
+            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.
+            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)
+            actual.close()
+            expected.close()
+
+            # Bogus paths should raise a script error.
+            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)
+            base._wdiff_available = True
+
+        # If wdiff does not exist _run_wdiff should throw an OSError.
+        port._path_to_wdiff = lambda: "/invalid/path/to/wdiff"
+        self.assertRaises(OSError, port._run_wdiff, "foo", "bar")
+
+        # 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
+
+
 class DriverTest(unittest.TestCase):
 
     def _assert_wrapper(self, wrapper_string, expected_wrapper):