Changeset 126188 in webkit

Timestamp:

Aug 21, 2012 2:36:15 PM (12 years ago)

Author:

dpranke@chromium.org

Message:

_compare_image() swaps actual and expected images by mistake
​https://bugs.webkit.org/show_bug.cgi?id=94567

Reviewed by Ojan Vafai.

Re-work the code so that we consistently pass (expected, actual)
across all of the compare/diff routines.

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

(SingleTestRunner._run_compare_test):
(SingleTestRunner._compare_output):
(SingleTestRunner._compare_text):
(SingleTestRunner._compare_audio):
(SingleTestRunner._compare_image):
(SingleTestRunner._run_reftest):
(SingleTestRunner._compare_output_with_reference):

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

(write_test_result):

Location:

trunk/Tools

Files:

• ChangeLog (modified) (1 diff)
• Scripts/webkitpy/layout_tests/controllers/single_test_runner.py (modified) (9 diffs)
• Scripts/webkitpy/layout_tests/controllers/test_result_writer.py (modified) (1 diff)
• Scripts/webkitpy/layout_tests/port/chromium.py (modified) (1 diff)
• Scripts/webkitpy/layout_tests/port/image_diff.py (modified) (1 diff)

trunk/Tools/ChangeLog

@@ +1 @@
+2012-08-21  Dirk Pranke  <dpranke@chromium.org>
+
+        _compare_image() swaps actual and expected images by mistake
+        https://bugs.webkit.org/show_bug.cgi?id=94567
+
+        Reviewed by Ojan Vafai.
+
+        Re-work the code so that we consistently pass (expected, actual)
+        across all of the compare/diff routines.
+
+        * Scripts/webkitpy/layout_tests/controllers/single_test_runner.py:
+        (SingleTestRunner._run_compare_test):
+        (SingleTestRunner._compare_output):
+        (SingleTestRunner._compare_text):
+        (SingleTestRunner._compare_audio):
+        (SingleTestRunner._compare_image):
+        (SingleTestRunner._run_reftest):
+        (SingleTestRunner._compare_output_with_reference):
+        * Scripts/webkitpy/layout_tests/controllers/test_result_writer.py:
+        (write_test_result):
+
 2012-08-21  Adam Barth  <abarth@webkit.org>
 

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

@@ -111 +111 @@
             driver_output.strip_metrics()
 
-        test_result = self._compare_output(driver_output, expected_driver_output)
+        test_result = self._compare_output(expected_driver_output, driver_output)
         if self._options.new_test_results:
             self._add_missing_baselines(test_result, driver_output)
@@ -210 +210 @@
         return failures
 
-    def _compare_output(self, driver_output, expected_driver_output):
+    def _compare_output(self, expected_driver_output, driver_output):
         failures = []
         failures.extend(self._handle_error(driver_output))
@@ -219 +219 @@
             return TestResult(self._test_name, failures, driver_output.test_time, driver_output.has_stderr())
 
-        failures.extend(self._compare_text(driver_output.text, expected_driver_output.text))
-        failures.extend(self._compare_audio(driver_output.audio, expected_driver_output.audio))
+        failures.extend(self._compare_text(expected_driver_output.text, driver_output.text))
+        failures.extend(self._compare_audio(expected_driver_output.audio, driver_output.audio))
         if self._should_run_pixel_test:
-            failures.extend(self._compare_image(driver_output, expected_driver_output))
+            failures.extend(self._compare_image(expected_driver_output, driver_output))
         return TestResult(self._test_name, failures, driver_output.test_time, driver_output.has_stderr())
 
-    def _compare_text(self, actual_text, expected_text):
+    def _compare_text(self, expected_text, actual_text):
         failures = []
         if (expected_text and actual_text and
             # Assuming expected_text is already normalized.
-            self._port.do_text_results_differ(self._get_normalized_output_text(actual_text), expected_text)):
+            self._port.do_text_results_differ(expected_text, self._get_normalized_output_text(actual_text))):
             failures.append(test_failures.FailureTextMismatch())
         elif actual_text and not expected_text:
@@ -235 +235 @@
         return failures
 
-    def _compare_audio(self, actual_audio, expected_audio):
+    def _compare_audio(self, expected_audio, actual_audio):
         failures = []
         if (expected_audio and actual_audio and
-            self._port.do_audio_results_differ(actual_audio, expected_audio)):
+            self._port.do_audio_results_differ(expected_audio, actual_audio)):
             failures.append(test_failures.FailureAudioMismatch())
         elif actual_audio and not expected_audio:
@@ -255 +255 @@
     # FIXME: This function also creates the image diff. Maybe that work should
     # be handled elsewhere?
-    def _compare_image(self, driver_output, expected_driver_output):
+    def _compare_image(self, expected_driver_output, driver_output):
         failures = []
         # If we didn't produce a hash file, this test must be text-only.
@@ -265 +265 @@
             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)
+            diff_result = self._port.diff_image(expected_driver_output.image, driver_output.image)
             err_str = diff_result[2]
             # FIXME: see https://bugs.webkit.org/show_bug.cgi?id=94277 and
@@ -299 +299 @@
             reference_test_name = self._port.relative_test_filename(reference_filename)
             reference_output = self._driver.run_test(DriverInput(reference_test_name, self._timeout, test_output.image_hash, should_run_pixel_test=True), self._stop_when_done)
-            test_result = self._compare_output_with_reference(test_output, reference_output, reference_filename, expectation == '!=')
+            test_result = self._compare_output_with_reference(reference_output, test_output, reference_filename, expectation == '!=')
 
             if (expectation == '!=' and test_result.failures) or (expectation == '==' and not test_result.failures):
@@ -309 +309 @@
         return TestResult(self._test_name, test_result.failures, total_test_time + test_result.test_run_time, test_result.has_stderr)
 
-    def _compare_output_with_reference(self, driver_output1, driver_output2, reference_filename, mismatch):
-        total_test_time = driver_output1.test_time + driver_output2.test_time
-        has_stderr = driver_output1.has_stderr() or driver_output2.has_stderr()
-        failures = []
-        failures.extend(self._handle_error(driver_output1))
+    def _compare_output_with_reference(self, reference_driver_output, actual_driver_output, reference_filename, mismatch):
+        total_test_time = reference_driver_output.test_time + actual_driver_output.test_time
+        has_stderr = reference_driver_output.has_stderr() or actual_driver_output.has_stderr()
+        failures = []
+        failures.extend(self._handle_error(actual_driver_output))
         if failures:
             # Don't continue any more if we already have crash or timeout.
             return TestResult(self._test_name, failures, total_test_time, has_stderr)
-        failures.extend(self._handle_error(driver_output2, reference_filename=reference_filename))
+        failures.extend(self._handle_error(reference_driver_output, reference_filename=reference_filename))
         if failures:
             return TestResult(self._test_name, failures, total_test_time, has_stderr)
@@ -325 +325 @@
             return TestResult(self._test_name, failures, total_test_time, has_stderr)
 
-        if not driver_output1.image_hash and not driver_output2.image_hash:
+        if not reference_driver_output.image_hash and not actual_driver_output.image_hash:
             failures.append(test_failures.FailureReftestNoImagesGenerated(reference_filename))
         elif mismatch:
-            if driver_output1.image_hash == driver_output2.image_hash:
+            if reference_driver_output.image_hash == actual_driver_output.image_hash:
                 failures.append(test_failures.FailureReftestMismatchDidNotOccur(reference_filename))
-        elif driver_output1.image_hash != driver_output2.image_hash:
+        elif reference_driver_output.image_hash != actual_driver_output.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()
+        has_stderr = reference_driver_output.has_stderr() or actual_driver_output.has_stderr()
 
         return TestResult(self._test_name, failures, total_test_time, has_stderr)

trunk/Tools/Scripts/webkitpy/layout_tests/controllers/test_result_writer.py

@@ -70 +70 @@
             # 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(driver_output.image, expected_driver_output.image, tolerance=0)
+                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)

trunk/Tools/Scripts/webkitpy/layout_tests/port/chromium.py

@@ -217 +217 @@
 
         executable = self._path_to_image_diff()
+        # Note that although we are handed 'old', 'new', image_diff wants 'new', 'old'.
         comand = [executable, '--diff', native_actual_filename, native_expected_filename, native_diff_filename]
 

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

@@ -58 +58 @@
             if not self._process:
                 self._start(tolerance)
+            # Note that although we are handed 'old', 'new', ImageDiff wants 'new', 'old'.
             self._process.write('Content-Length: %d\n%sContent-Length: %d\n%s' % (
                 len(actual_contents), actual_contents,