142425 2015-03-06 20:05:52 -0800 Crash count should be listed separately from failure count when layout tests fail 2016-06-21 11:41:05 -0700 1 1 1 Unclassified WebKit Tools / Tests 528+ (Nightly build) Unspecified Unspecified RESOLVED WONTFIX InRadar P2 Normal --- 1 ddkilzer ddkilzer ap bburg cdumez darin dbates dburkart jake.nielsen.webkit lforschler matthew_hanson mrowe ossy oldest_to_newest 1075213 0 ddkilzer 2015-03-06 20:05:52 -0800 When a layout tests fail, it's useful to print the crash count separately (as a subset of the failure count). This is because crashes are more severe than most other failures, and when running tests with ASan or GuardMalloc, the primary intent is to find tests that crash since those are the most interesting results for those bots. 1075215 1 ddkilzer 2015-03-06 20:07:36 -0800 <rdar://problem/20078699> 1075221 2 248137 ddkilzer 2015-03-06 20:23:30 -0800 Created attachment 248137 Patch v1 1075256 3 ossy 2015-03-07 01:50:26 -0800 (In reply to comment #0) > When a layout tests fail, it's useful to print the crash count separately > (as a subset of the failure count). > > This is because crashes are more severe than most other failures, and when > running tests with ASan or GuardMalloc, the primary intent is to find tests > that crash since those are the most interesting results for those bots. I like the idea to show crashes separately, but in my opinion counting them twice (as crashed and failures too) is very confusing if we don't change the output format. I could imagine a more detailed output, but it should be evident what means what. And maybe the bot watchers dashboard should be fixed to be able parse the new output. for example: (where x = y + u + v) x unexpected regressions: y crashes, u timeouts, v failures j new passes, k flakes, l missing results For example let's see the latest result from the GTK bot, where we can found all kind of failures: actual output: 22 failures 32 new passes 10 flakes 8 missing results proposed output, which is confusing for me: 22 failures 4 crashes 32 new passes 10 flakes 8 missing results Or what about: 22 failures (4 of them crashes) ... ? detailed results (actual): -------------------------- Expected to crash, but passed: (1) Expected to timeout, but passed: (2) Expected to fail, but passed: (29) ==> 32 new passes Unexpected flakiness: text-only failures (4) Unexpected flakiness: image-only failures (1) Unexpected flakiness: timeouts (5) ==> 10 flakes Regressions: Unexpected text-only failures (4) Regressions: Unexpected image-only failures (6) Regressions: Unexpected crashes (4) Regressions: Unexpected timeouts (8) ==> 22 failures Regressions: Unexpected missing results (8) ==> 8 missing results 1075259 4 248137 ap 2015-03-07 08:03:24 -0800 Comment on attachment 248137 Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=248137&action=review > Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:377 > + # Add crash count to failure count. I don't understand this part. Isn't it already included? 1075260 5 ap 2015-03-07 08:07:00 -0800 Ossy, I think that the intention here is to display crashes for crashesOnly queues (already supported by the dashboard), such as a potential GuardMalloc queue. I would object to displaying crash count separately for normal queues, that's too much information. 1075443 6 ddkilzer 2015-03-08 15:27:15 -0700 (In reply to comment #4) > Comment on attachment 248137 [details] > Patch v1 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=248137&action=review > > > Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:377 > > + # Add crash count to failure count. > > I don't understand this part. Isn't it already included? Prior to this change, crashes were counted as failures but not listed separately. This piece of code keeps that invariant the same so that crash count is listed separately, but crashes are still also counted as failures. FWIW, this matches how we report crashes on our internal buildbot instances (although the code accomplishes this slightly differently). I don't have strong opinions on whether crash count should be a subset of failure count or not (I originally implemented them as separate counts). (In reply to comment #5) > Ossy, I think that the intention here is to display crashes for crashesOnly > queues (already supported by the dashboard), such as a potential GuardMalloc > queue. Yes, that's the intention. > I would object to displaying crash count separately for normal queues, > that's too much information. I don't see the potential harm here, though. Can you explain what exactly your concern is other than "too much information"? Is it that the Info column might wrap to a second line? How is having a crash count a bad thing in this situation? Personally, I think crashes are an interesting enough subset of failures that I'd like to have a separate count in the summary text. It's especially useful for GuardMalloc or ASan queues, but I feel crashes in other queues are interesting as well. 1075717 7 ap 2015-03-09 12:30:00 -0700 > > > Tools/BuildSlaveSupport/build.webkit.org-config/master.cfg:377 > > > + # Add crash count to failure count. > > > > I don't understand this part. Isn't it already included? > > Prior to this change, crashes were counted as failures but not listed separately. What I'm saying is that failures come from all "Regressions: Unexpected" lines, and crashes come from "Regressions: Unexpected crash" lines. Which seems to mean that failures already include crashes, but then you add crashes to failures. Aren't they double-counted as a result? Am I misreading the code? > Can you explain what exactly your concern is other than "too much information"? Too much information is exactly what I'm concerned about. We do have a bug about improving summary strings for failures though - bug 122901 - please feel free to add the idea of separating crashes if you think that it would help those looking at the page. 1075718 8 ap 2015-03-09 12:31:36 -0700 One thing we could do without adding noise is to say "N crashes" when all the failures are crashes, and to say "N failures" when there is a mix of crashes, text failures, audio failures and/or image failures. 1144234 9 bburg 2015-11-23 15:22:39 -0800 (In reply to comment #8) > One thing we could do without adding noise is to say "N crashes" when all > the failures are crashes, and to say "N failures" when there is a mix of > crashes, text failures, audio failures and/or image failures. So, I think whether crashes should be included in the big red number or not is a dashboard UI issue. It would be nice to simply have the crash count included in the raw data (I would prefer it be subtracted from failures, unless we double count other types of failures already). 1204223 10 248137 ddkilzer 2016-06-21 11:39:52 -0700 Comment on attachment 248137 Patch v1 Clearing review flags. 1204224 11 ddkilzer 2016-06-21 11:41:05 -0700 Apparently I was trying to solve a problem that no one was having, so moving to RESOLVED/WONTFIX. 248137 2015-03-06 20:23:30 -0800 2016-06-21 11:39:52 -0700 Patch v1 bug-142425-20150306202309.patch text/plain 3484 ddkilzer U3VidmVyc2lvbiBSZXZpc2lvbjogMTgxMTcyCmRpZmYgLS1naXQgYS9Ub29scy9DaGFuZ2VMb2cg Yi9Ub29scy9DaGFuZ2VMb2cKaW5kZXggZGRkMzQ3YTY4Nzc1YzU1OGU3MzZmN2U2YjJiNjM0YTll MDg1MjA1Zi4uYjI1ZTIwODhjNzg5ZDUyM2Q3YzM4MGI1NzRlZDgxOWZiMzZiMDZhNSAxMDA2NDQK LS0tIGEvVG9vbHMvQ2hhbmdlTG9nCisrKyBiL1Rvb2xzL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE2 IEBACisyMDE1LTAzLTA2ICBEYXZpZCBLaWx6ZXIgIDxkZGtpbHplckBhcHBsZS5jb20+CisKKyAg ICAgICAgQ3Jhc2ggY291bnQgc2hvdWxkIGJlIGxpc3RlZCBzZXBhcmF0ZWx5IGZyb20gZmFpbHVy ZSBjb3VudCB3aGVuIGxheW91dCB0ZXN0cyBmYWlsCisgICAgICAgIDxodHRwOi8vd2Via2l0Lm9y Zy9iLzE0MjQyNT4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAg ICAgICAqIEJ1aWxkU2xhdmVTdXBwb3J0L2J1aWxkLndlYmtpdC5vcmctY29uZmlnL21hc3Rlci5j Zmc6CisgICAgICAgIChSdW5XZWJLaXRUZXN0cy5fcGFyc2VSdW5XZWJLaXRUZXN0c091dHB1dCk6 IENvdW50IGNyYXNoZXMKKyAgICAgICAgc2VwYXJhdGVseSwgYnV0IGluY2x1ZGUgdGhlbSBpbiBm YWlsdXJlIGNvdW50LgorICAgICAgICAqIEJ1aWxkU2xhdmVTdXBwb3J0L2J1aWxkLndlYmtpdC5v cmctY29uZmlnL21hc3RlcmNmZ191bml0dGVzdC5weToKKyAgICAgICAgVXBkYXRlIHVuaXQgdGVz dCB0byB0ZXN0IGZvciBjcmFzaCBjb3VudC4KKwogMjAxNS0wMy0wNiAgQ2FybG9zIEdhcmNpYSBD YW1wb3MgIDxjZ2FyY2lhQGlnYWxpYS5jb20+CiAKICAgICAgICAgW0dUS10gVGVzdCAvd2Via2l0 Mi9XZWJLaXRXZWJWaWV3L3N5bmMtcmVxdWVzdC1vbi1tYXgtY29ubnMgbWlnaHQgZmFpbCBhZnRl ciBmaW5pc2hlZApkaWZmIC0tZ2l0IGEvVG9vbHMvQnVpbGRTbGF2ZVN1cHBvcnQvYnVpbGQud2Vi a2l0Lm9yZy1jb25maWcvbWFzdGVyLmNmZyBiL1Rvb2xzL0J1aWxkU2xhdmVTdXBwb3J0L2J1aWxk LndlYmtpdC5vcmctY29uZmlnL21hc3Rlci5jZmcKaW5kZXggNWZiMGYxZmVhNTU4NzUzMTFmODBk NjExZGNlNTIzN2E4ZWNkZDdlMy4uNWI1MmY1MmQ4Y2UzYWQwOTliOTkyODAzODkyYjJhNTAwNzE5 OTE3YyAxMDA2NDQKLS0tIGEvVG9vbHMvQnVpbGRTbGF2ZVN1cHBvcnQvYnVpbGQud2Via2l0Lm9y Zy1jb25maWcvbWFzdGVyLmNmZworKysgYi9Ub29scy9CdWlsZFNsYXZlU3VwcG9ydC9idWlsZC53 ZWJraXQub3JnLWNvbmZpZy9tYXN0ZXIuY2ZnCkBAIC0zNTYsNiArMzU2LDcgQEAgY2xhc3MgUnVu V2ViS2l0VGVzdHMoc2hlbGwuVGVzdCk6CiAgICAgICAgICAgICAoJ2ZsYWtlcycsIHJlLmNvbXBp bGUocidVbmV4cGVjdGVkIGZsYWtpbmVzcy4rXCgoXGQrKVwpJykpLAogICAgICAgICAgICAgKCdu ZXcgcGFzc2VzJywgcmUuY29tcGlsZShyJ0V4cGVjdGVkIHRvIC4rLCBidXQgcGFzc2VkOlxzK1wo KFxkKylcKScpKSwKICAgICAgICAgICAgICgnbWlzc2luZyByZXN1bHRzJywgcmUuY29tcGlsZShy J1JlZ3Jlc3Npb25zOiBVbmV4cGVjdGVkIG1pc3NpbmcgcmVzdWx0c1xzK1woKFxkKylcKScpKSwK KyAgICAgICAgICAgICgnY3Jhc2hlcycsIHJlLmNvbXBpbGUocidSZWdyZXNzaW9uczogVW5leHBl Y3RlZCBjcmFzaC4rXCgoXGQrKVwpJykpLAogICAgICAgICAgICAgKCdmYWlsdXJlcycsIHJlLmNv bXBpbGUocidSZWdyZXNzaW9uczogVW5leHBlY3RlZC4rXCgoXGQrKVwpJykpLAogICAgICAgICBd CiAgICAgICAgIHRlc3RGYWlsdXJlcyA9IHt9CkBAIC0zNzMsNiArMzc0LDEwIEBAIGNsYXNzIFJ1 bldlYktpdFRlc3RzKHNoZWxsLlRlc3QpOgogCiAgICAgICAgICAgICAgICAgIyBGSVhNRTogUGFy c2UgZmlsZSBuYW1lcyBhbmQgcHV0IHRoZW0gaW4gcmVzdWx0cwogCisgICAgICAgICMgQWRkIGNy YXNoIGNvdW50IHRvIGZhaWx1cmUgY291bnQuCisgICAgICAgIGlmICdjcmFzaGVzJyBpbiB0ZXN0 RmFpbHVyZXM6CisgICAgICAgICAgICB0ZXN0RmFpbHVyZXNbJ2ZhaWx1cmVzJ10gPSB0ZXN0RmFp bHVyZXMuZ2V0KCdmYWlsdXJlcycsIDApICsgdGVzdEZhaWx1cmVzWydjcmFzaGVzJ10KKwogICAg ICAgICBmb3IgbmFtZSBpbiB0ZXN0RmFpbHVyZXM6CiAgICAgICAgICAgICBpbmNvcnJlY3RMYXlv dXRMaW5lcy5hcHBlbmQoc3RyKHRlc3RGYWlsdXJlc1tuYW1lXSkgKyAnICcgKyBuYW1lKQogCmRp ZmYgLS1naXQgYS9Ub29scy9CdWlsZFNsYXZlU3VwcG9ydC9idWlsZC53ZWJraXQub3JnLWNvbmZp Zy9tYXN0ZXJjZmdfdW5pdHRlc3QucHkgYi9Ub29scy9CdWlsZFNsYXZlU3VwcG9ydC9idWlsZC53 ZWJraXQub3JnLWNvbmZpZy9tYXN0ZXJjZmdfdW5pdHRlc3QucHkKaW5kZXggY2QwMzYyYTYzN2Nj NTE1MzJlODQ4YzAzOTc2MTQ4ZDRjYTNlMDlkMC4uMWM1YmM4Yjk0OTdhMjE1NjNhYWU2NTc0ODg1 OWJiYmJkZTgxMGJiYyAxMDA3NTUKLS0tIGEvVG9vbHMvQnVpbGRTbGF2ZVN1cHBvcnQvYnVpbGQu d2Via2l0Lm9yZy1jb25maWcvbWFzdGVyY2ZnX3VuaXR0ZXN0LnB5CisrKyBiL1Rvb2xzL0J1aWxk U2xhdmVTdXBwb3J0L2J1aWxkLndlYmtpdC5vcmctY29uZmlnL21hc3RlcmNmZ191bml0dGVzdC5w eQpAQCAtNzUsNiArNzUsMTAgQEAgVW5leHBlY3RlZCBmbGFraW5lc3M6IHRleHQtb25seSBmYWls dXJlcyAoMikKIFVuZXhwZWN0ZWQgZmxha2luZXNzOiB0aW1lb3V0cyAoMSkKICAgc3ZnL3RleHQv Zm9yZWlnbk9iamVjdC1yZXBhaW50LnhtbCBbIFRpbWVvdXQgUGFzcyBdCiAKK1JlZ3Jlc3Npb25z OiBVbmV4cGVjdGVkIGNyYXNoZXMgKDIpCisgIGh0dHAvdGVzdHMvbmF2aWdhdGlvbi9hbmNob3It YmFzaWMuaHRtbCBbIENyYXNoIF0KKyAgaHR0cC90ZXN0cy9uYXZpZ2F0aW9uL3RpbWVycmVkaXJl Y3QtYmFzaWMuaHRtbCBbIENyYXNoIF0KKwogUmVncmVzc2lvbnM6IFVuZXhwZWN0ZWQgbWlzc2lu ZyByZXN1bHRzICgxKQogICBzdmcvY3VzdG9tL3plcm8tcGF0aC1zcXVhcmUtY2FwLXJlbmRlcmlu ZzIuc3ZnIFsgTWlzc2luZyBdCiAKQEAgLTgzLDcgKzg3LDcgQEAgUmVncmVzc2lvbnM6IFVuZXhw ZWN0ZWQgdGV4dC1vbmx5IGZhaWx1cmVzICgxKQogIiIiCiAgICAgICAgIHJ1bl93ZWJraXRfdGVz dHMuX3BhcnNlUnVuV2ViS2l0VGVzdHNPdXRwdXQobG9nX3RleHQpCiAgICAgICAgIHNlbGYuYXNz ZXJ0RXF1YWwoc2V0KHJ1bl93ZWJraXRfdGVzdHMuaW5jb3JyZWN0TGF5b3V0TGluZXMpLAotICAg ICAgICAgICAgc2V0KFsnMiBuZXcgcGFzc2VzJywgJzMgZmxha2VzJywgJzEgbWlzc2luZyByZXN1 bHRzJywgJzEgZmFpbHVyZXMnXSkpCisgICAgICAgICAgICBzZXQoWycyIG5ldyBwYXNzZXMnLCAn MyBmbGFrZXMnLCAnMSBtaXNzaW5nIHJlc3VsdHMnLCAnMiBjcmFzaGVzJywgJzMgZmFpbHVyZXMn XSkpCiAKIGNsYXNzIFN0dWJTdGRpbyhvYmplY3QpOgogICAgIGRlZiBfX2luaXRfXyhzZWxmLCBz dGRpbyk6Cg==