Bug 55746 – [NRWT] Clear output image file used by DRT to make sure the previous result should not be used.

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Hayato Ito]

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.

[Hayato Ito]

Created attachment 84689 [details]
clear-output-image

[Mihai Parparita]

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?

[Dirk Pranke]

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.

[Hayato Ito]

Created attachment 84914 [details]
clear-output-image-and-make-sure-image-is-empty-string

[Hayato Ito]

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.

[Dirk Pranke]

LGTM.

[Hayato Ito]

Committed r80524: <http://trac.webkit.org/changeset/80524>