Diff
Modified: trunk/Tools/ChangeLog (124957 => 124958)
--- trunk/Tools/ChangeLog 2012-08-08 01:06:45 UTC (rev 124957)
+++ trunk/Tools/ChangeLog 2012-08-08 01:08:48 UTC (rev 124958)
@@ -1,5 +1,50 @@
2012-08-07 Dirk Pranke <dpra...@chromium.org>
+ 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):
+
+2012-08-07 Dirk Pranke <dpra...@chromium.org>
+
nrwt: --no-build isn't working
https://bugs.webkit.org/show_bug.cgi?id=93415
Modified: trunk/Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py (124957 => 124958)
--- trunk/Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py 2012-08-08 01:06:45 UTC (rev 124957)
+++ trunk/Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py 2012-08-08 01:08:48 UTC (rev 124958)
@@ -264,12 +264,18 @@
failures.append(test_failures.FailureMissingImageHash())
elif driver_output.image_hash != expected_driver_output.image_hash:
diff_result = self._port.diff_image(driver_output.image, expected_driver_output.image)
- driver_output.image_diff = diff_result[0]
- if driver_output.image_diff:
- failures.append(test_failures.FailureImageHashMismatch(diff_result[1]))
+ err_str = diff_result[2]
+ if err_str:
+ _log.warning(' %s : %s' % (self._test_name, err_str))
+ failures.append(test_failures.FailureImageHashMismatch())
+ driver_output.error = (driver_output.error or '') + err_str
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 pixel test still passes)' % self._test_name)
+ driver_output.image_diff = diff_result[0]
+ if driver_output.image_diff:
+ failures.append(test_failures.FailureImageHashMismatch(diff_result[1]))
+ 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 pixel test still passes)' % self._test_name)
return failures
def _run_reftest(self):
@@ -317,4 +323,8 @@
failures.append(test_failures.FailureReftestMismatchDidNotOccur(reference_filename))
elif driver_output1.image_hash != driver_output2.image_hash:
failures.append(test_failures.FailureReftestMismatch(reference_filename))
+
+ # recompute in case we added to stderr during diff_image
+ has_stderr = driver_output1.has_stderr() or driver_output2.has_stderr()
+
return TestResult(self._test_name, failures, total_test_time, has_stderr)
Modified: trunk/Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer.py (124957 => 124958)
--- trunk/Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer.py 2012-08-08 01:06:45 UTC (rev 124957)
+++ trunk/Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer.py 2012-08-08 01:08:48 UTC (rev 124958)
@@ -69,7 +69,7 @@
# 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 = port.diff_image(driver_output.image, expected_driver_output.image, tolerance=0)
+ diff_image, diff_percent, err_str = port.diff_image(driver_output.image, expected_driver_output.image, tolerance=0)
if diff_image:
writer.write_image_diff_files(diff_image)
failure.diff_percent = diff_percent
Modified: trunk/Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer_unittest.py (124957 => 124958)
--- trunk/Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer_unittest.py 2012-08-08 01:06:45 UTC (rev 124957)
+++ trunk/Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer_unittest.py 2012-08-08 01:08:48 UTC (rev 124958)
@@ -42,7 +42,7 @@
class ImageDiffTestPort(TestPort):
def diff_image(self, expected_contents, actual_contents, tolerance=None):
used_tolerance_values.append(tolerance)
- return (True, 1)
+ return (True, 1, None)
host = MockHost()
port = ImageDiffTestPort(host)
Modified: trunk/Tools/Scripts/webkitpy/layout_tests/port/base.py (124957 => 124958)
--- trunk/Tools/Scripts/webkitpy/layout_tests/port/base.py 2012-08-08 01:06:45 UTC (rev 124957)
+++ trunk/Tools/Scripts/webkitpy/layout_tests/port/base.py 2012-08-08 01:08:48 UTC (rev 124958)
@@ -319,7 +319,7 @@
return expected_audio != actual_audio
def diff_image(self, expected_contents, actual_contents, tolerance=None):
- """Compare two images and return a tuple of an image diff, and a percentage difference (0-100).
+ """Compare two images and return a tuple of an image diff, a percentage difference (0-100), and an error string.
|tolerance| should be a percentage value (0.0 - 100.0).
If it is omitted, the port default tolerance value is used.
@@ -327,9 +327,9 @@
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)
+ return (None, 0, None)
if not actual_contents or not expected_contents:
- return (True, 0)
+ return (True, 0, None)
if not self._image_differ:
self._image_differ = image_diff.ImageDiffer(self)
self.set_option_default('tolerance', 0.1)
Modified: trunk/Tools/Scripts/webkitpy/layout_tests/port/chromium.py (124957 => 124958)
--- trunk/Tools/Scripts/webkitpy/layout_tests/port/chromium.py 2012-08-08 01:06:45 UTC (rev 124957)
+++ trunk/Tools/Scripts/webkitpy/layout_tests/port/chromium.py 2012-08-08 01:08:48 UTC (rev 124958)
@@ -190,18 +190,16 @@
override_step, logging)
def diff_image(self, expected_contents, actual_contents, tolerance=None):
- # FIXME: need unit tests for this.
-
# tolerance is not used in chromium. Make sure caller doesn't pass tolerance other than zero or None.
assert (tolerance is None) or tolerance == 0
# If only one of them exists, return that one.
if not actual_contents and not expected_contents:
- return (None, 0)
+ return (None, 0, None)
if not actual_contents:
- return (expected_contents, 0)
+ return (expected_contents, 0, None)
if not expected_contents:
- return (actual_contents, 0)
+ return (actual_contents, 0, None)
tempdir = self._filesystem.mkdtemp()
@@ -221,29 +219,23 @@
comand = [executable, '--diff', native_actual_filename, native_expected_filename, native_diff_filename]
result = None
+ err_str = None
try:
exit_code = self._executive.run_command(comand, return_exit_code=True)
if exit_code == 0:
# The images are the same.
result = None
- elif exit_code != 1:
- _log.error("image diff returned an exit code of %s" % exit_code)
- # Returning None here causes the script to think that we
- # successfully created the diff even though we didn't.
- # FIXME: Consider raising an exception here, so that the error
- # is not accidentally overlooked while the test passes.
- result = None
- except OSError, e:
- if e.errno == errno.ENOENT or e.errno == errno.EACCES:
- _compare_available = False
+ elif exit_code == 1:
+ result = self._filesystem.read_binary_file(native_diff_filename)
else:
- raise
+ err_str = "image diff returned an exit code of %s" % exit_code
+ except OSError, e:
+ err_str = 'error running image diff: %s' % str(e)
finally:
- if exit_code == 1:
- result = self._filesystem.read_binary_file(native_diff_filename)
self._filesystem.rmtree(str(tempdir))
- return (result, 0) # FIXME: how to get % diff?
+ return (result, 0, err_str or None) # FIXME: how to get % diff?
+
def path_from_chromium_base(self, *comps):
"""Returns the full path to path made by joining the top of the
Chromium source tree and the list of path components in |*comps|."""
Modified: trunk/Tools/Scripts/webkitpy/layout_tests/port/chromium_port_testcase.py (124957 => 124958)
--- trunk/Tools/Scripts/webkitpy/layout_tests/port/chromium_port_testcase.py 2012-08-08 01:06:45 UTC (rev 124957)
+++ trunk/Tools/Scripts/webkitpy/layout_tests/port/chromium_port_testcase.py 2012-08-08 01:08:48 UTC (rev 124958)
@@ -158,6 +158,11 @@
exception_raised = True
self.assertFalse(exception_raised)
+ def test_diff_image_crashed(self):
+ port = ChromiumPortTestCase.TestLinuxPort()
+ port._executive = MockExecutive2(exit_code=2)
+ self.assertEquals(port.diff_image("EXPECTED", "ACTUAL"), (None, 0, 'image diff returned an exit code of 2'))
+
def test_expectations_files(self):
port = self.make_port()
port.port_name = 'chromium'
Modified: trunk/Tools/Scripts/webkitpy/layout_tests/port/image_diff.py (124957 => 124958)
--- trunk/Tools/Scripts/webkitpy/layout_tests/port/image_diff.py 2012-08-08 01:06:45 UTC (rev 124957)
+++ trunk/Tools/Scripts/webkitpy/layout_tests/port/image_diff.py 2012-08-08 01:08:48 UTC (rev 124958)
@@ -62,8 +62,7 @@
len(expected_contents), expected_contents))
return self._read()
except IOError as exception:
- _log.error("Failed to compute an image diff: %s" % str(exception))
- return (True, 0)
+ return (None, 0, "Failed to compute an image diff: %s" % str(exception))
def _start(self, tolerance):
command = [self._port._path_to_image_diff(), '--tolerance', str(tolerance)]
@@ -77,7 +76,7 @@
output = None
output_image = ""
- while True:
+ while not self._process.timed_out and not self._process.has_crashed():
output = self._process.read_stdout_line(deadline)
if self._process.timed_out or self._process.has_crashed() or not output:
break
@@ -93,12 +92,14 @@
break
stderr = self._process.pop_all_buffered_stderr()
+ err_str = ''
if stderr:
- _log.warn("ImageDiff produced stderr output:\n" + stderr)
+ err_str += "ImageDiff produced stderr output:\n" + stderr
if self._process.timed_out:
- _log.error("ImageDiff timed out")
+ err_str += "ImageDiff timed out\n"
if self._process.has_crashed():
- _log.error("ImageDiff crashed")
+ err_str += "ImageDiff crashed\n"
+
# FIXME: There is no need to shut down the ImageDiff server after every diff.
self._process.stop()
@@ -106,10 +107,10 @@
if output and output.startswith('diff'):
m = re.match('diff: (.+)% (passed|failed)', output)
if m.group(2) == 'passed':
- return [None, 0]
+ return (None, 0, None)
diff_percent = float(m.group(1))
- return (output_image, diff_percent)
+ return (output_image, diff_percent, err_str or None)
def stop(self):
if self._process:
Modified: trunk/Tools/Scripts/webkitpy/layout_tests/port/image_diff_unittest.py (124957 => 124958)
--- trunk/Tools/Scripts/webkitpy/layout_tests/port/image_diff_unittest.py 2012-08-08 01:06:45 UTC (rev 124957)
+++ trunk/Tools/Scripts/webkitpy/layout_tests/port/image_diff_unittest.py 2012-08-08 01:08:48 UTC (rev 124958)
@@ -46,7 +46,12 @@
class TestImageDiffer(unittest.TestCase):
- def test_diff_image(self):
+ def test_diff_image_failed(self):
port = FakePort(['diff: 100% failed\n'])
image_differ = ImageDiffer(port)
- self.assertEquals(image_differ.diff_image('foo', 'bar', 0.1), ('', 100.0))
+ self.assertEquals(image_differ.diff_image('foo', 'bar', 0.1), ('', 100.0, None))
+
+ def test_diff_image_passed(self):
+ port = FakePort(['diff: 0% passed\n'])
+ image_differ = ImageDiffer(port)
+ self.assertEquals(image_differ.diff_image('foo', 'bar', 0.1), (None, 0, None))
Modified: trunk/Tools/Scripts/webkitpy/layout_tests/port/mock_drt_unittest.py (124957 => 124958)
--- trunk/Tools/Scripts/webkitpy/layout_tests/port/mock_drt_unittest.py 2012-08-08 01:06:45 UTC (rev 124957)
+++ trunk/Tools/Scripts/webkitpy/layout_tests/port/mock_drt_unittest.py 2012-08-08 01:08:48 UTC (rev 124958)
@@ -60,6 +60,9 @@
def test_diff_image(self):
pass
+ def test_diff_image_crashed(self):
+ pass
+
def test_uses_apache(self):
pass
Modified: trunk/Tools/Scripts/webkitpy/layout_tests/port/port_testcase.py (124957 => 124958)
--- trunk/Tools/Scripts/webkitpy/layout_tests/port/port_testcase.py 2012-08-08 01:06:45 UTC (rev 124957)
+++ trunk/Tools/Scripts/webkitpy/layout_tests/port/port_testcase.py 2012-08-08 01:08:48 UTC (rev 124958)
@@ -259,19 +259,32 @@
port._server_process_constructor = make_proc
port.setup_test_run()
- self.assertEquals(port.diff_image('foo', 'bar'), ('', 100.0))
+ self.assertEquals(port.diff_image('foo', 'bar'), ('', 100.0, None))
self.assertEquals(self.proc.cmd[1:3], ["--tolerance", "0.1"])
- self.assertEquals(port.diff_image('foo', 'bar', None), ('', 100.0))
+ self.assertEquals(port.diff_image('foo', 'bar', None), ('', 100.0, None))
self.assertEquals(self.proc.cmd[1:3], ["--tolerance", "0.1"])
- self.assertEquals(port.diff_image('foo', 'bar', 0), ('', 100.0))
+ self.assertEquals(port.diff_image('foo', 'bar', 0), ('', 100.0, None))
self.assertEquals(self.proc.cmd[1:3], ["--tolerance", "0"])
port.clean_up_test_run()
self.assertTrue(self.proc.stopped)
self.assertEquals(port._image_differ, None)
+ def test_diff_image_crashed(self):
+ port = self.make_port()
+ self.proc = None
+
+ def make_proc(port, nm, cmd, env):
+ self.proc = MockServerProcess(port, nm, cmd, env, crashed=True)
+ return self.proc
+
+ port._server_process_constructor = make_proc
+ port.setup_test_run()
+ self.assertEquals(port.diff_image('foo', 'bar'), ('', 0, 'ImageDiff crashed\n'))
+ port.clean_up_test_run()
+
def test_check_wdiff(self):
port = self.make_port()
port.check_wdiff()
Modified: trunk/Tools/Scripts/webkitpy/layout_tests/port/server_process_mock.py (124957 => 124958)
--- trunk/Tools/Scripts/webkitpy/layout_tests/port/server_process_mock.py 2012-08-08 01:06:45 UTC (rev 124957)
+++ trunk/Tools/Scripts/webkitpy/layout_tests/port/server_process_mock.py 2012-08-08 01:08:48 UTC (rev 124958)
@@ -28,10 +28,10 @@
class MockServerProcess(object):
- def __init__(self, port_obj=None, name=None, cmd=None, env=None, universal_newlines=False, lines=None):
+ def __init__(self, port_obj=None, name=None, cmd=None, env=None, universal_newlines=False, lines=None, crashed=False):
self.timed_out = False
self.lines = lines or []
- self.crashed = False
+ self.crashed = crashed
self.writes = []
self.cmd = cmd
self.env = env
Modified: trunk/Tools/Scripts/webkitpy/layout_tests/port/test.py (124957 => 124958)
--- trunk/Tools/Scripts/webkitpy/layout_tests/port/test.py 2012-08-08 01:06:45 UTC (rev 124957)
+++ trunk/Tools/Scripts/webkitpy/layout_tests/port/test.py 2012-08-08 01:08:48 UTC (rev 124958)
@@ -406,8 +406,8 @@
def diff_image(self, expected_contents, actual_contents, tolerance=None):
diffed = actual_contents != expected_contents
if diffed:
- return ["< %s\n---\n> %s\n" % (expected_contents, actual_contents), 1]
- return (None, 0)
+ return ("< %s\n---\n> %s\n" % (expected_contents, actual_contents), 1, None)
+ return (None, 0, None)
def layout_tests_dir(self):
return LAYOUT_TEST_DIR
Modified: trunk/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py (124957 => 124958)
--- trunk/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py 2012-08-08 01:06:45 UTC (rev 124957)
+++ trunk/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py 2012-08-08 01:08:48 UTC (rev 124958)
@@ -783,7 +783,7 @@
class ImageDiffTestPort(TestPort):
def diff_image(self, expected_contents, actual_contents, tolerance=None):
self.tolerance_used_for_diff_image = self._options.tolerance
- return (True, 1)
+ return (True, 1, None)
def get_port_for_run(args):
options, parsed_args = run_webkit_tests.parse_args(args)
