nrwt: handle errors from image diff better
authordpranke@chromium.org <dpranke@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 6 Aug 2012 21:00:29 +0000 (21:00 +0000)
committerdpranke@chromium.org <dpranke@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 6 Aug 2012 21:00:29 +0000 (21:00 +0000)
https://bugs.webkit.org/show_bug.cgi?id=92934

Reviewed by Ojan Vafai.

Currently if ImageDiff crashes, returns a weird exit code, or
produces any stderr output, it's basically swallowed. This
change ensures that we log errors to stderr, and also appends
the error to the stderr for the test (so it'll show up in
results.html).

Most importantly, it'll cause diff_image() to fail and we'll
report ImageHashMismatch ... this may be kinda untrue, but I
think it's better than ignoring the error.

* Scripts/webkitpy/layout_tests/controllers/single_test_runner.py:
(SingleTestRunner._compare_image):
(SingleTestRunner._compare_output_with_reference):
* Scripts/webkitpy/layout_tests/controllers/test_result_writer.py:
(write_test_result):
* Scripts/webkitpy/layout_tests/controllers/test_result_writer_unittest.py:
(TestResultWriterTest.test_reftest_diff_image.ImageDiffTestPort.diff_image):
(TestResultWriterTest):
* Scripts/webkitpy/layout_tests/port/base.py:
(Port.diff_image):
* Scripts/webkitpy/layout_tests/port/chromium.py:
(ChromiumPort.diff_image):
* Scripts/webkitpy/layout_tests/port/chromium_port_testcase.py:
(ChromiumPortTestCase.test_diff_image_crashed):
* Scripts/webkitpy/layout_tests/port/driver.py:
(Driver.run_test):
* Scripts/webkitpy/layout_tests/port/image_diff.py:
(ImageDiffer.diff_image):
(ImageDiffer._read):
* Scripts/webkitpy/layout_tests/port/image_diff_unittest.py:
(TestImageDiffer.test_diff_image):
* Scripts/webkitpy/layout_tests/port/mock_drt_unittest.py:
(MockDRTPortTest.test_diff_image_crashed):
* Scripts/webkitpy/layout_tests/port/port_testcase.py:
(PortTestCase.test_diff_image):
(PortTestCase):
(PortTestCase.test_diff_image_crashed):
(PortTestCase.test_diff_image_crashed.make_proc):
* Scripts/webkitpy/layout_tests/port/server_process_mock.py:
(MockServerProcess.__init__):
* Scripts/webkitpy/layout_tests/port/test.py:
(TestPort.diff_image):
* Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py:
(MainTest.test_tolerance.ImageDiffTestPort.diff_image):

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

14 files changed:
Tools/ChangeLog
Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py
Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer.py
Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer_unittest.py
Tools/Scripts/webkitpy/layout_tests/port/base.py
Tools/Scripts/webkitpy/layout_tests/port/chromium.py
Tools/Scripts/webkitpy/layout_tests/port/chromium_port_testcase.py
Tools/Scripts/webkitpy/layout_tests/port/image_diff.py
Tools/Scripts/webkitpy/layout_tests/port/image_diff_unittest.py
Tools/Scripts/webkitpy/layout_tests/port/mock_drt_unittest.py
Tools/Scripts/webkitpy/layout_tests/port/port_testcase.py
Tools/Scripts/webkitpy/layout_tests/port/server_process_mock.py
Tools/Scripts/webkitpy/layout_tests/port/test.py
Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py

index 050e8af..3432e3d 100644 (file)
@@ -1,5 +1,57 @@
 2012-08-06  Dirk Pranke  <dpranke@chromium.org>
 
+        nrwt: handle errors from image diff better
+        https://bugs.webkit.org/show_bug.cgi?id=92934
+
+        Reviewed by Ojan Vafai.
+
+        Currently if ImageDiff crashes, returns a weird exit code, or
+        produces any stderr output, it's basically swallowed. This
+        change ensures that we log errors to stderr, and also appends
+        the error to the stderr for the test (so it'll show up in
+        results.html).
+
+        Most importantly, it'll cause diff_image() to fail and we'll
+        report ImageHashMismatch ... this may be kinda untrue, but I
+        think it's better than ignoring the error.
+
+        * Scripts/webkitpy/layout_tests/controllers/single_test_runner.py:
+        (SingleTestRunner._compare_image):
+        (SingleTestRunner._compare_output_with_reference):
+        * Scripts/webkitpy/layout_tests/controllers/test_result_writer.py:
+        (write_test_result):
+        * Scripts/webkitpy/layout_tests/controllers/test_result_writer_unittest.py:
+        (TestResultWriterTest.test_reftest_diff_image.ImageDiffTestPort.diff_image):
+        (TestResultWriterTest):
+        * Scripts/webkitpy/layout_tests/port/base.py:
+        (Port.diff_image):
+        * Scripts/webkitpy/layout_tests/port/chromium.py:
+        (ChromiumPort.diff_image):
+        * Scripts/webkitpy/layout_tests/port/chromium_port_testcase.py:
+        (ChromiumPortTestCase.test_diff_image_crashed):
+        * Scripts/webkitpy/layout_tests/port/driver.py:
+        (Driver.run_test):
+        * Scripts/webkitpy/layout_tests/port/image_diff.py:
+        (ImageDiffer.diff_image):
+        (ImageDiffer._read):
+        * Scripts/webkitpy/layout_tests/port/image_diff_unittest.py:
+        (TestImageDiffer.test_diff_image):
+        * Scripts/webkitpy/layout_tests/port/mock_drt_unittest.py:
+        (MockDRTPortTest.test_diff_image_crashed):
+        * Scripts/webkitpy/layout_tests/port/port_testcase.py:
+        (PortTestCase.test_diff_image):
+        (PortTestCase):
+        (PortTestCase.test_diff_image_crashed):
+        (PortTestCase.test_diff_image_crashed.make_proc):
+        * Scripts/webkitpy/layout_tests/port/server_process_mock.py:
+        (MockServerProcess.__init__):
+        * Scripts/webkitpy/layout_tests/port/test.py:
+        (TestPort.diff_image):
+        * Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py:
+        (MainTest.test_tolerance.ImageDiffTestPort.diff_image):
+
+2012-08-06  Dirk Pranke  <dpranke@chromium.org>
+
         nrwt: clean up printing.py
         https://bugs.webkit.org/show_bug.cgi?id=93026
 
index 88cbabf..b36130d 100644 (file)
@@ -264,12 +264,18 @@ class SingleTestRunner(object):
             failures.append(test_failures.FailureMissingImageHash())
         elif driver_output.image_hash != expected_driver_output.image_hash:
             diff_result = self._port.diff_image(driver_output.image, expected_driver_output.image)
-            driver_output.image_diff = diff_result[0]
-            if driver_output.image_diff:
-                failures.append(test_failures.FailureImageHashMismatch(diff_result[1]))
+            err_str = diff_result[2]
+            if err_str:
+                _log.warning('  %s : %s' % (self._test_name, err_str))
+                failures.append(test_failures.FailureImageHashMismatch())
+                driver_output.error = (driver_output.error or '') + err_str
             else:
-                # See https://bugs.webkit.org/show_bug.cgi?id=69444 for why this isn't a full failure.
-                _log.warning('  %s -> pixel hash failed (but pixel test still passes)' % self._test_name)
+                driver_output.image_diff = diff_result[0]
+                if driver_output.image_diff:
+                    failures.append(test_failures.FailureImageHashMismatch(diff_result[1]))
+                else:
+                    # See https://bugs.webkit.org/show_bug.cgi?id=69444 for why this isn't a full failure.
+                    _log.warning('  %s -> pixel hash failed (but pixel test still passes)' % self._test_name)
         return failures
 
     def _run_reftest(self):
@@ -317,4 +323,8 @@ class SingleTestRunner(object):
                 failures.append(test_failures.FailureReftestMismatchDidNotOccur(reference_filename))
         elif driver_output1.image_hash != driver_output2.image_hash:
             failures.append(test_failures.FailureReftestMismatch(reference_filename))
+
+        # recompute in case we added to stderr during diff_image
+        has_stderr = driver_output1.has_stderr() or driver_output2.has_stderr()
+
         return TestResult(self._test_name, failures, total_test_time, has_stderr)
index 243a11d..67f42e3 100644 (file)
@@ -69,7 +69,7 @@ def write_test_result(filesystem, port, test_name, driver_output,
             # FIXME: This work should be done earlier in the pipeline (e.g., when we compare images for non-ref tests).
             # FIXME: We should always have 2 images here.
             if driver_output.image and expected_driver_output.image:
-                diff_image, diff_percent = port.diff_image(driver_output.image, expected_driver_output.image, tolerance=0)
+                diff_image, diff_percent, err_str = port.diff_image(driver_output.image, expected_driver_output.image, tolerance=0)
                 if diff_image:
                     writer.write_image_diff_files(diff_image)
                     failure.diff_percent = diff_percent
index 3b9b522..dfd6041 100644 (file)
@@ -42,7 +42,7 @@ class TestResultWriterTest(unittest.TestCase):
         class ImageDiffTestPort(TestPort):
             def diff_image(self, expected_contents, actual_contents, tolerance=None):
                 used_tolerance_values.append(tolerance)
-                return (True, 1)
+                return (True, 1, None)
 
         host = MockHost()
         port = ImageDiffTestPort(host)
index 31a9ed4..f0207c7 100755 (executable)
@@ -315,7 +315,7 @@ class Port(object):
         return expected_audio != actual_audio
 
     def diff_image(self, expected_contents, actual_contents, tolerance=None):
-        """Compare two images and return a tuple of an image diff, and a percentage difference (0-100).
+        """Compare two images and return a tuple of an image diff, a percentage difference (0-100), and an error string.
 
         |tolerance| should be a percentage value (0.0 - 100.0).
         If it is omitted, the port default tolerance value is used.
@@ -323,9 +323,9 @@ class Port(object):
         If an error occurs (like ImageDiff isn't found, or crashes, we log an error and return True (for a diff).
         """
         if not actual_contents and not expected_contents:
-            return (None, 0)
+            return (None, 0, None)
         if not actual_contents or not expected_contents:
-            return (True, 0)
+            return (True, 0, None)
         if not self._image_differ:
             self._image_differ = image_diff.ImageDiffer(self)
         self.set_option_default('tolerance', 0.1)
index b72783c..38ce4b1 100755 (executable)
@@ -190,18 +190,16 @@ class ChromiumPort(Port):
                                        override_step, logging)
 
     def diff_image(self, expected_contents, actual_contents, tolerance=None):
-        # FIXME: need unit tests for this.
-
         # tolerance is not used in chromium. Make sure caller doesn't pass tolerance other than zero or None.
         assert (tolerance is None) or tolerance == 0
 
         # If only one of them exists, return that one.
         if not actual_contents and not expected_contents:
-            return (None, 0)
+            return (None, 0, None)
         if not actual_contents:
-            return (expected_contents, 0)
+            return (expected_contents, 0, None)
         if not expected_contents:
-            return (actual_contents, 0)
+            return (actual_contents, 0, None)
 
         tempdir = self._filesystem.mkdtemp()
 
@@ -221,28 +219,22 @@ class ChromiumPort(Port):
         comand = [executable, '--diff', native_actual_filename, native_expected_filename, native_diff_filename]
 
         result = None
+        err_str = None
         try:
             exit_code = self._executive.run_command(comand, return_exit_code=True)
             if exit_code == 0:
                 # The images are the same.
                 result = None
-            elif exit_code != 1:
-                _log.error("image diff returned an exit code of %s" % exit_code)
-                # Returning None here causes the script to think that we
-                # successfully created the diff even though we didn't.
-                # FIXME: Consider raising an exception here, so that the error
-                # is not accidentally overlooked while the test passes.
-                result = None
-        except OSError, e:
-            if e.errno == errno.ENOENT or e.errno == errno.EACCES:
-                _compare_available = False
+            elif exit_code == 1:
+                result = self._filesystem.read_binary_file(native_diff_filename)
             else:
-                raise
+                err_str = "image diff returned an exit code of %s" % exit_code
+        except OSError, e:
+            err_str = 'error running image diff: %s' % str(e)
         finally:
-            if exit_code == 1:
-                result = self._filesystem.read_binary_file(native_diff_filename)
             self._filesystem.rmtree(str(tempdir))
-        return (result, 0)  # FIXME: how to get % diff?
+
+        return (result, 0, err_str or None)  # FIXME: how to get % diff?
 
     def path_from_chromium_base(self, *comps):
         """Returns the full path to path made by joining the top of the
index 7d4c235..fb90d1b 100644 (file)
@@ -158,6 +158,11 @@ class ChromiumPortTestCase(port_testcase.PortTestCase):
             exception_raised = True
         self.assertFalse(exception_raised)
 
+    def test_diff_image_crashed(self):
+        port = ChromiumPortTestCase.TestLinuxPort()
+        port._executive = MockExecutive2(exit_code=2)
+        self.assertEquals(port.diff_image("EXPECTED", "ACTUAL"), (None, 0, 'image diff returned an exit code of 2'))
+
     def test_expectations_files(self):
         port = self.make_port()
         port.port_name = 'chromium'
index 2cccc1f..c6c9efb 100644 (file)
@@ -62,8 +62,7 @@ class ImageDiffer(object):
                 len(expected_contents), expected_contents))
             return self._read()
         except IOError as exception:
-            _log.error("Failed to compute an image diff: %s" % str(exception))
-            return (True, 0)
+            return (None, 0, "Failed to compute an image diff: %s" % str(exception))
 
     def _start(self, tolerance):
         command = [self._port._path_to_image_diff(), '--tolerance', str(tolerance)]
@@ -77,7 +76,7 @@ class ImageDiffer(object):
         output = None
         output_image = ""
 
-        while True:
+        while not self._process.timed_out and not self._process.has_crashed():
             output = self._process.read_stdout_line(deadline)
             if self._process.timed_out or self._process.has_crashed() or not output:
                 break
@@ -93,12 +92,14 @@ class ImageDiffer(object):
                 break
 
         stderr = self._process.pop_all_buffered_stderr()
+        err_str = ''
         if stderr:
-            _log.warn("ImageDiff produced stderr output:\n" + stderr)
+            err_str += "ImageDiff produced stderr output:\n" + stderr
         if self._process.timed_out:
-            _log.error("ImageDiff timed out")
+            err_str += "ImageDiff timed out\n"
         if self._process.has_crashed():
-            _log.error("ImageDiff crashed")
+            err_str += "ImageDiff crashed\n"
+
         # FIXME: There is no need to shut down the ImageDiff server after every diff.
         self._process.stop()
 
@@ -109,7 +110,7 @@ class ImageDiffer(object):
                 return [None, 0]
             diff_percent = float(m.group(1))
 
-        return (output_image, diff_percent)
+        return (output_image, diff_percent, err_str or None)
 
     def stop(self):
         if self._process:
index b06756c..1d7a62b 100755 (executable)
@@ -49,4 +49,4 @@ class TestImageDiffer(unittest.TestCase):
     def test_diff_image(self):
         port = FakePort(['diff: 100% failed\n'])
         image_differ = ImageDiffer(port)
-        self.assertEquals(image_differ.diff_image('foo', 'bar', 0.1), ('', 100.0))
+        self.assertEquals(image_differ.diff_image('foo', 'bar', 0.1), ('', 100.0, None))
index aa79e1e..334db42 100755 (executable)
@@ -60,6 +60,9 @@ class MockDRTPortTest(port_testcase.PortTestCase):
     def test_diff_image(self):
         pass
 
+    def test_diff_image_crashed(self):
+        pass
+
     def test_uses_apache(self):
         pass
 
index 37adc42..acc516d 100755 (executable)
@@ -259,19 +259,32 @@ class PortTestCase(unittest.TestCase):
 
         port._server_process_constructor = make_proc
         port.setup_test_run()
-        self.assertEquals(port.diff_image('foo', 'bar'), ('', 100.0))
+        self.assertEquals(port.diff_image('foo', 'bar'), ('', 100.0, None))
         self.assertEquals(self.proc.cmd[1:3], ["--tolerance", "0.1"])
 
-        self.assertEquals(port.diff_image('foo', 'bar', None), ('', 100.0))
+        self.assertEquals(port.diff_image('foo', 'bar', None), ('', 100.0, None))
         self.assertEquals(self.proc.cmd[1:3], ["--tolerance", "0.1"])
 
-        self.assertEquals(port.diff_image('foo', 'bar', 0), ('', 100.0))
+        self.assertEquals(port.diff_image('foo', 'bar', 0), ('', 100.0, None))
         self.assertEquals(self.proc.cmd[1:3], ["--tolerance", "0"])
 
         port.clean_up_test_run()
         self.assertTrue(self.proc.stopped)
         self.assertEquals(port._image_differ, None)
 
+    def test_diff_image_crashed(self):
+        port = self.make_port()
+        self.proc = None
+
+        def make_proc(port, nm, cmd, env):
+            self.proc = MockServerProcess(port, nm, cmd, env, crashed=True)
+            return self.proc
+
+        port._server_process_constructor = make_proc
+        port.setup_test_run()
+        self.assertEquals(port.diff_image('foo', 'bar'), ('', 0, 'ImageDiff crashed\n'))
+        port.clean_up_test_run()
+
     def test_check_wdiff(self):
         port = self.make_port()
         port.check_wdiff()
index ae48523..d234ebd 100644 (file)
 
 
 class MockServerProcess(object):
-    def __init__(self, port_obj=None, name=None, cmd=None, env=None, universal_newlines=False, lines=None):
+    def __init__(self, port_obj=None, name=None, cmd=None, env=None, universal_newlines=False, lines=None, crashed=False):
         self.timed_out = False
         self.lines = lines or []
-        self.crashed = False
+        self.crashed = crashed
         self.writes = []
         self.cmd = cmd
         self.env = env
index 0cd171f..6946c48 100644 (file)
@@ -402,8 +402,8 @@ class TestPort(Port):
     def diff_image(self, expected_contents, actual_contents, tolerance=None):
         diffed = actual_contents != expected_contents
         if diffed:
-            return ["< %s\n---\n> %s\n" % (expected_contents, actual_contents), 1]
-        return (None, 0)
+            return ("< %s\n---\n> %s\n" % (expected_contents, actual_contents), 1, None)
+        return (None, 0, None)
 
     def layout_tests_dir(self):
         return LAYOUT_TEST_DIR
index 162a079..e90f936 100755 (executable)
@@ -783,7 +783,7 @@ class MainTest(unittest.TestCase, StreamTestingMixin):
         class ImageDiffTestPort(TestPort):
             def diff_image(self, expected_contents, actual_contents, tolerance=None):
                 self.tolerance_used_for_diff_image = self._options.tolerance
-                return (True, 1)
+                return (True, 1, None)
 
         def get_port_for_run(args):
             options, parsed_args = run_webkit_tests.parse_args(args)