Bug 38246

Summary: wdiff_text throws ScriptError because wdiff returns non-zero when files differ
Product: WebKit Reporter: Eric Seidel (no email) <eric>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, cjerdonek, dpranke, eric, jorlow, tony, ukai, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
Fixed missing return and added more unit tests hamaji: review+, jorlow: commit-queue+

Eric Seidel (no email)
Reported 2010-04-28 00:48:48 PDT
wdiff_text throws ScriptError because wdiff returns non-zero when files differ
Attachments
Patch (8.65 KB, patch)
2010-04-28 00:51 PDT, Eric Seidel (no email) 		no flags
DetailsFormatted DiffDiff
Fixed missing return and added more unit tests (9.85 KB, patch)
2010-04-28 01:19 PDT, Eric Seidel (no email) 		hamaji: review+
jorlow: commit-queue+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Eric Seidel (no email)
Comment 1 2010-04-28 00:51:38 PDT
Created attachment 54533 [details] Patch
Eric Seidel (no email)
Comment 2 2010-04-28 00:56:02 PDT
*** Bug 38240 has been marked as a duplicate of this bug. ***
Fumitoshi Ukai
Comment 3 2010-04-28 01:03:44 PDT
Comment on attachment 54533 [details] Patch > 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@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@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):
Tony Chang
Comment 4 2010-04-28 01:04:07 PDT
Comment on attachment 54533 [details] Patch What's the benefit of a custom error handler vs putting endAfterSelection in a try/except block? Unittests look great. Should they be limited to 80 cols? (I don't remember the decision). WebKitTools/Scripts/webkitpy/layout_tests/port/base_unittest.py:73 + except Exception, e: Can we make this exception more tightly scoped?
Eric Seidel (no email)
Comment 5 2010-04-28 01:16:20 PDT
(In reply to comment #4) > (From update of attachment 54533 [details]) > What's the benefit of a custom error handler vs putting endAfterSelection in a > try/except block? > > Unittests look great. Should they be limited to 80 cols? (I don't remember the > decision). I don't think there was a decision. Generally I wrap to 80c, except when it hurts readability. I can look at wrapping the expected results. > WebKitTools/Scripts/webkitpy/layout_tests/port/base_unittest.py:73 > + except Exception, e: > Can we make this exception more tightly scoped? We could. I didn't want the unit tests to die unexpectedly on other systems. I'd rather they just ignore these tests if "which wdiff" doesn't work for any reason.
Tony Chang
Comment 6 2010-04-28 01:18:46 PDT
(In reply to comment #4) > (From update of attachment 54533 [details]) > What's the benefit of a custom error handler vs putting endAfterSelection in a > try/except block? Ooops, junk in my clipboard. That was supposed to read |self._executive.run_command| in a try/except block. Anyway, not a big deal either way, just seems like less code to catch an exception.
Eric Seidel (no email)
Comment 7 2010-04-28 01:19:20 PDT
Created attachment 54537 [details] Fixed missing return and added more unit tests
Eric Seidel (no email)
Comment 8 2010-04-28 01:21:56 PDT
(In reply to comment #6) > (In reply to comment #4) > > (From update of attachment 54533 [details] [details]) > > What's the benefit of a custom error handler vs putting endAfterSelection in a > > try/except block? > > Ooops, junk in my clipboard. That was supposed to read > |self._executive.run_command| in a try/except block. Anyway, not a big deal > either way, just seems like less code to catch an exception. The only advantage is the theoretical one of reserving exceptions for exceptional situations. :)
Fumitoshi Ukai
Comment 9 2010-04-28 01:25:17 PDT
(In reply to comment #7) > Created an attachment (id=54537) [details] > Fixed missing return and added more unit tests LGTM
Shinichiro Hamaji
Comment 10 2010-04-28 01:43:13 PDT
Comment on attachment 54537 [details] Fixed missing return and added more unit tests rs=me
Shinichiro Hamaji
Comment 11 2010-04-28 01:48:30 PDT
Comment on attachment 54537 [details] Fixed missing return and added more unit tests WebKitTools/Scripts/webkitpy/layout_tests/port/base.py:595 + raise e I think we don't need "e" to re-raise the exception. Also, I'm not sure if we need this except-clause, but I guess we want to emphasize the ScriptError can be thrown.
Eric Seidel (no email)
Comment 12 2010-04-28 01:59:02 PDT
(In reply to comment #11) > (From update of attachment 54537 [details]) > WebKitTools/Scripts/webkitpy/layout_tests/port/base.py:595 > + raise e > I think we don't need "e" to re-raise the exception. Also, I'm not sure if we > need this except-clause, but I guess we want to emphasize the ScriptError can > be thrown. You're right. We want just "raise" as that has the magical property of not messing up the stacktrace.
Jeremy Orlow
Comment 13 2010-04-28 02:23:14 PDT
cq+ since this is breaking Chromium bots and it seems as though there are no other outstanding comments.
Eric Seidel (no email)
Comment 14 2010-04-28 02:45:11 PDT
Committed r58395: <http://trac.webkit.org/changeset/58395>
WebKit Review Bot
Comment 15 2010-04-28 03:42:56 PDT
http://trac.webkit.org/changeset/58395 might have broken Tiger Intel Release The following changes are on the blame list: http://trac.webkit.org/changeset/58393 http://trac.webkit.org/changeset/58394 http://trac.webkit.org/changeset/58395
Note You need to log in before you can comment on or make changes to this bug.