Bug 38246 - wdiff_text throws ScriptError because wdiff returns non-zero when files differ
Summary: wdiff_text throws ScriptError because wdiff returns non-zero when files differ
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 38240 (view as bug list)
Depends on:
Blocks:
 
Reported: 2010-04-28 00:48 PDT by Eric Seidel (no email)
Modified: 2010-04-28 03:42 PDT (History)
8 users (show)

See Also:


Attachments
Patch (8.65 KB, patch)
2010-04-28 00:51 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
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+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2010-04-28 00:48:48 PDT
wdiff_text throws ScriptError because wdiff returns non-zero when files differ
Comment 1 Eric Seidel (no email) 2010-04-28 00:51:38 PDT
Created attachment 54533 [details]
Patch
Comment 2 Eric Seidel (no email) 2010-04-28 00:56:02 PDT
*** Bug 38240 has been marked as a duplicate of this bug. ***
Comment 3 Fumitoshi Ukai 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):
Comment 4 Tony Chang 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?
Comment 5 Eric Seidel (no email) 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.
Comment 6 Tony Chang 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.
Comment 7 Eric Seidel (no email) 2010-04-28 01:19:20 PDT
Created attachment 54537 [details]
Fixed missing return and added more unit tests
Comment 8 Eric Seidel (no email) 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. :)
Comment 9 Fumitoshi Ukai 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
Comment 10 Shinichiro Hamaji 2010-04-28 01:43:13 PDT
Comment on attachment 54537 [details]
Fixed missing return and added more unit tests

rs=me
Comment 11 Shinichiro Hamaji 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.
Comment 12 Eric Seidel (no email) 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.
Comment 13 Jeremy Orlow 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.
Comment 14 Eric Seidel (no email) 2010-04-28 02:45:11 PDT
Committed r58395: <http://trac.webkit.org/changeset/58395>
Comment 15 WebKit Review Bot 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