Bug 21124 - --pixel should be ignored for tests that use dumpAsText()
: --pixel should be ignored for tests that use dumpAsText()
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Tools / Tests
: 528+ (Nightly build)
: Macintosh Intel Mac OS X 10.5
: P2 Normal
Assigned To: Simon Fraser (smfr)
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-09-25 16:59 PDT by Pierre-Olivier Latour
Modified: 2008-09-30 17:43 PDT (History)
1 user (show)

See Also:


Attachments
Patch, changelog (2.09 KB, patch)
2008-09-30 17:40 PDT, Simon Fraser (smfr)
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pierre-Olivier Latour 2008-09-25 16:59:21 PDT
If I understand things right, tests that use the dumpAsText() instead of dumping the render tree are supposed to be platform agnostic, so their results are in the same directory as the tests instead of in "platform".

Pixel test are by definition not platform agnostic (destination screen color profile, font rendering differences, etc...), so their result should always be in "platform".

If you combine these two assumptions, then having a test that uses dumpAsText() also have its rendering be pixel-tested does not make sense.
But today, if you run "WebKitTools/Scripts/run-webkit-tests --pixel LayoutTests/", it will generate images for such tests.

For instance, 3 out of the 6 tests in LayoutTests/transforms/2d are dumpAsText() tests and do not print the render tree:
- compound-2d-transforms.html
- transform-2d.html
- transform-accuracy.html
But since there are no "base" images in LayoutTests/platform/mac/transforms/2d for these tests, DRT always creates news ones in LayoutTests/transforms/2d and this pollutes the directory.

I think the DRT tool when running in --pixel-test mode should not compare pixel rendering of tests that use dumpAsText().
Comment 1 Mark Rowe (bdash) 2008-09-25 17:05:59 PDT
I thought that this was already the case.  Perhaps we broke this behaviour somewhere along the way.
Comment 2 Pierre-Olivier Latour 2008-09-25 17:12:19 PDT
We want to add a bunch of tests for transforms, transitions and animations, and some of them require pixel testing. Having this fixed sooner than later would help, thanks.
Comment 3 Pierre-Olivier Latour 2008-09-26 13:06:09 PDT
For some reason, you can't just bypass dumpWebViewAsPixelsAndCompareWithExpected() completely in DRT or the tests hangs.

Here's a diff that seems to fix the issue (it's a one-line change, but because of (re)tabbing a bunch of lines, it makes it look quite bigger):

Index: WebKitTools/DumpRenderTree/cg/PixelDumpSupportCG.cpp
===================================================================
--- WebKitTools/DumpRenderTree/cg/PixelDumpSupportCG.cpp	(revision 36958)
+++ WebKitTools/DumpRenderTree/cg/PixelDumpSupportCG.cpp	(working copy)
@@ -106,21 +106,25 @@
     if (gLayoutTestController->dumpSelectionRect())
         drawSelectionRect(context.get(), getSelectionRect());
 #endif
+    
+    // Only process the rendered image if have a test that is not platform-agnositc i.e. doesn't use "dumpAsText()"
+    if(!gLayoutTestController->dumpAsText()) {
 
-    // Compute the actual hash to compare to the expected image's hash.
-    char actualHash[33];
-    getMD5HashStringForBitmap(context.get(), actualHash);
-    printf("\nActualHash: %s\n", actualHash);
+        // Compute the actual hash to compare to the expected image's hash.
+        char actualHash[33];
+        getMD5HashStringForBitmap(context.get(), actualHash);
+        printf("\nActualHash: %s\n", actualHash);
 
-    // FIXME: We should compare the actualHash to the expected hash here and
-    // only set dumpImage to true if they don't match, but DRT doesn't have
-    // enough information currently to find the expected checksum file.
-    bool dumpImage = true;
+        // FIXME: We should compare the actualHash to the expected hash here and
+        // only set dumpImage to true if they don't match, but DRT doesn't have
+        // enough information currently to find the expected checksum file.
+        bool dumpImage = true;
 
-    if (dumpImage) {
-        RetainPtr<CGImageRef> image(AdoptCF, CGBitmapContextCreateImage(context.get()));
-        printPNG(image.get());
+        if (dumpImage) {
+            RetainPtr<CGImageRef> image(AdoptCF, CGBitmapContextCreateImage(context.get()));
+            printPNG(image.get());
+        }
     }
-
+    
     printf("#EOF\n");
 }
Comment 4 Simon Fraser (smfr) 2008-09-30 17:40:25 PDT
Created attachment 23965 [details]
Patch, changelog
Comment 5 mitz@webkit.org 2008-09-30 17:42:16 PDT
Comment on attachment 23965 [details]
Patch, changelog

r=me
Comment 6 Simon Fraser (smfr) 2008-09-30 17:43:44 PDT
	M	WebKitTools/DumpRenderTree/mac/DumpRenderTree.mm
	M	WebKitTools/ChangeLog
	M	WebKitTools/Scripts/run-webkit-tests
r37129 = 783a6ba5d73a582590947cb76f7859d8d42b4f9c (trunk)