55746 2011-03-03 22:21:55 -0800 [NRWT] Clear output image file used by DRT to make sure the previous result should not be used. 2011-03-07 19:14:11 -0800 1 1 1 Unclassified WebKit Tools / Tests 528+ (Nightly build) PC OS X 10.5 RESOLVED FIXED P2 Normal --- 1 hayato webkit-unassigned dpranke mihaip oldest_to_newest 362278 0 hayato 2011-03-03 22:21:55 -0800 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. 362281 1 84689 hayato 2011-03-03 22:24:02 -0800 Created attachment 84689 clear-output-image 362576 2 84689 mihaip 2011-03-04 09:51:47 -0800 Comment on attachment 84689 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? 362685 3 84689 dpranke 2011-03-04 13:49:13 -0800 Comment on attachment 84689 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. 363195 4 84914 hayato 2011-03-06 20:06:53 -0800 Created attachment 84914 clear-output-image-and-make-sure-image-is-empty-string 363199 5 84689 hayato 2011-03-06 20:13:17 -0800 Comment on attachment 84689 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. 363610 6 dpranke 2011-03-07 14:51:40 -0800 LGTM. 363766 7 hayato 2011-03-07 19:14:11 -0800 Committed r80524: <http://trac.webkit.org/changeset/80524> 84689 2011-03-03 22:24:02 -0800 2011-03-06 20:13:17 -0800 clear-output-image bug-55746-20110304152400.patch text/plain 1660 hayato U3VidmVyc2lvbiBSZXZpc2lvbjogODAyMTEKZGlmZiAtLWdpdCBhL1Rvb2xzL0NoYW5nZUxvZyBi L1Rvb2xzL0NoYW5nZUxvZwppbmRleCAxZjNmYThjYmFlODAzN2I3Y2I4OTg0YjZhOGIzZmYyNzlj OGFkYmU5Li42ZDgxZmMwYzNlMTJjMDcxZmYzNjA0ZTkxNTYyMGYzNjcyZWZhMTgwIDEwMDY0NAot LS0gYS9Ub29scy9DaGFuZ2VMb2cKKysrIGIvVG9vbHMvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTMg QEAKKzIwMTEtMDMtMDMgIEhheWF0byBJdG8gIDxoYXlhdG9AY2hyb21pdW0ub3JnPgorCisgICAg ICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFtOUldUXSBDbGVhciBv dXRwdXQgaW1hZ2UgZmlsZSB1c2VkIGJ5IER1bXBSZW5kZXJUcmVlIHRvIG1ha2Ugc3VyZSB0aGUg cHJldmlvdXMgaW1hZ2Ugc2hvdWxkIG5vdCBiZSB1c2VkLgorCisgICAgICAgIGh0dHBzOi8vYnVn cy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD01NTc0NgorCisgICAgICAgICogU2NyaXB0cy93 ZWJraXRweS9sYXlvdXRfdGVzdHMvcG9ydC9jaHJvbWl1bS5weToKKwogMjAxMS0wMy0wMiAgT2ph biBWYWZhaSAgPG9qYW5AY2hyb21pdW0ub3JnPgogCiAgICAgICAgIFJldmlld2VkIGJ5IEFkYW0g QmFydGguCmRpZmYgLS1naXQgYS9Ub29scy9TY3JpcHRzL3dlYmtpdHB5L2xheW91dF90ZXN0cy9w b3J0L2Nocm9taXVtLnB5IGIvVG9vbHMvU2NyaXB0cy93ZWJraXRweS9sYXlvdXRfdGVzdHMvcG9y dC9jaHJvbWl1bS5weQppbmRleCA3ZDU2ZmEyMzM0ZjM2MzUxNmU3ZGM1YzQ3MDA3NmMwNDdiMzg2 MmRmLi41ODgxYzQ1MDBlYzAwMzNmNmRiYjA0NzM0MjdkNWZmZTk2ZTVkNTUyIDEwMDY0NAotLS0g YS9Ub29scy9TY3JpcHRzL3dlYmtpdHB5L2xheW91dF90ZXN0cy9wb3J0L2Nocm9taXVtLnB5Cisr KyBiL1Rvb2xzL1NjcmlwdHMvd2Via2l0cHkvbGF5b3V0X3Rlc3RzL3BvcnQvY2hyb21pdW0ucHkK QEAgLTQ0Myw2ICs0NDMsMTEgQEAgY2xhc3MgQ2hyb21pdW1Ecml2ZXIoYmFzZS5Ecml2ZXIpOgog ICAgICAgICAgICAgICAgICAgICByYWlzZSBlCiAgICAgICAgIHJldHVybiBzZWxmLl9vdXRwdXRf aW1hZ2UoKQogCisgICAgZGVmIF9jbGVhcl9vdXRwdXRfaW1hZ2Uoc2VsZik6CisgICAgICAgIHBu Z19wYXRoID0gc2VsZi5faW1hZ2VfcGF0aAorICAgICAgICBpZiBwbmdfcGF0aCBhbmQgc2VsZi5f cG9ydC5fZmlsZXN5c3RlbS5pc2ZpbGUocG5nX3BhdGgpOgorICAgICAgICAgICAgc2VsZi5fcG9y dC5fZmlsZXN5c3RlbS53cml0ZV9iaW5hcnlfZmlsZShwbmdfcGF0aCwgJycpCisKICAgICBkZWYg cnVuX3Rlc3Qoc2VsZiwgZHJpdmVyX2lucHV0KToKICAgICAgICAgb3V0cHV0ID0gW10KICAgICAg ICAgZXJyb3IgPSBbXQpAQCAtNDUwLDcgKzQ1NSw3IEBAIGNsYXNzIENocm9taXVtRHJpdmVyKGJh c2UuRHJpdmVyKToKICAgICAgICAgdGltZW91dCA9IEZhbHNlCiAgICAgICAgIGFjdHVhbF91cmkg PSBOb25lCiAgICAgICAgIGFjdHVhbF9jaGVja3N1bSA9IE5vbmUKLQorICAgICAgICBzZWxmLl9j bGVhcl9vdXRwdXRfaW1hZ2UoKQogICAgICAgICBzdGFydF90aW1lID0gdGltZS50aW1lKCkKIAog ICAgICAgICB1cmkgPSBzZWxmLl9wb3J0LmZpbGVuYW1lX3RvX3VyaShkcml2ZXJfaW5wdXQuZmls ZW5hbWUpCg== 84914 2011-03-06 20:06:53 -0800 2011-03-07 14:53:55 -0800 clear-output-image-and-make-sure-image-is-empty-string bug-55746-20110307130651.patch text/plain 4408 hayato U3VidmVyc2lvbiBSZXZpc2lvbjogODAyMTEKZGlmZiAtLWdpdCBhL1Rvb2xzL0NoYW5nZUxvZyBi L1Rvb2xzL0NoYW5nZUxvZwppbmRleCAxZjNmYThjYmFlODAzN2I3Y2I4OTg0YjZhOGIzZmYyNzlj OGFkYmU5Li5kNjJiYzgwNmJkZjRjY2ViYjQ1ZjkyODg4ZDMxZDZiYzMyZWViZmIzIDEwMDY0NAot LS0gYS9Ub29scy9DaGFuZ2VMb2cKKysrIGIvVG9vbHMvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTgg QEAKKzIwMTEtMDMtMDMgIEhheWF0byBJdG8gIDxoYXlhdG9AY2hyb21pdW0ub3JnPgorCisgICAg ICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFtOUldUXSBDbGVhciBv dXRwdXQgaW1hZ2UgZmlsZSB1c2VkIGJ5IER1bXBSZW5kZXJUcmVlIHRvIG1ha2Ugc3VyZSB0aGUK KyAgICAgICAgcHJldmlvdXMgaW1hZ2Ugc2hvdWxkIG5vdCBiZSB1c2VkIGluIHRoZSBDaHJvbWl1 bURyaXZlci4KKworICAgICAgICBBbHNvIG1ha2Ugc3VyZSB0aGF0IG91dHB1dCBpbWFnZSBmcm9t IHRoZSBDaHJvbWl1bURyaXZlciBzaG91bGQgYmUgJycKKyAgICAgICAgKGVtcHR5IHN0cmluZykg aWYgYSB0ZXN0IGNyYXNoZXMgZm9yIGNvbnNpc3RlbmN5IHdpdGggdGhlIFdlYktpdERyaXZlcgor ICAgICAgICBpbXBsZW1lbnRhdGlvbi4KKworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9y Zy9zaG93X2J1Zy5jZ2k/aWQ9NTU3NDYKKworICAgICAgICAqIFNjcmlwdHMvd2Via2l0cHkvbGF5 b3V0X3Rlc3RzL3BvcnQvY2hyb21pdW0ucHk6CisKIDIwMTEtMDMtMDIgIE9qYW4gVmFmYWkgIDxv amFuQGNocm9taXVtLm9yZz4KIAogICAgICAgICBSZXZpZXdlZCBieSBBZGFtIEJhcnRoLgpkaWZm IC0tZ2l0IGEvVG9vbHMvU2NyaXB0cy93ZWJraXRweS9sYXlvdXRfdGVzdHMvbGF5b3V0X3BhY2th Z2Uvc2luZ2xlX3Rlc3RfcnVubmVyLnB5IGIvVG9vbHMvU2NyaXB0cy93ZWJraXRweS9sYXlvdXRf dGVzdHMvbGF5b3V0X3BhY2thZ2Uvc2luZ2xlX3Rlc3RfcnVubmVyLnB5CmluZGV4IDIzNjIzZWNk MGFkMjc5MGQzODE4NDFlNTc3NmY5ZmEwYTBkYTBkY2YuLmQ3NTVmNjc4N2E1MTM4NThlZjI0NmI5 NWE4NTZhZmVjNjYyZjNmZWMgMTAwNjQ0Ci0tLSBhL1Rvb2xzL1NjcmlwdHMvd2Via2l0cHkvbGF5 b3V0X3Rlc3RzL2xheW91dF9wYWNrYWdlL3NpbmdsZV90ZXN0X3J1bm5lci5weQorKysgYi9Ub29s cy9TY3JpcHRzL3dlYmtpdHB5L2xheW91dF90ZXN0cy9sYXlvdXRfcGFja2FnZS9zaW5nbGVfdGVz dF9ydW5uZXIucHkKQEAgLTEyNyw3ICsxMjcsNyBAQCBjbGFzcyBTaW5nbGVUZXN0UnVubmVyOgog ICAgICAgICAgIGdlbmVyYXRlX25ld19iYXNlbGluZTogd2hldGhlciB0byBlbmVyYXRlIGEgbmV3 LCBwbGF0Zm9ybS1zcGVjaWZpYwogICAgICAgICAgICAgYmFzZWxpbmUsIG9yIHVwZGF0ZSB0aGUg ZXhpc3Rpbmcgb25lCiAgICAgICAgICIiIgotCisgICAgICAgIGFzc2VydCBkYXRhIGlzIG5vdCBO b25lCiAgICAgICAgIHBvcnQgPSBzZWxmLl9wb3J0CiAgICAgICAgIGZzID0gcG9ydC5fZmlsZXN5 c3RlbQogICAgICAgICBpZiBnZW5lcmF0ZV9uZXdfYmFzZWxpbmU6CmRpZmYgLS1naXQgYS9Ub29s cy9TY3JpcHRzL3dlYmtpdHB5L2xheW91dF90ZXN0cy9wb3J0L2Jhc2UucHkgYi9Ub29scy9TY3Jp cHRzL3dlYmtpdHB5L2xheW91dF90ZXN0cy9wb3J0L2Jhc2UucHkKaW5kZXggMGMyOGQ0NDE4ZGMy Yjk4ZjJkNDMwZGE1ODRlNWNiMjBjMTMxMWE3Mi4uMjQ3YTI2MDY0YzVkOTViNDlhMjk0NThiNWM2 ODkxNmQwNjZlNWU5YyAxMDA2NDQKLS0tIGEvVG9vbHMvU2NyaXB0cy93ZWJraXRweS9sYXlvdXRf dGVzdHMvcG9ydC9iYXNlLnB5CisrKyBiL1Rvb2xzL1NjcmlwdHMvd2Via2l0cHkvbGF5b3V0X3Rl c3RzL3BvcnQvYmFzZS5weQpAQCAtOTAxLDYgKzkwMSw3IEBAIGNsYXNzIERyaXZlcjoKICAgICAg ICAgICBkcml2ZXJfaW5wdXQ6IGEgRHJpdmVySW5wdXQgb2JqZWN0CiAKICAgICAgICAgUmV0dXJu cyBhIERyaXZlck91dHB1dCBvYmplY3QuCisgICAgICAgICAgTm90ZSB0aGF0IERyaXZlck91dHB1 dC5pbWFnZSB3aWxsIGJlICcnIChlbXB0eSBzdHJpbmcpIGlmIGEgdGVzdCBjcmFzaGVzLgogICAg ICAgICAiIiIKICAgICAgICAgcmFpc2UgTm90SW1wbGVtZW50ZWRFcnJvcignRHJpdmVyLnJ1bl90 ZXN0JykKIApkaWZmIC0tZ2l0IGEvVG9vbHMvU2NyaXB0cy93ZWJraXRweS9sYXlvdXRfdGVzdHMv cG9ydC9jaHJvbWl1bS5weSBiL1Rvb2xzL1NjcmlwdHMvd2Via2l0cHkvbGF5b3V0X3Rlc3RzL3Bv cnQvY2hyb21pdW0ucHkKaW5kZXggN2Q1NmZhMjMzNGYzNjM1MTZlN2RjNWM0NzAwNzZjMDQ3YjM4 NjJkZi4uYmFmMTg5MzNiNDRjYzJiZjc0YTJkNTFhMTRlY2UzNjFiMWQ0NjEyMyAxMDA2NDQKLS0t IGEvVG9vbHMvU2NyaXB0cy93ZWJraXRweS9sYXlvdXRfdGVzdHMvcG9ydC9jaHJvbWl1bS5weQor KysgYi9Ub29scy9TY3JpcHRzL3dlYmtpdHB5L2xheW91dF90ZXN0cy9wb3J0L2Nocm9taXVtLnB5 CkBAIC00MjMsMTAgKzQyMywxMCBAQCBjbGFzcyBDaHJvbWl1bURyaXZlcihiYXNlLkRyaXZlcik6 CiAgICAgZGVmIF9vdXRwdXRfaW1hZ2Uoc2VsZik6CiAgICAgICAgICIiIlJldHVybnMgdGhlIGlt YWdlIG91dHB1dCB3aGljaCBkcml2ZXIgZ2VuZXJhdGVkLiIiIgogICAgICAgICBwbmdfcGF0aCA9 IHNlbGYuX2ltYWdlX3BhdGgKLSAgICAgICAgaWYgcG5nX3BhdGggYW5kIHNlbGYuX3BvcnQuX2Zp bGVzeXN0ZW0uaXNmaWxlKHBuZ19wYXRoKToKKyAgICAgICAgaWYgcG5nX3BhdGggYW5kIHNlbGYu X3BvcnQuX2ZpbGVzeXN0ZW0uZXhpc3RzKHBuZ19wYXRoKToKICAgICAgICAgICAgIHJldHVybiBz ZWxmLl9wb3J0Ll9maWxlc3lzdGVtLnJlYWRfYmluYXJ5X2ZpbGUocG5nX3BhdGgpCiAgICAgICAg IGVsc2U6Ci0gICAgICAgICAgICByZXR1cm4gTm9uZQorICAgICAgICAgICAgcmV0dXJuICcnCiAK ICAgICBkZWYgX291dHB1dF9pbWFnZV93aXRoX3JldHJ5KHNlbGYpOgogICAgICAgICAjIFJldHJ5 IGEgZmV3IG1vcmUgdGltZXMgYmVjYXVzZSBvcGVuKCkgc29tZXRpbWVzIGZhaWxzIG9uIFdpbmRv d3MsCkBAIC00NDMsNiArNDQzLDExIEBAIGNsYXNzIENocm9taXVtRHJpdmVyKGJhc2UuRHJpdmVy KToKICAgICAgICAgICAgICAgICAgICAgcmFpc2UgZQogICAgICAgICByZXR1cm4gc2VsZi5fb3V0 cHV0X2ltYWdlKCkKIAorICAgIGRlZiBfY2xlYXJfb3V0cHV0X2ltYWdlKHNlbGYpOgorICAgICAg ICBwbmdfcGF0aCA9IHNlbGYuX2ltYWdlX3BhdGgKKyAgICAgICAgaWYgcG5nX3BhdGggYW5kIHNl bGYuX3BvcnQuX2ZpbGVzeXN0ZW0uZXhpc3RzKHBuZ19wYXRoKToKKyAgICAgICAgICAgIHNlbGYu X3BvcnQuX2ZpbGVzeXN0ZW0ucmVtb3ZlKHBuZ19wYXRoKQorCiAgICAgZGVmIHJ1bl90ZXN0KHNl bGYsIGRyaXZlcl9pbnB1dCk6CiAgICAgICAgIG91dHB1dCA9IFtdCiAgICAgICAgIGVycm9yID0g W10KQEAgLTQ1MCw3ICs0NTUsNyBAQCBjbGFzcyBDaHJvbWl1bURyaXZlcihiYXNlLkRyaXZlcik6 CiAgICAgICAgIHRpbWVvdXQgPSBGYWxzZQogICAgICAgICBhY3R1YWxfdXJpID0gTm9uZQogICAg ICAgICBhY3R1YWxfY2hlY2tzdW0gPSBOb25lCi0KKyAgICAgICAgc2VsZi5fY2xlYXJfb3V0cHV0 X2ltYWdlKCkKICAgICAgICAgc3RhcnRfdGltZSA9IHRpbWUudGltZSgpCiAKICAgICAgICAgdXJp ID0gc2VsZi5fcG9ydC5maWxlbmFtZV90b191cmkoZHJpdmVyX2lucHV0LmZpbGVuYW1lKQpAQCAt NDk3LDkgKzUwMiwxMCBAQCBjbGFzcyBDaHJvbWl1bURyaXZlcihiYXNlLkRyaXZlcik6CiAgICAg ICAgICAgICAobGluZSwgY3Jhc2gpID0gc2VsZi5fd3JpdGVfY29tbWFuZF9hbmRfcmVhZF9saW5l KGlucHV0PU5vbmUpCiAKICAgICAgICAgcnVuX3RpbWUgPSB0aW1lLnRpbWUoKSAtIHN0YXJ0X3Rp bWUKLSAgICAgICAgcmV0dXJuIGJhc2UuRHJpdmVyT3V0cHV0KAotICAgICAgICAgICAgJycuam9p bihvdXRwdXQpLCBzZWxmLl9vdXRwdXRfaW1hZ2Vfd2l0aF9yZXRyeSgpLCBhY3R1YWxfY2hlY2tz dW0sCi0gICAgICAgICAgICBjcmFzaCwgcnVuX3RpbWUsIHRpbWVvdXQsICcnLmpvaW4oZXJyb3Ip KQorICAgICAgICBvdXRwdXRfaW1hZ2UgPSBzZWxmLl9vdXRwdXRfaW1hZ2Vfd2l0aF9yZXRyeSgp CisgICAgICAgIGFzc2VydCBvdXRwdXRfaW1hZ2UgaXMgbm90IE5vbmUKKyAgICAgICAgcmV0dXJu IGJhc2UuRHJpdmVyT3V0cHV0KCcnLmpvaW4ob3V0cHV0KSwgb3V0cHV0X2ltYWdlLCBhY3R1YWxf Y2hlY2tzdW0sCisgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICBjcmFzaCwgcnVuX3Rp bWUsIHRpbWVvdXQsICcnLmpvaW4oZXJyb3IpKQogCiAgICAgZGVmIHN0b3Aoc2VsZik6CiAgICAg ICAgIGlmIHNlbGYuX3Byb2M6Cg==