[Webkit-unassigned] [Bug 38246] wdiff_text throws ScriptError because wdiff returns non-zero when files differ
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Apr 28 01:03:45 PDT 2010
https://bugs.webkit.org/show_bug.cgi?id=38246
--- Comment #3 from Fumitoshi Ukai <ukai at chromium.org> 2010-04-28 01:03:44 PST ---
(From update of attachment 54533)
> diff --git a/WebKitTools/ChangeLog b/WebKitTools/ChangeLog
> index 3d9222a3e85a47986f55ae9f7a3f398345884caa..5c6796145e587a69b076d228536fd09dba0bcbb2 100644
> --- a/WebKitTools/ChangeLog
> +++ b/WebKitTools/ChangeLog
> @@ -1,3 +1,22 @@
> +2010-04-28 Eric Seidel <eric at webkit.org>
> +
> + Reviewed by NOBODY (OOPS!).
> +
> + 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-27 Michael Nordman <michaeln at google.com>
>
> Reviewed by Dmitry Titov.
> diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/port/base.py b/WebKitTools/Scripts/webkitpy/layout_tests/port/base.py
> index 1ca465c316a78d0e44a9e126ab1d9ccdfa40b02e..5bc47d973ec4f9919ed4a85ca264bff6907c9e6f 100644
> --- a/WebKitTools/Scripts/webkitpy/layout_tests/port/base.py
> +++ b/WebKitTools/Scripts/webkitpy/layout_tests/port/base.py
> @@ -535,39 +535,61 @@ 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>'
> + self._run_wdiff(actual_filename, expected_filename)
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):
> - _wdiff_available = False
> - else:
> + # Silently ignore cases where wdiff is missing.
> + if e.errno not in [errno.ENOENT, errno.EACCES, errno.ECHILD]:
> 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
> + _wdiff_available = False
> + return ""
>
> _pretty_patch_error_html = "Failed to run PrettyPatch, see error console."
>
> diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/port/base_unittest.py b/WebKitTools/Scripts/webkitpy/layout_tests/port/base_unittest.py
> index 8ea916528880149d7fa61fb6cbdc377f156bf812..f388956f7c44cdf9e865cbfed09b219ee5aa0dde 100644
> --- a/WebKitTools/Scripts/webkitpy/layout_tests/port/base_unittest.py
> +++ b/WebKitTools/Scripts/webkitpy/layout_tests/port/base_unittest.py
> @@ -28,10 +28,72 @@
>
> 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)
> + actual.close()
> + expected.close()
> +
> + # Bogus paths should raise a script error.
> + self.assertRaises(ScriptError, port._run_wdiff, "/does/not/exist", "/does/not/exist2")
> +
> + # 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")
> +
> +
> class DriverTest(unittest.TestCase):
>
> def _assert_wrapper(self, wrapper_string, expected_wrapper):
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list