Diff
Modified: trunk/Tools/ChangeLog (90137 => 90138)
--- trunk/Tools/ChangeLog 2011-06-30 18:32:30 UTC (rev 90137)
+++ trunk/Tools/ChangeLog 2011-06-30 18:37:16 UTC (rev 90138)
@@ -1,3 +1,33 @@
+2011-06-30 Adam Barth <[email protected]>
+
+ Reviewed by Eric Seidel.
+
+ REGRESSION(r84294): new-run-webkit-tests results.html generate links to diffs.html or diff.png that don't exist
+ https://bugs.webkit.org/show_bug.cgi?id=63494
+
+ This patch does two things:
+
+ 1) Refactor diff_image to return the image diff instead of writing the
+ diff to a file. This fixes a bunch of hacks and disentangles a bunch
+ of code.
+
+ 2) When there's a checksum mismatch but not image diff, we no longer
+ report an IMAGE failure to results.html. That fixes the bug in
+ question because results.html won't try to link to a non-existent image
+ diff.
+
+ * Scripts/webkitpy/common/system/executive_mock.py:
+ * Scripts/webkitpy/layout_tests/layout_package/single_test_runner.py:
+ * Scripts/webkitpy/layout_tests/layout_package/test_result_writer.py:
+ * Scripts/webkitpy/layout_tests/port/base.py:
+ * Scripts/webkitpy/layout_tests/port/chromium.py:
+ * Scripts/webkitpy/layout_tests/port/chromium_unittest.py:
+ * Scripts/webkitpy/layout_tests/port/port_testcase.py:
+ * Scripts/webkitpy/layout_tests/port/test.py:
+ * Scripts/webkitpy/layout_tests/port/webkit.py:
+ * Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py:
+ * Scripts/webkitpy/to_be_moved/rebaseline_chromium_webkit_tests.py:
+
2011-06-28 Hans Wennborg <[email protected]>
Reviewed by Tony Chang.
Modified: trunk/Tools/Scripts/webkitpy/common/system/executive_mock.py (90137 => 90138)
--- trunk/Tools/Scripts/webkitpy/common/system/executive_mock.py 2011-06-30 18:32:30 UTC (rev 90137)
+++ trunk/Tools/Scripts/webkitpy/common/system/executive_mock.py 2011-06-30 18:37:16 UTC (rev 90138)
@@ -61,10 +61,10 @@
decode_output=False):
if self._exception:
raise self._exception
+ if self._run_command_fn:
+ return self._run_command_fn(args)
if return_exit_code:
return self._exit_code
- if self._run_command_fn:
- return self._run_command_fn(args)
if self._exit_code and error_handler:
script_error = executive.ScriptError(script_args=args,
exit_code=self._exit_code,
Modified: trunk/Tools/Scripts/webkitpy/layout_tests/layout_package/single_test_runner.py (90137 => 90138)
--- trunk/Tools/Scripts/webkitpy/layout_tests/layout_package/single_test_runner.py 2011-06-30 18:32:30 UTC (rev 90137)
+++ trunk/Tools/Scripts/webkitpy/layout_tests/layout_package/single_test_runner.py 2011-06-30 18:37:16 UTC (rev 90138)
@@ -266,17 +266,21 @@
# the normalized text expectation files.
return output.replace("\r\r\n", "\r\n").replace("\r\n", "\n")
- def _compare_image(self, driver_output, expected_driver_outputs):
+ # FIXME: This function also creates the image diff. Maybe that work should
+ # be handled elsewhere?
+ def _compare_image(self, driver_output, expected_driver_output):
failures = []
# If we didn't produce a hash file, this test must be text-only.
if driver_output.image_hash is None:
return failures
- if not expected_driver_outputs.image:
+ if not expected_driver_output.image:
failures.append(test_failures.FailureMissingImage())
- elif not expected_driver_outputs.image_hash:
+ elif not expected_driver_output.image_hash:
failures.append(test_failures.FailureMissingImageHash())
- elif driver_output.image_hash != expected_driver_outputs.image_hash:
- failures.append(test_failures.FailureImageHashMismatch())
+ elif driver_output.image_hash != expected_driver_output.image_hash:
+ driver_output.image_diff = self._port.diff_image(driver_output.image, expected_driver_output.image)
+ if driver_output.image_diff:
+ failures.append(test_failures.FailureImageHashMismatch())
return failures
def _run_reftest(self):
Modified: trunk/Tools/Scripts/webkitpy/layout_tests/layout_package/test_result_writer.py (90137 => 90138)
--- trunk/Tools/Scripts/webkitpy/layout_tests/layout_package/test_result_writer.py 2011-06-30 18:32:30 UTC (rev 90137)
+++ trunk/Tools/Scripts/webkitpy/layout_tests/layout_package/test_result_writer.py 2011-06-30 18:37:16 UTC (rev 90138)
@@ -41,8 +41,6 @@
expected_driver_output, failures):
"""Write the test result to the result output directory."""
root_output_dir = port.results_directory()
- checksums_mismatch_but_images_are_same = False
- imagehash_mismatch_failure = None
writer = TestResultWriter(port, root_output_dir, filename)
if driver_output.error:
writer.write_stderr(driver_output.error)
@@ -60,11 +58,7 @@
writer.write_image_files(driver_output.image, expected_driver_output.image)
elif isinstance(failure, test_failures.FailureImageHashMismatch):
writer.write_image_files(driver_output.image, expected_driver_output.image)
- images_are_different = writer.create_image_diff_and_write_result(
- driver_output.image, expected_driver_output.image)
- if not images_are_different:
- checksums_mismatch_but_images_are_same = True
- imagehash_mismatch_failure = failure
+ writer.write_image_diff_files(driver_output.image_diff)
elif isinstance(failure, (test_failures.FailureAudioMismatch,
test_failures.FailureMissingAudio)):
writer.write_audio_files(driver_output.audio, expected_driver_output.audio)
@@ -75,7 +69,10 @@
writer.write_crash_report(driver_output.error)
elif isinstance(failure, test_failures.FailureReftestMismatch):
writer.write_image_files(driver_output.image, expected_driver_output.image)
- writer.create_image_diff_and_write_result(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).
+ image_diff = port.diff_image(driver_output.image, expected_driver_output.image)
+ if image_diff:
+ writer.write_image_diff_files(image_diff)
writer.copy_file(port.reftest_expected_filename(filename), '-expected.html')
elif isinstance(failure, test_failures.FailureReftestMismatchDidNotOccur):
writer.write_image_files(driver_output.image, expected_image=None)
@@ -83,16 +80,7 @@
else:
assert isinstance(failure, (test_failures.FailureTimeout,))
- # FIXME: This is an ugly hack to handle FailureImageHashIncorrect case.
- # Ideally, FailureImageHashIncorrect case should be detected before this
- # function is called. But it requires calling create_diff_image() to detect
- # whether two images are same or not. So we need this hack until we have a better approach.
- if checksums_mismatch_but_images_are_same:
- # Replace FailureImageHashMismatch with FailureImageHashIncorrect.
- failures.remove(imagehash_mismatch_failure)
- failures.append(test_failures.FailureImageHashIncorrect())
-
class TestResultWriter(object):
"""A class which handles all writing operations to the result directory."""
@@ -215,20 +203,11 @@
def write_image_files(self, actual_image, expected_image):
self.write_output_files('.png', actual_image, expected_image)
- def create_image_diff_and_write_result(self, actual_image, expected_image):
- """Writes the visual diff of the expected/actual PNGs.
-
- Returns True if the images are different.
- """
- # FIXME: This function is actually doing the diff as well as writing a result.
- # It might be better to extract 'diff' code and make it a separate function.
- # To do so, we have to change port.diff_image() as well.
+ def write_image_diff_files(self, image_diff):
diff_filename = self.output_filename(self.FILENAME_SUFFIX_IMAGE_DIFF)
- images_are_different = self._port.diff_image(actual_image, expected_image, diff_filename)
+ fs = self._port._filesystem
+ fs.write_binary_file(diff_filename, image_diff)
- if not images_are_different:
- return False
-
diffs_html_filename = self.output_filename(self.FILENAME_SUFFIX_IMAGE_DIFFS_HTML)
# FIXME: old-run-webkit-tests shows the diff percentage as the text contents of the "diff" link.
# FIXME: old-run-webkit-tests include a link to the test file.
@@ -278,11 +257,14 @@
</script>
</body>
</html>
-""" % { 'title': self._testname, 'diff_filename': self._output_testname('-diff.png'), 'prefix': self._output_testname('') }
+""" % {
+ 'title': self._testname,
+ 'diff_filename': self._output_testname(self.FILENAME_SUFFIX_IMAGE_DIFF),
+ 'prefix': self._output_testname(''),
+ }
+ # FIXME: This seems like a text file, not a binary file.
self._port._filesystem.write_binary_file(diffs_html_filename, html)
- return True
-
def copy_file(self, src_filepath, dst_extension):
fs = self._port._filesystem
assert fs.exists(src_filepath), 'src_filepath: %s' % src_filepath
Modified: trunk/Tools/Scripts/webkitpy/layout_tests/port/base.py (90137 => 90138)
--- trunk/Tools/Scripts/webkitpy/layout_tests/port/base.py 2011-06-30 18:32:30 UTC (rev 90137)
+++ trunk/Tools/Scripts/webkitpy/layout_tests/port/base.py 2011-06-30 18:37:16 UTC (rev 90138)
@@ -244,16 +244,11 @@
"""Return whether the two audio files are *not* equal."""
return expected_audio != actual_audio
- def diff_image(self, expected_contents, actual_contents, diff_filename=None, tolerance=0):
- """Compare two images and produce a delta image file.
+ def diff_image(self, expected_contents, actual_contents, tolerance=0):
+ """Compare two images and return an image diff.
- Return True if the two images are different, False if they are the same.
- Also produce a delta image of the two images and write that into
- |diff_filename| if it is not None.
-
|tolerance| should be a percentage value (0.0 - 100.0).
If it is omitted, the port default tolerance value is used.
-
"""
raise NotImplementedError('Port.diff_image')
@@ -939,6 +934,7 @@
self.text = text
self.image = image
self.image_hash = image_hash
+ self.image_diff = None # image_diff gets filled in after construction.
self.audio = audio
self.crash = crash
self.test_time = test_time
Modified: trunk/Tools/Scripts/webkitpy/layout_tests/port/chromium.py (90137 => 90138)
--- trunk/Tools/Scripts/webkitpy/layout_tests/port/chromium.py 2011-06-30 18:32:30 UTC (rev 90137)
+++ trunk/Tools/Scripts/webkitpy/layout_tests/port/chromium.py 2011-06-30 18:37:16 UTC (rev 90138)
@@ -138,8 +138,7 @@
return self._check_file_exists(image_diff_path, 'image diff exe',
override_step, logging)
- def diff_image(self, expected_contents, actual_contents,
- diff_filename=None):
+ def diff_image(self, expected_contents, actual_contents):
# FIXME: need unit tests for this.
if not actual_contents and not expected_contents:
return False
@@ -147,39 +146,40 @@
return True
tempdir = self._filesystem.mkdtemp()
+
expected_filename = self._filesystem.join(str(tempdir), "expected.png")
self._filesystem.write_binary_file(expected_filename, expected_contents)
+
actual_filename = self._filesystem.join(str(tempdir), "actual.png")
self._filesystem.write_binary_file(actual_filename, actual_contents)
+ diff_filename = self._filesystem.join(str(tempdir), "diff.png")
+
executable = self._path_to_image_diff()
- if diff_filename:
- cmd = [executable, '--diff', expected_filename,
- actual_filename, diff_filename]
- else:
- cmd = [executable, expected_filename, actual_filename]
+ comand = [executable, '--diff', expected_filename, actual_filename, diff_filename]
- result = True
+ result = None
try:
- exit_code = self._executive.run_command(cmd, return_exit_code=True)
+ exit_code = self._executive.run_command(comand, return_exit_code=True)
if exit_code == 0:
# The images are the same.
- result = False
+ result = None
elif exit_code != 1:
- _log.error("image diff returned an exit code of "
- + str(exit_code))
+ _log.error("image diff returned an exit code of %s" % exit_code)
# Returning False here causes the script to think that we
# successfully created the diff even though we didn't. If
# we return True, we think that the images match but the hashes
# don't match.
# FIXME: Figure out why image_diff returns other values.
- result = False
+ result = None
except OSError, e:
if e.errno == errno.ENOENT or e.errno == errno.EACCES:
_compare_available = False
else:
- raise e
+ raise
finally:
+ if exit_code == 1:
+ result = self._filesystem.read_binary_file(diff_filename)
self._filesystem.rmtree(str(tempdir))
return result
Modified: trunk/Tools/Scripts/webkitpy/layout_tests/port/chromium_unittest.py (90137 => 90138)
--- trunk/Tools/Scripts/webkitpy/layout_tests/port/chromium_unittest.py 2011-06-30 18:32:30 UTC (rev 90137)
+++ trunk/Tools/Scripts/webkitpy/layout_tests/port/chromium_unittest.py 2011-06-30 18:37:16 UTC (rev 90138)
@@ -214,13 +214,19 @@
mock_options = mocktool.MockOptions()
port = ChromiumPortTest.TestLinuxPort(mock_options)
+ mock_image_diff = "MOCK Image Diff"
+
+ def mock_run_command(args):
+ port._filesystem.write_binary_file(args[4], mock_image_diff)
+ return 1
+
# Images are different.
- port._executive = executive_mock.MockExecutive2(exit_code=0)
- self.assertEquals(False, port.diff_image("EXPECTED", "ACTUAL"))
+ port._executive = executive_mock.MockExecutive2(run_command_fn=mock_run_command)
+ self.assertEquals(mock_image_diff, port.diff_image("EXPECTED", "ACTUAL"))
# Images are the same.
- port._executive = executive_mock.MockExecutive2(exit_code=1)
- self.assertEquals(True, port.diff_image("EXPECTED", "ACTUAL"))
+ port._executive = executive_mock.MockExecutive2(exit_code=0)
+ self.assertEquals(None, port.diff_image("EXPECTED", "ACTUAL"))
# There was some error running image_diff.
port._executive = executive_mock.MockExecutive2(exit_code=2)
Modified: trunk/Tools/Scripts/webkitpy/layout_tests/port/port_testcase.py (90137 => 90138)
--- trunk/Tools/Scripts/webkitpy/layout_tests/port/port_testcase.py 2011-06-30 18:32:30 UTC (rev 90137)
+++ trunk/Tools/Scripts/webkitpy/layout_tests/port/port_testcase.py 2011-06-30 18:37:16 UTC (rev 90138)
@@ -229,24 +229,24 @@
port = self.make_port()
if not port:
return
- self.assertFalse(port.diff_image(None, None, None))
- self.assertFalse(port.diff_image(None, '', None))
- self.assertFalse(port.diff_image('', None, None))
- self.assertFalse(port.diff_image('', '', None))
+ self.assertFalse(port.diff_image(None, None))
+ self.assertFalse(port.diff_image(None, ''))
+ self.assertFalse(port.diff_image('', None))
+ self.assertFalse(port.diff_image('', ''))
def test_diff_image__missing_actual(self):
port = self.make_port()
if not port:
return
- self.assertTrue(port.diff_image(None, 'foo', None))
- self.assertTrue(port.diff_image('', 'foo', None))
+ self.assertTrue(port.diff_image(None, 'foo'))
+ self.assertTrue(port.diff_image('', 'foo'))
def test_diff_image__missing_expected(self):
port = self.make_port()
if not port:
return
- self.assertTrue(port.diff_image('foo', None, None))
- self.assertTrue(port.diff_image('foo', '', None))
+ self.assertTrue(port.diff_image('foo', None))
+ self.assertTrue(port.diff_image('foo', ''))
def test_check_build(self):
port = self.make_port()
Modified: trunk/Tools/Scripts/webkitpy/layout_tests/port/test.py (90137 => 90138)
--- trunk/Tools/Scripts/webkitpy/layout_tests/port/test.py 2011-06-30 18:32:30 UTC (rev 90137)
+++ trunk/Tools/Scripts/webkitpy/layout_tests/port/test.py 2011-06-30 18:37:16 UTC (rev 90138)
@@ -144,6 +144,8 @@
tests.add('failures/unexpected/text-image-checksum.html',
actual_text='text-image-checksum_fail-txt',
actual_checksum='text-image-checksum_fail-checksum')
+ tests.add('failures/unexpected/checksum-with-matching-image.html',
+ actual_checksum='text-image-checksum_fail-checksum')
tests.add('failures/unexpected/timeout.html', timeout=True)
tests.add('http/tests/passes/text.html')
tests.add('http/tests/passes/image.html')
@@ -313,13 +315,11 @@
def default_configuration(self):
return 'Release'
- def diff_image(self, expected_contents, actual_contents,
- diff_filename=None):
+ def diff_image(self, expected_contents, actual_contents):
diffed = actual_contents != expected_contents
- if diffed and diff_filename:
- self._filesystem.write_binary_file(diff_filename,
- "< %s\n---\n> %s\n" % (expected_contents, actual_contents))
- return diffed
+ if diffed:
+ return "< %s\n---\n> %s\n" % (expected_contents, actual_contents)
+ return None
def layout_tests_dir(self):
return LAYOUT_TEST_DIR
Modified: trunk/Tools/Scripts/webkitpy/layout_tests/port/webkit.py (90137 => 90138)
--- trunk/Tools/Scripts/webkitpy/layout_tests/port/webkit.py 2011-06-30 18:32:30 UTC (rev 90137)
+++ trunk/Tools/Scripts/webkitpy/layout_tests/port/webkit.py 2011-06-30 18:37:16 UTC (rev 90138)
@@ -132,22 +132,20 @@
return False
return True
- def diff_image(self, expected_contents, actual_contents,
- diff_filename=None):
- """Return True if the two files are different. Also write a delta
- image of the two images into |diff_filename| if it is not None."""
-
+ def diff_image(self, expected_contents, actual_contents):
# Handle the case where the test didn't actually generate an image.
# FIXME: need unit tests for this.
if not actual_contents and not expected_contents:
- return False
+ return None
if not actual_contents or not expected_contents:
+ # FIXME: It's not clear what we should return in this case.
+ # Maybe we should throw an exception?
return True
- sp = self._diff_image_request(expected_contents, actual_contents)
- return self._diff_image_reply(sp, diff_filename)
+ process = self._start_image_diff_process(expected_contents, actual_contents)
+ return self._read_image_diff(process)
- def _diff_image_request(self, expected_contents, actual_contents):
+ def _start_image_diff_process(self, expected_contents, actual_contents):
# FIXME: There needs to be a more sane way of handling default
# values for options so that you can distinguish between a default
# value of None and a default value that wasn't set.
@@ -156,15 +154,14 @@
else:
tolerance = 0.1
command = [self._path_to_image_diff(), '--tolerance', str(tolerance)]
- sp = server_process.ServerProcess(self, 'ImageDiff', command)
+ process = server_process.ServerProcess(self, 'ImageDiff', command)
- sp.write('Content-Length: %d\n%sContent-Length: %d\n%s' %
- (len(actual_contents), actual_contents,
- len(expected_contents), expected_contents))
+ process.write('Content-Length: %d\n%sContent-Length: %d\n%s' % (
+ len(actual_contents), actual_contents,
+ len(expected_contents), expected_contents))
+ return process
- return sp
-
- def _diff_image_reply(self, sp, diff_filename):
+ def _read_image_diff(self, sp):
timeout = 2.0
deadline = time.time() + timeout
output = sp.read_line(timeout)
@@ -181,19 +178,16 @@
timeout = deadline - time.time()
output = sp.read_line(deadline)
- result = True
- if output.startswith('diff'):
- m = re.match('diff: (.+)% (passed|failed)', output)
- if m.group(2) == 'passed':
- result = False
- elif output and diff_filename:
- self._filesystem.write_binary_file(diff_filename, output)
- elif sp.timed_out:
+ if sp.timed_out:
_log.error("ImageDiff timed out")
- elif sp.crashed:
+ if sp.crashed:
_log.error("ImageDiff crashed")
sp.stop()
- return result
+ if output.startswith('diff'):
+ m = re.match('diff: (.+)% (passed|failed)', output)
+ if m.group(2) == 'passed':
+ return None
+ return output
def default_results_directory(self):
# Results are store relative to the built products to make it easy
Modified: trunk/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py (90137 => 90138)
--- trunk/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py 2011-06-30 18:32:30 UTC (rev 90137)
+++ trunk/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py 2011-06-30 18:37:16 UTC (rev 90138)
@@ -264,9 +264,8 @@
# We do a logging run here instead of a passing run in order to
# suppress the output from the json generator.
res, buildbot_output, regular_output, user = logging_run(['--clobber-old-results'], record_results=True, filesystem=fs)
- res, buildbot_output, regular_output, user = logging_run(
- ['--print-last-failures'], filesystem=fs)
- self.assertEqual(regular_output.get(), ['\n\n'])
+ res, buildbot_output, regular_output, user = logging_run(['--print-last-failures'], filesystem=fs)
+ self.assertEqual(regular_output.get(), [u'failures/expected/checksum.html\n\n'])
self.assertEqual(buildbot_output.get(), [])
def test_lint_test_files(self):
@@ -421,6 +420,16 @@
filesystem=fs)
self.assertTrue(fs.read_text_file('/tmp/layout-test-results/unexpected_results.json').find('{"crash-with-stderr.html":{"expected":"PASS","actual":"CRASH","has_stderr":true}}') != -1)
+ def test_no_image_failure_with_image_diff(self):
+ fs = port.unit_test_filesystem()
+ res, buildbot_output, regular_output, user = logging_run([
+ 'failures/unexpected/checksum-with-matching-image.html',
+ ],
+ tests_included=True,
+ record_results=True,
+ filesystem=fs)
+ self.assertTrue(fs.read_text_file('/tmp/layout-test-results/unexpected_results.json').find('"num_regressions":0') != -1)
+
def test_crash_log(self):
mock_crash_report = 'mock-crash-report'
fs = port.unit_test_filesystem()
@@ -559,8 +568,7 @@
def test_tolerance(self):
class ImageDiffTestPort(TestPort):
- def diff_image(self, expected_contents, actual_contents,
- diff_filename=None):
+ def diff_image(self, expected_contents, actual_contents):
self.tolerance_used_for_diff_image = self._options.tolerance
return True
Modified: trunk/Tools/Scripts/webkitpy/to_be_moved/rebaseline_chromium_webkit_tests.py (90137 => 90138)
--- trunk/Tools/Scripts/webkitpy/to_be_moved/rebaseline_chromium_webkit_tests.py 2011-06-30 18:32:30 UTC (rev 90137)
+++ trunk/Tools/Scripts/webkitpy/to_be_moved/rebaseline_chromium_webkit_tests.py 2011-06-30 18:37:16 UTC (rev 90138)
@@ -517,7 +517,7 @@
"""
if is_image:
- return self._port.diff_image(output1, output2, None)
+ return self._port.diff_image(output1, output2)
return self._port.compare_text(output1, output2)
@@ -602,7 +602,8 @@
_log.debug(' Html: diffing "%s" and "%s"', old_file, new_file)
old_output = self._filesystem.read_binary_file(old_file)
new_output = self._filesystem.read_binary_file(new_file)
- has_diff = self._port.diff_image(old_output, new_output, diff_file)
+ image_diff = self._port.diff_image(old_output, new_output)
+ self._filesystem.write_binary_file(diff_file, image_diff)
if has_diff:
_log.debug(' Html: created baseline diff file: "%s".', diff_file)
