2010-04-11 Eric Seidel <eric@webkit.org>
authoreric@webkit.org <eric@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 11 Apr 2010 07:41:56 +0000 (07:41 +0000)
committereric@webkit.org <eric@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 11 Apr 2010 07:41:56 +0000 (07:41 +0000)
        Reviewed by Adam Barth.

        Add PrettyPatch links to new-run-webkit-tests output
        https://bugs.webkit.org/show_bug.cgi?id=37406

        * Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:
         - We're leaking a file handle here, add a FIXME.
        * Scripts/webkitpy/layout_tests/layout_package/test_failures.py:
         - Add pretty diff links.
        * Scripts/webkitpy/layout_tests/port/base.py:
         - Add support for generating pretty diffs using PrettyPatch.
        * Scripts/webkitpy/layout_tests/port/webkit.py:
         - We're leaking another file handle here, another FIXME.
        * Scripts/webkitpy/layout_tests/test_types/image_diff.py:
         - Update write_output_files signature.
        * Scripts/webkitpy/layout_tests/test_types/test_type_base.py:
         - Remove unused arguments from write_output_files.
         - Add support for dumping pretty diffs to write_output_files.
         - Fix a bunch of file descriptor leaks in this file.
        * Scripts/webkitpy/layout_tests/test_types/text_diff.py:
         - Update write_output_files signature.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@57446 268f45cc-cd09-0410-ab3c-d52691b4dbfc

WebKitTools/ChangeLog
WebKitTools/Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py
WebKitTools/Scripts/webkitpy/layout_tests/layout_package/test_failures.py
WebKitTools/Scripts/webkitpy/layout_tests/port/base.py
WebKitTools/Scripts/webkitpy/layout_tests/port/webkit.py
WebKitTools/Scripts/webkitpy/layout_tests/test_types/image_diff.py
WebKitTools/Scripts/webkitpy/layout_tests/test_types/test_type_base.py
WebKitTools/Scripts/webkitpy/layout_tests/test_types/text_diff.py

index b1e0733..9f7159d 100644 (file)
@@ -1,3 +1,27 @@
+2010-04-11  Eric Seidel  <eric@webkit.org>
+
+        Reviewed by Adam Barth.
+
+        Add PrettyPatch links to new-run-webkit-tests output
+        https://bugs.webkit.org/show_bug.cgi?id=37406
+
+        * Scripts/webkitpy/layout_tests/layout_package/dump_render_tree_thread.py:
+         - We're leaking a file handle here, add a FIXME.
+        * Scripts/webkitpy/layout_tests/layout_package/test_failures.py:
+         - Add pretty diff links.
+        * Scripts/webkitpy/layout_tests/port/base.py:
+         - Add support for generating pretty diffs using PrettyPatch.
+        * Scripts/webkitpy/layout_tests/port/webkit.py:
+         - We're leaking another file handle here, another FIXME.
+        * Scripts/webkitpy/layout_tests/test_types/image_diff.py:
+         - Update write_output_files signature.
+        * Scripts/webkitpy/layout_tests/test_types/test_type_base.py:
+         - Remove unused arguments from write_output_files.
+         - Add support for dumping pretty diffs to write_output_files.
+         - Fix a bunch of file descriptor leaks in this file.
+        * Scripts/webkitpy/layout_tests/test_types/text_diff.py:
+         - Update write_output_files signature.
+
 2010-04-10  Adam Barth  <abarth@webkit.org>
 
         Reviewed by Eric Seidel.
index 3fd38fe..93b4c79 100644 (file)
@@ -89,7 +89,7 @@ def process_output(port, test_info, test_types, test_args, configuration,
                                 test_info.filename))
         filename = os.path.splitext(filename)[0] + "-stack.txt"
         port.maybe_make_directory(os.path.split(filename)[0])
-        open(filename, "wb").write(error)
+        open(filename, "wb").write(error)  # FIXME: This leaks a file handle.
     elif error:
         _log.debug("Previous test output extra lines after dump:\n%s" %
                    error)
index 022973a..60bdbca 100644 (file)
@@ -110,7 +110,7 @@ class FailureWithType(TestFailure):
 
     def __init__(self, test_type):
         TestFailure.__init__(self)
-        # TODO(ojan): This class no longer needs to know the test_type.
+        # FIXME: This class no longer needs to know the test_type.
         self._test_type = test_type
 
     # Filename suffixes used by ResultHtmlOutput.
@@ -127,6 +127,9 @@ class FailureWithType(TestFailure):
               single item is the [actual] filename suffix.
               If out_names is empty, returns the empty string.
         """
+        # FIXME: Seems like a bad idea to separate the display name data
+        # from the path data by hard-coding the display name here
+        # and passing in the path information via out_names.
         links = ['']
         uris = [self.relative_output_filename(filename, fn) for
                 fn in out_names]
@@ -138,6 +141,8 @@ class FailureWithType(TestFailure):
             links.append("<a href='%s'>diff</a>" % uris[2])
         if len(uris) > 3:
             links.append("<a href='%s'>wdiff</a>" % uris[3])
+        if len(uris) > 4:
+            links.append("<a href='%s'>pretty diff</a>" % uris[4])
         return ' '.join(links)
 
     def result_html_output(self, filename):
@@ -192,9 +197,10 @@ class FailureMissingResult(FailureWithType):
 class FailureTextMismatch(FailureWithType):
     """Text diff output failed."""
     # Filename suffixes used by ResultHtmlOutput.
+    # FIXME: Why don't we use the constants from TestTypeBase here?
     OUT_FILENAMES = ["-actual.txt", "-expected.txt", "-diff.txt"]
     OUT_FILENAMES_WDIFF = ["-actual.txt", "-expected.txt", "-diff.txt",
-                           "-wdiff.html"]
+                           "-wdiff.html", "-pretty-diff.html"]
 
     def __init__(self, test_type, has_wdiff):
         FailureWithType.__init__(self, test_type)
index 2f2163b..988e529 100644 (file)
@@ -519,6 +519,7 @@ class Port(object):
                '--end-insert=##WDIFF_END##',
                actual_filename,
                expected_filename]
+        # FIXME: Why not just check os.exists(executable) once?
         global _wdiff_available
         result = ''
         try:
@@ -541,6 +542,7 @@ class Port(object):
             # http://bugs.python.org/issue1236
             if _wdiff_available:
                 try:
+                    # FIXME: Use Executive() here.
                     wdiff = subprocess.Popen(cmd,
                         stdout=subprocess.PIPE).communicate()[0]
                 except ValueError, e:
@@ -562,6 +564,12 @@ class Port(object):
                 raise e
         return result
 
+    def pretty_patch_text(self, diff_path):
+        pretty_patch_path = self.path_from_webkit_base("BugsSite", "PrettyPatch")
+        prettify_path = os.path.join(pretty_patch_path, "prettify.rb")
+        command = ["ruby", "-I", pretty_patch_path, prettify_path, diff_path]
+        return self._executive.run_command(command)
+
     def default_configuration(self):
         return "Release"
 
index 2f4badc..dd8d180 100644 (file)
@@ -163,7 +163,7 @@ class WebKitPort(base.Port):
             if m.group(2) == 'passed':
                 result = False
         elif output and diff_filename:
-            open(diff_filename, 'w').write(output)
+            open(diff_filename, 'w').write(output)  # FIXME: This leaks a file handle.
         elif sp.timed_out:
             _log.error("ImageDiff timed out on %s" % expected_filename)
         elif sp.crashed:
index dca2fbc..b414358 100644 (file)
@@ -150,9 +150,9 @@ class ImageDiff(test_type_base.TestTypeBase):
 
         if not os.path.isfile(expected_png_file):
             # Report a missing expected PNG file.
-            self.write_output_files(port, filename, '', '.checksum',
+            self.write_output_files(port, filename, '.checksum',
                                     test_args.hash, expected_hash,
-                                    diff=False, wdiff=False)
+                                    print_text_diffs=False)
             self._copy_output_png(filename, test_args.png_path, '-actual.png')
             failures.append(test_failures.FailureMissingImage(self))
             return failures
@@ -160,10 +160,9 @@ class ImageDiff(test_type_base.TestTypeBase):
             # Hash matched (no diff needed, okay to return).
             return failures
 
-
-        self.write_output_files(port, filename, '', '.checksum',
+        self.write_output_files(port, filename, '.checksum',
                                 test_args.hash, expected_hash,
-                                diff=False, wdiff=False)
+                                print_text_diffs=False)
         self._copy_output_png(filename, test_args.png_path, '-actual.png')
         self._copy_output_png(filename, expected_png_file, '-expected.png')
 
index e4b9766..4c99be0 100644 (file)
@@ -70,6 +70,7 @@ class TestTypeBase(object):
     FILENAME_SUFFIX_EXPECTED = "-expected"
     FILENAME_SUFFIX_DIFF = "-diff"
     FILENAME_SUFFIX_WDIFF = "-wdiff.html"
+    FILENAME_SUFFIX_PRETTY_PATCH = "-pretty-diff.html"
     FILENAME_SUFFIX_COMPARE = "-diff.png"
 
     def __init__(self, port, root_output_dir):
@@ -111,7 +112,7 @@ class TestTypeBase(object):
         self._port.maybe_make_directory(output_dir)
         output_path = os.path.join(output_dir, output_file)
         _log.debug('writing new baseline to "%s"' % (output_path))
-        open(output_path, "wb").write(data)
+        self._write_into_file_at_path(output_path, data)
 
     def output_filename(self, filename, modifier):
         """Returns a filename inside the output dir that contains modifier.
@@ -149,49 +150,52 @@ class TestTypeBase(object):
         """
         raise NotImplemented
 
-    def write_output_files(self, port, filename, test_type, file_type,
-                           output, expected, diff=True, wdiff=False):
+    def _write_into_file_at_path(self, file_path, contents):
+        file = open(file_path, "wb")
+        file.write(contents)
+        file.close()
+
+    def write_output_files(self, port, filename, file_type,
+                           output, expected, print_text_diffs=False):
         """Writes the test output, the expected output and optionally the diff
         between the two to files in the results directory.
 
         The full output filename of the actual, for example, will be
-          <filename><test_type>-actual<file_type>
+          <filename>-actual<file_type>
         For instance,
-          my_test-simp-actual.txt
+          my_test-actual.txt
 
         Args:
           filename: The test filename
-          test_type: A string describing the test type, e.g. "simp"
           file_type: A string describing the test output file type, e.g. ".txt"
           output: A string containing the test output
           expected: A string containing the expected test output
-          diff: if True, write a file containing the diffs too. This should be
-              False for results that are not text
-          wdiff: if True, write an HTML file containing word-by-word diffs
+          print_text_diffs: True for text diffs. (FIXME: We should be able to get this from the file type?)
         """
         self._make_output_directory(filename)
-        actual_filename = self.output_filename(filename,
-            test_type + self.FILENAME_SUFFIX_ACTUAL + file_type)
-        expected_filename = self.output_filename(filename,
-            test_type + self.FILENAME_SUFFIX_EXPECTED + file_type)
+        actual_filename = self.output_filename(filename, self.FILENAME_SUFFIX_ACTUAL + file_type)
+        expected_filename = self.output_filename(filename, self.FILENAME_SUFFIX_EXPECTED + file_type)
         if output:
-            open(actual_filename, "wb").write(output)
+            self._write_into_file_at_path(actual_filename, output)
         if expected:
-            open(expected_filename, "wb").write(expected)
+            self._write_into_file_at_path(expected_filename, expected)
 
         if not output or not expected:
             return
 
-        if diff:
-            diff = port.diff_text(expected, output, expected_filename,
-                                  actual_filename)
-            diff_filename = self.output_filename(filename,
-                test_type + self.FILENAME_SUFFIX_DIFF + file_type)
-            open(diff_filename, "wb").write(diff)
-
-        if wdiff:
-            # Shell out to wdiff to get colored inline diffs.
-            wdiff = port.wdiff_text(expected_filename, actual_filename)
-            filename = self.output_filename(filename, test_type +
-                                            self.FILENAME_SUFFIX_WDIFF)
-            out = open(filename, 'wb').write(wdiff)
+        if not print_text_diffs:
+            return
+
+        diff = port.diff_text(expected, output, expected_filename, actual_filename)
+        diff_filename = self.output_filename(filename, self.FILENAME_SUFFIX_DIFF + file_type)
+        self._write_into_file_at_path(diff_filename, diff)
+
+        # Shell out to wdiff to get colored inline diffs.
+        wdiff = port.wdiff_text(expected_filename, actual_filename)
+        wdiff_filename = self.output_filename(filename, self.FILENAME_SUFFIX_WDIFF)
+        self._write_into_file_at_path(wdiff_filename, wdiff)
+
+        # Use WebKit's PrettyPatch.rb to get an HTML diff.
+        pretty_patch = port.pretty_patch_text(diff_filename)
+        pretty_patch_filename = self.output_filename(filename, self.FILENAME_SUFFIX_PRETTY_PATCH)
+        self._write_into_file_at_path(pretty_patch_filename, pretty_patch)
index 1af786c..8f7907c 100644 (file)
@@ -98,8 +98,8 @@ class TestTextDiff(test_type_base.TestTypeBase):
         # Write output files for new tests, too.
         if port.compare_text(output, expected):
             # Text doesn't match, write output files.
-            self.write_output_files(port, filename, "", ".txt", output,
-                                    expected, diff=True, wdiff=True)
+            self.write_output_files(port, filename, ".txt", output,
+                                    expected, print_text_diffs=True)
 
             if expected == '':
                 failures.append(test_failures.FailureMissingResult(self))