Bug 55746 - [NRWT] Clear output image file used by DRT to make sure the previous result should not be used.
Summary: [NRWT] Clear output image file used by DRT to make sure the previous result s...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-03-03 22:21 PST by Hayato Ito
Modified: 2011-03-07 19:14 PST (History)
2 users (show)

See Also:


Attachments
clear-output-image (1.62 KB, patch)
2011-03-03 22:24 PST, Hayato Ito
no flags Details | Formatted Diff | Diff
clear-output-image-and-make-sure-image-is-empty-string (4.30 KB, patch)
2011-03-06 20:06 PST, Hayato Ito
mihaip: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hayato Ito 2011-03-03 22:21:55 PST
We need to clear output image before calling DRT in ChromiumDriver's run_test() in chromium.py.

In current implementation, the previous test's result might be returned when DRT crashes.

Because WebKitDriver in webkit.py seems to return '' as image when crash occurs, ChromiumDriver should do the same thing.
Comment 1 Hayato Ito 2011-03-03 22:24:02 PST
Created attachment 84689 [details]
clear-output-image
Comment 2 Mihai Parparita 2011-03-04 09:51:47 PST
Comment on attachment 84689 [details]
clear-output-image

View in context: https://bugs.webkit.org/attachment.cgi?id=84689&action=review

> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:449
> +            self._port._filesystem.write_binary_file(png_path, '')

Isn't filesystem.remove more appropriate?
Comment 3 Dirk Pranke 2011-03-04 13:49:13 PST
Comment on attachment 84689 [details]
clear-output-image

View in context: https://bugs.webkit.org/attachment.cgi?id=84689&action=review

>> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:449
>> +            self._port._filesystem.write_binary_file(png_path, '')
> 
> Isn't filesystem.remove more appropriate?

For consistency w/ the WebKitDriver implementation, we should *always* return '' if there is no actual PNG data, regardless of whether pixel tests are enabled or if it's a text-only test or if DRT crashed.

So, that said, I agree with Mihai that it would probably be clearer to write this as

if png_path and self._port._filesystem.exists(png_path):
   self._port._filesystem.remove(png_path)

I think the extra write may actually slow things down a bit, too.

Then, you'll need to change _output_image to return '' if the file doesn't exist (currently it'll return None, which will trigger one of the other bugs).

We should actually add an assert in single_test_runner to enforce image being not None.
Comment 4 Hayato Ito 2011-03-06 20:06:53 PST
Created attachment 84914 [details]
clear-output-image-and-make-sure-image-is-empty-string
Comment 5 Hayato Ito 2011-03-06 20:13:17 PST
Comment on attachment 84689 [details]
clear-output-image

View in context: https://bugs.webkit.org/attachment.cgi?id=84689&action=review

>>> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:449
>>> +            self._port._filesystem.write_binary_file(png_path, '')
>> 
>> Isn't filesystem.remove more appropriate?
> 
> For consistency w/ the WebKitDriver implementation, we should *always* return '' if there is no actual PNG data, regardless of whether pixel tests are enabled or if it's a text-only test or if DRT crashed.
> 
> So, that said, I agree with Mihai that it would probably be clearer to write this as
> 
> if png_path and self._port._filesystem.exists(png_path):
>    self._port._filesystem.remove(png_path)
> 
> I think the extra write may actually slow things down a bit, too.
> 
> Then, you'll need to change _output_image to return '' if the file doesn't exist (currently it'll return None, which will trigger one of the other bugs).
> 
> We should actually add an assert in single_test_runner to enforce image being not None.

Thank you for the reviews.

I didn't notice that filesystem provides remove(path) function. Thank you.

I updated the patch to make sure the output image should be '', not None, if the file doesn't exist.
And I added some assertion in single_test_runner and ChromiumDriver to make sure image is not None.
Comment 6 Dirk Pranke 2011-03-07 14:51:40 PST
LGTM.
Comment 7 Hayato Ito 2011-03-07 19:14:11 PST
Committed r80524: <http://trac.webkit.org/changeset/80524>