2010-03-26 Dirk Pranke <dpranke@chromium.org>
authordpranke@chromium.org <dpranke@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 26 Mar 2010 23:38:37 +0000 (23:38 +0000)
committerdpranke@chromium.org <dpranke@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 26 Mar 2010 23:38:37 +0000 (23:38 +0000)
        Reviewed by Eric Seidel.

        Implement pixel tests (image diff) properly on the Mac port.

        This change introduces a new "ServerPocess" class that can be used
        to manage processes that the run-webkit-tests harness forks off and
        expects to stay up for longer than a single request/response session.
        Both DumpRenderTree and ImageDiff use this style of communication,
        although the current code forks off a new ImageDiff for each diff
        (We need to restructure other parts of the code to be able to do this
        safely in a multi-threaded environment).

        Also, now that the ServerProcess abstraction exists, we can probably
        clean up and simplify some of the thread management logic in
        test_shell_thread as well.

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

        * Scripts/webkitpy/layout_tests/port/mac.py:
        * Scripts/webkitpy/layout_tests/port/server_process.py:
        * Scripts/webkitpy/layout_tests/test_types/image_diff.py:

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

WebKitTools/ChangeLog
WebKitTools/Scripts/webkitpy/layout_tests/port/mac.py
WebKitTools/Scripts/webkitpy/layout_tests/port/server_process.py [new file with mode: 0644]
WebKitTools/Scripts/webkitpy/layout_tests/test_types/image_diff.py

index e2cb5a9..15a446a 100644 (file)
@@ -1,3 +1,27 @@
+2010-03-26  Dirk Pranke  <dpranke@chromium.org>
+
+        Reviewed by Eric Seidel.
+
+        Implement pixel tests (image diff) properly on the Mac port.
+
+        This change introduces a new "ServerPocess" class that can be used
+        to manage processes that the run-webkit-tests harness forks off and
+        expects to stay up for longer than a single request/response session.
+        Both DumpRenderTree and ImageDiff use this style of communication,
+        although the current code forks off a new ImageDiff for each diff
+        (We need to restructure other parts of the code to be able to do this
+        safely in a multi-threaded environment).
+
+        Also, now that the ServerProcess abstraction exists, we can probably
+        clean up and simplify some of the thread management logic in
+        test_shell_thread as well.
+
+        https://bugs.webkit.org/show_bug.cgi?id=34826
+
+        * Scripts/webkitpy/layout_tests/port/mac.py:
+        * Scripts/webkitpy/layout_tests/port/server_process.py:
+        * Scripts/webkitpy/layout_tests/test_types/image_diff.py:
+
 2010-03-26  Sergio Villar Senin  <svillar@igalia.com>
 
         Reviewed by Eric Seidel.
index 30d2ab8..8994200 100644 (file)
 
 """WebKit Mac implementation of the Port interface."""
 
-import fcntl
 import logging
 import os
 import pdb
 import platform
-import select
+import re
+import shutil
 import signal
 import subprocess
 import sys
@@ -42,6 +42,7 @@ import time
 import webbrowser
 
 import base
+import server_process
 
 import webkitpy
 import webkitpy.common.system.executive as executive
@@ -85,11 +86,74 @@ class MacPort(base.Port):
             _log.error("DumpRenderTree was not found at %s" % driver_path)
             return False
 
-        # This should also validate that the ImageDiff path is valid
-        # (once this script knows how to use ImageDiff).
-        # https://bugs.webkit.org/show_bug.cgi?id=34826
+        image_diff_path = self._path_to_image_diff()
+        if not os.path.exists(image_diff_path):
+            _log.error("ImageDiff was not found at %s" % image_diff_path)
+            return False
+
         return True
 
+    def diff_image(self, expected_filename, actual_filename,
+                   diff_filename=None):
+        """Return True if the two files are different. Also write a delta
+        image of the two images into |diff_filename| if it is not None."""
+
+        # Handle the case where the test didn't actually generate an image.
+        actual_length = os.stat(actual_filename).st_size
+        if actual_length == 0:
+            if diff_filename:
+                shutil.copyfile(actual_filename, expected_filename)
+            return True
+
+        sp = self._diff_image_request(expected_filename, actual_filename)
+        return self._diff_image_reply(sp, expected_filename, diff_filename)
+
+    def _diff_image_request(self, expected_filename, actual_filename):
+        # FIXME: either expose the tolerance argument as a command-line
+        # parameter, or make it go away and aways use exact matches.
+        cmd = [self._path_to_image_diff(), '--tolerance', '0.1']
+        sp = server_process.ServerProcess(self, 'ImageDiff', cmd)
+
+        actual_length = os.stat(actual_filename).st_size
+        actual_file = open(actual_filename).read()
+        expected_length = os.stat(expected_filename).st_size
+        expected_file = open(expected_filename).read()
+        sp.write('Content-Length: %d\n%sContent-Length: %d\n%s' %
+                 (actual_length, actual_file, expected_length, expected_file))
+
+        return sp
+
+    def _diff_image_reply(self, sp, expected_filename, diff_filename):
+        timeout = 2.0
+        deadline = time.time() + timeout
+        output = sp.read_line(timeout)
+        while not sp.timed_out and not sp.crashed and output:
+            if output.startswith('Content-Length'):
+                m = re.match('Content-Length: (\d+)', output)
+                content_length = int(m.group(1))
+                timeout = deadline - time.time()
+                output = sp.read(timeout, content_length)
+                break
+            elif output.startswith('diff'):
+                break
+            else:
+                timeout = deadline - time.time()
+                output = sp.read_line(deadline)
+
+        result = True
+        if output.startswith('diff'):
+            m = re.match('diff: (.+)% (passed|failed)', output)
+            if m.group(2) == 'passed':
+                result = False
+        elif output and diff_filename:
+            open(diff_filename, 'w').write(output)
+        elif sp.timed_out:
+            _log.error("ImageDiff timed out on %s" % expected_filename)
+        elif sp.crashed:
+            _log.error("ImageDiff crashed")
+        sp.stop()
+        return result
+
     def num_cores(self):
         return int(os.popen2("sysctl -n hw.ncpu")[1].read())
 
@@ -253,7 +317,7 @@ class MacPort(base.Port):
         return None
 
     def _path_to_image_diff(self):
-        return self._build_path('image_diff') # FIXME: This is wrong and should be "ImageDiff", but having the correct path causes other parts of the script to hang.
+        return self._build_path('ImageDiff')
 
     def _path_to_wdiff(self):
         return 'wdiff' # FIXME: This does not exist on a default Mac OS X Leopard install.
@@ -288,13 +352,7 @@ class MacDriver(base.Driver):
     def __init__(self, port, image_path, driver_options):
         self._port = port
         self._driver_options = driver_options
-        self._target = port._options.target
         self._image_path = image_path
-        self._stdout_fd = None
-        self._cmd = None
-        self._env = None
-        self._proc = None
-        self._read_buffer = ''
 
         cmd = []
         # Hook for injecting valgrind or other runtime instrumentation,
@@ -308,49 +366,28 @@ class MacDriver(base.Driver):
             # practice it shouldn't come up and the --help output warns
             # about it anyway.
             cmd += self._options.wrapper.split()
-        # FIXME: Using arch here masks any possible file-not-found errors from a non-existant driver executable.
-        cmd += ['arch', '-i386', port._path_to_driver(), '-']
 
-        # FIXME: This is a hack around our lack of ImageDiff support for now.
-        if not self._port._options.no_pixel_tests:
-            _log.warn("This port does not yet support pixel tests.")
-            self._port._options.no_pixel_tests = True
-            #cmd.append('--pixel-tests')
+        cmd += ['arch', '-i386', port._path_to_driver(), '-']
 
-        #if driver_options:
-        #    cmd += driver_options
+        if image_path:
+            cmd.append('--pixel-tests')
         env = os.environ
         env['DYLD_FRAMEWORK_PATH'] = self._port._build_path()
-        self._cmd = cmd
-        self._env = env
-        self.restart()
+        self._sproc = server_process.ServerProcess(self._port,
+            "DumpRenderTree", cmd, env)
 
     def poll(self):
-        return self._proc.poll()
+        return self._sproc.poll()
 
     def restart(self):
-        self.stop()
-        # We need to pass close_fds=True to work around Python bug #2320
-        # (otherwise we can hang when we kill test_shell when we are running
-        # multiple threads). See http://bugs.python.org/issue2320 .
-        self._proc = subprocess.Popen(self._cmd, stdin=subprocess.PIPE,
-                                      stdout=subprocess.PIPE,
-                                      stderr=subprocess.PIPE,
-                                      env=self._env,
-                                      close_fds=True)
+        self._sproc.stop()
+        self._sproc.start()
+        return
 
     def returncode(self):
-        return self._proc.returncode
+        return self._proc.returncode()
 
     def run_test(self, uri, timeoutms, image_hash):
-        output = []
-        error = []
-        image = ''
-        crash = False
-        timeout = False
-        actual_uri = None
-        actual_image_hash = None
-
         if uri.startswith("file:///"):
             cmd = uri[7:]
         else:
@@ -360,130 +397,51 @@ class MacDriver(base.Driver):
             cmd += "'" + image_hash
         cmd += "\n"
 
-        self._proc.stdin.write(cmd)
-        self._stdout_fd = self._proc.stdout.fileno()
-        fl = fcntl.fcntl(self._stdout_fd, fcntl.F_GETFL)
-        fcntl.fcntl(self._stdout_fd, fcntl.F_SETFL, fl | os.O_NONBLOCK)
+        # pdb.set_trace()
+        sp = self._sproc
+        sp.write(cmd)
 
-        stop_time = time.time() + (int(timeoutms) / 1000.0)
-        resp = ''
-        (timeout, line) = self._read_line(timeout, stop_time)
-        resp += line
         have_seen_content_type = False
-        while not timeout and line.rstrip() != "#EOF":
-            # Make sure we haven't crashed.
-            if line == '' and self.poll() is not None:
-                # This is hex code 0xc000001d, which is used for abrupt
-                # termination. This happens if we hit ctrl+c from the prompt
-                # and we happen to be waiting on the test_shell.
-                # sdoyon: Not sure for which OS and in what circumstances the
-                # above code is valid. What works for me under Linux to detect
-                # ctrl+c is for the subprocess returncode to be negative
-                # SIGINT. And that agrees with the subprocess documentation.
-                if (-1073741510 == self.returncode() or
-                    - signal.SIGINT == self.returncode()):
-                    raise KeyboardInterrupt
-                crash = True
-                break
+        actual_image_hash = None
+        output = ''
+        image = ''
 
-            elif (line.startswith('Content-Type:') and not
-                  have_seen_content_type):
+        timeout = int(timeoutms) / 1000.0
+        deadline = time.time() + timeout
+        line = sp.read_line(timeout)
+        while not sp.timed_out and not sp.crashed and line.rstrip() != "#EOF":
+            if (line.startswith('Content-Type:') and not
+                have_seen_content_type):
                 have_seen_content_type = True
-                pass
             else:
-                output.append(line)
-
-            (timeout, line) = self._read_line(timeout, stop_time)
-            resp += line
+                output += line
+            line = sp.read_line(timeout)
+            timeout = deadline - time.time()
 
         # Now read a second block of text for the optional image data
-        image_length = 0
-        (timeout, line) = self._read_line(timeout, stop_time)
-        resp += line
+        remaining_length = -1
         HASH_HEADER = 'ActualHash: '
         LENGTH_HEADER = 'Content-Length: '
-        while not timeout and not crash and line.rstrip() != "#EOF":
-            if line == '' and self.poll() is not None:
-                if (-1073741510 == self.returncode() or
-                    - signal.SIGINT == self.returncode()):
-                    raise KeyboardInterrupt
-                crash = True
-                break
-            elif line.startswith(HASH_HEADER):
+        line = sp.read_line(timeout)
+        while not sp.timed_out and not sp.crashed and line.rstrip() != "#EOF":
+            if line.startswith(HASH_HEADER):
                 actual_image_hash = line[len(HASH_HEADER):].strip()
             elif line.startswith('Content-Type:'):
                 pass
             elif line.startswith(LENGTH_HEADER):
-                image_length = int(line[len(LENGTH_HEADER):])
-            elif image_length:
-                image += line
-
-            (timeout, line) = self._read_line(timeout, stop_time, image_length)
-            resp += line
-
-        if timeout:
-            self.restart()
+                timeout = deadline - time.time()
+                content_length = int(line[len(LENGTH_HEADER):])
+                image = sp.read(timeout, content_length)
+            timeout = deadline - time.time()
+            line = sp.read_line(timeout)
 
         if self._image_path and len(self._image_path):
             image_file = file(self._image_path, "wb")
             image_file.write(image)
             image_file.close()
-
-        return (crash, timeout, actual_image_hash,
-                ''.join(output), ''.join(error))
+        return (sp.crashed, sp.timed_out, actual_image_hash, output, sp.error)
 
     def stop(self):
-        if self._proc:
-            self._proc.stdin.close()
-            self._proc.stdout.close()
-            if self._proc.stderr:
-                self._proc.stderr.close()
-            if sys.platform not in ('win32', 'cygwin'):
-                # Closing stdin/stdout/stderr hangs sometimes on OS X,
-                # (see restart(), above), and anyway we don't want to hang
-                # the harness if test_shell is buggy, so we wait a couple
-                # seconds to give test_shell a chance to clean up, but then
-                # force-kill the process if necessary.
-                KILL_TIMEOUT = 3.0
-                timeout = time.time() + KILL_TIMEOUT
-                while self._proc.poll() is None and time.time() < timeout:
-                    time.sleep(0.1)
-                if self._proc.poll() is None:
-                    _log.warning('stopping test driver timed out, '
-                                 'killing it')
-                    null = open(os.devnull, "w")
-                    subprocess.Popen(["kill", "-9",
-                                     str(self._proc.pid)], stderr=null)
-                    null.close()
-
-    def _read_line(self, timeout, stop_time, image_length=0):
-        now = time.time()
-        read_fds = []
-
-        # first check to see if we have a line already read or if we've
-        # read the entire image
-        if image_length and len(self._read_buffer) >= image_length:
-            out = self._read_buffer[0:image_length]
-            self._read_buffer = self._read_buffer[image_length:]
-            return (timeout, out)
-
-        idx = self._read_buffer.find('\n')
-        if not image_length and idx != -1:
-            out = self._read_buffer[0:idx + 1]
-            self._read_buffer = self._read_buffer[idx + 1:]
-            return (timeout, out)
-
-        # If we've timed out, return just what we have, if anything
-        if timeout or now >= stop_time:
-            out = self._read_buffer
-            self._read_buffer = ''
-            return (True, out)
-
-        (read_fds, write_fds, err_fds) = select.select(
-            [self._stdout_fd], [], [], stop_time - now)
-        try:
-            if timeout or len(read_fds) == 1:
-                self._read_buffer += self._proc.stdout.read()
-        except IOError, e:
-            read = []
-        return self._read_line(timeout, stop_time)
+        if self._sproc:
+            self._sproc.stop()
+            self._sproc = None
diff --git a/WebKitTools/Scripts/webkitpy/layout_tests/port/server_process.py b/WebKitTools/Scripts/webkitpy/layout_tests/port/server_process.py
new file mode 100644 (file)
index 0000000..d072587
--- /dev/null
@@ -0,0 +1,223 @@
+#!/usr/bin/env python
+# Copyright (C) 2010 Google Inc. All rights reserved.
+#
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions are
+# met:
+#
+#     * Redistributions of source code must retain the above copyright
+# notice, this list of conditions and the following disclaimer.
+#     * Redistributions in binary form must reproduce the above
+# copyright notice, this list of conditions and the following disclaimer
+# in the documentation and/or other materials provided with the
+# distribution.
+#     * Neither the Google name nor the names of its
+# contributors may be used to endorse or promote products derived from
+# this software without specific prior written permission.
+#
+# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+# "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+# LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+# A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+# OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+# DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+# THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+"""Package that implements the ServerProcess wrapper class"""
+
+import fcntl
+import logging
+import os
+import select
+import signal
+import subprocess
+import sys
+import time
+
+_log = logging.getLogger("webkitpy.layout_tests.port.server_process")
+
+
+class ServerProcess:
+    """This class provides a wrapper around a subprocess that
+    implements a simple request/response usage model. The primary benefit
+    is that reading responses takes a timeout, so that we don't ever block
+    indefinitely. The class also handles transparently restarting processes
+    as necessary to keep issuing commands."""
+
+    def __init__(self, port_obj, name, cmd, env=None):
+        self._port = port_obj
+        self._name = name
+        self._cmd = cmd
+        self._env = env
+        self._reset()
+
+    def _reset(self):
+        self._proc = None
+        self._output = ''
+        self.crashed = False
+        self.timed_out = False
+        self.error = ''
+
+    def _start(self):
+        if self._proc:
+            raise ValueError("%s already running" % self._name)
+        self._reset()
+        close_fds = sys.platform not in ('win32', 'cygwin')
+        self._proc = subprocess.Popen(self._cmd, stdin=subprocess.PIPE,
+                                      stdout=subprocess.PIPE,
+                                      stderr=subprocess.PIPE,
+                                      close_fds=close_fds,
+                                      env=self._env)
+        fd = self._proc.stdout.fileno()
+        fl = fcntl.fcntl(fd, fcntl.F_GETFL)
+        fcntl.fcntl(fd, fcntl.F_SETFL, fl | os.O_NONBLOCK)
+        fd = self._proc.stderr.fileno()
+        fl = fcntl.fcntl(fd, fcntl.F_GETFL)
+        fcntl.fcntl(fd, fcntl.F_SETFL, fl | os.O_NONBLOCK)
+
+    def handle_interrupt(self):
+        """This routine checks to see if the process crashed or exited
+        because of a keyboard interrupt and raises KeyboardInterrupt
+        accordingly."""
+        if self.crashed:
+            # This is hex code 0xc000001d, which is used for abrupt
+            # termination. This happens if we hit ctrl+c from the prompt
+            # and we happen to be waiting on the test_shell.
+            # sdoyon: Not sure for which OS and in what circumstances the
+            # above code is valid. What works for me under Linux to detect
+            # ctrl+c is for the subprocess returncode to be negative
+            # SIGINT. And that agrees with the subprocess documentation.
+            if (-1073741510 == self._proc.returncode or
+                - signal.SIGINT == self._proc.returncode):
+                raise KeyboardInterrupt
+            return
+
+    def poll(self):
+        """Check to see if the underlying process is running; returns None
+        if it still is (wrapper around subprocess.poll)."""
+        if self._proc:
+            return self._proc.poll()
+        return None
+
+    def returncode(self):
+        """Returns the exit code from the subprcoess; returns None if the
+        process hasn't exited (this is a wrapper around subprocess.returncode).
+        """
+        if self._proc:
+            return self._proc.returncode
+        return None
+
+    def write(self, input):
+        """Write a request to the subprocess. The subprocess is (re-)start()'ed
+        if is not already running."""
+        if not self._proc:
+            self._start()
+        self._proc.stdin.write(input)
+
+    def read_line(self, timeout):
+        """Read a single line from the subprocess, waiting until the deadline.
+        If the deadline passes, the call times out. Note that even if the
+        subprocess has crashed or the deadline has passed, if there is output
+        pending, it will be returned.
+
+        Args:
+            timeout: floating-point number of seconds the call is allowed
+                to block for. A zero or negative number will attempt to read
+                any existing data, but will not block. There is no way to
+                block indefinitely.
+        Returns:
+            output: data returned, if any. If no data is available and the
+                call times out or crashes, an empty string is returned. Note
+                that the returned string includes the newline ('\n')."""
+        return self._read(timeout, size=0)
+
+    def read(self, timeout, size):
+        """Attempts to read size characters from the subprocess, waiting until
+        the deadline passes. If the deadline passes, any available data will be
+        returned. Note that even if the deadline has passed or if the
+        subprocess has crashed, any available data will still be returned.
+
+        Args:
+            timeout: floating-point number of seconds the call is allowed
+                to block for. A zero or negative number will attempt to read
+                any existing data, but will not block. There is no way to
+                block indefinitely.
+            size: amount of data to read. Must be a postive integer.
+        Returns:
+            output: data returned, if any. If no data is available, an empty
+                string is returned.
+        """
+        if size <= 0:
+            raise ValueError('ServerProcess.read() called with a '
+                             'non-positive size: %d ' % size)
+        return self._read(timeout, size)
+
+    def _read(self, timeout, size):
+        """Internal routine that actually does the read."""
+        index = -1
+        out_fd = self._proc.stdout.fileno()
+        err_fd = self._proc.stderr.fileno()
+        select_fds = (out_fd, err_fd)
+        deadline = time.time() + timeout
+        while not self.timed_out and not self.crashed:
+            if self._proc.poll() != None:
+                self.crashed = True
+                self.handle_interrupt()
+
+            now = time.time()
+            if now > deadline:
+                self.timed_out = True
+
+            # Check to see if we have any output we can return.
+            if size and len(self._output) >= size:
+                index = size
+            elif size == 0:
+                index = self._output.find('\n') + 1
+
+            if index or self.crashed or self.timed_out:
+                output = self._output[0:index]
+                self._output = self._output[index:]
+                return output
+
+            # Nope - wait for more data.
+            (read_fds, write_fds, err_fds) = select.select(select_fds, [],
+                                                           select_fds,
+                                                           deadline - now)
+            try:
+                if out_fd in read_fds:
+                    self._output += self._proc.stdout.read()
+                if err_fd in read_fds:
+                    self.error += self._proc.stderr.read()
+            except IOError, e:
+                pass
+
+    def stop(self):
+        """Stop (shut down) the subprocess), if it is running."""
+        pid = self._proc.pid
+        self._proc.stdin.close()
+        self._proc.stdout.close()
+        if self._proc.stderr:
+            self._proc.stderr.close()
+        if sys.platform not in ('win32', 'cygwin'):
+            # Closing stdin/stdout/stderr hangs sometimes on OS X,
+            # (see restart(), above), and anyway we don't want to hang
+            # the harness if test_shell is buggy, so we wait a couple
+            # seconds to give test_shell a chance to clean up, but then
+            # force-kill the process if necessary.
+            KILL_TIMEOUT = 3.0
+            timeout = time.time() + KILL_TIMEOUT
+            while self._proc.poll() is None and time.time() < timeout:
+                time.sleep(0.1)
+            if self._proc.poll() is None:
+                _log.warning('stopping %s timed out, killing it' %
+                             self._name)
+                null = open(os.devnull, "w")
+                subprocess.Popen(["kill", "-9",
+                                  str(self._proc.pid)], stderr=null)
+                null.close()
+                _log.warning('killed')
+        self._reset()
index 42928ba..d9e784a 100644 (file)
@@ -90,6 +90,7 @@ class ImageDiff(test_type_base.TestTypeBase):
         Args:
           filename: the name of the test
           target: Debug or Release
+        Returns True if the files are different, False if they match
         """
         diff_filename = self.output_filename(filename,
           self.FILENAME_SUFFIX_COMPARE)
@@ -98,6 +99,7 @@ class ImageDiff(test_type_base.TestTypeBase):
         expected_filename = self.output_filename(filename,
           self.FILENAME_SUFFIX_EXPECTED + '.png')
 
+        result = True
         try:
             _compare_available = True
             result = port.diff_image(expected_filename, actual_filename,
@@ -165,18 +167,16 @@ class ImageDiff(test_type_base.TestTypeBase):
         self._copy_output_png(filename, test_args.png_path, '-actual.png')
         self._copy_output_png(filename, expected_png_file, '-expected.png')
 
-        # Even though we only use result in one codepath below but we
+        # Even though we only use the result in one codepath below but we
         # still need to call CreateImageDiff for other codepaths.
-        result = self._create_image_diff(port, filename, target)
+        images_are_different = self._create_image_diff(port, filename, target)
         if expected_hash == '':
             failures.append(test_failures.FailureMissingImageHash(self))
         elif test_args.hash != expected_hash:
-            # Hashes don't match, so see if the images match. If they do, then
-            # the hash is wrong.
-            if result == 0:
-                failures.append(test_failures.FailureImageHashIncorrect(self))
-            else:
+            if images_are_different:
                 failures.append(test_failures.FailureImageHashMismatch(self))
+            else:
+                failures.append(test_failures.FailureImageHashIncorrect(self))
 
         return failures
 
@@ -190,10 +190,7 @@ class ImageDiff(test_type_base.TestTypeBase):
           True if two files are different.
           False otherwise.
         """
-
         try:
-            result = port.diff_image(file1, file2)
+            return port.diff_image(file1, file2)
         except ValueError, e:
             return True
-
-        return result == 1