Bug 47128

Summary: [chromium] fix image diffing in NRWT
Product: WebKit Reporter: Tony Chang <tony>
Component: New BugsAssignee: Tony Chang <tony>
Status: RESOLVED FIXED    
Severity: Normal CC: dpranke, ojan, tkent
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Bug Depends on: 47163    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch ojan: review+

Tony Chang
Reported 2010-10-04 16:01:55 PDT
[chromium] fix image diffing in NRWT
Attachments
Patch (4.35 KB, patch)
2010-10-04 16:04 PDT, Tony Chang 		no flags
DetailsFormatted DiffDiff
Patch (5.15 KB, patch)
2010-10-05 12:17 PDT, Tony Chang 		ojan: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Tony Chang
Comment 1 2010-10-04 16:04:33 PDT
Created attachment 69710 [details] Patch
Tony Chang
Comment 2 2010-10-04 16:04:56 PDT
This explains why the flakiness dashboard hasn't been generating new image diffs.
Tony Chang
Comment 3 2010-10-04 16:05:14 PDT
...at least on Linux. Haven't tested on other platforms.
Dirk Pranke
Comment 4 2010-10-04 21:15:16 PDT
Comment on attachment 69710 [details] Patch patch LGTM, although I'm not a reviewer.
Kent Tamura
Comment 5 2010-10-04 21:20:04 PDT
Comment on attachment 69710 [details] Patch ok, I believe the dpanke's comment :-)
Kent Tamura
Comment 6 2010-10-04 21:21:21 PDT
Comment on attachment 69710 [details] Patch Clearing flags on attachment: 69710 Committed r69066: <http://trac.webkit.org/changeset/69066>
Kent Tamura
Comment 7 2010-10-04 21:21:28 PDT
All reviewed patches have been landed. Closing bug.
Kent Tamura
Comment 8 2010-10-04 22:29:32 PDT
(In reply to comment #6) > (From update of attachment 69710 [details]) > Committed r69066: <http://trac.webkit.org/changeset/69066> I rolled it out. If image_diff exited with neither 0 nor 1, NRWT stopped by a ValueError.
Kent Tamura
Comment 9 2010-10-04 22:35:57 PDT
(In reply to comment #8) > (In reply to comment #6) > > (From update of attachment 69710 [details] [details]) > > Committed r69066: <http://trac.webkit.org/changeset/69066> > > I rolled it out. > If image_diff exited with neither 0 nor 1, NRWT stopped by a ValueError. An example: http://build.chromium.org/buildbot/waterfall.fyi/builders/Webkit%20(webkit.org)/builds/35915/steps/webkit_tests/logs/stdio
Tony Chang
Comment 10 2010-10-05 09:30:21 PDT
(In reply to comment #9) > (In reply to comment #8) > > (In reply to comment #6) > > > (From update of attachment 69710 [details] [details] [details]) > > > Committed r69066: <http://trac.webkit.org/changeset/69066> > > > > I rolled it out. > > If image_diff exited with neither 0 nor 1, NRWT stopped by a ValueError. > > An example: > http://build.chromium.org/buildbot/waterfall.fyi/builders/Webkit%20(webkit.org)/builds/35915/steps/webkit_tests/logs/stdio That's too bad. Maybe flushing the temp files isn't enough on Windows. We might have to switch to real files that we can close(). I'll try testing on Windows. Thanks for trying to land this for me.
Tony Chang
Comment 11 2010-10-05 12:17:07 PDT
Created attachment 69825 [details] Patch
Tony Chang
Comment 12 2010-10-05 12:18:55 PDT
It turns out that on Windows, you can't open the the file created by NamedTemporaryFile. So I updated the patch to just make a temp dir and open the files using open().
Tony Chang
Comment 13 2010-10-05 15:33:28 PDT
Committed r69153: <http://trac.webkit.org/changeset/69153>
Dirk Pranke
Comment 14 2010-10-05 20:03:20 PDT
how annoying. I'll have to remember that for next time.
Dirk Pranke
Comment 15 2010-10-05 20:04:48 PDT
also, it sure seems lately that I (at least) would benefit from try servers that ran test-webkitpy. I'll see if I can add that step to the layout bots ...
Note You need to log in before you can comment on or make changes to this bug.