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.
Created attachment 84689 [details] clear-output-image
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 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.
Created attachment 84914 [details] clear-output-image-and-make-sure-image-is-empty-string
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.
LGTM.
Committed r80524: <http://trac.webkit.org/changeset/80524>