Diff
Modified: trunk/Tools/ChangeLog (284865 => 284866)
--- trunk/Tools/ChangeLog 2021-10-26 14:55:58 UTC (rev 284865)
+++ trunk/Tools/ChangeLog 2021-10-26 15:04:13 UTC (rev 284866)
@@ -1,3 +1,45 @@
+2021-10-26 Simon Fraser <[email protected]>
+
+ Don't run ImageDiff a second time to generate diff images
+ https://bugs.webkit.org/show_bug.cgi?id=232288
+
+ Reviewed by Martin Robinson.
+
+ Currently, for a ref test failure (which is always run with tolerance=0), we run ImageDiff a
+ second time in FailureReftestMismatch.write_failure() with the intent of generating a diff
+ image with zero tolerance.
+
+ Fix by storing the ImageDiffResult in FailureReftestMismatch and FailureImageHashMismatch so
+ we already have the diff image. We only regenerate it when the first diff was run with a
+ non-zero tolerance (only relevant for pixel tests). To faciliate this, store the tolerance
+ that was used inside ImageDiffResult too.
+
+ * ImageDiff/ImageDiff.cpp:
+ (main): Show tolerance in verbose logging.
+ * 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):
+ (TestResultWriterTest.test_reftest_diff_image):
+ * Scripts/webkitpy/layout_tests/models/test_failures.py:
+ (FailureImageHashMismatch.__init__):
+ (FailureReftestMismatch.__init__):
+ (FailureReftestMismatch.write_failure):
+ * Scripts/webkitpy/layout_tests/models/test_run_results.py:
+ (_interpret_test_failures):
+ * Scripts/webkitpy/layout_tests/models/test_run_results_unittest.py:
+ (InterpretTestFailuresTest.test_interpret_test_failures):
+ * Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py:
+ (RunTest.test_tolerance.ImageDiffTestPort.diff_image):
+ * Scripts/webkitpy/port/image_diff.py:
+ (ImageDiffResult.__init__):
+ (ImageDiffResult.__repr__):
+ (ImageDiffer._read):
+ * Scripts/webkitpy/port/port_testcase.py:
+ (PortTestCase.test_diff_image):
+
2021-10-25 Ryan Haddad <[email protected]>
Change default iOS simulator to one with a larger screen size
Modified: trunk/Tools/ImageDiff/ImageDiff.cpp (284865 => 284866)
--- trunk/Tools/ImageDiff/ImageDiff.cpp 2021-10-26 14:55:58 UTC (rev 284865)
+++ trunk/Tools/ImageDiff/ImageDiff.cpp 2021-10-26 15:04:13 UTC (rev 284866)
@@ -217,7 +217,7 @@
if (actualImage && baselineImage) {
if (verbose)
- fprintf(stderr, "ImageDiff: processing images\n");
+ fprintf(stderr, "ImageDiff: processing images with tolerance %01.2f%%\n", tolerance);
auto result = processImages(std::exchange(actualImage, { }), std::exchange(baselineImage, { }), tolerance, printDifference);
if (result != EXIT_SUCCESS)
return result;
Modified: trunk/Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py (284865 => 284866)
--- trunk/Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py 2021-10-26 14:55:58 UTC (rev 284865)
+++ trunk/Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py 2021-10-26 15:04:13 UTC (rev 284866)
@@ -295,7 +295,7 @@
elif driver_output.image_hash != expected_driver_output.image_hash:
diff_result = self._port.diff_image(expected_driver_output.image, driver_output.image)
if not diff_result.passed:
- failures.append(test_failures.FailureImageHashMismatch(diff_result.diff_percent))
+ failures.append(test_failures.FailureImageHashMismatch(diff_result))
if diff_result.error_string:
_log.warning(' %s : %s' % (self._test_name, diff_result.error_string))
driver_output.error = (driver_output.error or '') + diff_result.error_string
@@ -356,7 +356,7 @@
# 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)
if not diff_result.passed:
- failures.append(test_failures.FailureReftestMismatch(reference_filename))
+ failures.append(test_failures.FailureReftestMismatch(reference_filename, diff_result))
if diff_result.error_string:
_log.warning(' %s : %s' % (self._test_name, diff_result.error_string))
actual_driver_output.error = (actual_driver_output.error or '') + diff_result.error_string
Modified: trunk/Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer_unittest.py (284865 => 284866)
--- trunk/Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer_unittest.py 2021-10-26 14:55:58 UTC (rev 284865)
+++ trunk/Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer_unittest.py 2021-10-26 15:04:13 UTC (rev 284866)
@@ -40,9 +40,9 @@
used_tolerance_values = []
class ImageDiffTestPort(TestPort):
- def diff_image(self, expected_contents, actual_contents, tolerance=None):
+ def diff_image(self, expected_contents, actual_contents, tolerance):
used_tolerance_values.append(tolerance)
- return ImageDiffResult(passed=False, diff_image=b'', difference=1)
+ return ImageDiffResult(passed=False, diff_image=b'', difference=1, tolerance=tolerance)
host = MockHost()
port = ImageDiffTestPort(host)
@@ -50,7 +50,7 @@
test_reference_file = host.filesystem.join(port.layout_tests_dir(), 'failures/unexpected/reftest-expected.html')
driver_output1 = DriverOutput('text1', 'image1', 'imagehash1', 'audio1')
driver_output2 = DriverOutput('text2', 'image2', 'imagehash2', 'audio2')
- failures = [test_failures.FailureReftestMismatch(test_reference_file)]
+ failures = [test_failures.FailureReftestMismatch(test_reference_file, ImageDiffResult(passed=False, diff_image=b'', difference=1, tolerance=1))]
test_result_writer.write_test_result(host.filesystem, ImageDiffTestPort(host), port.results_directory(), test_name,
driver_output1, driver_output2, failures)
self.assertEqual([0], used_tolerance_values)
Modified: trunk/Tools/Scripts/webkitpy/layout_tests/models/test_failures.py (284865 => 284866)
--- trunk/Tools/Scripts/webkitpy/layout_tests/models/test_failures.py 2021-10-26 14:55:58 UTC (rev 284865)
+++ trunk/Tools/Scripts/webkitpy/layout_tests/models/test_failures.py 2021-10-26 15:04:13 UTC (rev 284866)
@@ -201,9 +201,9 @@
class FailureImageHashMismatch(TestFailure):
- def __init__(self, diff_percent=0):
+ def __init__(self, image_diff_result=None):
super(FailureImageHashMismatch, self).__init__()
- self.diff_percent = diff_percent
+ self.image_diff_result = image_diff_result
def message(self):
return "image diff"
@@ -219,10 +219,10 @@
class FailureReftestMismatch(TestFailure):
- def __init__(self, reference_filename=None):
+ def __init__(self, reference_filename=None, image_diff_result=None):
super(FailureReftestMismatch, self).__init__()
self.reference_filename = reference_filename
- self.diff_percent = None
+ self.image_diff_result = image_diff_result
def message(self):
return "reference mismatch"
@@ -229,15 +229,16 @@
def write_failure(self, writer, driver_output, expected_driver_output, port):
writer.write_image_files(driver_output.image, expected_driver_output.image)
- # 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_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
+ if self.image_diff_result:
+ # If the ref test was run with non-zero tolerance, generate the image diff again with zero tolerance.
+ if self.image_diff_result.tolerance != 0:
+ diff_image = port.diff_image(expected_driver_output.image, driver_output.image, tolerance=0).diff_image
else:
- _log.warn('ref test mismatch did not produce an image diff.')
+ diff_image = self.image_diff_result.diff_image
+
+ writer.write_image_diff_files(diff_image)
+ 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/models/test_run_results.py (284865 => 284866)
--- trunk/Tools/Scripts/webkitpy/layout_tests/models/test_run_results.py 2021-10-26 14:55:58 UTC (rev 284865)
+++ trunk/Tools/Scripts/webkitpy/layout_tests/models/test_run_results.py 2021-10-26 15:04:13 UTC (rev 284866)
@@ -213,7 +213,7 @@
if 'image_diff_percent' not in test_dict:
for failure in failures:
if isinstance(failure, test_failures.FailureImageHashMismatch) or isinstance(failure, test_failures.FailureReftestMismatch):
- test_dict['image_diff_percent'] = failure.diff_percent
+ test_dict['image_diff_percent'] = failure.image_diff_result.diff_percent
return test_dict
Modified: trunk/Tools/Scripts/webkitpy/layout_tests/models/test_run_results_unittest.py (284865 => 284866)
--- trunk/Tools/Scripts/webkitpy/layout_tests/models/test_run_results_unittest.py 2021-10-26 14:55:58 UTC (rev 284865)
+++ trunk/Tools/Scripts/webkitpy/layout_tests/models/test_run_results_unittest.py 2021-10-26 15:04:13 UTC (rev 284866)
@@ -33,6 +33,7 @@
from webkitpy.layout_tests.models import test_failures
from webkitpy.layout_tests.models import test_results
from webkitpy.layout_tests.models import test_run_results
+from webkitpy.port.image_diff import ImageDiffResult
from webkitpy.tool.mocktool import MockOptions
from webkitcorepy import OutputCapture
@@ -124,10 +125,10 @@
self.port = host.port_factory.get(port_name='test')
def test_interpret_test_failures(self):
- test_dict = test_run_results._interpret_test_failures([test_failures.FailureImageHashMismatch(diff_percent=0.42)])
+ test_dict = test_run_results._interpret_test_failures([test_failures.FailureImageHashMismatch(ImageDiffResult(passed=False, diff_image=b'', difference=0.42))])
self.assertEqual(test_dict['image_diff_percent'], 0.42)
- test_dict = test_run_results._interpret_test_failures([test_failures.FailureReftestMismatch(self.port.abspath_for_test('foo/reftest-expected.html'))])
+ test_dict = test_run_results._interpret_test_failures([test_failures.FailureReftestMismatch(self.port.abspath_for_test('foo/reftest-expected.html'), ImageDiffResult(passed=False, diff_image=b'', difference=100.0))])
self.assertIn('image_diff_percent', test_dict)
test_dict = test_run_results._interpret_test_failures([test_failures.FailureReftestMismatchDidNotOccur(self.port.abspath_for_test('foo/reftest-expected-mismatch.html'))])
Modified: trunk/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py (284865 => 284866)
--- trunk/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py 2021-10-26 14:55:58 UTC (rev 284865)
+++ trunk/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py 2021-10-26 15:04:13 UTC (rev 284866)
@@ -752,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 ImageDiffResult(passed=False, diff_image=b'', difference=1)
+ return ImageDiffResult(passed=False, diff_image=b'', difference=1, tolerance=self._options.tolerance or 0)
def get_port_for_run(args):
options, parsed_args = run_webkit_tests.parse_args(args)
Modified: trunk/Tools/Scripts/webkitpy/port/image_diff.py (284865 => 284866)
--- trunk/Tools/Scripts/webkitpy/port/image_diff.py 2021-10-26 14:55:58 UTC (rev 284865)
+++ trunk/Tools/Scripts/webkitpy/port/image_diff.py 2021-10-26 15:04:13 UTC (rev 284866)
@@ -40,10 +40,11 @@
class ImageDiffResult(object):
- def __init__(self, passed, diff_image, difference, error_string=None):
+ def __init__(self, passed, diff_image, difference, tolerance=0, error_string=None):
self.passed = passed
self.diff_image = diff_image
self.diff_percent = difference
+ self.tolerance = tolerance
self.error_string = error_string
def __eq__(self, other):
@@ -51,6 +52,7 @@
return (self.passed == other.passed and
self.diff_image == other.diff_image and
self.diff_percent == other.diff_percent and
+ self.tolerance == other.tolerance and
self.error_string == other.error_string)
return False
@@ -59,7 +61,7 @@
return not self.__eq__(other)
def __repr__(self):
- return 'ImageDiffResult(Passed {} {} {} {})'.format(self.passed, self.diff_image, self.diff_percent, self.error_string)
+ return 'ImageDiffResult(Passed {} {} diff {} tolerance {} {})'.format(self.passed, self.diff_image, self.diff_percent, self.tolerance, self.error_string)
class ImageDiffer(object):
def __init__(self, port):
@@ -133,7 +135,7 @@
return ImageDiffResult(passed=True, diff_image=None, difference=0)
diff_percent = float(string_utils.decode(m.group(1), target_type=str))
- return ImageDiffResult(passed=False, diff_image=output_image, difference=diff_percent, error_string=err_str or None)
+ return ImageDiffResult(passed=False, diff_image=output_image, difference=diff_percent, tolerance=self._tolerance, error_string=err_str or None)
def stop(self):
if self._process:
Modified: trunk/Tools/Scripts/webkitpy/port/port_testcase.py (284865 => 284866)
--- trunk/Tools/Scripts/webkitpy/port/port_testcase.py 2021-10-26 14:55:58 UTC (rev 284865)
+++ trunk/Tools/Scripts/webkitpy/port/port_testcase.py 2021-10-26 15:04:13 UTC (rev 284866)
@@ -297,11 +297,13 @@
# 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'), ImageDiffResult(passed=False, diff_image=b'', difference=100.0))
+ self.assertEqual(port.diff_image(b'foo', b'bar'), ImageDiffResult(passed=False, diff_image=b'', difference=100.0, tolerance=0.1))
self.assertEqual(self.proc.cmd, [port._path_to_image_diff(), "--tolerance", "0.1"])
- self.assertEqual(port.diff_image(b'foo', b'bar', None), ImageDiffResult(passed=False, diff_image=b'', difference=100.0))
+
+ self.assertEqual(port.diff_image(b'foo', b'bar', tolerance=None), ImageDiffResult(passed=False, diff_image=b'', difference=100.0, tolerance=0.1))
self.assertEqual(self.proc.cmd, [port._path_to_image_diff(), "--tolerance", "0.1"])
- self.assertEqual(port.diff_image(b'foo', b'bar', 0), ImageDiffResult(passed=False, diff_image=b'', difference=100.0))
+
+ self.assertEqual(port.diff_image(b'foo', b'bar', tolerance=0), ImageDiffResult(passed=False, diff_image=b'', difference=100.0))
self.assertEqual(self.proc.cmd, [port._path_to_image_diff(), "--tolerance", "0"])
# Now test the case of using JHBuild wrapper.
@@ -308,11 +310,13 @@
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'), ImageDiffResult(passed=False, diff_image=b'', difference=100.0))
+ self.assertEqual(port.diff_image(b'foo', b'bar'), ImageDiffResult(passed=False, diff_image=b'', difference=100.0, tolerance=0.1))
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), ImageDiffResult(passed=False, diff_image=b'', difference=100.0))
+
+ self.assertEqual(port.diff_image(b'foo', b'bar', tolerance=None), ImageDiffResult(passed=False, diff_image=b'', difference=100.0, tolerance=0.1))
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), ImageDiffResult(passed=False, diff_image=b'', difference=100.0))
+
+ self.assertEqual(port.diff_image(b'foo', b'bar', tolerance=0), ImageDiffResult(passed=False, diff_image=b'', difference=100.0))
self.assertEqual(self.proc.cmd, port._jhbuild_wrapper + [port._path_to_image_diff(), "--tolerance", "0"])
port.clean_up_test_run()
@@ -323,13 +327,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), ImageDiffResult(passed=True, diff_image=None, difference=0))
+ self.assertEqual(image_differ.diff_image(b'foo', b'bar', tolerance=0.1), ImageDiffResult(passed=True, diff_image=None, difference=0))
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), ImageDiffResult(passed=False, diff_image=b'', difference=100.0))
+ self.assertEqual(image_differ.diff_image(b'foo', b'bar', tolerance=0.1), ImageDiffResult(passed=False, diff_image=b'', difference=100.0, tolerance=0.1))
def test_diff_image_crashed(self):
port = self.make_port()
@@ -345,7 +349,7 @@
port._server_process_constructor = make_proc
port.setup_test_run()
- self.assertEqual(port.diff_image(b'foo', b'bar'), ImageDiffResult(passed=False, diff_image=b'', difference=0, error_string='ImageDiff crashed\n'))
+ self.assertEqual(port.diff_image(b'foo', b'bar'), ImageDiffResult(passed=False, diff_image=b'', difference=0, tolerance=0.1, error_string='ImageDiff crashed\n'))
port.clean_up_test_run()
@slow
Modified: trunk/Tools/Scripts/webkitpy/port/test.py (284865 => 284866)
--- trunk/Tools/Scripts/webkitpy/port/test.py 2021-10-26 14:55:58 UTC (rev 284865)
+++ trunk/Tools/Scripts/webkitpy/port/test.py 2021-10-26 15:04:13 UTC (rev 284866)
@@ -408,10 +408,10 @@
actual_contents = string_utils.encode(actual_contents)
diffed = actual_contents != expected_contents
if not actual_contents and not expected_contents:
- return ImageDiffResult(passed=True, diff_image=None, difference=0)
+ return ImageDiffResult(passed=True, diff_image=None, difference=0, tolerance=tolerance or 0)
if not actual_contents or not expected_contents:
- return ImageDiffResult(passed=False, diff_image=b'', difference=0)
+ return ImageDiffResult(passed=False, diff_image=b'', difference=0, tolerance=tolerance or 0)
if b'ref' in expected_contents:
assert tolerance == 0
@@ -422,9 +422,10 @@
string_utils.decode(expected_contents, target_type=str),
string_utils.decode(actual_contents, target_type=str),
),
- difference=1)
+ difference=1,
+ tolerance=tolerance or 0)
- return ImageDiffResult(passed=True, diff_image=None, difference=0)
+ return ImageDiffResult(passed=True, diff_image=None, difference=0, tolerance=tolerance or 0)
def layout_tests_dir(self):
return LAYOUT_TEST_DIR
