Diff
Modified: trunk/Tools/ChangeLog (284784 => 284785)
--- trunk/Tools/ChangeLog 2021-10-25 15:17:47 UTC (rev 284784)
+++ trunk/Tools/ChangeLog 2021-10-25 15:26:54 UTC (rev 284785)
@@ -1,3 +1,44 @@
+2021-10-25 Simon Fraser <simon.fra...@apple.com>
+
+ webkitpy: have diff_image() return a ImageDiffResult
+ https://bugs.webkit.org/show_bug.cgi?id=232222
+
+ Reviewed by Jonathan Bedard.
+
+ diff_image() returned a list which resulted in hard to read code. In addition, the
+ presence of a diff_image is used to indicate failure; future patches will change this,
+ so having diff_image() return a class makes that easier.
+
+ * 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_unittest.py:
+ (TestResultWriterTest.test_reftest_diff_image.ImageDiffTestPort.diff_image):
+ (TestResultWriterTest):
+ * Scripts/webkitpy/layout_tests/models/test_failures.py:
+ (FailureReftestMismatch.write_failure):
+ * 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):
+ (ImageDiffResult.__init__):
+ (ImageDiffResult.__eq__):
+ (ImageDiffResult.__ne__):
+ (ImageDiffResult.__repr__):
+ (ImageDiffer._read):
+ * Scripts/webkitpy/port/port_testcase.py:
+ (PortTestCase.integration_test_image_diff):
+ (PortTestCase.test_diff_image__missing_both):
+ (PortTestCase.test_diff_image__missing_actual):
+ (PortTestCase.test_diff_image__missing_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:
+
2021-10-25 Carlos Alberto Lopez Perez <clo...@igalia.com>
[webkitpy] webkit-test-runner doesn't report all results when a test is run several times
Modified: trunk/Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py (284784 => 284785)
--- trunk/Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py 2021-10-25 15:17:47 UTC (rev 284784)
+++ trunk/Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py 2021-10-25 15:26:54 UTC (rev 284785)
@@ -294,15 +294,14 @@
failures.append(test_failures.FailureMissingImageHash())
elif driver_output.image_hash != expected_driver_output.image_hash:
diff_result = self._port.diff_image(expected_driver_output.image, driver_output.image)
- error_string = diff_result[2]
- if error_string:
- _log.warning(' %s : %s' % (self._test_name, error_string))
+ if diff_result.error_string:
+ _log.warning(' %s : %s' % (self._test_name, diff_result.error_string))
failures.append(test_failures.FailureImageHashMismatch())
- driver_output.error = (driver_output.error or '') + error_string
+ driver_output.error = (driver_output.error or '') + diff_result.error_string
else:
- driver_output.image_diff = diff_result[0]
+ driver_output.image_diff = diff_result.diff_image
if driver_output.image_diff:
- failures.append(test_failures.FailureImageHashMismatch(diff_result[1]))
+ failures.append(test_failures.FailureImageHashMismatch(diff_result.diff_percent))
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 diff passed)' % self._test_name)
@@ -358,12 +357,11 @@
elif reference_driver_output.image_hash != actual_driver_output.image_hash:
# ImageDiff has a hard coded color distance threshold even though tolerance=0 is specified.
diff_result = self._port.diff_image(reference_driver_output.image, actual_driver_output.image, tolerance=0)
- error_string = diff_result[2]
- if error_string:
- _log.warning(' %s : %s' % (self._test_name, error_string))
+ if diff_result.error_string:
+ _log.warning(' %s : %s' % (self._test_name, diff_result.error_string))
failures.append(test_failures.FailureReftestMismatch(reference_filename))
- actual_driver_output.error = (actual_driver_output.error or '') + error_string
- elif diff_result[0]:
+ actual_driver_output.error = (actual_driver_output.error or '') + diff_result.error_string
+ elif diff_result.diff_image:
failures.append(test_failures.FailureReftestMismatch(reference_filename))
return TestResult(self._test_input, failures, total_test_time, has_stderr, pid=actual_driver_output.pid)
Modified: trunk/Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer_unittest.py (284784 => 284785)
--- trunk/Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer_unittest.py 2021-10-25 15:17:47 UTC (rev 284784)
+++ trunk/Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer_unittest.py 2021-10-25 15:26:54 UTC (rev 284785)
@@ -30,6 +30,7 @@
from webkitpy.layout_tests.models import test_failures
from webkitpy.port.driver import DriverOutput
from webkitpy.port.test import TestPort
+from webkitpy.port.image_diff import ImageDiffResult
class TestResultWriterTest(unittest.TestCase):
@@ -41,7 +42,7 @@
class ImageDiffTestPort(TestPort):
def diff_image(self, expected_contents, actual_contents, tolerance=None):
used_tolerance_values.append(tolerance)
- return (True, 1, None)
+ return ImageDiffResult(True, 1, None)
host = MockHost()
port = ImageDiffTestPort(host)
Modified: trunk/Tools/Scripts/webkitpy/layout_tests/models/test_failures.py (284784 => 284785)
--- trunk/Tools/Scripts/webkitpy/layout_tests/models/test_failures.py 2021-10-25 15:17:47 UTC (rev 284784)
+++ trunk/Tools/Scripts/webkitpy/layout_tests/models/test_failures.py 2021-10-25 15:26:54 UTC (rev 284785)
@@ -232,10 +232,10 @@
# 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, err_str = port.diff_image(expected_driver_output.image, driver_output.image, tolerance=0)
- if diff_image:
- writer.write_image_diff_files(diff_image)
- self.diff_percent = diff_percent
+ diff_result = port.diff_image(expected_driver_output.image, driver_output.image, tolerance=0)
+ if diff_result.diff_image:
+ writer.write_image_diff_files(diff_result.diff_image)
+ self.diff_percent = diff_result.diff_percent
else:
_log.warn('ref test mismatch did not produce an image diff.')
writer.write_reftest(self.reference_filename)
Modified: trunk/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py (284784 => 284785)
--- trunk/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py 2021-10-25 15:17:47 UTC (rev 284784)
+++ trunk/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py 2021-10-25 15:26:54 UTC (rev 284785)
@@ -42,6 +42,7 @@
from webkitpy.layout_tests import run_webkit_tests
from webkitpy.layout_tests.models.test_run_results import INTERRUPTED_EXIT_STATUS
from webkitpy.port import test
+from webkitpy.port.image_diff import ImageDiffResult
from webkitpy.xcode.device_type import DeviceType
@@ -751,7 +752,7 @@
class ImageDiffTestPort(test.TestPort):
def diff_image(self, expected_contents, actual_contents, tolerance=None):
self.tolerance_used_for_diff_image = self._options.tolerance
- return (True, 1, None)
+ return ImageDiffResult(True, 1, None)
def get_port_for_run(args):
options, parsed_args = run_webkit_tests.parse_args(args)
Modified: trunk/Tools/Scripts/webkitpy/port/base.py (284784 => 284785)
--- trunk/Tools/Scripts/webkitpy/port/base.py 2021-10-25 15:17:47 UTC (rev 284784)
+++ trunk/Tools/Scripts/webkitpy/port/base.py 2021-10-25 15:26:54 UTC (rev 284785)
@@ -56,6 +56,7 @@
from webkitpy.port import image_diff
from webkitpy.port import server_process
from webkitpy.port.factory import PortFactory
+from webkitpy.port.image_diff import ImageDiffResult
from webkitpy.layout_tests.servers import apache_http_server, http_server, http_server_base
from webkitpy.layout_tests.servers import web_platform_test_server
from webkitpy.layout_tests.servers import websocket_server
@@ -306,11 +307,14 @@
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, None)
+ return ImageDiffResult(None, 0, None)
+
if not actual_contents or not expected_contents:
- return (True, 0, None)
+ return ImageDiffResult(True, 0, None)
+
if not self._image_differ:
self._image_differ = image_diff.ImageDiffer(self)
+
self.set_option_default('tolerance', 0.1)
if tolerance is None:
tolerance = self.get_option('tolerance')
Modified: trunk/Tools/Scripts/webkitpy/port/image_diff.py (284784 => 284785)
--- trunk/Tools/Scripts/webkitpy/port/image_diff.py 2021-10-25 15:17:47 UTC (rev 284784)
+++ trunk/Tools/Scripts/webkitpy/port/image_diff.py 2021-10-25 15:26:54 UTC (rev 284785)
@@ -36,10 +36,29 @@
from webkitcorepy import BytesIO, string_utils
-
_log = logging.getLogger(__name__)
+class ImageDiffResult(object):
+ def __init__(self, diff_image, difference, error_string):
+ self.diff_image = diff_image
+ self.diff_percent = difference
+ self.error_string = error_string
+
+ def __eq__(self, other):
+ if isinstance(other, self.__class__):
+ return (self.diff_image == other.diff_image and
+ self.diff_percent == other.diff_percent and
+ self.error_string == other.error_string)
+
+ return False
+
+ def __ne__(self, other):
+ return not self.__eq__(other)
+
+ def __repr__(self):
+ return 'ImageDiffResult({} {} {})'.format(self.diff_image, self.diff_percent, self.error_string)
+
class ImageDiffer(object):
def __init__(self, port):
self._port = port
@@ -109,10 +128,10 @@
if output and output.startswith(b'diff'):
m = re.match(b'diff: (.+)% (passed|failed)', output)
if m.group(2) == b'passed':
- return (None, 0, None)
+ return ImageDiffResult(None, 0, None)
diff_percent = float(string_utils.decode(m.group(1), target_type=str))
- return (output_image, diff_percent, err_str or None)
+ return ImageDiffResult(output_image, diff_percent, err_str or None)
def stop(self):
if self._process:
Modified: trunk/Tools/Scripts/webkitpy/port/port_testcase.py (284784 => 284785)
--- trunk/Tools/Scripts/webkitpy/port/port_testcase.py 2021-10-25 15:17:47 UTC (rev 284784)
+++ trunk/Tools/Scripts/webkitpy/port/port_testcase.py 2021-10-25 15:26:54 UTC (rev 284785)
@@ -45,7 +45,7 @@
from webkitpy.common.version_name_map import INTERNAL_TABLE
from webkitpy.port.base import Port
from webkitpy.port.config import apple_additions, clear_cached_configuration
-from webkitpy.port.image_diff import ImageDiffer
+from webkitpy.port.image_diff import ImageDiffer, ImageDiffResult
from webkitpy.port.server_process_mock import MockServerProcess
from webkitpy.layout_tests.servers import http_server_base
from webkitpy.tool.mocktool import MockOptions
@@ -255,30 +255,30 @@
tmpfd, tmpfile = port._filesystem.open_binary_tempfile('')
tmpfd.close()
- self.assertFalse(port.diff_image(contents1, contents1)[0])
- self.assertTrue(port.diff_image(contents1, contents2)[0])
+ self.assertFalse(port.diff_image(contents1, contents1).diff_image)
+ self.assertTrue(port.diff_image(contents1, contents2).diff_image)
- self.assertTrue(port.diff_image(contents1, contents2, tmpfile)[0])
+ self.assertTrue(port.diff_image(contents1, contents2, tmpfile).diff_image)
port._filesystem.remove(tmpfile)
def test_diff_image__missing_both(self):
port = self.make_port()
- self.assertFalse(port.diff_image(None, None)[0])
- self.assertFalse(port.diff_image(None, b'')[0])
- self.assertFalse(port.diff_image(b'', None)[0])
+ self.assertFalse(port.diff_image(None, None).diff_image)
+ self.assertFalse(port.diff_image(None, b'').diff_image)
+ self.assertFalse(port.diff_image(b'', None).diff_image)
- self.assertFalse(port.diff_image(b'', b'')[0])
+ self.assertFalse(port.diff_image(b'', b'').diff_image)
def test_diff_image__missing_actual(self):
port = self.make_port()
- self.assertTrue(port.diff_image(None, b'foo')[0])
- self.assertTrue(port.diff_image(b'', b'foo')[0])
+ self.assertTrue(port.diff_image(None, b'foo').diff_image)
+ self.assertTrue(port.diff_image(b'', b'foo').diff_image)
def test_diff_image__missing_expected(self):
port = self.make_port()
- self.assertTrue(port.diff_image(b'foo', None)[0])
- self.assertTrue(port.diff_image(b'foo', b'')[0])
+ self.assertTrue(port.diff_image(b'foo', None).diff_image)
+ self.assertTrue(port.diff_image(b'foo', b'').diff_image)
def test_diff_image(self):
port = self.make_port()
@@ -298,11 +298,11 @@
# First test the case of not using the JHBuild wrapper.
self.assertFalse(port._should_use_jhbuild())
- self.assertEqual(port.diff_image(b'foo', b'bar'), (b'', 100.0, None))
+ self.assertEqual(port.diff_image(b'foo', b'bar'), ImageDiffResult(b'', 100.0, None))
self.assertEqual(self.proc.cmd, [port._path_to_image_diff(), "--tolerance", "0.1"])
- self.assertEqual(port.diff_image(b'foo', b'bar', None), (b'', 100.0, None))
+ self.assertEqual(port.diff_image(b'foo', b'bar', None), ImageDiffResult(b'', 100.0, None))
self.assertEqual(self.proc.cmd, [port._path_to_image_diff(), "--tolerance", "0.1"])
- self.assertEqual(port.diff_image(b'foo', b'bar', 0), (b'', 100.0, None))
+ self.assertEqual(port.diff_image(b'foo', b'bar', 0), ImageDiffResult(b'', 100.0, None))
self.assertEqual(self.proc.cmd, [port._path_to_image_diff(), "--tolerance", "0"])
# Now test the case of using JHBuild wrapper.
@@ -309,11 +309,11 @@
port._filesystem.maybe_make_directory(port.path_from_webkit_base('WebKitBuild', 'Dependencies%s' % port.port_name.upper()))
self.assertTrue(port._should_use_jhbuild())
- self.assertEqual(port.diff_image(b'foo', b'bar'), (b'', 100.0, None))
+ self.assertEqual(port.diff_image(b'foo', b'bar'), ImageDiffResult(b'', 100.0, None))
self.assertEqual(self.proc.cmd, port._jhbuild_wrapper + [port._path_to_image_diff(), "--tolerance", "0.1"])
- self.assertEqual(port.diff_image(b'foo', b'bar', None), (b'', 100.0, None))
+ self.assertEqual(port.diff_image(b'foo', b'bar', None), ImageDiffResult(b'', 100.0, None))
self.assertEqual(self.proc.cmd, port._jhbuild_wrapper + [port._path_to_image_diff(), "--tolerance", "0.1"])
- self.assertEqual(port.diff_image(b'foo', b'bar', 0), (b'', 100.0, None))
+ self.assertEqual(port.diff_image(b'foo', b'bar', 0), ImageDiffResult(b'', 100.0, None))
self.assertEqual(self.proc.cmd, port._jhbuild_wrapper + [port._path_to_image_diff(), "--tolerance", "0"])
port.clean_up_test_run()
@@ -324,13 +324,13 @@
port = self.make_port()
port._server_process_constructor = lambda port, nm, cmd, env, crash_message=None: MockServerProcess(lines=['diff: 0% passed\n'])
image_differ = ImageDiffer(port)
- self.assertEqual(image_differ.diff_image(b'foo', b'bar', 0.1), (None, 0, None))
+ self.assertEqual(image_differ.diff_image(b'foo', b'bar', 0.1), ImageDiffResult(None, 0, None))
def test_diff_image_failed(self):
port = self.make_port()
port._server_process_constructor = lambda port, nm, cmd, env, crash_message=None: MockServerProcess(lines=['diff: 100% failed\n'])
image_differ = ImageDiffer(port)
- self.assertEqual(image_differ.diff_image(b'foo', b'bar', 0.1), (b'', 100.0, None))
+ self.assertEqual(image_differ.diff_image(b'foo', b'bar', 0.1), ImageDiffResult(b'', 100.0, None))
def test_diff_image_crashed(self):
port = self.make_port()
@@ -346,7 +346,7 @@
port._server_process_constructor = make_proc
port.setup_test_run()
- self.assertEqual(port.diff_image(b'foo', b'bar'), (b'', 0, 'ImageDiff crashed\n'))
+ self.assertEqual(port.diff_image(b'foo', b'bar'), ImageDiffResult(b'', 0, 'ImageDiff crashed\n'))
port.clean_up_test_run()
@slow
Modified: trunk/Tools/Scripts/webkitpy/port/test.py (284784 => 284785)
--- trunk/Tools/Scripts/webkitpy/port/test.py 2021-10-25 15:17:47 UTC (rev 284784)
+++ trunk/Tools/Scripts/webkitpy/port/test.py 2021-10-25 15:26:54 UTC (rev 284785)
@@ -34,6 +34,7 @@
from webkitpy.layout_tests.models.test_configuration import TestConfiguration
from webkitpy.common.system.crashlogs import CrashLogs
from webkitpy.common.version_name_map import PUBLIC_TABLE, VersionNameMap
+from webkitpy.port.image_diff import ImageDiffResult
# This sets basic expectations for a test. Each individual expectation
@@ -407,18 +408,21 @@
actual_contents = string_utils.encode(actual_contents)
diffed = actual_contents != expected_contents
if not actual_contents and not expected_contents:
- return (None, 0, None)
+ return ImageDiffResult(None, 0, None)
+
if not actual_contents or not expected_contents:
- return (True, 0, None)
+ return ImageDiffResult(True, 0, None)
+
if b'ref' in expected_contents:
assert tolerance == 0
if diffed:
- return ("< {}\n---\n> {}\n".format(
+ return ImageDiffResult("< {}\n---\n> {}\n".format(
string_utils.decode(expected_contents, target_type=str),
string_utils.decode(actual_contents, target_type=str),
), 1, None)
- return (None, 0, None)
+ return ImageDiffResult(None, 0, None)
+
def layout_tests_dir(self):
return LAYOUT_TEST_DIR
