37738 2010-04-16 17:30:37 -0700 new-run-webkit-tests low timeout is too sensitive to runaway processes or test crashes 2011-04-06 18:47:40 -0700 1 1 1 Unclassified WebKit Tools / Tests 528+ (Nightly build) PC OS X 10.5 RESOLVED FIXED P2 Normal --- 34984 1 mjs dpranke abarth dpranke eric lforschler ojan tony oldest_to_newest 213525 0 mjs 2010-04-16 17:30:37 -0700 new-run-webkit-tests has a much lower default timeout than run-webkit-tests. This makes it extremely sensitive to runaway processes, since anything going on in the background will make a bunch of tests time out. One extra bad factor is that crash logging for a crashed test (ReportCrash) can cause a system to get slow enough to time out a bunch of tests in a row, or even time out a DumpRenderTree process (whereupon it is restarted from scratch). Thus, the low timeout can actually make overall test time longer instead of shorter. I suggest matching the old timeout initially, and changing it over time once we solve these problems. 213532 1 eric 2010-04-16 17:45:21 -0700 We need bot data to confirm or deny how much of a problem this is in practice. Low timeouts are a good thing as they allow us to run tests which expect to TIMEOUT every time w/ minimal delay on total test time. It's certainly possible to detect load and increase timeouts for tests which are not expected to timeout when run under load. 213590 2 abarth 2010-04-17 01:18:03 -0700 Why not run tests with a TIMEOUT expectation with a lower timeout? 213606 3 ojan 2010-04-17 08:45:35 -0700 (In reply to comment #2) > Why not run tests with a TIMEOUT expectation with a lower timeout? I prefer that to the current model actually. I don't much like that we need to mark things as SLOW. It's confusing and complicated (mea culpa). The downside, which is relatively minor, is that the test may start passing with the regular timeout and we wouldn't know. The downside seems preferable to the confusion of the current model, namely, people have a lot of trouble understanding when they should mark something TIMEOUT vs SLOW. Also, is there a way we can kill ReportCrash from NRWT? When I run the tests locally, I frequently will "sudo killall ReportCrash" in a loop. I'd much rather have NRWT do that for me. The sudo is the problem of course. 213931 4 mjs 2010-04-19 00:20:50 -0700 (In reply to comment #1) > We need bot data to confirm or deny how much of a problem this is in practice. > Low timeouts are a good thing as they allow us to run tests which expect to > TIMEOUT every time w/ minimal delay on total test time. It's certainly > possible to detect load and increase timeouts for tests which are not expected > to timeout when run under load. It's definitely a problem on my computer. I've gotten the failure cascade multiple times. I've also heard of other people experiencing it. It seems to me that this needs to be fixed whether or not we observe it on a buildbot. 213962 5 mjs 2010-04-19 01:55:42 -0700 (In reply to comment #3) > (In reply to comment #2) > > Why not run tests with a TIMEOUT expectation with a lower timeout? > > I prefer that to the current model actually. I don't much like that we need to > mark things as SLOW. It's confusing and complicated (mea culpa). The downside, > which is relatively minor, is that the test may start passing with the regular > timeout and we wouldn't know. The downside seems preferable to the confusion of > the current model, namely, people have a lot of trouble understanding when they > should mark something TIMEOUT vs SLOW. Sounds like a neat idea. It would somewhat reduce the incentive to make your tests fast though. Maybe there could be a "soft" timeout that would result in a non-failure warning, to make slow tests more visible? > Also, is there a way we can kill ReportCrash from NRWT? When I run the tests > locally, I frequently will "sudo killall ReportCrash" in a loop. I'd much > rather have NRWT do that for me. The sudo is the problem of course. I don't think that would be a good thing to do. The crash logs are very useful for diagnosing what went wrong, especially on the buildbots. 213975 6 eric 2010-04-19 02:50:33 -0700 (In reply to comment #5) > Sounds like a neat idea. It would somewhat reduce the incentive to make your > tests fast though. Maybe there could be a "soft" timeout that would result in a > non-failure warning, to make slow tests more visible? All timeouts are currently "soft" in that all failures are re-run at the end. Tests which timed out once due to machine load are unlikely to time out twice. > > Also, is there a way we can kill ReportCrash from NRWT? When I run the tests > > locally, I frequently will "sudo killall ReportCrash" in a loop. I'd much > > rather have NRWT do that for me. The sudo is the problem of course. > > I don't think that would be a good thing to do. The crash logs are very useful > for diagnosing what went wrong, especially on the buildbots. I've long wanted such an option to run-webkit-tests. It's easy enough to do. Just requires adding a flag to DumpRenderTree to have it catch the necessary signals and exit(1) instead of letting the OS catch them for us and calling ReportCrash. I don't think we'd want this mode on by default, but it would be useful. Even if there was just an easy way to renice ReportCrash a few times that would make its behavior way easier to deal with. 214020 7 ojan 2010-04-19 07:35:56 -0700 (In reply to comment #6) > (In reply to comment #5) > > Sounds like a neat idea. It would somewhat reduce the incentive to make your > > tests fast though. Maybe there could be a "soft" timeout that would result in a > > non-failure warning, to make slow tests more visible? This seems like a great idea. If we could take it one step further and show the number of slow tests on the waterfall, that might provide incentive to fix them. What I like about this is that we can be more flexible about the timeout. The current 6 second timeout (12 seconds on debug) is just what happened to seem reasonable given the data from the Chromium bots. We can start with something conservative (6 seconds?) and make it more aggressive as we fix the slower tests. > All timeouts are currently "soft" in that all failures are re-run at the end. > Tests which timed out once due to machine load are unlikely to time out twice. This is half true. If ReportCrash is the problem and your patch causes crashes, then crashes will happen when you retry and tests might timeout if you don't have sufficient extra cores on your machine. > > > Also, is there a way we can kill ReportCrash from NRWT? When I run the tests > > > locally, I frequently will "sudo killall ReportCrash" in a loop. I'd much > > > rather have NRWT do that for me. The sudo is the problem of course. > > > > I don't think that would be a good thing to do. The crash logs are very useful > > for diagnosing what went wrong, especially on the buildbots. > > I've long wanted such an option to run-webkit-tests. It's easy enough to do. > Just requires adding a flag to DumpRenderTree to have it catch the necessary > signals and exit(1) instead of letting the OS catch them for us and calling > ReportCrash. I don't think we'd want this mode on by default, but it would be > useful. Filed bug 37797. 378177 8 dpranke 2011-04-01 16:18:34 -0700 Okay, running against current time of tree (one year later), we have a grand total of *two* tests that take longer than 6 seconds on Mac Snow Leopard. I've marked them as SLOW in the test_expectations file. I suggest that the defaults of 6 seconds for regular tests and 30 seconds for slow tests is good enough. We can always adjust this if it turns out to be a problem in practice. For reference old-run-webkit-tests appears to default to 35 seconds and of course doesn't have a SLOW concept. Maciej, what do you think? 379149 9 dpranke 2011-04-04 15:47:24 -0700 Okay, it looks like the mac port of NRWT seems much less happy with the default of six seconds. I'm going to reopen this and change the default to be 35 seconds to match ORWT. We can gradually pull the time down as necessary in the future. 379196 10 88154 dpranke 2011-04-04 16:35:00 -0700 Created attachment 88154 Patch 379269 11 88154 tony 2011-04-04 18:14:56 -0700 Comment on attachment 88154 Patch This doesn't impact chromium-mac, right? 379278 12 dpranke 2011-04-04 18:36:02 -0700 (In reply to comment #11) > (From update of attachment 88154 [details]) > This doesn't impact chromium-mac, right? Correct. 381041 13 dpranke 2011-04-06 18:47:40 -0700 Committed r83130: <http://trac.webkit.org/changeset/83130> 88154 2011-04-04 16:35:00 -0700 2011-04-04 18:14:56 -0700 Patch bug-37738-20110404163459.patch text/plain 1404 dpranke U3VidmVyc2lvbiBSZXZpc2lvbjogODI4OTIKZGlmZiAtLWdpdCBhL1Rvb2xzL0NoYW5nZUxvZyBi L1Rvb2xzL0NoYW5nZUxvZwppbmRleCA3ZmQ5MTI3MjEyMjg1MDI5Y2I1OGRmZGEyYjM4ZjRmMjE4 MzI1YjFiLi45MmY2MmY4MmY1Y2Y3YWZkODRhMjBjY2ZlYWY5M2Y5ODgyYjUyY2YwIDEwMDY0NAot LS0gYS9Ub29scy9DaGFuZ2VMb2cKKysrIGIvVG9vbHMvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTQg QEAKKzIwMTEtMDQtMDQgIERpcmsgUHJhbmtlICA8ZHByYW5rZUBjaHJvbWl1bS5vcmc+CisKKyAg ICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgQWRqdXN0IHRoZSBh cHBsZSB3ZWJraXQgcG9ydCdzIGRlZmF1bHQgdGltZW91dCB0byBtYXRjaAorICAgICAgICBvbGQt cnVuLXdlYmtpdC10ZXN0cyBhdCAzNSBzZWNvbmRzLgorCisgICAgICAgIGh0dHBzOi8vYnVncy53 ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0zNzczOAorCisgICAgICAgICogU2NyaXB0cy93ZWJr aXRweS9sYXlvdXRfdGVzdHMvcG9ydC9tYWMucHk6CisKIDIwMTEtMDQtMDQgIFRvbnkgQ2hhbmcg IDx0b255QGNocm9taXVtLm9yZz4KIAogICAgICAgICBSZXZpZXdlZCBieSBPamFuIFZhZmFpLgpk aWZmIC0tZ2l0IGEvVG9vbHMvU2NyaXB0cy93ZWJraXRweS9sYXlvdXRfdGVzdHMvcG9ydC9tYWMu cHkgYi9Ub29scy9TY3JpcHRzL3dlYmtpdHB5L2xheW91dF90ZXN0cy9wb3J0L21hYy5weQppbmRl eCBiNTk1ODczNTIxNTQ0N2VlODlhMTdhNmYyNzI1MjZmY2U5ZTVkYWNjLi40MmYxMDE4Nzc0MjQ5 MGEyZmFjMjVkMzlhMzVmMjQ1MmM2MTdiZTc4IDEwMDY0NAotLS0gYS9Ub29scy9TY3JpcHRzL3dl YmtpdHB5L2xheW91dF90ZXN0cy9wb3J0L21hYy5weQorKysgYi9Ub29scy9TY3JpcHRzL3dlYmtp dHB5L2xheW91dF90ZXN0cy9wb3J0L21hYy5weQpAQCAtODEsOCArODEsOSBAQCBjbGFzcyBNYWNQ b3J0KFdlYktpdFBvcnQpOgogICAgICAgICBlbHNlOgogICAgICAgICAgICAgc2VsZi5fdmVyc2lv biA9IHBvcnRfbmFtZVs0Ol0KICAgICAgICAgICAgIGFzc2VydCBzZWxmLl92ZXJzaW9uIGluIHNl bGYuU1VQUE9SVEVEX1ZFUlNJT05TCi0KICAgICAgICAgc2VsZi5fb3BlcmF0aW5nX3N5c3RlbSA9 ICdtYWMnCisgICAgICAgIGlmIG5vdCBoYXNhdHRyKHNlbGYuX29wdGlvbnMsICd0aW1lLW91dC1t cycpIG9yIHNlbGYuX29wdGlvbnMudGltZV9vdXRfbXMgaXMgTm9uZToKKyAgICAgICAgICAgIHNl bGYuX29wdGlvbnMudGltZV9vdXRfbXMgPSAzNTAwMAogCiAgICAgZGVmIGRlZmF1bHRfY2hpbGRf cHJvY2Vzc2VzKHNlbGYpOgogICAgICAgICAjIEZJWE1FOiBuZXctcnVuLXdlYmtpdC10ZXN0cyBp cyB1bnN0YWJsZSBvbiBNYWMgcnVubmluZyBtb3JlIHRoYW4K