⚠ Archived content — this site is no longer maintained.   Current WebKit documentation is at docs.webkit.org.

Changeset 284845 in webkit


Ignore:
Timestamp:
Oct 25, 2021, 4:25:03 PM (5 years ago)
Author:
Simon Fraser
Message:

webkitpy: Add an explicit ImageDiffResult.passed rather than relying on diff_image being non-None
​https://bugs.webkit.org/show_bug.cgi?id=232266

Reviewed by Jonathan Bedard.

A ref test was considered failing if it ImageDiffResult.diff_image was not None, allowing
for confusing unit tests that used a value of True for diff_image.

Fix by giving ImageDiffResult an explicit 'passes' field, and testing it rather than
diff_image.

Also used named arguments when creating ImageDiffResults for readability.

  • Scripts/webkitpy/layout_tests/controllers/single_test_runner.py:

(SingleTestRunner._compare_image): Use diff_result.passed as truth for whether the test passed,
and only store the error_string in failure cases (which allows ImageDiff to spew diagnostic errors
without triggering failures).
(SingleTestRunner._compare_output_with_reference):

  • Scripts/webkitpy/layout_tests/controllers/test_result_writer_unittest.py:

(TestResultWriterTest.test_reftest_diff_image.ImageDiffTestPort.diff_image):
(TestResultWriterTest):

  • Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py:

(RunTest.test_tolerance.ImageDiffTestPort.diff_image):

  • Scripts/webkitpy/port/base.py:

(Port.diff_image):

  • Scripts/webkitpy/port/image_diff.py:

(ImageDiffResult.init):
(ImageDiffResult.eq):
(ImageDiffResult.repr):
(ImageDiffer._read):

  • Scripts/webkitpy/port/port_testcase.py:

(PortTestCase.integration_test_image_diff):
(PortTestCase.test_diff_imagemissing_both):
(PortTestCase.test_diff_imagemissing_actual):
(PortTestCase.test_diff_imagemissing_expected):
(PortTestCase.test_diff_image):
(PortTestCase.test_diff_image_passed):
(PortTestCase.test_diff_image_failed):
(PortTestCase.test_diff_image_crashed):

  • Scripts/webkitpy/port/test.py:
Location:
trunk/Tools
Files:
8 edited

Legend:

Unmodified
Added
Removed

  • trunk/Tools/ChangeLog

    r284816 r284845  
     12021-10-25  Simon Fraser  <simon.fraser@apple.com>
     2
     3        webkitpy: Add an explicit ImageDiffResult.passed rather than relying on diff_image being non-None
     4        https://bugs.webkit.org/show_bug.cgi?id=232266
     5
     6        Reviewed by Jonathan Bedard.
     7       
     8        A ref test was considered failing if it ImageDiffResult.diff_image was not None, allowing
     9        for confusing unit tests that used a value of True for diff_image.
     10
     11        Fix by giving ImageDiffResult an explicit 'passes' field, and testing it rather than
     12        diff_image.
     13
     14        Also used named arguments when creating ImageDiffResults for readability.
     15
     16        * Scripts/webkitpy/layout_tests/controllers/single_test_runner.py:
     17        (SingleTestRunner._compare_image): Use `diff_result.passed` as truth for whether the test passed,
     18        and only store the error_string in failure cases (which allows ImageDiff to spew diagnostic errors
     19        without triggering failures).
     20        (SingleTestRunner._compare_output_with_reference):
     21        * Scripts/webkitpy/layout_tests/controllers/test_result_writer_unittest.py:
     22        (TestResultWriterTest.test_reftest_diff_image.ImageDiffTestPort.diff_image):
     23        (TestResultWriterTest):
     24        * Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py:
     25        (RunTest.test_tolerance.ImageDiffTestPort.diff_image):
     26        * Scripts/webkitpy/port/base.py:
     27        (Port.diff_image):
     28        * Scripts/webkitpy/port/image_diff.py:
     29        (ImageDiffResult.__init__):
     30        (ImageDiffResult.__eq__):
     31        (ImageDiffResult.__repr__):
     32        (ImageDiffer._read):
     33        * Scripts/webkitpy/port/port_testcase.py:
     34        (PortTestCase.integration_test_image_diff):
     35        (PortTestCase.test_diff_image__missing_both):
     36        (PortTestCase.test_diff_image__missing_actual):
     37        (PortTestCase.test_diff_image__missing_expected):
     38        (PortTestCase.test_diff_image):
     39        (PortTestCase.test_diff_image_passed):
     40        (PortTestCase.test_diff_image_failed):
     41        (PortTestCase.test_diff_image_crashed):
     42        * Scripts/webkitpy/port/test.py:
     43
    1442021-10-25  Alex Christensen  <achristensen@webkit.org>
    245

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

    r284785 r284845  
    295295        elif driver_output.image_hash != expected_driver_output.image_hash:
    296296            diff_result = self._port.diff_image(expected_driver_output.image, driver_output.image)
    297             if diff_result.error_string:
    298                 _log.warning('  %s : %s' % (self._test_name, diff_result.error_string))
    299                 failures.append(test_failures.FailureImageHashMismatch())
    300                 driver_output.error = (driver_output.error or '') + diff_result.error_string
     297            if not diff_result.passed:
     298                failures.append(test_failures.FailureImageHashMismatch(diff_result.diff_percent))
     299                if diff_result.error_string:
     300                    _log.warning('  %s : %s' % (self._test_name, diff_result.error_string))
     301                    driver_output.error = (driver_output.error or '') + diff_result.error_string
     302                driver_output.image_diff = diff_result.diff_image
    301303            else:
    302                 driver_output.image_diff = diff_result.diff_image
    303                 if driver_output.image_diff:
    304                     failures.append(test_failures.FailureImageHashMismatch(diff_result.diff_percent))
    305                 else:
    306                     # See https://bugs.webkit.org/show_bug.cgi?id=69444 for why this isn't a full failure.
    307                     _log.warning('  %s -> pixel hash failed (but diff passed)' % self._test_name)
     304                # See https://bugs.webkit.org/show_bug.cgi?id=69444 for why this isn't a full failure.
     305                _log.warning('  %s -> pixel hash failed (but diff passed)' % self._test_name)
    308306        return failures
    309307
    … …  
    358356            # ImageDiff has a hard coded color distance threshold even though tolerance=0 is specified.
    359357            diff_result = self._port.diff_image(reference_driver_output.image, actual_driver_output.image, tolerance=0)
    360             if diff_result.error_string:
    361                 _log.warning('  %s : %s' % (self._test_name, diff_result.error_string))
     358            if not diff_result.passed:
    362359                failures.append(test_failures.FailureReftestMismatch(reference_filename))
    363                 actual_driver_output.error = (actual_driver_output.error or '') + diff_result.error_string
    364             elif diff_result.diff_image:
    365                 failures.append(test_failures.FailureReftestMismatch(reference_filename))
     360                if diff_result.error_string:
     361                    _log.warning('  %s : %s' % (self._test_name, diff_result.error_string))
     362                    actual_driver_output.error = (actual_driver_output.error or '') + diff_result.error_string
    366363
    367364        return TestResult(self._test_input, failures, total_test_time, has_stderr, pid=actual_driver_output.pid)

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

    r284785 r284845  
    4343            def diff_image(self, expected_contents, actual_contents, tolerance=None):
    4444                used_tolerance_values.append(tolerance)
    45                 return ImageDiffResult(True, 1, None)
     45                return ImageDiffResult(passed=False, diff_image=b'', difference=1)
    4646
    4747        host = MockHost()

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

    r284785 r284845  
    753753            def diff_image(self, expected_contents, actual_contents, tolerance=None):
    754754                self.tolerance_used_for_diff_image = self._options.tolerance
    755                 return ImageDiffResult(True, 1, None)
     755                return ImageDiffResult(passed=False, diff_image=b'', difference=1)
    756756
    757757        def get_port_for_run(args):

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

    r284785 r284845  
    308308        """
    309309        if not actual_contents and not expected_contents:
    310             return ImageDiffResult(None, 0, None)
     310            return ImageDiffResult(passed=True, diff_image=None, difference=0)
    311311
    312312        if not actual_contents or not expected_contents:
    313             return ImageDiffResult(True, 0, None)
     313            return ImageDiffResult(passed=False, diff_image=b'', difference=1)
    314314
    315315        if not self._image_differ:

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

    r284785 r284845  
    4141
    4242class ImageDiffResult(object):
    43     def __init__(self, diff_image, difference, error_string):
     43    def __init__(self, passed, diff_image, difference, error_string=None):
     44        self.passed = passed
    4445        self.diff_image = diff_image
    4546        self.diff_percent = difference
    … …  
    4849    def __eq__(self, other):
    4950        if isinstance(other, self.__class__):
    50             return (self.diff_image == other.diff_image and
     51            return (self.passed == other.passed and
     52                    self.diff_image == other.diff_image and
    5153                    self.diff_percent == other.diff_percent and
    5254                    self.error_string == other.error_string)
    … …  
    5860
    5961    def __repr__(self):
    60         return 'ImageDiffResult({} {} {})'.format(self.diff_image, self.diff_percent, self.error_string)
     62        return 'ImageDiffResult(Passed {} {} {} {})'.format(self.passed, self.diff_image, self.diff_percent, self.error_string)
    6163
    6264class ImageDiffer(object):
    … …  
    129131            m = re.match(b'diff: (.+)% (passed|failed)', output)
    130132            if m.group(2) == b'passed':
    131                 return ImageDiffResult(None, 0, None)
     133                return ImageDiffResult(passed=True, diff_image=None, difference=0)
    132134            diff_percent = float(string_utils.decode(m.group(1), target_type=str))
    133135
    134         return ImageDiffResult(output_image, diff_percent, err_str or None)
     136        return ImageDiffResult(passed=False, diff_image=output_image, difference=diff_percent, error_string=err_str or None)
    135137
    136138    def stop(self):

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

    r284785 r284845  
    250250        file1 = port._filesystem.join(dir, 'fast', 'css', 'button_center.png')
    251251        contents1 = port._filesystem.read_binary_file(file1)
    252         file2 = port._filesystem.join(dir, 'fast', 'css',
    253                                       'remove-shorthand-expected.png')
     252        file2 = port._filesystem.join(dir, 'fast', 'css', 'remove-shorthand-expected.png')
    254253        contents2 = port._filesystem.read_binary_file(file2)
    255254        tmpfd, tmpfile = port._filesystem.open_binary_tempfile('')
    256255        tmpfd.close()
    257256
    258         self.assertFalse(port.diff_image(contents1, contents1).diff_image)
    259         self.assertTrue(port.diff_image(contents1, contents2).diff_image)
    260 
    261         self.assertTrue(port.diff_image(contents1, contents2, tmpfile).diff_image)
     257        self.assertTrue(port.diff_image(contents1, contents1).passed)
     258        self.assertFalse(port.diff_image(contents1, contents2).passed)
     259
     260        self.assertFalse(port.diff_image(contents1, contents2, tmpfile).passed)
    262261
    263262        port._filesystem.remove(tmpfile)
    … …  
    265264    def test_diff_image__missing_both(self):
    266265        port = self.make_port()
    267         self.assertFalse(port.diff_image(None, None).diff_image)
    268         self.assertFalse(port.diff_image(None, b'').diff_image)
    269         self.assertFalse(port.diff_image(b'', None).diff_image)
    270 
    271         self.assertFalse(port.diff_image(b'', b'').diff_image)
     266        self.assertTrue(port.diff_image(None, None).passed)
     267        self.assertTrue(port.diff_image(None, b'').passed)
     268        self.assertTrue(port.diff_image(b'', None).passed)
     269
     270        self.assertTrue(port.diff_image(b'', b'').passed)
    272271
    273272    def test_diff_image__missing_actual(self):
    274273        port = self.make_port()
    275         self.assertTrue(port.diff_image(None, b'foo').diff_image)
    276         self.assertTrue(port.diff_image(b'', b'foo').diff_image)
     274        self.assertFalse(port.diff_image(None, b'foo').passed)
     275        self.assertFalse(port.diff_image(b'', b'foo').passed)
    277276
    278277    def test_diff_image__missing_expected(self):
    279278        port = self.make_port()
    280         self.assertTrue(port.diff_image(b'foo', None).diff_image)
    281         self.assertTrue(port.diff_image(b'foo', b'').diff_image)
     279        self.assertFalse(port.diff_image(b'foo', None).passed)
     280        self.assertFalse(port.diff_image(b'foo', b'').passed)
    282281
    283282    def test_diff_image(self):
    … …  
    299298        self.assertFalse(port._should_use_jhbuild())
    300299
    301         self.assertEqual(port.diff_image(b'foo', b'bar'), ImageDiffResult(b'', 100.0, None))
     300        self.assertEqual(port.diff_image(b'foo', b'bar'), ImageDiffResult(passed=False, diff_image=b'', difference=100.0))
    302301        self.assertEqual(self.proc.cmd, [port._path_to_image_diff(), "--tolerance", "0.1"])
    303         self.assertEqual(port.diff_image(b'foo', b'bar', None), ImageDiffResult(b'', 100.0, None))
     302        self.assertEqual(port.diff_image(b'foo', b'bar', None), ImageDiffResult(passed=False, diff_image=b'', difference=100.0))
    304303        self.assertEqual(self.proc.cmd, [port._path_to_image_diff(), "--tolerance", "0.1"])
    305         self.assertEqual(port.diff_image(b'foo', b'bar', 0), ImageDiffResult(b'', 100.0, None))
     304        self.assertEqual(port.diff_image(b'foo', b'bar', 0), ImageDiffResult(passed=False, diff_image=b'', difference=100.0))
    306305        self.assertEqual(self.proc.cmd, [port._path_to_image_diff(), "--tolerance", "0"])
    307306
    … …  
    310309        self.assertTrue(port._should_use_jhbuild())
    311310
    312         self.assertEqual(port.diff_image(b'foo', b'bar'), ImageDiffResult(b'', 100.0, None))
     311        self.assertEqual(port.diff_image(b'foo', b'bar'), ImageDiffResult(passed=False, diff_image=b'', difference=100.0))
    313312        self.assertEqual(self.proc.cmd, port._jhbuild_wrapper + [port._path_to_image_diff(), "--tolerance", "0.1"])
    314         self.assertEqual(port.diff_image(b'foo', b'bar', None), ImageDiffResult(b'', 100.0, None))
     313        self.assertEqual(port.diff_image(b'foo', b'bar', None), ImageDiffResult(passed=False, diff_image=b'', difference=100.0))
    315314        self.assertEqual(self.proc.cmd, port._jhbuild_wrapper + [port._path_to_image_diff(), "--tolerance", "0.1"])
    316         self.assertEqual(port.diff_image(b'foo', b'bar', 0), ImageDiffResult(b'', 100.0, None))
     315        self.assertEqual(port.diff_image(b'foo', b'bar', 0), ImageDiffResult(passed=False, diff_image=b'', difference=100.0))
    317316        self.assertEqual(self.proc.cmd, port._jhbuild_wrapper + [port._path_to_image_diff(), "--tolerance", "0"])
    318317
    … …  
    325324        port._server_process_constructor = lambda port, nm, cmd, env, crash_message=None: MockServerProcess(lines=['diff: 0% passed\n'])
    326325        image_differ = ImageDiffer(port)
    327         self.assertEqual(image_differ.diff_image(b'foo', b'bar', 0.1), ImageDiffResult(None, 0, None))
     326        self.assertEqual(image_differ.diff_image(b'foo', b'bar', 0.1), ImageDiffResult(passed=True, diff_image=None, difference=0))
    328327
    329328    def test_diff_image_failed(self):
    … …  
    331330        port._server_process_constructor = lambda port, nm, cmd, env, crash_message=None: MockServerProcess(lines=['diff: 100% failed\n'])
    332331        image_differ = ImageDiffer(port)
    333         self.assertEqual(image_differ.diff_image(b'foo', b'bar', 0.1), ImageDiffResult(b'', 100.0, None))
     332        self.assertEqual(image_differ.diff_image(b'foo', b'bar', 0.1), ImageDiffResult(passed=False, diff_image=b'', difference=100.0))
    334333
    335334    def test_diff_image_crashed(self):
    … …  
    347346        port._server_process_constructor = make_proc
    348347        port.setup_test_run()
    349         self.assertEqual(port.diff_image(b'foo', b'bar'), ImageDiffResult(b'', 0, 'ImageDiff crashed\n'))
     348        self.assertEqual(port.diff_image(b'foo', b'bar'), ImageDiffResult(passed=False, diff_image=b'', difference=0, error_string='ImageDiff crashed\n'))
    350349        port.clean_up_test_run()
    351350

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

    r284785 r284845  
    409409        diffed = actual_contents != expected_contents
    410410        if not actual_contents and not expected_contents:
    411             return ImageDiffResult(None, 0, None)
     411            return ImageDiffResult(passed=True, diff_image=None, difference=0)
    412412
    413413        if not actual_contents or not expected_contents:
    414             return ImageDiffResult(True, 0, None)
     414            return ImageDiffResult(passed=False, diff_image=b'', difference=0)
    415415
    416416        if b'ref' in expected_contents:
    417417            assert tolerance == 0
    418418        if diffed:
    419             return ImageDiffResult("< {}\n---\n> {}\n".format(
    420                 string_utils.decode(expected_contents, target_type=str),
    421                 string_utils.decode(actual_contents, target_type=str),
    422             ), 1, None)
    423 
    424         return ImageDiffResult(None, 0, None)
     419            return ImageDiffResult(
     420                passed=False,
     421                diff_image="< {}\n---\n> {}\n".format(
     422                    string_utils.decode(expected_contents, target_type=str),
     423                    string_utils.decode(actual_contents, target_type=str),
     424                ),
     425                difference=1)
     426
     427        return ImageDiffResult(passed=True, diff_image=None, difference=0)
    425428
    426429    def layout_tests_dir(self):
Note: See TracChangeset for help on using the changeset viewer.