86501 2012-05-15 11:00:43 -0700 add unit test for NRWT's --additional-expectations option 2012-05-15 15:05:15 -0700 1 1 1 Unclassified WebKit Tools / Tests 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 epoger epoger abarth dpranke ojan webkit.review.bot oldest_to_newest 624271 0 epoger 2012-05-15 11:00:43 -0700 This is in addition to the integration test already added in http://trac.webkit.org/changeset/114559 624275 1 142007 epoger 2012-05-15 11:02:52 -0700 Created attachment 142007 Patch 624284 2 142007 ojan 2012-05-15 11:09:51 -0700 Comment on attachment 142007 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142007&action=review tests++ > Tools/Scripts/webkitpy/layout_tests/port/base_unittest.py:296 > + '/tmp/additional-expectations-1.txt'] > + self.assertEquals('content1\n', port.test_expectations_overrides()) > + > + port._options.additional_expectations = [ > + '/tmp/nonexistent-file', '/tmp/additional-expectations-1.txt'] It surprises me that we don't raise an error when you pass in a non-existant file. Is there a use-case for that? Obviously, this patch is fine since it's just testing the existing behavior. 624307 3 142007 epoger 2012-05-15 11:27:45 -0700 Comment on attachment 142007 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142007&action=review >> Tools/Scripts/webkitpy/layout_tests/port/base_unittest.py:296 >> + '/tmp/nonexistent-file', '/tmp/additional-expectations-1.txt'] > > It surprises me that we don't raise an error when you pass in a non-existant file. Is there a use-case for that? > > Obviously, this patch is fine since it's just testing the existing behavior. I just happened upon this behavior... but I can think of at least one potential use case: I want to add an expectations override file within the Skia tree that NRWT always observes, and depending on a given checkout's DEPS, that expectations override file might or might not be found... But I'm sure there are other ways to handle that. I would not be opposed to failing on file-not-found. 624309 4 142007 webkit.review.bot 2012-05-15 11:28:38 -0700 Comment on attachment 142007 Patch Rejecting attachment 142007 from commit-queue. epoger@google.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py. - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your committer rights. 624320 5 epoger 2012-05-15 11:37:15 -0700 Please, nobody hit commit+ on this... I'm supposedly a webkit committer now, so I want to make sure I can get this to work. :-) 624338 6 142007 dpranke 2012-05-15 11:49:34 -0700 Comment on attachment 142007 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142007&action=review >>> Tools/Scripts/webkitpy/layout_tests/port/base_unittest.py:296 >>> + '/tmp/nonexistent-file', '/tmp/additional-expectations-1.txt'] >> >> It surprises me that we don't raise an error when you pass in a non-existant file. Is there a use-case for that? >> >> Obviously, this patch is fine since it's just testing the existing behavior. > > I just happened upon this behavior... but I can think of at least one potential use case: I want to add an expectations override file within the Skia tree that NRWT always observes, and depending on a given checkout's DEPS, that expectations override file might or might not be found... > > But I'm sure there are other ways to handle that. I would not be opposed to failing on file-not-found. The code logs a warning, but intentionally does not fail. At least until proven otherwise, it didn't seem like this should be a fatal error. There are actually a couple of tests for this functionality in run_webkit_tests_integrationtest.py, but I'm not opposed to adding a unit test here as well. 624462 7 142007 webkit.review.bot 2012-05-15 13:18:19 -0700 Comment on attachment 142007 Patch Rejecting attachment 142007 from commit-queue. epoger@google.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py. - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your committer rights. 624624 8 142007 webkit.review.bot 2012-05-15 15:05:10 -0700 Comment on attachment 142007 Patch Clearing flags on attachment: 142007 Committed r117168: <http://trac.webkit.org/changeset/117168> 624625 9 webkit.review.bot 2012-05-15 15:05:15 -0700 All reviewed patches have been landed. Closing bug. 142007 2012-05-15 11:02:52 -0700 2012-05-15 15:05:09 -0700 Patch bug-86501-20120515110251.patch text/plain 2307 epoger SW5kZXg6IFRvb2xzL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBUb29scy9DaGFuZ2VMb2cJKHJl dmlzaW9uIDExNzA5NCkKKysrIFRvb2xzL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwz ICsxLDEzIEBACisyMDEyLTA1LTE1ICBFbGxpb3QgUG9nZXIgIDxlcG9nZXJAZ29vZ2xlLmNvbT4K KworICAgICAgICBhZGQgdW5pdCB0ZXN0IGZvciBOUldUJ3MgLS1hZGRpdGlvbmFsLWV4cGVjdGF0 aW9ucyBvcHRpb24KKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dp P2lkPTg2NTAxCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAg ICAgKiBTY3JpcHRzL3dlYmtpdHB5L2xheW91dF90ZXN0cy9wb3J0L2Jhc2VfdW5pdHRlc3QucHk6 CisgICAgICAgIChQb3J0VGVzdC50ZXN0X2FkZGl0aW9uYWxfZXhwZWN0YXRpb25zKToKKwogMjAx Mi0wNS0xNSAgS2VubmV0aCBSb2hkZSBDaHJpc3RpYW5zZW4gIDxrZW5uZXRoQHdlYmtpdC5vcmc+ CiAKICAgICAgICAgW1F0XSBBZGQgaW5mcmEgZm9yIHRlc3RpbmcgZG91YmxlLXRhcCB0byB6b29t IGZ1bmN0aW9uYWxpdHkgZXRjCkluZGV4OiBUb29scy9TY3JpcHRzL3dlYmtpdHB5L2xheW91dF90 ZXN0cy9wb3J0L2Jhc2VfdW5pdHRlc3QucHkKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gVG9vbHMvU2NyaXB0cy93 ZWJraXRweS9sYXlvdXRfdGVzdHMvcG9ydC9iYXNlX3VuaXR0ZXN0LnB5CShyZXZpc2lvbiAxMTY3 ODUpCisrKyBUb29scy9TY3JpcHRzL3dlYmtpdHB5L2xheW91dF90ZXN0cy9wb3J0L2Jhc2VfdW5p dHRlc3QucHkJKHdvcmtpbmcgY29weSkKQEAgLTI3OCw2ICsyNzgsMjggQEAgY2xhc3MgUG9ydFRl c3QodW5pdHRlc3QuVGVzdENhc2UpOgogICAgICAgICAgICAgWygnL3RtcC9sb2NhbC1iYXNlbGlu ZXMnLCAnZmFzdC90ZXN0LWV4cGVjdGVkLnR4dCcpXSkKICAgICAgICAgc2VsZi5hc3NlcnRFcXVh bChwb3J0LmJhc2VsaW5lX3BhdGgoKSwgJy9mb28nKQogCisgICAgZGVmIHRlc3RfYWRkaXRpb25h bF9leHBlY3RhdGlvbnMoc2VsZik6CisgICAgICAgIHBvcnQgPSBzZWxmLm1ha2VfcG9ydChwb3J0 X25hbWU9J2ZvbycpCisKKyAgICAgICAgcG9ydC5fZmlsZXN5c3RlbS53cml0ZV90ZXh0X2ZpbGUo CisgICAgICAgICAgICAnL3RtcC9hZGRpdGlvbmFsLWV4cGVjdGF0aW9ucy0xLnR4dCcsICdjb250 ZW50MVxuJykKKyAgICAgICAgcG9ydC5fZmlsZXN5c3RlbS53cml0ZV90ZXh0X2ZpbGUoCisgICAg ICAgICAgICAnL3RtcC9hZGRpdGlvbmFsLWV4cGVjdGF0aW9ucy0yLnR4dCcsICdjb250ZW50Mlxu JykKKworICAgICAgICBzZWxmLmFzc2VydEVxdWFscyhOb25lLCBwb3J0LnRlc3RfZXhwZWN0YXRp b25zX292ZXJyaWRlcygpKQorCisgICAgICAgIHBvcnQuX29wdGlvbnMuYWRkaXRpb25hbF9leHBl Y3RhdGlvbnMgPSBbCisgICAgICAgICAgICAnL3RtcC9hZGRpdGlvbmFsLWV4cGVjdGF0aW9ucy0x LnR4dCddCisgICAgICAgIHNlbGYuYXNzZXJ0RXF1YWxzKCdjb250ZW50MVxuJywgcG9ydC50ZXN0 X2V4cGVjdGF0aW9uc19vdmVycmlkZXMoKSkKKworICAgICAgICBwb3J0Ll9vcHRpb25zLmFkZGl0 aW9uYWxfZXhwZWN0YXRpb25zID0gWworICAgICAgICAgICAgJy90bXAvbm9uZXhpc3RlbnQtZmls ZScsICcvdG1wL2FkZGl0aW9uYWwtZXhwZWN0YXRpb25zLTEudHh0J10KKyAgICAgICAgc2VsZi5h c3NlcnRFcXVhbHMoJ2NvbnRlbnQxXG4nLCBwb3J0LnRlc3RfZXhwZWN0YXRpb25zX292ZXJyaWRl cygpKQorCisgICAgICAgIHBvcnQuX29wdGlvbnMuYWRkaXRpb25hbF9leHBlY3RhdGlvbnMgPSBb CisgICAgICAgICAgICAnL3RtcC9hZGRpdGlvbmFsLWV4cGVjdGF0aW9ucy0xLnR4dCcsICcvdG1w L2FkZGl0aW9uYWwtZXhwZWN0YXRpb25zLTIudHh0J10KKyAgICAgICAgc2VsZi5hc3NlcnRFcXVh bHMoJ2NvbnRlbnQxXG5jb250ZW50MlxuJywgcG9ydC50ZXN0X2V4cGVjdGF0aW9uc19vdmVycmlk ZXMoKSkKKwogICAgIGRlZiB0ZXN0X3VzZXNfdGVzdF9leHBlY3RhdGlvbnNfZmlsZShzZWxmKToK ICAgICAgICAgcG9ydCA9IHNlbGYubWFrZV9wb3J0KHBvcnRfbmFtZT0nZm9vJykKICAgICAgICAg cG9ydC5wYXRoX3RvX3Rlc3RfZXhwZWN0YXRpb25zX2ZpbGUgPSBsYW1iZGE6ICcvbW9jay1yZXN1 bHRzL3Rlc3RfZXhwZWN0YXRpb25zLnR4dCcK