Bug 50233

Summary: Remove unnecessary pixel results, use platform-independent text results, re Changeset 72802
Product: WebKit Reporter: W. James MacLean <wjmaclean>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, eric, ojan, ossy, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description W. James MacLean 2010-11-30 06:38:37 PST
Remove unnecessary pixel results, use platform-independent text results, re Changeset 72802
Comment 1 W. James MacLean 2010-11-30 06:41:16 PST
Created attachment 75137 [details]
Patch
Comment 2 W. James MacLean 2010-11-30 07:10:56 PST
Marking as "Platform All"
Comment 3 Ojan Vafai 2010-11-30 07:44:09 PST
Comment on attachment 75137 [details]
Patch

Thanks for doing this. In order to avoid needing pixel results, you need to explicitly mark each test as dumpAsText (see http://webkit.org/quality/testwriting.html). Examples: http://codesearch.google.com/codesearch?as_q=dumpAsText&vert=chromium&as_lang=javascript

Also, please delete the chromium-mac and chromium-win pixel results for these tests as well.
Comment 4 W. James MacLean 2010-11-30 08:37:24 PST
Created attachment 75153 [details]
Patch
Comment 5 Ojan Vafai 2010-11-30 09:12:13 PST
Comment on attachment 75153 [details]
Patch

Please delete the platform/mac results too.
Comment 6 W. James MacLean 2010-11-30 09:43:43 PST
Created attachment 75160 [details]
Patch
Comment 7 Ojan Vafai 2010-11-30 09:48:40 PST
Comment on attachment 75160 [details]
Patch

Yay! Thanks again for following up.
Comment 8 W. James MacLean 2010-11-30 10:00:46 PST
(In reply to comment #7)
> (From update of attachment 75160 [details])
> Yay! Thanks again for following up.

My pleasure! :-)
Comment 9 WebKit Commit Bot 2010-11-30 21:10:49 PST
Comment on attachment 75160 [details]
Patch

Rejecting patch 75160 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']" exit_code: 2
Last 500 characters of output:
s/websocket/tests/workers ......
http/tests/workers .....
http/tests/xhtmlmp .
http/tests/xmlhttprequest ............................................................................................................................................................................
http/tests/xmlhttprequest/web-apps ...............
http/tests/xmlhttprequest/workers .........
582.95s total testing time

22077 test cases (99%) succeeded
5 test cases (<1%) were new
12 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/6610002
Comment 10 Csaba Osztrogon√°c 2010-12-01 04:05:24 PST
Comment on attachment 75160 [details]
Patch

try again
Comment 11 WebKit Commit Bot 2010-12-01 06:24:29 PST
The commit-queue encountered the following flaky tests while processing attachment 75160 [details]:

compositing/iframes/overlapped-nested-iframes.html

Please file bugs against the tests.  These tests were authored by simon.fraser@apple.com.  The commit-queue is continuing to process your patch.
Comment 12 WebKit Commit Bot 2010-12-01 07:04:40 PST
The commit-queue encountered the following flaky tests while processing attachment 75160 [details]:

media/event-attributes.html

Please file bugs against the tests.  These tests were authored by eric.carlson@apple.com.  The commit-queue is continuing to process your patch.
Comment 13 WebKit Commit Bot 2010-12-02 03:10:31 PST
The commit-queue encountered the following flaky tests while processing attachment 75160 [details]:

fast/workers/storage/use-same-database-in-page-and-workers.html

Please file bugs against the tests.  These tests were authored by dumi@chromium.org.  The commit-queue is continuing to process your patch.
Comment 14 WebKit Commit Bot 2010-12-02 03:51:51 PST
Comment on attachment 75160 [details]
Patch

Rejecting patch 75160 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=abarth-cq-sl', 'build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']" exit_code: 2
Last 500 characters of output:
s/websocket/tests/workers ......
http/tests/workers .....
http/tests/xhtmlmp .
http/tests/xmlhttprequest ............................................................................................................................................................................
http/tests/xmlhttprequest/web-apps ...............
http/tests/xmlhttprequest/workers .........
538.85s total testing time

22083 test cases (99%) succeeded
5 test cases (<1%) were new
12 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/6774004
Comment 15 Ojan Vafai 2010-12-02 08:29:49 PST
I think the reason this is failing the commit queue is that the tests are showing up as "new". I don't know exactly what that means, but I think it means something is not right. James, do you not seeing it claiming these tests are "new" when you run this locally?
Comment 16 W. James MacLean 2010-12-02 08:36:02 PST
(In reply to comment #15)
> I think the reason this is failing the commit queue is that the tests are showing up as "new". I don't know exactly what that means, but I think it means something is not right. James, do you not seeing it claiming these tests are "new" when you run this locally?

I don't recall seeing this. When I created the tests, the first thing I did was generate baselines, so after that there was no mention of 'new'. Perhaps it thinks the expectation files are missing?
Comment 17 Eric Seidel (no email) 2010-12-02 09:28:03 PST
Yes.  5 "new" tests, means 5 tests missing expectations.  Are you sure you have Mac or Mac Snow Leopard expectations?
Comment 18 W. James MacLean 2010-12-02 09:54:57 PST
(In reply to comment #17)
> Yes.  5 "new" tests, means 5 tests missing expectations.  Are you sure you have Mac or Mac Snow Leopard expectations?

The point of this patch was to remove unnecessary pixel expectations and use a single set of -expected.txt files. All the tests use dumpAsText() to facilitate this, and none of them produce anything other than a blank screen.

So this patch deletes all the png and checksum files checked in earlier and uses a single set of text expectations in LayoutTests/svg/custom/.

Do we have to have platform-specific expectations for Mac? In my testing on linux and snow leopard the test harness seemed happy with this setup.
Comment 19 Ojan Vafai 2010-12-02 10:19:12 PST
I looked carefully at the patch. Everything looks fine. I wonder if webkit-patch is not handling the file renames correctly?
Comment 20 WebKit Commit Bot 2010-12-02 10:37:28 PST
Comment on attachment 75160 [details]
Patch

Rejecting patch 75160 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=abarth-cq-sl', 'build-and-test', '--no-clean', '--no-update', '--test', '--non-interactive']" exit_code: 2
Last 500 characters of output:
s/websocket/tests/workers ......
http/tests/workers .....
http/tests/xhtmlmp .
http/tests/xmlhttprequest ............................................................................................................................................................................
http/tests/xmlhttprequest/web-apps ...............
http/tests/xmlhttprequest/workers .........
538.18s total testing time

22084 test cases (99%) succeeded
5 test cases (<1%) were new
12 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/6836007
Comment 21 Ojan Vafai 2010-12-02 12:41:07 PST
I tried patching this locally (using webkit-patch). The expected results don't come in. Looking at the raw diff of this patch, I don't see the rename anywhere in the patch, just in the changelog. :( How did you do the rename? Are you on git or svn?
Comment 22 W. James MacLean 2010-12-02 13:03:40 PST
(In reply to comment #21)
> I tried patching this locally (using webkit-patch). The expected results don't come in. Looking at the raw diff of this patch, I don't see the rename anywhere in the patch, just in the changelog. :( How did you do the rename? Are you on git or svn?

I 'renamed' using 'mv' followed by 'git add'. My git repo certainly sees the files as moved ...
Comment 23 Eric Seidel (no email) 2010-12-02 13:05:02 PST
You hit bug 49154. :(
Comment 24 W. James MacLean 2010-12-02 13:23:22 PST
(In reply to comment #23)
> You hit bug 49154. :(

Ahhh ....

What's the best way to get around this, given that we can't change the content of the moved file. Two patches, one to delete and one to restore? Applying the one that deletes will be difficult, as the tests will have no expected output ...
Comment 25 Tony Chang 2010-12-02 14:39:45 PST
(In reply to comment #24)

> What's the best way to get around this

Use svn.
Comment 26 W. James MacLean 2010-12-03 06:02:38 PST
(In reply to comment #25)
> (In reply to comment #24)
> 
> > What's the best way to get around this
> 
> Use svn.

I'd rather not have to build an SVN repo just to submit this patch. Surely there's another way.
Comment 27 Eric Seidel (no email) 2010-12-03 08:33:34 PST
I suspect there is a way to get git patches to reflect renames  Maybe git format-patch --binary?  donno.
Comment 28 W. James MacLean 2010-12-03 08:48:04 PST
(In reply to comment #27)
> I suspect there is a way to get git patches to reflect renames  Maybe git format-patch --binary?  donno.

I don't think so ... webkit-patch must already be using --binary as the png images in the patch appear OK.
Comment 29 W. James MacLean 2010-12-06 08:00:59 PST
Created attachment 75697 [details]
Patch
Comment 30 W. James MacLean 2010-12-06 08:02:59 PST
Comment on attachment 75697 [details]
Patch

I've split the patch into two parts to get around the git issue. This part deletes all pixel tests and adds the platform-independent text expectations, the next part of the patch will delete the platform-specific expectations.
Comment 31 Ojan Vafai 2010-12-07 16:28:17 PST
Comment on attachment 75697 [details]
Patch

Doing it this way is fine, but I think maybe you left the html files out of this patch by accident?
Comment 32 W. James MacLean 2010-12-08 07:07:13 PST
Created attachment 75899 [details]
Patch
Comment 33 W. James MacLean 2010-12-08 07:08:42 PST
Comment on attachment 75899 [details]
Patch

Sorry about forgetting the actual test files :-)

This patch should be good to go.
Comment 34 WebKit Commit Bot 2010-12-08 11:34:30 PST
Comment on attachment 75899 [details]
Patch

Rejecting patch 75899 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'build', '--no-clean', '--no-update', '--build-style=both']" exit_code: 2
Last 500 characters of output:
86_64 objective-c++ com.apple.compilers.gcc.4_2
	CompileC /Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/x86_64/ScriptExecutionContext.o /Projects/CommitQueue/WebCore/dom/ScriptExecutionContext.cpp normal x86_64 c++ com.apple.compilers.gcc.4_2
	CompileC /Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/x86_64/Settings.o /Projects/CommitQueue/WebCore/page/Settings.cpp normal x86_64 c++ com.apple.compilers.gcc.4_2
(28 failures)


Full output: http://queues.webkit.org/results/6717110
Comment 35 W. James MacLean 2010-12-08 13:23:04 PST
(In reply to comment #34)
> (From update of attachment 75899 [details])
> Rejecting patch 75899 from commit-queue.
> 
> Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'build', '--no-clean', '--no-update', '--build-style=both']" exit_code: 2
> Last 500 characters of output:
> 86_64 objective-c++ com.apple.compilers.gcc.4_2
>     CompileC /Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/x86_64/ScriptExecutionContext.o /Projects/CommitQueue/WebCore/dom/ScriptExecutionContext.cpp normal x86_64 c++ com.apple.compilers.gcc.4_2
>     CompileC /Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/x86_64/Settings.o /Projects/CommitQueue/WebCore/page/Settings.cpp normal x86_64 c++ com.apple.compilers.gcc.4_2
> (28 failures)
> 
> 
> Full output: http://queues.webkit.org/results/6717110

Apparently this failed to build, which I don't think is related to the patch. Can we try committing again?
Comment 36 WebKit Commit Bot 2010-12-09 10:17:12 PST
Comment on attachment 75899 [details]
Patch

Rejecting patch 75899 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=abarth-cq-sl', 'apply-attachment', '--force-clean', '--non-interactive', 75899]" exit_code: 2
Last 500 characters of output:
g OpenSource
From git://git.webkit.org/WebKit
   20980e9..32fd619  master     -> origin/master
	M	WebKitLibraries/libWebKitSystemInterfaceSnowLeopard.a
	M	WebKitLibraries/WebKitSystemInterface.h
	M	WebKitLibraries/ChangeLog
	M	WebKitLibraries/libWebKitSystemInterfaceLeopard.a
Incomplete data: Delta source ended unexpectedly at /usr/local/git/libexec/git-core/git-svn line 4618

Died at WebKitTools/Scripts/update-webkit line 132.

Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2

Full output: http://queues.webkit.org/results/6966008
Comment 37 WebKit Commit Bot 2010-12-09 11:10:57 PST
Comment on attachment 75899 [details]
Patch

Rejecting patch 75899 from commit-queue.

Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'build', '--no-clean', '--no-update', '--build-style=both']" exit_code: 2
Last 500 characters of output:
leC /Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/x86_64/JSPositionError.o /Projects/CommitQueue/WebKitBuild/Debug/DerivedSources/WebCore/JSPositionError.cpp normal x86_64 c++ com.apple.compilers.gcc.4_2
	CompileC /Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/x86_64/JSPluginElementFunctions.o /Projects/CommitQueue/WebCore/bindings/js/JSPluginElementFunctions.cpp normal x86_64 c++ com.apple.compilers.gcc.4_2
(41 failures)


Full output: http://queues.webkit.org/results/7008012
Comment 38 W. James MacLean 2010-12-10 07:10:08 PST
(In reply to comment #37)
> (From update of attachment 75899 [details])
> Rejecting patch 75899 from commit-queue.
> 
> Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sl', 'build', '--no-clean', '--no-update', '--build-style=both']" exit_code: 2
> Last 500 characters of output:
> leC /Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/x86_64/JSPositionError.o /Projects/CommitQueue/WebKitBuild/Debug/DerivedSources/WebCore/JSPositionError.cpp normal x86_64 c++ com.apple.compilers.gcc.4_2
>     CompileC /Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/x86_64/JSPluginElementFunctions.o /Projects/CommitQueue/WebCore/bindings/js/JSPluginElementFunctions.cpp normal x86_64 c++ com.apple.compilers.gcc.4_2
> (41 failures)
> 
> 
> Full output: http://queues.webkit.org/results/7008012

Again, the failure appears to have nothing to do with the content of the patch - please let me know if I'm missing something :-)

Can we try again and, if it fails, put it through manually?
Comment 39 Eric Seidel (no email) 2010-12-10 12:24:53 PST
Comment on attachment 75899 [details]
Patch

Sure.  Maybe this machine is just running too many bots.
Comment 40 WebKit Commit Bot 2010-12-10 17:48:32 PST
Comment on attachment 75899 [details]
Patch

Clearing flags on attachment: 75899

Committed r73824: <http://trac.webkit.org/changeset/73824>
Comment 41 WebKit Commit Bot 2010-12-10 17:48:41 PST
All reviewed patches have been landed.  Closing bug.
Comment 42 W. James MacLean 2010-12-13 05:48:33 PST
Created attachment 76376 [details]
Patch
Comment 43 W. James MacLean 2010-12-13 05:49:33 PST
Comment on attachment 76376 [details]
Patch

This is the second part of the cleanup patch ... nearly done!
Comment 44 Csaba Osztrogon√°c 2010-12-13 06:00:47 PST
Reopen to make CQ land https://bugs.webkit.org/attachment.cgi?id=76376
Comment 45 WebKit Review Bot 2010-12-13 06:20:37 PST
Comment on attachment 76376 [details]
Patch

Clearing flags on attachment: 76376

Committed r73911: <http://trac.webkit.org/changeset/73911>
Comment 46 WebKit Review Bot 2010-12-13 06:20:45 PST
All reviewed patches have been landed.  Closing bug.