58196 2011-04-09 19:47:22 -0700 new-run-webkit-tests: add unit tests for Port.diff_image() 2011-04-11 12:28:22 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Other OS X 10.5 RESOLVED FIXED P2 Normal --- 58195 1 dpranke dpranke abarth eric mihaip ojan tony oldest_to_newest 382830 0 dpranke 2011-04-09 19:47:22 -0700 new-run-webkit-tests: add unit tests for Port.diff_image() 382831 1 88942 dpranke 2011-04-09 19:51:53 -0700 Created attachment 88942 Patch 382872 2 88942 ojan 2011-04-10 02:01:57 -0700 Comment on attachment 88942 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=88942&action=review > Tools/Scripts/webkitpy/layout_tests/port/chromium_unittest.py:123 > + # Routine is not implemented. this comment here and below doesn't actually communicate anything more to me that the code. > Tools/Scripts/webkitpy/layout_tests/port/port_testcase.py:135 > + if not port: > + return i don't get when we would ever return a null port given that this is a unittest. but i see that the other tests all do this, so i won't block this patch on that. 382932 3 dpranke 2011-04-10 14:08:43 -0700 (In reply to comment #2) > (From update of attachment 88942 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=88942&action=review > > > Tools/Scripts/webkitpy/layout_tests/port/chromium_unittest.py:123 > > + # Routine is not implemented. > > this comment here and below doesn't actually communicate anything more to me that the code. > Yeah, in retrospect you're right. I'm not sure what the best way to convey why those routines are no-ops is. The base class (PortTestCase) implements a bunch of tests that should be done against any Port implementation, e.g., test_baseline_search_path() tests that each port has implemented baseline_search_path() correctly. The default implementation of baseline_search_path() throws a NotImplementedError, but the ChromiumPort doesn't override it, so test_baseline_search_path() would fail. So, we override the test here. Any suggestions on how to summarize that? Or would you say that no summary is necessary? > > Tools/Scripts/webkitpy/layout_tests/port/port_testcase.py:135 > > + if not port: > > + return > > i don't get when we would ever return a null port given that this is a unittest. but i see that the other tests all do this, so i won't block this patch on that. The original intent of the PortTestCase class was to do a bunch of integration tests (like actually diffing images). In that case, the make_port() routine was allowed to return None if it could not actually function (like, trying to run the windows ImageDiff on a mac). I've gradually moved away from this and been adding unit tests that should pass on every port, like these, in which case the port routine should be non-null. I need a better way to indicate which type of test is which and figure out which test should run and which should be skipped. 382943 4 ojan 2011-04-10 15:24:43 -0700 (In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 88942 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=88942&action=review > > > > > Tools/Scripts/webkitpy/layout_tests/port/chromium_unittest.py:123 > > > + # Routine is not implemented. > > > > this comment here and below doesn't actually communicate anything more to me that the code. > > > > Yeah, in retrospect you're right. I'm not sure what the best way to convey why those routines are no-ops is. > > The base class (PortTestCase) implements a bunch of tests that should be done against any Port implementation, e.g., test_baseline_search_path() tests that each port has implemented baseline_search_path() correctly. The default implementation of baseline_search_path() throws a NotImplementedError, but the ChromiumPort doesn't override it, so test_baseline_search_path() would fail. So, we override the test here. > > Any suggestions on how to summarize that? Or would you say that no summary is necessary? Something like: "Override this test since ChromiumPort doesn't implement the APIs necessary to pass it." Although, it's weird to have a test in the base test class that doesn't pass on all the ports. A couple options: -Move the test into a subclass -Implement noop versions of the appropriate methods in ChromiumPort I'm fine with just adding the comment though. > > > Tools/Scripts/webkitpy/layout_tests/port/port_testcase.py:135 > > > + if not port: > > > + return > > > > i don't get when we would ever return a null port given that this is a unittest. but i see that the other tests all do this, so i won't block this patch on that. > > The original intent of the PortTestCase class was to do a bunch of integration tests (like actually diffing images). In that case, the make_port() routine was allowed to return None if it could not actually function (like, trying to run the windows ImageDiff on a mac). > > I've gradually moved away from this and been adding unit tests that should pass on every port, like these, in which case the port routine should be non-null. I need a better way to indicate which type of test is which and figure out which test should run and which should be skipped. Makes sense. 383367 5 dpranke 2011-04-11 12:25:02 -0700 (In reply to comment #4) > > Although, it's weird to have a test in the base test class that doesn't pass on all the ports. A couple options: > -Move the test into a subclass > -Implement noop versions of the appropriate methods in ChromiumPort > I agree that it's a bit weird; The problem is more that ChromiumPort isn't a real port. It's probably debatable whether it should even be subclassing PortTestCase. 383371 6 dpranke 2011-04-11 12:28:22 -0700 Committed r83476: <http://trac.webkit.org/changeset/83476> 88942 2011-04-09 19:51:53 -0700 2011-04-10 02:01:57 -0700 Patch bug-58196-20110409195152.patch text/plain 4884 dpranke U3VidmVyc2lvbiBSZXZpc2lvbjogODMzODQKZGlmZiAtLWdpdCBhL1Rvb2xzL0NoYW5nZUxvZyBi L1Rvb2xzL0NoYW5nZUxvZwppbmRleCA5NjRlOTEzNWU2NzZmMWRiZjc0MWQ2ZjJkZTkxMWUzMGFm OTdlZjE5Li42YWU2ZWQwN2EwODExYjM5NGNlZjYzZjM5N2E0MGY5NGViNDFiYWJiIDEwMDY0NAot LS0gYS9Ub29scy9DaGFuZ2VMb2cKKysrIGIvVG9vbHMvQ2hhbmdlTG9nCkBAIC0yLDYgKzIsMTkg QEAKIAogICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KIAorICAgICAgICBuZXct cnVuLXdlYmtpdC10ZXN0czogYWRkIHVuaXQgdGVzdHMgZm9yIFBvcnQuZGlmZl9pbWFnZSgpCisg ICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD01ODE5NgorCisg ICAgICAgIEFkZCB0aGUgdW5pdCB0ZXN0cyBmb3IgdGhlIGZpeCBpbiBidWcgNTgxOTUuCisKKyAg ICAgICAgKiBTY3JpcHRzL3dlYmtpdHB5L2xheW91dF90ZXN0cy9wb3J0L2Nocm9taXVtX3VuaXR0 ZXN0LnB5OgorICAgICAgICAqIFNjcmlwdHMvd2Via2l0cHkvbGF5b3V0X3Rlc3RzL3BvcnQvcG9y dF90ZXN0Y2FzZS5weToKKyAgICAgICAgKiBTY3JpcHRzL3dlYmtpdHB5L2xheW91dF90ZXN0cy9w b3J0L3dlYmtpdF91bml0dGVzdC5weToKKworMjAxMS0wNC0wOSAgRGlyayBQcmFua2UgIDxkcHJh bmtlQGNocm9taXVtLm9yZz4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4K KwogICAgICAgICBuZXctcnVuLXdlYmtpdC10ZXN0czogaW1wbGVtZW50IHN1cHBvcnQgZm9yIGF1 ZGlvLCB0YWtlIHR3bwogICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5j Z2k/aWQ9NTgxOTUKIApkaWZmIC0tZ2l0IGEvVG9vbHMvU2NyaXB0cy93ZWJraXRweS9sYXlvdXRf dGVzdHMvcG9ydC9jaHJvbWl1bV91bml0dGVzdC5weSBiL1Rvb2xzL1NjcmlwdHMvd2Via2l0cHkv bGF5b3V0X3Rlc3RzL3BvcnQvY2hyb21pdW1fdW5pdHRlc3QucHkKaW5kZXggZTc5ODAzNDc1MDYy ZmM0MmNjZWNmN2ExMGNhMzFlZTkxZTcxYmIyNi4uOTI0YjUyOTJiMjFlZjdjODJhMzFkYjEwOTg4 MmM1MTIzN2QzNTc5NCAxMDA2NDQKLS0tIGEvVG9vbHMvU2NyaXB0cy93ZWJraXRweS9sYXlvdXRf dGVzdHMvcG9ydC9jaHJvbWl1bV91bml0dGVzdC5weQorKysgYi9Ub29scy9TY3JpcHRzL3dlYmtp dHB5L2xheW91dF90ZXN0cy9wb3J0L2Nocm9taXVtX3VuaXR0ZXN0LnB5CkBAIC00MSw4ICs0MSwx MCBAQCBpbXBvcnQgY2hyb21pdW1fbGludXgKIGltcG9ydCBjaHJvbWl1bV9tYWMKIGltcG9ydCBj aHJvbWl1bV93aW4KIAotY2xhc3MgQ2hyb21pdW1Ecml2ZXJUZXN0KHVuaXR0ZXN0LlRlc3RDYXNl KToKK2Zyb20gd2Via2l0cHkubGF5b3V0X3Rlc3RzLnBvcnQgaW1wb3J0IHBvcnRfdGVzdGNhc2UK KwogCitjbGFzcyBDaHJvbWl1bURyaXZlclRlc3QodW5pdHRlc3QuVGVzdENhc2UpOgogICAgIGRl ZiBzZXRVcChzZWxmKToKICAgICAgICAgbW9ja19wb3J0ID0gTW9jaygpCiAgICAgICAgIG1vY2tf cG9ydC5nZXRfb3B0aW9uID0gbGFtYmRhIG9wdGlvbl9uYW1lOiAnJwpAQCAtMTEyLDcgKzExNCwx OSBAQCBjbGFzcyBDaHJvbWl1bURyaXZlclRlc3QodW5pdHRlc3QuVGVzdENhc2UpOgogICAgICAg ICBzZWxmLmFzc2VydFRydWUoc2VsZi53YWl0X2NhbGxlZCkKICAgICAgICAgc2VsZi5hc3NlcnRF cXVhbHMoc2VsZi5waWQsIDEpCiAKLWNsYXNzIENocm9taXVtUG9ydFRlc3QodW5pdHRlc3QuVGVz dENhc2UpOgorCitjbGFzcyBDaHJvbWl1bVBvcnRUZXN0KHBvcnRfdGVzdGNhc2UuUG9ydFRlc3RD YXNlKToKKyAgICBkZWYgcG9ydF9tYWtlcihzZWxmLCBwbGF0Zm9ybSk6CisgICAgICAgIHJldHVy biBjaHJvbWl1bS5DaHJvbWl1bVBvcnQKKworICAgIGRlZiB0ZXN0X2RyaXZlcl9jbWRfbGluZShz ZWxmKToKKyAgICAgICAgIyBSb3V0aW5lIGlzIG5vdCBpbXBsZW1lbnRlZC4KKyAgICAgICAgcGFz cworCisgICAgZGVmIHRlc3RfYmFzZWxpbmVfc2VhcmNoX3BhdGgoc2VsZik6CisgICAgICAgICMg Um91dGluZSBpcyBub3QgaW1wbGVtZW50ZWQuCisgICAgICAgIHBhc3MKKwogICAgIGNsYXNzIFRl c3RNYWNQb3J0KGNocm9taXVtX21hYy5DaHJvbWl1bU1hY1BvcnQpOgogICAgICAgICBkZWYgX19p bml0X18oc2VsZiwgb3B0aW9ucyk6CiAgICAgICAgICAgICBjaHJvbWl1bV9tYWMuQ2hyb21pdW1N YWNQb3J0Ll9faW5pdF9fKHNlbGYsCmRpZmYgLS1naXQgYS9Ub29scy9TY3JpcHRzL3dlYmtpdHB5 L2xheW91dF90ZXN0cy9wb3J0L3BvcnRfdGVzdGNhc2UucHkgYi9Ub29scy9TY3JpcHRzL3dlYmtp dHB5L2xheW91dF90ZXN0cy9wb3J0L3BvcnRfdGVzdGNhc2UucHkKaW5kZXggNjQ5ZTMzY2Y4Nzhj YzA0Yjc0ZTNkNGVlMGU3OWVhOTg3YjBkZWY1OS4uYjVmOTRlZDdhOGRkMmEwN2U2YTQ2OGYwMjRj YjgyZDM5ZTE3ZmRlZCAxMDA2NDQKLS0tIGEvVG9vbHMvU2NyaXB0cy93ZWJraXRweS9sYXlvdXRf dGVzdHMvcG9ydC9wb3J0X3Rlc3RjYXNlLnB5CisrKyBiL1Rvb2xzL1NjcmlwdHMvd2Via2l0cHkv bGF5b3V0X3Rlc3RzL3BvcnQvcG9ydF90ZXN0Y2FzZS5weQpAQCAtMTEzLDYgKzExMywzMCBAQCBj bGFzcyBQb3J0VGVzdENhc2UodW5pdHRlc3QuVGVzdENhc2UpOgogCiAgICAgICAgIHBvcnQuX2Zp bGVzeXN0ZW0ucmVtb3ZlKHRtcGZpbGUpCiAKKyAgICBkZWYgdGVzdF9kaWZmX2ltYWdlX19taXNz aW5nX2JvdGgoc2VsZik6CisgICAgICAgIHBvcnQgPSBzZWxmLm1ha2VfcG9ydCgpCisgICAgICAg IGlmIG5vdCBwb3J0OgorICAgICAgICAgICAgcmV0dXJuCisgICAgICAgIHNlbGYuYXNzZXJ0RmFs c2UocG9ydC5kaWZmX2ltYWdlKE5vbmUsIE5vbmUsIE5vbmUpKQorICAgICAgICBzZWxmLmFzc2Vy dEZhbHNlKHBvcnQuZGlmZl9pbWFnZShOb25lLCAnJywgTm9uZSkpCisgICAgICAgIHNlbGYuYXNz ZXJ0RmFsc2UocG9ydC5kaWZmX2ltYWdlKCcnLCBOb25lLCBOb25lKSkKKyAgICAgICAgc2VsZi5h c3NlcnRGYWxzZShwb3J0LmRpZmZfaW1hZ2UoJycsICcnLCBOb25lKSkKKworICAgIGRlZiB0ZXN0 X2RpZmZfaW1hZ2VfX21pc3NpbmdfYWN0dWFsKHNlbGYpOgorICAgICAgICBwb3J0ID0gc2VsZi5t YWtlX3BvcnQoKQorICAgICAgICBpZiBub3QgcG9ydDoKKyAgICAgICAgICAgIHJldHVybgorICAg ICAgICBzZWxmLmFzc2VydFRydWUocG9ydC5kaWZmX2ltYWdlKE5vbmUsICdmb28nLCBOb25lKSkK KyAgICAgICAgc2VsZi5hc3NlcnRUcnVlKHBvcnQuZGlmZl9pbWFnZSgnJywgJ2ZvbycsIE5vbmUp KQorCisgICAgZGVmIHRlc3RfZGlmZl9pbWFnZV9fbWlzc2luZ19leHBlY3RlZChzZWxmKToKKyAg ICAgICAgcG9ydCA9IHNlbGYubWFrZV9wb3J0KCkKKyAgICAgICAgaWYgbm90IHBvcnQ6CisgICAg ICAgICAgICByZXR1cm4KKyAgICAgICAgc2VsZi5hc3NlcnRUcnVlKHBvcnQuZGlmZl9pbWFnZSgn Zm9vJywgTm9uZSwgTm9uZSkpCisgICAgICAgIHNlbGYuYXNzZXJ0VHJ1ZShwb3J0LmRpZmZfaW1h Z2UoJ2ZvbycsICcnLCBOb25lKSkKKworCiAgICAgZGVmIGRpc2FibGVkX3Rlc3Rfd2Vic29ja2V0 X3NlcnZlcihzZWxmKToKICAgICAgICAgcG9ydCA9IHNlbGYubWFrZV9wb3J0KCkKICAgICAgICAg aWYgbm90IHBvcnQ6CmRpZmYgLS1naXQgYS9Ub29scy9TY3JpcHRzL3dlYmtpdHB5L2xheW91dF90 ZXN0cy9wb3J0L3dlYmtpdF91bml0dGVzdC5weSBiL1Rvb2xzL1NjcmlwdHMvd2Via2l0cHkvbGF5 b3V0X3Rlc3RzL3BvcnQvd2Via2l0X3VuaXR0ZXN0LnB5CmluZGV4IGM3MmE0MTFhY2IwMDNmYjg4 NmQ2MWFmNDE2NWQxMWRmMjAxNmVjZTQuLmVmMWExYzIzNjY0OTA3ZDQ1NjU0ZDFkYjk3OTIwZDJm YTkwNDk2MzAgMTAwNjQ0Ci0tLSBhL1Rvb2xzL1NjcmlwdHMvd2Via2l0cHkvbGF5b3V0X3Rlc3Rz L3BvcnQvd2Via2l0X3VuaXR0ZXN0LnB5CisrKyBiL1Rvb2xzL1NjcmlwdHMvd2Via2l0cHkvbGF5 b3V0X3Rlc3RzL3BvcnQvd2Via2l0X3VuaXR0ZXN0LnB5CkBAIC0zMCw2ICszMCw3IEBAIGltcG9y dCB1bml0dGVzdAogZnJvbSB3ZWJraXRweS5jb21tb24uc3lzdGVtIGltcG9ydCBmaWxlc3lzdGVt X21vY2sKIAogZnJvbSB3ZWJraXRweS5sYXlvdXRfdGVzdHMucG9ydC53ZWJraXQgaW1wb3J0IFdl YktpdFBvcnQKK2Zyb20gd2Via2l0cHkubGF5b3V0X3Rlc3RzLnBvcnQgaW1wb3J0IHBvcnRfdGVz dGNhc2UKIAogCiBjbGFzcyBUZXN0V2ViS2l0UG9ydChXZWJLaXRQb3J0KToKQEAgLTYzLDcgKzY0 LDE4IEBAIGNsYXNzIFRlc3RXZWJLaXRQb3J0KFdlYktpdFBvcnQpOgogICAgICAgICAgICAgcmV0 dXJuIFtzZWxmLnNraXBzX2ZpbGVdCiAgICAgICAgIHJldHVybiBbXQogCi1jbGFzcyBXZWJLaXRQ b3J0VGVzdCh1bml0dGVzdC5UZXN0Q2FzZSk6CisKK2NsYXNzIFdlYktpdFBvcnRUZXN0KHBvcnRf dGVzdGNhc2UuUG9ydFRlc3RDYXNlKToKKyAgICBkZWYgcG9ydF9tYWtlcihzZWxmLCBwbGF0Zm9y bSk6CisgICAgICAgIHJldHVybiBXZWJLaXRQb3J0CisKKyAgICBkZWYgdGVzdF9kcml2ZXJfY21k X2xpbmUoc2VsZik6CisgICAgICAgICMgUm91dGluZSBpcyBub3QgaW1wbGVtZW50ZWQuCisgICAg ICAgIHBhc3MKKworICAgIGRlZiB0ZXN0X2Jhc2VsaW5lX3NlYXJjaF9wYXRoKHNlbGYpOgorICAg ICAgICAjIFJvdXRpbmUgaXMgbm90IGltcGxlbWVudGVkLgorICAgICAgICBwYXNzCiAKICAgICBk ZWYgdGVzdF9za2lwcGVkX2RpcmVjdG9yaWVzX2Zvcl9zeW1ib2xzKHNlbGYpOgogICAgICAgICBz dXBwb3J0ZWRfc3ltYm9scyA9IFsiR3JhcGhpY3NMYXllciIsICJXZWJDb3JlSGFzM0RSZW5kZXJp bmciLCAiaXNYSFRNTE1QRG9jdW1lbnQiLCAiZm9vU3ltYm9sIl0K