Changeset 124958 in webkit


Ignore:
Timestamp:
Aug 7, 2012 6:08:48 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.

Re-land the change in r124801 with a fix ... in the case where
the ImageDiff is passed a tolerance and passes the fuzzy check,
we were returning the wrong value (missing an empty error
string) and crashing; this patch fixes that and adds a test for
that case (TestImageDiffer.test_image_diff_passed).

  • 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/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.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

    r124957 r124958  
     12012-08-07  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        Re-land the change in r124801 with a fix ... in the case where
     9        the ImageDiff is passed a tolerance and passes the fuzzy check,
     10        we were returning the wrong value (missing an empty error
     11        string) and crashing; this patch fixes that and adds a test for
     12        that case (TestImageDiffer.test_image_diff_passed).
     13
     14        * Scripts/webkitpy/layout_tests/controllers/single_test_runner.py:
     15        (SingleTestRunner._compare_image):
     16        (SingleTestRunner._compare_output_with_reference):
     17        * Scripts/webkitpy/layout_tests/controllers/test_result_writer.py:
     18        (write_test_result):
     19        * Scripts/webkitpy/layout_tests/controllers/test_result_writer_unittest.py:
     20        (TestResultWriterTest.test_reftest_diff_image.ImageDiffTestPort.diff_image):
     21        (TestResultWriterTest):
     22        * Scripts/webkitpy/layout_tests/port/base.py:
     23        (Port.diff_image):
     24        * Scripts/webkitpy/layout_tests/port/chromium.py:
     25        (ChromiumPort.diff_image):
     26        * Scripts/webkitpy/layout_tests/port/chromium_port_testcase.py:
     27        (ChromiumPortTestCase.test_diff_image_crashed):
     28        * Scripts/webkitpy/layout_tests/port/image_diff.py:
     29        (ImageDiffer.diff_image):
     30        (ImageDiffer._read):
     31        * Scripts/webkitpy/layout_tests/port/image_diff_unittest.py:
     32        (TestImageDiffer.test_diff_image):
     33        * Scripts/webkitpy/layout_tests/port/mock_drt_unittest.py:
     34        (MockDRTPortTest.test_diff_image_crashed):
     35        * Scripts/webkitpy/layout_tests/port/port_testcase.py:
     36        (PortTestCase.test_diff_image):
     37        (PortTestCase.test_diff_image_crashed):
     38        (PortTestCase.test_diff_image_crashed.make_proc):
     39        * Scripts/webkitpy/layout_tests/port/server_process_mock.py:
     40        (MockServerProcess.__init__):
     41        * Scripts/webkitpy/layout_tests/port/test.py:
     42        (TestPort.diff_image):
     43        * Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py:
     44        (MainTest.test_tolerance.ImageDiffTestPort.diff_image):
     45
    1462012-08-07  Dirk Pranke  <dpranke@chromium.org>
    247

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

    r124854 r124958  
    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

    r124854 r124958  
    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

    r124854 r124958  
    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

    r124957 r124958  
    320320
    321321    def diff_image(self, expected_contents, actual_contents, tolerance=None):
    322         """Compare two images and return a tuple of an image diff, and a percentage difference (0-100).
     322        """Compare two images and return a tuple of an image diff, a percentage difference (0-100), and an error string.
    323323
    324324        |tolerance| should be a percentage value (0.0 - 100.0).
    … …  
    328328        """
    329329        if not actual_contents and not expected_contents:
    330             return (None, 0)
     330            return (None, 0, None)
    331331        if not actual_contents or not expected_contents:
    332             return (True, 0)
     332            return (True, 0, None)
    333333        if not self._image_differ:
    334334            self._image_differ = image_diff.ImageDiffer(self)

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

    r124854 r124958  
    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

    r124854 r124958  
    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

    r124854 r124958  
    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()
    … …  
    107108            m = re.match('diff: (.+)% (passed|failed)', output)
    108109            if m.group(2) == 'passed':
    109                 return [None, 0]
     110                return (None, 0, None)
    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

    r124854 r124958  
    4747
    4848class TestImageDiffer(unittest.TestCase):
    49     def test_diff_image(self):
     49    def test_diff_image_failed(self):
    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))
     53
     54    def test_diff_image_passed(self):
     55        port = FakePort(['diff: 0% passed\n'])
     56        image_differ = ImageDiffer(port)
     57        self.assertEquals(image_differ.diff_image('foo', 'bar', 0.1), (None, 0, None))

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

    r124957 r124958  
    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

    r124957 r124958  
    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

    r124854 r124958  
    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

    r124862 r124958  
    407407        diffed = actual_contents != expected_contents
    408408        if diffed:
    409             return ["< %s\n---\n> %s\n" % (expected_contents, actual_contents), 1]
    410         return (None, 0)
     409            return ("< %s\n---\n> %s\n" % (expected_contents, actual_contents), 1, None)
     410        return (None, 0, None)
    411411
    412412    def layout_tests_dir(self):

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

    r124870 r124958  
    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.