101455 2012-11-07 05:24:44 -0800 webkitpy/layouttests integration test fails if high shards/processes environment variables are used 2012-11-09 09:50:34 -0800 1 1 1 Unclassified WebKit Tools / Tests 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 101442 1 d-r d-r abarth dpranke ojan webkit.review.bot oldest_to_newest 760560 0 d-r 2012-11-07 05:24:44 -0800 I propose to skip those tests if the environment variables are set. 760565 1 d-r 2012-11-07 05:29:27 -0800 [522/1598] webkitpy.layout_tests.run_webkit_tests_integrationtest.MainTest.test_max_locked_shards failed: Traceback (most recent call last): File "/home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py", line 315, in test_max_locked_shards self.assertTrue(any(['(1 locked)' in line for line in regular_output.buflist])) AssertionError: False is not true is failing, the test itself looks like this: def test_max_locked_shards(self): if not self.should_test_processes: return _, _, regular_output, _ = logging_run(['--debug-rwt-logging', '--child-processes', '2'], shared_port=False) self.assertTrue(any(['(1 locked)' in line for line in regular_output.buflist])) Shouldn't we use the --max-locked-shards switch here instead of making assumptions based on --child-processes? 760584 2 d-r 2012-11-07 05:42:46 -0800 Dirk, I am actually not sure what we should do with this test. Whenever I set something like export WEBKIT_TEST_MAX_LOCKED_SHARDS=4 This test will fail. Changing the test to use the --max-locked-shards=1 parameter doesn't work either when the env var is set. Can you suggest what we should do with this? I'd like to use the setting on our bots to improve layout test time, but currently it makes the python tests fail then. 761002 3 dpranke 2012-11-07 13:12:11 -0800 The intent of that test is to ensure that we still use only one locked shard by default even if we have more than one child process. We should probably document that with a comment or something :). So, using --max-locked-shards would defeat the point of the test. I would suggest we mock out/override os.environ in this particular test to preserve the code path. However, this also points out that we don't have tests ensuring that WEBKIT_TEST_MAX_LOCKED_SHARDS actually does do something, or ensuring that --max-locked-shards actually does the right thing. We should probably add tests for that as well. I don't think we need to (or should) skip the tests. Does that help? 761621 4 173005 d-r 2012-11-08 04:06:17 -0800 Created attachment 173005 Patch, clearing env of WTMLS var. 761622 5 d-r 2012-11-08 04:07:55 -0800 (In reply to comment #3) > The intent of that test is to ensure that we still use only one locked shard by default even if we have more than one child process. We should probably document that with a comment or something :). I put a comment into the test. > > So, using --max-locked-shards would defeat the point of the test. I would suggest we mock out/override os.environ in this particular test to preserve the code path. Attaching a patch for this, hope it looks okay. I have a feeling this could be simpler. > However, this also points out that we don't have tests ensuring that WEBKIT_TEST_MAX_LOCKED_SHARDS actually does do something, or ensuring that --max-locked-shards actually does the right thing. We should probably add tests for that as well. Unfortunately I don't have time to create additional tests, but if you could r+ and land this fix, that'd be helpful for us to start using these env vars on our bots. 761876 6 173005 dpranke 2012-11-08 09:30:29 -0800 Comment on attachment 173005 Patch, clearing env of WTMLS var. View in context: https://bugs.webkit.org/attachment.cgi?id=173005&action=review > Tools/Scripts/webkitpy/layout_tests/run_webkit_tests_integrationtest.py:323 > + os.environ["WEBKIT_TEST_MAX_LOCKED_SHARDS"] = save_env_webkit_test_max_locked_shards This should be in a try/catch block, otherwise if the test fails the variable won't be restored. Feel free to land it with that change. 762905 7 173280 d-r 2012-11-09 05:50:20 -0800 Created attachment 173280 Always restore env. 762911 8 d-r 2012-11-09 05:53:18 -0800 (In reply to comment #7) > Created an attachment (id=173280) [details] > Always restore env. Hopefully I understood you right and that's what you meant. 762952 9 173280 webkit.review.bot 2012-11-09 06:29:07 -0800 Comment on attachment 173280 Always restore env. Clearing flags on attachment: 173280 Committed r134060: <http://trac.webkit.org/changeset/134060> 762953 10 webkit.review.bot 2012-11-09 06:29:11 -0800 All reviewed patches have been landed. Closing bug. 763105 11 173280 dpranke 2012-11-09 09:50:34 -0800 Comment on attachment 173280 Always restore env. Yup, that's good. I meant to say try/finally and not try/catch :) 173005 2012-11-08 04:06:17 -0800 2012-11-09 05:50:12 -0800 Patch, clearing env of WTMLS var. bug-101455-20121108140804.patch text/plain 2562 d-r U3VidmVyc2lvbiBSZXZpc2lvbjogMTMzODc4CmRpZmYgLS1naXQgYS9Ub29scy9DaGFuZ2VMb2cg Yi9Ub29scy9DaGFuZ2VMb2cKaW5kZXggOTBhMDA0MWY4ZjhkYzRkZTYxNmIwYTdkMzBhODU3ZDNi ZDBhZmI5Yi4uMGViNmJhMDc2Mzg5MzQzOTc2YzlkNzkyYzJjYmM5NGU2Yjk0MGYwOCAxMDA2NDQK LS0tIGEvVG9vbHMvQ2hhbmdlTG9nCisrKyBiL1Rvb2xzL0NoYW5nZUxvZwpAQCAtMSw1ICsxLDE3 IEBACiAyMDEyLTExLTA4ICBEb21pbmlrIFLDtnR0c2NoZXMgIDxkb21pbmlrLnJvdHRzY2hlc0Bp bnRlbC5jb20+CiAKKyAgICAgICAgd2Via2l0cHkvbGF5b3V0dGVzdHMgaW50ZWdyYXRpb24gdGVz dCBmYWlscyBpZiBoaWdoIHNoYXJkcy9wcm9jZXNzZXMgZW52aXJvbm1lbnQgdmFyaWFibGVzIGFy ZSB1c2VkCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0x MDE0NTUKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBV cGRhdGluZyB0ZXN0IHRvIGRpc3JlZ2FyZCBXRUJLSVRfVEVTVF9NQVhfTE9DS0VEX1NIQVJEUyBl bnZpcm9ubWVudCB2YXJpYWJsZS4KKworICAgICAgICAqIFNjcmlwdHMvd2Via2l0cHkvbGF5b3V0 X3Rlc3RzL3J1bl93ZWJraXRfdGVzdHNfaW50ZWdyYXRpb250ZXN0LnB5OgorICAgICAgICAoTWFp blRlc3QudGVzdF9tYXhfbG9ja2VkX3NoYXJkcyk6CisKKzIwMTItMTEtMDggIERvbWluaWsgUsO2 dHRzY2hlcyAgPGRvbWluaWsucm90dHNjaGVzQGludGVsLmNvbT4KKwogICAgICAgICBVbnJldmll d2VkLCByb2xsaW5nIG91dCByMTMzODU5LgogICAgICAgICBodHRwOi8vdHJhYy53ZWJraXQub3Jn L2NoYW5nZXNldC8xMzM4NTkKICAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19i dWcuY2dpP2lkPTEwMTUyNgpkaWZmIC0tZ2l0IGEvVG9vbHMvU2NyaXB0cy93ZWJraXRweS9sYXlv dXRfdGVzdHMvcnVuX3dlYmtpdF90ZXN0c19pbnRlZ3JhdGlvbnRlc3QucHkgYi9Ub29scy9TY3Jp cHRzL3dlYmtpdHB5L2xheW91dF90ZXN0cy9ydW5fd2Via2l0X3Rlc3RzX2ludGVncmF0aW9udGVz dC5weQppbmRleCAwY2Y0MmQwZWViMzdmNjUzYTNmZDIxNTM0YzI0MmNmYWUzY2I5N2Y3Li4zOGIw MmM4MzE3NjgwZjhjMzNhODAwNjEwODUyMGRhZjcxMDFlY2I1IDEwMDc1NQotLS0gYS9Ub29scy9T Y3JpcHRzL3dlYmtpdHB5L2xheW91dF90ZXN0cy9ydW5fd2Via2l0X3Rlc3RzX2ludGVncmF0aW9u dGVzdC5weQorKysgYi9Ub29scy9TY3JpcHRzL3dlYmtpdHB5L2xheW91dF90ZXN0cy9ydW5fd2Vi a2l0X3Rlc3RzX2ludGVncmF0aW9udGVzdC5weQpAQCAtMzMsNiArMzMsNyBAQCBpbXBvcnQgY29k ZWNzCiBpbXBvcnQgaXRlcnRvb2xzCiBpbXBvcnQganNvbgogaW1wb3J0IGxvZ2dpbmcKK2ltcG9y dCBvcwogaW1wb3J0IHBsYXRmb3JtCiBpbXBvcnQgUXVldWUKIGltcG9ydCByZQpAQCAtMzA5LDEw ICszMTAsMTcgQEAgY2xhc3MgTWFpblRlc3QodW5pdHRlc3QuVGVzdENhc2UsIFN0cmVhbVRlc3Rp bmdNaXhpbik6CiAgICAgICAgICAgICBzZWxmLmFzc2VydFRydWUobGVuKGJhdGNoKSA8PSAyLCAn JXMgaGFkIHRvbyBtYW55IHRlc3RzJyAlICcsICcuam9pbihiYXRjaCkpCiAKICAgICBkZWYgdGVz dF9tYXhfbG9ja2VkX3NoYXJkcyhzZWxmKToKKyAgICAgICAgIyBUZXN0cyBmb3IgdGhlIGRlZmF1 bHQgb2YgdXNpbmcgb25lIGxvY2tlZCBzaGFyZCBldmVuIGluIHRoZSBjYXNlIG9mIG1vcmUgdGhh biBvbmUgY2hpbGQgcHJvY2Vzcy4KICAgICAgICAgaWYgbm90IHNlbGYuc2hvdWxkX3Rlc3RfcHJv Y2Vzc2VzOgogICAgICAgICAgICAgcmV0dXJuCisgICAgICAgIHNhdmVfZW52X3dlYmtpdF90ZXN0 X21heF9sb2NrZWRfc2hhcmRzID0gTm9uZQorICAgICAgICBpZiAiV0VCS0lUX1RFU1RfTUFYX0xP Q0tFRF9TSEFSRFMiIGluIG9zLmVudmlyb246CisgICAgICAgICAgICBzYXZlX2Vudl93ZWJraXRf dGVzdF9tYXhfbG9ja2VkX3NoYXJkcyA9IG9zLmVudmlyb25bIldFQktJVF9URVNUX01BWF9MT0NL RURfU0hBUkRTIl0KKyAgICAgICAgICAgIGRlbCBvcy5lbnZpcm9uWyJXRUJLSVRfVEVTVF9NQVhf TE9DS0VEX1NIQVJEUyJdCiAgICAgICAgIF8sIF8sIHJlZ3VsYXJfb3V0cHV0LCBfID0gbG9nZ2lu Z19ydW4oWyctLWRlYnVnLXJ3dC1sb2dnaW5nJywgJy0tY2hpbGQtcHJvY2Vzc2VzJywgJzInXSwg c2hhcmVkX3BvcnQ9RmFsc2UpCiAgICAgICAgIHNlbGYuYXNzZXJ0VHJ1ZShhbnkoWycoMSBsb2Nr ZWQpJyBpbiBsaW5lIGZvciBsaW5lIGluIHJlZ3VsYXJfb3V0cHV0LmJ1Zmxpc3RdKSkKKyAgICAg ICAgaWYgc2F2ZV9lbnZfd2Via2l0X3Rlc3RfbWF4X2xvY2tlZF9zaGFyZHM6CisgICAgICAgICAg ICBvcy5lbnZpcm9uWyJXRUJLSVRfVEVTVF9NQVhfTE9DS0VEX1NIQVJEUyJdID0gc2F2ZV9lbnZf d2Via2l0X3Rlc3RfbWF4X2xvY2tlZF9zaGFyZHMKIAogICAgIGRlZiB0ZXN0X2NoaWxkX3Byb2Nl c3Nlc18yKHNlbGYpOgogICAgICAgICBpZiBzZWxmLnNob3VsZF90ZXN0X3Byb2Nlc3NlczoK 173280 2012-11-09 05:50:20 -0800 2012-11-09 09:50:34 -0800 Always restore env. bug-101455-20121109155207.patch text/plain 2692 d-r U3VidmVyc2lvbiBSZXZpc2lvbjogMTMzODc4CmRpZmYgLS1naXQgYS9Ub29scy9DaGFuZ2VMb2cg Yi9Ub29scy9DaGFuZ2VMb2cKaW5kZXggOTBhMDA0MWY4ZjhkYzRkZTYxNmIwYTdkMzBhODU3ZDNi ZDBhZmI5Yi4uZTA3YjQ2OTUyZWE3ZmRmOGU1OTg1YTdkNGJlMDFkOWRkYzUwZDg5NiAxMDA2NDQK LS0tIGEvVG9vbHMvQ2hhbmdlTG9nCisrKyBiL1Rvb2xzL0NoYW5nZUxvZwpAQCAtMSw1ICsxLDE3 IEBACiAyMDEyLTExLTA4ICBEb21pbmlrIFLDtnR0c2NoZXMgIDxkb21pbmlrLnJvdHRzY2hlc0Bp bnRlbC5jb20+CiAKKyAgICAgICAgd2Via2l0cHkvbGF5b3V0dGVzdHMgaW50ZWdyYXRpb24gdGVz dCBmYWlscyBpZiBoaWdoIHNoYXJkcy9wcm9jZXNzZXMgZW52aXJvbm1lbnQgdmFyaWFibGVzIGFy ZSB1c2VkCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0x MDE0NTUKKworICAgICAgICBSZXZpZXdlZCBieSBEaXJrIFByYW5rZS4KKworICAgICAgICBVcGRh dGluZyB0ZXN0IHRvIGRpc3JlZ2FyZCBXRUJLSVRfVEVTVF9NQVhfTE9DS0VEX1NIQVJEUyBlbnZp cm9ubWVudCB2YXJpYWJsZS4KKworICAgICAgICAqIFNjcmlwdHMvd2Via2l0cHkvbGF5b3V0X3Rl c3RzL3J1bl93ZWJraXRfdGVzdHNfaW50ZWdyYXRpb250ZXN0LnB5OgorICAgICAgICAoTWFpblRl c3QudGVzdF9tYXhfbG9ja2VkX3NoYXJkcyk6CisKKzIwMTItMTEtMDggIERvbWluaWsgUsO2dHRz Y2hlcyAgPGRvbWluaWsucm90dHNjaGVzQGludGVsLmNvbT4KKwogICAgICAgICBVbnJldmlld2Vk LCByb2xsaW5nIG91dCByMTMzODU5LgogICAgICAgICBodHRwOi8vdHJhYy53ZWJraXQub3JnL2No YW5nZXNldC8xMzM4NTkKICAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcu Y2dpP2lkPTEwMTUyNgpkaWZmIC0tZ2l0IGEvVG9vbHMvU2NyaXB0cy93ZWJraXRweS9sYXlvdXRf dGVzdHMvcnVuX3dlYmtpdF90ZXN0c19pbnRlZ3JhdGlvbnRlc3QucHkgYi9Ub29scy9TY3JpcHRz L3dlYmtpdHB5L2xheW91dF90ZXN0cy9ydW5fd2Via2l0X3Rlc3RzX2ludGVncmF0aW9udGVzdC5w eQppbmRleCAwY2Y0MmQwZWViMzdmNjUzYTNmZDIxNTM0YzI0MmNmYWUzY2I5N2Y3Li44ZTM1NzQ3 ZDM5YTZmZmQ4NTc1NjA5NDc5N2Y5YzE0NTQ4NDkxZTJjIDEwMDc1NQotLS0gYS9Ub29scy9TY3Jp cHRzL3dlYmtpdHB5L2xheW91dF90ZXN0cy9ydW5fd2Via2l0X3Rlc3RzX2ludGVncmF0aW9udGVz dC5weQorKysgYi9Ub29scy9TY3JpcHRzL3dlYmtpdHB5L2xheW91dF90ZXN0cy9ydW5fd2Via2l0 X3Rlc3RzX2ludGVncmF0aW9udGVzdC5weQpAQCAtMzMsNiArMzMsNyBAQCBpbXBvcnQgY29kZWNz CiBpbXBvcnQgaXRlcnRvb2xzCiBpbXBvcnQganNvbgogaW1wb3J0IGxvZ2dpbmcKK2ltcG9ydCBv cwogaW1wb3J0IHBsYXRmb3JtCiBpbXBvcnQgUXVldWUKIGltcG9ydCByZQpAQCAtMzA5LDEwICsz MTAsMTkgQEAgY2xhc3MgTWFpblRlc3QodW5pdHRlc3QuVGVzdENhc2UsIFN0cmVhbVRlc3RpbmdN aXhpbik6CiAgICAgICAgICAgICBzZWxmLmFzc2VydFRydWUobGVuKGJhdGNoKSA8PSAyLCAnJXMg aGFkIHRvbyBtYW55IHRlc3RzJyAlICcsICcuam9pbihiYXRjaCkpCiAKICAgICBkZWYgdGVzdF9t YXhfbG9ja2VkX3NoYXJkcyhzZWxmKToKKyAgICAgICAgIyBUZXN0cyBmb3IgdGhlIGRlZmF1bHQg b2YgdXNpbmcgb25lIGxvY2tlZCBzaGFyZCBldmVuIGluIHRoZSBjYXNlIG9mIG1vcmUgdGhhbiBv bmUgY2hpbGQgcHJvY2Vzcy4KICAgICAgICAgaWYgbm90IHNlbGYuc2hvdWxkX3Rlc3RfcHJvY2Vz c2VzOgogICAgICAgICAgICAgcmV0dXJuCisgICAgICAgIHNhdmVfZW52X3dlYmtpdF90ZXN0X21h eF9sb2NrZWRfc2hhcmRzID0gTm9uZQorICAgICAgICBpZiAiV0VCS0lUX1RFU1RfTUFYX0xPQ0tF RF9TSEFSRFMiIGluIG9zLmVudmlyb246CisgICAgICAgICAgICBzYXZlX2Vudl93ZWJraXRfdGVz dF9tYXhfbG9ja2VkX3NoYXJkcyA9IG9zLmVudmlyb25bIldFQktJVF9URVNUX01BWF9MT0NLRURf U0hBUkRTIl0KKyAgICAgICAgICAgIGRlbCBvcy5lbnZpcm9uWyJXRUJLSVRfVEVTVF9NQVhfTE9D S0VEX1NIQVJEUyJdCiAgICAgICAgIF8sIF8sIHJlZ3VsYXJfb3V0cHV0LCBfID0gbG9nZ2luZ19y dW4oWyctLWRlYnVnLXJ3dC1sb2dnaW5nJywgJy0tY2hpbGQtcHJvY2Vzc2VzJywgJzInXSwgc2hh cmVkX3BvcnQ9RmFsc2UpCi0gICAgICAgIHNlbGYuYXNzZXJ0VHJ1ZShhbnkoWycoMSBsb2NrZWQp JyBpbiBsaW5lIGZvciBsaW5lIGluIHJlZ3VsYXJfb3V0cHV0LmJ1Zmxpc3RdKSkKKyAgICAgICAg dHJ5OgorICAgICAgICAgICAgc2VsZi5hc3NlcnRUcnVlKGFueShbJygxIGxvY2tlZCknIGluIGxp bmUgZm9yIGxpbmUgaW4gcmVndWxhcl9vdXRwdXQuYnVmbGlzdF0pKQorICAgICAgICBmaW5hbGx5 OgorICAgICAgICAgICAgaWYgc2F2ZV9lbnZfd2Via2l0X3Rlc3RfbWF4X2xvY2tlZF9zaGFyZHM6 CisgICAgICAgICAgICAgICAgb3MuZW52aXJvblsiV0VCS0lUX1RFU1RfTUFYX0xPQ0tFRF9TSEFS RFMiXSA9IHNhdmVfZW52X3dlYmtpdF90ZXN0X21heF9sb2NrZWRfc2hhcmRzCiAKICAgICBkZWYg dGVzdF9jaGlsZF9wcm9jZXNzZXNfMihzZWxmKToKICAgICAgICAgaWYgc2VsZi5zaG91bGRfdGVz dF9wcm9jZXNzZXM6Cg==