Bug 47240 – new-run-webkit-tests: getting an "error 2" back from ImageDiff

• Component: Tools / Tests
• Status: RESOLVED INVALID

[Dirk Pranke]

After tony's change in r69160, we're getting an "error 2" back from ImageDiff, and the code is throwing the error away without figuring out whether or not that's a good or bad thing. We should figure out what that error code means and deal with it properly.

[Tony Chang]

It's only happening on osx.  Here's an example:

2010-10-06 09:23:34,593 executive.py:315  DEBUG "/b/slave/webkit-rel-mac-webkit-org/build/src/xcodebuild/Release/image_diff --diff /tmp/tmparACnM/expected.png /tmp/tmparACnM/actual.png /b/slave/webkit-rel-mac-webkit-org/build/src/webkit/Release/../../../layout-test-results/tables/mozilla_expected_failures/bugs/bug10140-diff.png" took 0.06s
2010-10-06 09:23:34,594 chromium.py:160  ERROR image diff returned an exit code of 2

This is the only test that causes the error.  Maybe the checked in png is bad?

[Tony Chang]

Created attachment 69994 [details]
actual

Here's the png that image_diff is failing to read.  AFAICT, this is a valid png but we never get the end callback when decoding it.

[Tony Chang]

(In reply to comment #2)
> Created an attachment (id=69994) [details]
> actual
> 
> Here's the png that image_diff is failing to read.  AFAICT, this is a valid png but we never get the end callback when decoding it.

But it works fine for me when I copy the image to my Linux machine and feed it to image diff.  Maybe there's some OSX specific code causing libpng decode to fail for this file?

[Dirk Pranke]

hm. i thought someone (shans@?) saw this on windows as well ...

regardless of whether not there's something wrong with this PNG, it's clear that we hadn't really considered what would happen if image_diff/ImageDiff returns an error. 

I'm not sure what the right thing to do is, but I would propose that it's something along the lines of (a) log a useful error message at the ERROR level so that it actually shows up all the time, and  (b) mark the test as failed, but continue on.

By "useful error message", I mean something like we have in the above log, but actually return the full command that failed (like we do in the execute debug statement, but at the ERROR level).

[Dirk Pranke]

Taking ownership to see if this is still an issue and if we need to do anything more about it.

[Dirk Pranke]

I don't think it should block bug 34984 and I plan to clear the blocking flag on Mon 4/11. If you disagree, now would be a good time to speak up :).

[Dirk Pranke]

clearing blocking flag.

[Dirk Pranke]

disclaiming ownership on this as I'm probably not going to be working on it any time soon. Adding Eric and Adam in case they are interested.

[Shawn Singh]

I'm able to reproduce this problem on windows.  Furthermore, it seems like I can switch the args of the ImageDiff command, and it always fails on the first png being given to it.  So its unlikely to do with the validity of the png itself.

I'll keep digging into it, but only for a short while.  If I can solve it, I'll take ownership of this bug.

[Shawn Singh]

OK, isolated the problem:

the new-run-webkit-tests  version of /tmp refers to my cygwin environment temp.  the ImageDiff  /tmp refers to the windows environment, C:\tmp.


So, if people can suggest answers for the following, I can try to resolve it:

 - is there a way to mark that new-run-webkit-tests failed due to internal error, rather than test failure?  That seems like the most ideal choice.  the ERROR message is there now, but is easily overlooked when the test passes, and failing the test would be badly misleading.

 - what is the "right" way to solve it?

option 1: if there is a way to detect cygwin environment in python, we can export a CYGWIN_ROOT path, and if that environment exists, ImageDiff can prepend absolute paths

option 2: somehow use relative paths to ImageDiff ?

[Eric Seidel (no email)]

ORWT lauches the ImageDiff tool and feeds it Images via stdin/stdout.
I think NRWT does that for normal WebKit ports too.

TestShell/Chromium has always written the image files out to disk (and I assume it launches a new copy of ImageDiff for each diff it produces.

[Dirk Pranke]

(In reply to comment #10)
> OK, isolated the problem:
> 
> the new-run-webkit-tests  version of /tmp refers to my cygwin environment temp.  the ImageDiff  /tmp refers to the windows environment, C:\tmp.
> 

It sounds like we simply need to wrap the paths we are passing to ImageDiff in calls to cygpath() to get them converted to the native win32 paths.

Try changing ChromiumPort.diff_image() in chromium.py to something like the following (trying from memory; don't have a win machine in front of me to test):

        tempdir = self._filesystem.mkdtemp()                                    
        expected_filename = self._filesystem.join(str(tempdir), "expected.png") 
        self._filesystem.write_binary_file(expected_filename, expected_contents)
        actual_filename = self._filesystem.join(str(tempdir), "actual.png")     
        self._filesystem.write_binary_file(actual_filename, actual_contents)    

        if sys.platform == 'cygwin':
            native_expected_filename = path.cygpath(expected_filename)
            native_actual_filename = path.cygpath(actual_filename)
            if diff_filename:
                 native_diff_filename = path.cygpath(diff_filename)
        else:
            native_expected_filename = expected_filename
            native_actual_filename = actual_filename
            if diff_filename:
                 native_diff_filename = diff_filename
                                                                                
        executable = self._path_to_image_diff()                                 
        if diff_filename:                                                       
            cmd = [executable, '--diff', native_expected_filename,                     
                   native_actual_filename, native_diff_filename]                              
        else:                                                                   
            cmd = [executable, native_expected_filename, native_actual_filename]              

If this doesn't work, I can track down the right incantation necessary tomorrow. We shouldn't have to do the more complicated things you're suggesting since cygpath() can handle the conversions for us.

[Shawn Singh]

Created attachment 104374 [details]
Patch

[Dirk Pranke]

Thanks for hunting this down!

[WebKit Review Bot]

Attachment 104374 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy..." exit_code: 1

Tools/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Shawn Singh]

Created attachment 104375 [details]
Patch

[WebKit Review Bot]

Comment on attachment 104375 [details]
Patch

Clearing flags on attachment: 104375

Committed r93345: <http://trac.webkit.org/changeset/93345>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Tony Chang]

Does this patch fix the original problem mentioned in the bug?  Namely that running ImageDiff on mac against the attached png returns an error code of 2.

[Shawn Singh]

For windows/cygwin, it should.   There could be several other reasons ImageDiff returns and exit status of 2, so the original problem especially on osx may have had a different cause as well.

If you think its best to change the state of this bug, that makes sense to me.

[Tony Chang]

Still likely a problem on OSX.

[Dirk Pranke]

tony, to be clear, are you looking for NRWT to do something different if there is an error returned, or are you looking to see why an error is being returned in this case (or both)?

[Tony Chang]

(In reply to comment #22)
> tony, to be clear, are you looking for NRWT to do something different if there is an error returned, or are you looking to see why an error is being returned in this case (or both)?

The latter.  Seems like a bug in image_diff.

[Ryosuke Niwa]

This patch appears to have broken a python test on Windows 7 bots:
http://build.webkit.org/builders/Windows%207%20Release%20%28Tests%29/builds/15620
http://build.webkit.org/builders/Windows%207%20Release%20%28Tests%29/builds/15621

[Shawn Singh]

OK, I checked... it seems like test_diff_image in chromium_unittest.py needs updating.   I'll submit another patch after I verify the most appropriate way to fix it.

[Shawn Singh]

FYI, I submitted the NRWT windows/cygwin fix in another bug:
https://bugs.webkit.org/show_bug.cgi?id=67078

[Eric Seidel (no email)]

Curious what is the status here?  Should this still be open?  Should it block bug 34984 or bug 64491?

[Shawn Singh]

I think the status is that its unclear how to reproduce it on osx.  Personally I have never encountered the problem on osx.

However, its not my place to say that we can close the bug.  If it stays open, I think it is worth putting on the blocks list for those issues you mentioned.

[Eric Seidel (no email)]

Dirk?  Is there more to do here?  Just trying to clean up the list of bugs so I can get a sense for how much NRWT work there is left to do. :)

[Dirk Pranke]

(In reply to comment #29)
> Dirk?  Is there more to do here?  Just trying to clean up the list of bugs so I can get a sense for how much NRWT work there is left to do. :)

Yes, the original issue (also described by tony in comment #11) is still unanswered.

[Dirk Pranke]

I've just posted a patch in bug 92934 that'll actually cause this to be an image diff failure, so if this is still happening we'll likely have to treat it seriously.

[Alexey Proskuryakov]

Chromium specific.