Changeset 124801 in webkit


Ignore:
Timestamp:
Aug 6, 2012 2:00:29 PM (12 years ago)
Author:
dpranke@chromium.org
Message:

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):

Location:
trunk/Tools
Files:
14 edited

Legend:

Unmodified
Added
Removed

  • trunk/Tools/ChangeLog

    r124800 r124801  
     12012-08-06  Dirk Pranke  <dpranke@chromium.org>
     2
     3        nrwt: handle errors from image diff better
     4        https://bugs.webkit.org/show_bug.cgi?id=92934
     5
     6        Reviewed by Ojan Vafai.
     7
     8        Currently if ImageDiff crashes, returns a weird exit code, or
     9        produces any stderr output, it's basically swallowed. This
     10        change ensures that we log errors to stderr, and also appends
     11        the error to the stderr for the test (so it'll show up in
     12        results.html).
     13
     14        Most importantly, it'll cause diff_image() to fail and we'll
     15        report ImageHashMismatch ... this may be kinda untrue, but I
     16        think it's better than ignoring the error.
     17
     18        * Scripts/webkitpy/layout_tests/controllers/single_test_runner.py:
     19        (SingleTestRunner._compare_image):
     20        (SingleTestRunner._compare_output_with_reference):
     21        * Scripts/webkitpy/layout_tests/controllers/test_result_writer.py:
     22        (write_test_result):
     23        * Scripts/webkitpy/layout_tests/controllers/test_result_writer_unittest.py:
     24        (TestResultWriterTest.test_reftest_diff_image.ImageDiffTestPort.diff_image):
     25        (TestResultWriterTest):
     26        * Scripts/webkitpy/layout_tests/port/base.py:
     27        (Port.diff_image):
     28        * Scripts/webkitpy/layout_tests/port/chromium.py:
     29        (ChromiumPort.diff_image):
     30        * Scripts/webkitpy/layout_tests/port/chromium_port_testcase.py:
     31        (ChromiumPortTestCase.test_diff_image_crashed):
     32        * Scripts/webkitpy/layout_tests/port/driver.py:
     33        (Driver.run_test):
     34        * Scripts/webkitpy/layout_tests/port/image_diff.py:
     35        (ImageDiffer.diff_image):
     36        (ImageDiffer._read):
     37        * Scripts/webkitpy/layout_tests/port/image_diff_unittest.py:
     38        (TestImageDiffer.test_diff_image):
     39        * Scripts/webkitpy/layout_tests/port/mock_drt_unittest.py:
     40        (MockDRTPortTest.test_diff_image_crashed):
     41        * Scripts/webkitpy/layout_tests/port/port_testcase.py:
     42        (PortTestCase.test_diff_image):
     43        (PortTestCase):
     44        (PortTestCase.test_diff_image_crashed):
     45        (PortTestCase.test_diff_image_crashed.make_proc):
     46        * Scripts/webkitpy/layout_tests/port/server_process_mock.py:
     47        (MockServerProcess.__init__):
     48        * Scripts/webkitpy/layout_tests/port/test.py:
     49        (TestPort.diff_image):
     50        * Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py:
     51        (MainTest.test_tolerance.ImageDiffTestPort.diff_image):
     52
    1532012-08-06  Dirk Pranke  <dpranke@chromium.org>
    254

  • trunk/Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py

    r123895 r124801  
    265265        elif driver_output.image_hash != expected_driver_output.image_hash:
    266266            diff_result = self._port.diff_image(driver_output.image, expected_driver_output.image)
    267             driver_output.image_diff = diff_result[0]
    268             if driver_output.image_diff:
    269                 failures.append(test_failures.FailureImageHashMismatch(diff_result[1]))
     267            err_str = diff_result[2]
     268            if err_str:
     269                _log.warning('  %s : %s' % (self._test_name, err_str))
     270                failures.append(test_failures.FailureImageHashMismatch())
     271                driver_output.error = (driver_output.error or '') + err_str
    270272            else:
    271                 # See https://bugs.webkit.org/show_bug.cgi?id=69444 for why this isn't a full failure.
    272                 _log.warning('  %s -> pixel hash failed (but pixel test still passes)' % self._test_name)
     273                driver_output.image_diff = diff_result[0]
     274                if driver_output.image_diff:
     275                    failures.append(test_failures.FailureImageHashMismatch(diff_result[1]))
     276                else:
     277                    # See https://bugs.webkit.org/show_bug.cgi?id=69444 for why this isn't a full failure.
     278                    _log.warning('  %s -> pixel hash failed (but pixel test still passes)' % self._test_name)
    273279        return failures
    274280
    … …  
    318324        elif driver_output1.image_hash != driver_output2.image_hash:
    319325            failures.append(test_failures.FailureReftestMismatch(reference_filename))
     326
     327        # recompute in case we added to stderr during diff_image
     328        has_stderr = driver_output1.has_stderr() or driver_output2.has_stderr()
     329
    320330        return TestResult(self._test_name, failures, total_test_time, has_stderr)

  • trunk/Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer.py

    r122551 r124801  
    7070            # FIXME: We should always have 2 images here.
    7171            if driver_output.image and expected_driver_output.image:
    72                 diff_image, diff_percent = port.diff_image(driver_output.image, expected_driver_output.image, tolerance=0)
     72                diff_image, diff_percent, err_str = port.diff_image(driver_output.image, expected_driver_output.image, tolerance=0)
    7373                if diff_image:
    7474                    writer.write_image_diff_files(diff_image)

  • trunk/Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer_unittest.py

    r111289 r124801  
    4343            def diff_image(self, expected_contents, actual_contents, tolerance=None):
    4444                used_tolerance_values.append(tolerance)
    45                 return (True, 1)
     45                return (True, 1, None)
    4646
    4747        host = MockHost()

  • trunk/Tools/Scripts/webkitpy/layout_tests/port/base.py

    r124762 r124801  
    316316
    317317    def diff_image(self, expected_contents, actual_contents, tolerance=None):
    318         """Compare two images and return a tuple of an image diff, and a percentage difference (0-100).
     318        """Compare two images and return a tuple of an image diff, a percentage difference (0-100), and an error string.
    319319
    320320        |tolerance| should be a percentage value (0.0 - 100.0).
    … …  
    324324        """
    325325        if not actual_contents and not expected_contents:
    326             return (None, 0)
     326            return (None, 0, None)
    327327        if not actual_contents or not expected_contents:
    328             return (True, 0)
     328            return (True, 0, None)
    329329        if not self._image_differ:
    330330            self._image_differ = image_diff.ImageDiffer(self)

  • trunk/Tools/Scripts/webkitpy/layout_tests/port/chromium.py

    r123916 r124801  
    191191
    192192    def diff_image(self, expected_contents, actual_contents, tolerance=None):
    193         # FIXME: need unit tests for this.
    194 
    195193        # tolerance is not used in chromium. Make sure caller doesn't pass tolerance other than zero or None.
    196194        assert (tolerance is None) or tolerance == 0
    … …  
    198196        # If only one of them exists, return that one.
    199197        if not actual_contents and not expected_contents:
    200             return (None, 0)
     198            return (None, 0, None)
    201199        if not actual_contents:
    202             return (expected_contents, 0)
     200            return (expected_contents, 0, None)
    203201        if not expected_contents:
    204             return (actual_contents, 0)
     202            return (actual_contents, 0, None)
    205203
    206204        tempdir = self._filesystem.mkdtemp()
    … …  
    222220
    223221        result = None
     222        err_str = None
    224223        try:
    225224            exit_code = self._executive.run_command(comand, return_exit_code=True)
    … …  
    227226                # The images are the same.
    228227                result = None
    229             elif exit_code != 1:
    230                 _log.error("image diff returned an exit code of %s" % exit_code)
    231                 # Returning None here causes the script to think that we
    232                 # successfully created the diff even though we didn't.
    233                 # FIXME: Consider raising an exception here, so that the error
    234                 # is not accidentally overlooked while the test passes.
    235                 result = None
     228            elif exit_code == 1:
     229                result = self._filesystem.read_binary_file(native_diff_filename)
     230            else:
     231                err_str = "image diff returned an exit code of %s" % exit_code
    236232        except OSError, e:
    237             if e.errno == errno.ENOENT or e.errno == errno.EACCES:
    238                 _compare_available = False
    239             else:
    240                 raise
     233            err_str = 'error running image diff: %s' % str(e)
    241234        finally:
    242             if exit_code == 1:
    243                 result = self._filesystem.read_binary_file(native_diff_filename)
    244235            self._filesystem.rmtree(str(tempdir))
    245         return (result, 0)  # FIXME: how to get % diff?
     236
     237        return (result, 0, err_str or None)  # FIXME: how to get % diff?
    246238
    247239    def path_from_chromium_base(self, *comps):

  • trunk/Tools/Scripts/webkitpy/layout_tests/port/chromium_port_testcase.py

    r123916 r124801  
    159159        self.assertFalse(exception_raised)
    160160
     161    def test_diff_image_crashed(self):
     162        port = ChromiumPortTestCase.TestLinuxPort()
     163        port._executive = MockExecutive2(exit_code=2)
     164        self.assertEquals(port.diff_image("EXPECTED", "ACTUAL"), (None, 0, 'image diff returned an exit code of 2'))
     165
    161166    def test_expectations_files(self):
    162167        port = self.make_port()

  • trunk/Tools/Scripts/webkitpy/layout_tests/port/image_diff.py

    r123893 r124801  
    6363            return self._read()
    6464        except IOError as exception:
    65             _log.error("Failed to compute an image diff: %s" % str(exception))
    66             return (True, 0)
     65            return (None, 0, "Failed to compute an image diff: %s" % str(exception))
    6766
    6867    def _start(self, tolerance):
    … …  
    7877        output_image = ""
    7978
    80         while True:
     79        while not self._process.timed_out and not self._process.has_crashed():
    8180            output = self._process.read_stdout_line(deadline)
    8281            if self._process.timed_out or self._process.has_crashed() or not output:
    … …  
    9493
    9594        stderr = self._process.pop_all_buffered_stderr()
     95        err_str = ''
    9696        if stderr:
    97             _log.warn("ImageDiff produced stderr output:\n" + stderr)
     97            err_str += "ImageDiff produced stderr output:\n" + stderr
    9898        if self._process.timed_out:
    99             _log.error("ImageDiff timed out")
     99            err_str += "ImageDiff timed out\n"
    100100        if self._process.has_crashed():
    101             _log.error("ImageDiff crashed")
     101            err_str += "ImageDiff crashed\n"
     102
    102103        # FIXME: There is no need to shut down the ImageDiff server after every diff.
    103104        self._process.stop()
    … …  
    110111            diff_percent = float(m.group(1))
    111112
    112         return (output_image, diff_percent)
     113        return (output_image, diff_percent, err_str or None)
    113114
    114115    def stop(self):

  • trunk/Tools/Scripts/webkitpy/layout_tests/port/image_diff_unittest.py

    r123893 r124801  
    5050        port = FakePort(['diff: 100% failed\n'])
    5151        image_differ = ImageDiffer(port)
    52         self.assertEquals(image_differ.diff_image('foo', 'bar', 0.1), ('', 100.0))
     52        self.assertEquals(image_differ.diff_image('foo', 'bar', 0.1), ('', 100.0, None))

  • trunk/Tools/Scripts/webkitpy/layout_tests/port/mock_drt_unittest.py

    r124503 r124801  
    6161        pass
    6262
     63    def test_diff_image_crashed(self):
     64        pass
     65
    6366    def test_uses_apache(self):
    6467        pass

  • trunk/Tools/Scripts/webkitpy/layout_tests/port/port_testcase.py

    r124503 r124801  
    260260        port._server_process_constructor = make_proc
    261261        port.setup_test_run()
    262         self.assertEquals(port.diff_image('foo', 'bar'), ('', 100.0))
     262        self.assertEquals(port.diff_image('foo', 'bar'), ('', 100.0, None))
    263263        self.assertEquals(self.proc.cmd[1:3], ["--tolerance", "0.1"])
    264264
    265         self.assertEquals(port.diff_image('foo', 'bar', None), ('', 100.0))
     265        self.assertEquals(port.diff_image('foo', 'bar', None), ('', 100.0, None))
    266266        self.assertEquals(self.proc.cmd[1:3], ["--tolerance", "0.1"])
    267267
    268         self.assertEquals(port.diff_image('foo', 'bar', 0), ('', 100.0))
     268        self.assertEquals(port.diff_image('foo', 'bar', 0), ('', 100.0, None))
    269269        self.assertEquals(self.proc.cmd[1:3], ["--tolerance", "0"])
    270270
    … …  
    272272        self.assertTrue(self.proc.stopped)
    273273        self.assertEquals(port._image_differ, None)
     274
     275    def test_diff_image_crashed(self):
     276        port = self.make_port()
     277        self.proc = None
     278
     279        def make_proc(port, nm, cmd, env):
     280            self.proc = MockServerProcess(port, nm, cmd, env, crashed=True)
     281            return self.proc
     282
     283        port._server_process_constructor = make_proc
     284        port.setup_test_run()
     285        self.assertEquals(port.diff_image('foo', 'bar'), ('', 0, 'ImageDiff crashed\n'))
     286        port.clean_up_test_run()
    274287
    275288    def test_check_wdiff(self):

  • trunk/Tools/Scripts/webkitpy/layout_tests/port/server_process_mock.py

    r123893 r124801  
    2929
    3030class MockServerProcess(object):
    31     def __init__(self, port_obj=None, name=None, cmd=None, env=None, universal_newlines=False, lines=None):
     31    def __init__(self, port_obj=None, name=None, cmd=None, env=None, universal_newlines=False, lines=None, crashed=False):
    3232        self.timed_out = False
    3333        self.lines = lines or []
    34         self.crashed = False
     34        self.crashed = crashed
    3535        self.writes = []
    3636        self.cmd = cmd

  • trunk/Tools/Scripts/webkitpy/layout_tests/port/test.py

    r124581 r124801  
    403403        diffed = actual_contents != expected_contents
    404404        if diffed:
    405             return ["< %s\n---\n> %s\n" % (expected_contents, actual_contents), 1]
    406         return (None, 0)
     405            return ("< %s\n---\n> %s\n" % (expected_contents, actual_contents), 1, None)
     406        return (None, 0, None)
    407407
    408408    def layout_tests_dir(self):

  • trunk/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py

    r124800 r124801  
    784784            def diff_image(self, expected_contents, actual_contents, tolerance=None):
    785785                self.tolerance_used_for_diff_image = self._options.tolerance
    786                 return (True, 1)
     786                return (True, 1, None)
    787787
    788788        def get_port_for_run(args):
Note: See TracChangeset for help on using the changeset viewer.