Bug 47128 - [chromium] fix image diffing in NRWT
Summary: [chromium] fix image diffing in NRWT
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Tony Chang
URL:
Keywords:
Depends on: 47163
Blocks:
  Show dependency tree / graph
 
Reported: 2010-10-04 16:01 PDT by Tony Chang
Modified: 2010-10-05 20:04 PDT (History)
3 users (show)

See Also:

Attachments
Patch (4.35 KB, patch)
2010-10-04 16:04 PDT, Tony Chang 		no flags Details | Formatted Diff | Diff
Patch (5.15 KB, patch)
2010-10-05 12:17 PDT, Tony Chang 		ojan: review+
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Tony Chang 2010-10-04 16:01:55 PDT 
[chromium] fix image diffing in NRWT
Comment 1 Tony Chang 2010-10-04 16:04:33 PDT 
Created attachment 69710 [details]
Patch
Comment 2 Tony Chang 2010-10-04 16:04:56 PDT 
This explains why the flakiness dashboard hasn't been generating new image diffs.
Comment 3 Tony Chang 2010-10-04 16:05:14 PDT 
...at least on Linux.  Haven't tested on other platforms.
Comment 4 Dirk Pranke 2010-10-04 21:15:16 PDT 
Comment on attachment 69710 [details]
Patch

patch LGTM, although I'm not a reviewer.
Comment 5 Kent Tamura 2010-10-04 21:20:04 PDT 
Comment on attachment 69710 [details]
Patch

ok, I believe the dpanke's comment :-)
Comment 6 Kent Tamura 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>
Comment 7 Kent Tamura 2010-10-04 21:21:28 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 8 Kent Tamura 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.
Comment 9 Kent Tamura 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
Comment 10 Tony Chang 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.
Comment 11 Tony Chang 2010-10-05 12:17:07 PDT 
Created attachment 69825 [details]
Patch
Comment 12 Tony Chang 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().
Comment 13 Tony Chang 2010-10-05 15:33:28 PDT 
Committed r69153: <http://trac.webkit.org/changeset/69153>
Comment 14 Dirk Pranke 2010-10-05 20:03:20 PDT 
how annoying. I'll have to remember that for next time.
Comment 15 Dirk Pranke 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 ...