151262 2015-11-13 08:56:02 -0800 WebKit should not need to modify testharness.js 2015-11-16 10:25:17 -0800 1 1 1 Unclassified WebKit Tools / Tests WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED P2 Normal --- 150332 1 youennf youennf ap commit-queue darin lforschler oldest_to_newest 1141836 0 youennf 2015-11-13 08:56:02 -0800 Currently WebKit has its own version of testharness.js. The modification is related to timeout values. It may just be better to disable testharness timeout in testharnessreport.js This will simplify further updates of testharness.js 1141837 1 265481 youennf 2015-11-13 08:58:13 -0800 Created attachment 265481 Patch 1141840 2 ap 2015-11-13 09:17:01 -0800 I'm not sure if we can expect that the default timeout will suffice for us. I believe that the tests are designed to work sequentially, but we run them in parallel, which can cause almost 10x slowdown. 1141846 3 youennf 2015-11-13 09:28:26 -0800 (In reply to comment #2) > I'm not sure if we can expect that the default timeout will suffice for us. > I believe that the tests are designed to work sequentially, but we run them > in parallel, which can cause almost 10x slowdown. AFAICT, there are two testharness timeouts, one for each subtest and one for each file. The values modified in testharness.js only apply to the global timeout. explicit_timeout=true disables the global timeout, so this should be fine to reset those value to their default. As of the subtest timeout, by default, none is set. This is done by the author, writing something like async_test("title", {timeout: 1000}). If we want to increase these values compared to what is done currently, we can use timeout_multiplier in testharnessreport.js. 1141849 4 ap 2015-11-13 09:33:09 -0800 > AFAICT, there are two testharness timeouts, one for each subtest and one for each file. Right, what I'm saying is that per-file timeouts are likely wrong for our environment. > If we want to increase these values compared to what is done currently, we can use timeout_multiplier in testharnessreport.js. That may be OK, especially if that means that per-file timeouts are also increased. 1141850 5 youennf 2015-11-13 09:41:41 -0800 > That may be OK, especially if that means that per-file timeouts are also > increased. timeout_multiplier only applies to per-test timeout. This is ok since this patch disables per-file timeout by setting "explicit_timeout" to true. Doing a quick grep, it seems that a little bit more than 100 tests do set per-test timeouts. 1141857 6 ap 2015-11-13 09:59:35 -0800 So do we need to keep doing what we are doing, but also set the multiplier in addition? 1141866 7 youennf 2015-11-13 10:15:23 -0800 (In reply to comment #6) > So do we need to keep doing what we are doing, but also set the multiplier > in addition? First step is this patch. It should not change the testing behavior, but is just a cleaner way to integrate testharness.js. Then, we may consider using multiplier, which actually changes the testing behavior. It may be the case that a subtest is timeouting but not the whole test file. Using multiplier may make the whole test file timeouting. 1142017 8 ap 2015-11-13 19:53:02 -0800 I'm not sure if I follow. I do not object to this patch, just don't understand what we are up to. 1142042 9 youennf 2015-11-14 01:20:54 -0800 (In reply to comment #8) > I'm not sure if I follow. I do not object to this patch, just don't > understand what we are up to. This patch does two things: 1. Remove WebKit custom changes to testharness.js. For future syncing of testharness.js, we will no longer need to port WebKit changes to the new testharness.js. This will also allow bug 150332 which ensures that web-platform-test tests use the expected testharness.js version. 2. Update testharnessreport.js to disable testharness.js per-file timeout that would otherwise kick in before WebKit timeout due to changes done in step 1. Basically, this patch does not change anything except that it uses testharness.js/testharnessreport.js in a proper manner. 1142061 10 ap 2015-11-14 12:05:35 -0800 Let's consider a test that currently takes 15 seconds, and doesn't have a per-file timeout. It currently passes on Mac, because run-webkit-tests timeout is 30 seconds. What will keep it passing after these changes? 1142072 11 youennf 2015-11-14 16:15:18 -0800 (In reply to comment #10) > Let's consider a test that currently takes 15 seconds, and doesn't have a > per-file timeout. It currently passes on Mac, because run-webkit-tests > timeout is 30 seconds. > > What will keep it passing after these changes? This test will see no change as this test is not testharness.js based. A test with testharness.js currently always has a per-file js-controlled timeout, which is set to a big value. After this change, all testharness.js tests will no longer have per-test js-controlled timeouts. 1142113 12 265481 commit-queue 2015-11-15 08:01:51 -0800 Comment on attachment 265481 Patch Clearing flags on attachment: 265481 Committed r192461: <http://trac.webkit.org/changeset/192461> 1142114 13 commit-queue 2015-11-15 08:01:54 -0800 All reviewed patches have been landed. Closing bug. 1142118 14 ap 2015-11-15 10:45:24 -0800 > A test with testharness.js currently always has a per-file js-controlled timeout, which is set to a big value. In comment 9, you said that opposite - that "per-file timeout that would otherwise kick in before WebKit timeout". I still don't understand this change. It may be correct, but the explanations are self-contradicting. 1142126 15 youennf 2015-11-15 11:33:27 -0800 (In reply to comment #14) > > A test with testharness.js currently always has a per-file js-controlled timeout, which is set to a big value. > > In comment 9, you said that opposite - that "per-file timeout that would > otherwise kick in before WebKit timeout". I am sorry if my explanations are not clear enough, but I do not see any contradiction between c9 and c14. Here is another try. Before this patch, the situation is as follow: - testharness.js sets a per-file JS timeout of 1000 seconds (c14) - WKR sets a WK timeout of 30 seconds in debug mode - testharnessreport.js does not change testharness.js JS timeout Conclusion: - testharness.js JS timeout never kicks in After this patch, the situation can be described as follow: - testharness.js sets a per-file JS timeout of 10 seconds - WKR sets a WK timeout of 30 seconds in debug mode, hence why the JS timeout would kick (c9) in except that... - testharnessreport.js disables testharness.js JS timeout Conclusion: - testharness.js JS timeout never kicks in Is that clearer? 1142132 16 ap 2015-11-15 13:17:16 -0800 Where does the phrase "per-file timeout" come from? I don't see it in code. What this patch does is change default timeout in testharness.js, not a "per-file timeout". 1142199 17 youennf 2015-11-16 02:13:35 -0800 (In reply to comment #16) > Where does the phrase "per-file timeout" come from? I don't see it in code. Ah, maybe that is where the confusion comes from. This term was only coined as part of this discussion (#c4). To summarize, each testharness.js test is guarded by 3 different timeout mechanisms: 1. WTR timeout (global "per-test-file" timeout) 2. testharness.js Tests.timeout_id (global "per-test-file" timeout, default length defined by harness_timeout) 3. testharness.js Test.timeout_id ("per-test" timeout, disabled by default, can be set for each test/async_test explicitly in the html test file). What this patch does is disabling the second timeout, Tests.timeout_id. > What this patch does is change default timeout in testharness.js, not a > "per-file timeout". It does so because this patch also ensures Tests.timeout_id is always cleared during testharness setup phase. 1142247 18 ap 2015-11-16 09:56:02 -0800 > 2. testharness.js Tests.timeout_id (global "per-test-file" timeout, default length defined by harness_timeout) > 3. testharness.js Test.timeout_id ("per-test" timeout, disabled by default, can be set for each test/async_test explicitly in the html test file). What is the difference between "per-test-file" and "per-test"? 1142251 19 youennf 2015-11-16 10:14:50 -0800 > What is the difference between "per-test-file" and "per-test"? per-test-file means the execution of a whole HTML file. per-test means the execution of JS code inside a test() or async_test() block. As can be seen in LayputTests/streams/reference-implementation, a single HTML test file may contain several tests. Testharness.js defines Tests and Test to handle that. 1142253 20 youennf 2015-11-16 10:25:17 -0800 https://darobin.github.io/test-harness-tutorial/docs/using-testharness.html contains useful information. 265481 2015-11-13 08:58:13 -0800 2015-11-15 08:01:51 -0800 Patch bug-151262-20151113175806.patch text/plain 2376 youennf U3VidmVyc2lvbiBSZXZpc2lvbjogMTkyNDEwCmRpZmYgLS1naXQgYS9MYXlvdXRUZXN0cy9DaGFu Z2VMb2cgYi9MYXlvdXRUZXN0cy9DaGFuZ2VMb2cKaW5kZXggM2UzZmMwZDQ1NWY3MmFmOWFlMGJi ZTIyNWZlYzlmZDlmYjYzYTg4NC4uMGVlNzE4YTRiODQwNGFlMzFjM2YxN2JkNGU4OWM1MTc4M2E1 ZjhmOSAxMDA2NDQKLS0tIGEvTGF5b3V0VGVzdHMvQ2hhbmdlTG9nCisrKyBiL0xheW91dFRlc3Rz L0NoYW5nZUxvZwpAQCAtMSwzICsxLDEzIEBACisyMDE1LTExLTEzICBZb3Vlbm4gRmFibGV0ICA8 eW91ZW5uLmZhYmxldEBjcmYuY2Fub24uZnI+CisKKyAgICAgICAgV2ViS2l0IHNob3VsZCBub3Qg bmVlZCB0byBtb2RpZnkgdGVzdGhhcm5lc3MuanMKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtp dC5vcmcvc2hvd19idWcuY2dpP2lkPTE1MTI2MgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9E WSAoT09QUyEpLgorCisgICAgICAgICogcmVzb3VyY2VzL3Rlc3RoYXJuZXNzLmpzOiBSZXNldHRp bmcgZ2xvYmFsIHRlc3RoYXJuZXNzLmpzIHRpbWVvdXQgdmFsdWVzIHRvIHRoZSBkZWZhdWx0IG9u ZXMuCisgICAgICAgICogcmVzb3VyY2VzL3Rlc3RoYXJuZXNzcmVwb3J0LmpzOiBEaXNhYmxpbmcg Z2xvYmFsIHRlc3RoYXJuZXNzLmpzIHRpbWVvdXQuCisKIDIwMTUtMTEtMTIgIEJyYWR5IEVpZHNv biAgPGJlaWRzb25AYXBwbGUuY29tPgogCiAgICAgICAgIE1vZGVybiBJREI6IFBpcGUgdGhyb3Vn aCBjdXJzb3IgZnVuY3Rpb25zIGZyb20gY2xpZW50IHRvIHNlcnZlci4KZGlmZiAtLWdpdCBhL0xh eW91dFRlc3RzL3Jlc291cmNlcy90ZXN0aGFybmVzcy5qcyBiL0xheW91dFRlc3RzL3Jlc291cmNl cy90ZXN0aGFybmVzcy5qcwppbmRleCAwYzQzYjhiMmU0ZmZmODdkMGY4ODgzNDRmZWEyN2ZlNjlk ZTMzN2ZiLi43NmM4MDZhYTIxYzMxMTYwMWY5NzNjY2JjY2E4MDg0NDc1ZGE0MzZlIDEwMDY0NAot LS0gYS9MYXlvdXRUZXN0cy9yZXNvdXJjZXMvdGVzdGhhcm5lc3MuanMKKysrIGIvTGF5b3V0VGVz dHMvcmVzb3VyY2VzL3Rlc3RoYXJuZXNzLmpzCkBAIC0xNSwxMyArMTUsMTMgQEAgcG9saWNpZXMg YW5kIGNvbnRyaWJ1dGlvbiBmb3JtcyBbM10uCiAoZnVuY3Rpb24gKCkKIHsKICAgICB2YXIgZGVi dWcgPSBmYWxzZTsKLSAgICAvLyBGb3IgV2ViS2l0LCB3ZSBzZXQgaGFybmVzcyB0aW1lb3V0IHRv IGEgdmVyeSBsYXJnZSB2YWx1ZSwgZXhwZWN0aW5nIHJ1bi13ZWJraXQtdGVzdHMKLSAgICAvLyB0 byBlbmZvcmNlIHRoZSByaWdodCBvbmUuCisKKyAgICAvLyBkZWZhdWx0IHRpbWVvdXQgaXMgMTAg c2Vjb25kcywgdGVzdCBjYW4gb3ZlcnJpZGUgaWYgbmVlZGVkCiAgICAgdmFyIHNldHRpbmdzID0g ewogICAgICAgICBvdXRwdXQ6ZmFsc2UsCiAgICAgICAgIGhhcm5lc3NfdGltZW91dDp7Ci0gICAg ICAgICAgICAibm9ybWFsIjoxMDAwMDAwLAotICAgICAgICAgICAgImxvbmciOjEwMDAwMDAKKyAg ICAgICAgICAgICJub3JtYWwiOjEwMDAwLAorICAgICAgICAgICAgImxvbmciOjYwMDAwCiAgICAg ICAgIH0sCiAgICAgICAgIHRlc3RfdGltZW91dDpudWxsLAogICAgICAgICBtZXNzYWdlX2V2ZW50 czogWyJzdGFydCIsICJ0ZXN0X3N0YXRlIiwgInJlc3VsdCIsICJjb21wbGV0aW9uIl0KZGlmZiAt LWdpdCBhL0xheW91dFRlc3RzL3Jlc291cmNlcy90ZXN0aGFybmVzc3JlcG9ydC5qcyBiL0xheW91 dFRlc3RzL3Jlc291cmNlcy90ZXN0aGFybmVzc3JlcG9ydC5qcwppbmRleCBiMjVkZGQ3OTQ0Y2M5 ZWMwNzFmZDM5NGMyZjZmMzJhODQ5ZjYwZGFiLi4wZWRmMWZiMDNhMmFiYjMzYTIzMWM1YmI2ZDEx ZWEzZTc3NjViNWQ4IDEwMDY0NAotLS0gYS9MYXlvdXRUZXN0cy9yZXNvdXJjZXMvdGVzdGhhcm5l c3NyZXBvcnQuanMKKysrIGIvTGF5b3V0VGVzdHMvcmVzb3VyY2VzL3Rlc3RoYXJuZXNzcmVwb3J0 LmpzCkBAIC0zNiw3ICszNiw3IEBAIGZ1bmN0aW9uIGNvbnZlcnRSZXN1bHQocmVzdWx0U3RhdHVz KXsKICogIHNwYWNpbmcgYmV0d2VlbiBjZWxscyBpcyBwcmVzZXJ2ZWQsIGFuZCBpdCBpcyB0aGVy ZWZvcmUgbm90IHJlYWRhYmxlLiBCeQ0KICogIHNldHRpbmcgb3V0cHV0IHRvIGZhbHNlLCB0aGUg SFRNTCB0YWJsZSB3aWxsIG5vdCBiZSBjcmVhdGVkDQogKi8NCi1zZXR1cCh7Im91dHB1dCI6ZmFs c2V9KTsNCitzZXR1cCh7Im91dHB1dCI6ZmFsc2UsICJleHBsaWNpdF90aW1lb3V0IjogdHJ1ZX0p Ow0KIA0KIC8qICBVc2luZyBhIGNhbGxiYWNrIGZ1bmN0aW9uLCB0ZXN0IHJlc3VsdHMgd2lsbCBi ZSBhZGRlZCB0byB0aGUgcGFnZSBpbiBhIA0KICogICBtYW5uZXIgdGhhdCBhbGxvd3MgZHVtcEFz VGV4dCB0byBwcm9kdWNlIHJlYWRhYmxlIHRlc3QgcmVzdWx0cw0K