105860 2012-12-29 10:51:32 -0800 Tests with WontFix expectation are (indirectly) skipped 2013-01-15 14:04:34 -0800 1 1 1 Unclassified WebKit Tools / Tests 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 zan zan abarth dpranke eric inferno ojan webkit.review.bot oldest_to_newest 798151 0 zan 2012-12-29 10:51:32 -0800 Any test with a WontFix expectation is not run (i.e. skipped). I find this rather inconvenient as I'd like to test as many tests as possible, even if a specific test is expected to fail. My main reason behind this is to have as much test coverage as possible just so any crash occurrences do not go unnoticed. I'd like to propose at least a compromise of noting the expected failure along with the WontFix modifier, for example: fast/harness/sample-fail-mismatch-reftest.html [ WontFix ImageOnlyFailure ] 798153 1 eric 2012-12-29 10:56:45 -0800 I believe the extra crash-coverage we get from WONTFIX tests is so miniscule compared to the (also small) cost of maintaining any expectations for them, that it's not worth running them. NRWT has various options to allow running skipped tests. If you're interested in crash-coverage from skipped tests, I would suggest setting up a separate bot with one of those options. 798154 2 eric 2012-12-29 11:02:12 -0800 The Chromium port gets a large amount of crash-coverage from the huge fuzzing system which Inferno runs. Each of those crashers is turned into a layout test and committed to svn.webkit.org for other ports to run. I suspect that this "ClusterFuzz" provides infinitely more value than any ancient WONTFIX/Skipped, etc. layout test could. IMO we should probably start culling old/broken layout tests which no one runs. (We could also continue to ignore them in the noise, since I'm sure they're a tiny fraction of our 30k+ layout tests.) 798173 3 dpranke 2012-12-29 14:44:26 -0800 Note that NRWT actually used to do what you wanted (i.e., WontFix did not imply Skip). There was a fair amount of debate over the years as to whether or not running the WontFix-but-not-Skip tests was at all useful for catching the sort of crashes you mention. The closest anyone came was the belief that it did catch a few things when the port was very new (i.e., we were still bringing it up and crashes were not uncommon). Eventually we mostly agreed w/ Eric; the cost of running tests we didn't care about seemed excessive, especially given that we had so much other coverage, and so we changed what WontFix meant (IIRC, Apple also wanted WontFix to imply Skip, although they probably would've been happy w/ just Skip). But, not every port is Chromium - I could see an argument that other, especially newer, ports might want or need better coverage from the suite. I don't think we're likely to change back to the old behavior generally, but I could possibly be convinced to make this a port-specific configurable parameter whether things are skipped or not. Even then, this might be a bad idea since different ports would work differently. 798385 4 zan 2012-12-31 13:30:26 -0800 (In reply to comment #1) > I believe the extra crash-coverage we get from WONTFIX tests is so miniscule compared to the (also small) cost of maintaining any expectations for them, that it's not worth running them. While I pointed out only the crash coverage in comment #0, there are other benefits. The fast/harness/sample-fail-mismatch-reftest.html[1][2] layout test tests that an image-only failure occurs when the reftest and its expected mismatch baseline provide equal output. This test is marked as WontFix on both Chromium and GTK (and probably other ports as well) but isn't run even though it's integral to checking the correct harness behavior on which plenty of tests rely. I remember one occurrence of problems in harness that were indicated by this test unexpectedly passing (but it wasn't reported due to an otherwise unrelated bug), so not running this test can backfire. Also, the Chromium port currently marks over 6700 tests as WontFix. That's ~20% of all the tests the port collects. IMO that's way too much to ignore and not run. > NRWT has various options to allow running skipped tests. If you're interested in crash-coverage from skipped tests, I would suggest setting up a separate bot with one of those options. No such resources are available, unfortunately. I also think additional systems could be used for better purpose, such as perfbots. (In reply to comment #2) > The Chromium port gets a large amount of crash-coverage from the huge fuzzing system which Inferno runs. Each of those crashers is turned into a layout test and committed to svn.webkit.org for other ports to run. I suspect that this "ClusterFuzz" provides infinitely more value than any ancient WONTFIX/Skipped, etc. layout test could. IMO we should probably start culling old/broken layout tests which no one runs. (We could also continue to ignore them in the noise, since I'm sure they're a tiny fraction of our 30k+ layout tests.) Reading about ClusterFuzz on the Chromium blog[3], I understand (could be reading it incorrectly, though) that only exploitable crashes are analyzed and upstreamed. I appreciate the effort put into this system and process, but this in no way tests neither the WebKit1 layer of the GTK (or any other non-Chromium) port nor the WebKit2 layer. [1] http://trac.webkit.org/browser/trunk/LayoutTests/fast/harness/sample-fail-mismatch-reftest.html [2] http://trac.webkit.org/browser/trunk/LayoutTests/fast/harness/sample-fail-mismatch-reftest-expected-mismatch.html [3] http://blog.chromium.org/2012/04/fuzzing-for-security.html 798386 5 zan 2012-12-31 13:45:20 -0800 (In reply to comment #3) > Note that NRWT actually used to do what you wanted (i.e., WontFix did not imply Skip). There was a fair amount of debate over the years as to whether or not running the WontFix-but-not-Skip tests was at all useful for catching the sort of crashes you mention. The closest anyone came was the belief that it did catch a few things when the port was very new (i.e., we were still bringing it up and crashes were not uncommon). > > Eventually we mostly agreed w/ Eric; the cost of running tests we didn't care about seemed excessive, especially given that we had so much other coverage, and so we changed what WontFix meant (IIRC, Apple also wanted WontFix to imply Skip, although they probably would've been happy w/ just Skip). As said in comment #4, fast/harness/sample-fail-mismatch-reftest.html is an example of a layout test that should definitely be run but is not really fixable either. OTOH, it is probably the only test of such nature, but I urge every port to run it. And again, the Chromium port marks ~20% of collected layout tests as WontFix. That's a good chunk of coverage out of the window right at the base of the complete Chromium project structure. > But, not every port is Chromium - I could see an argument that other, especially newer, ports might want or need better coverage from the suite. I don't think we're likely to change back to the old behavior generally, but I could possibly be convinced to make this a port-specific configurable parameter whether things are skipped or not. Even then, this might be a bad idea since different ports would work differently. I'd very like to see at least a compromise of this in the form of an opt-in configuration to run tests marked as WontFix. I'd still prefer a possibility of an expanded modifier set, where the WontFix modifier would only indicate the non-fixable nature of the test while the extra modifier would indicate the actual expected failure. Using only the WontFix modifier could still just skip the test, as is done now. 798439 6 dpranke 2013-01-01 09:19:32 -0800 (In reply to comment #4) > (In reply to comment #1) > > I believe the extra crash-coverage we get from WONTFIX tests is so miniscule compared to the (also small) cost of maintaining any expectations for them, that it's not worth running them. > > While I pointed out only the crash coverage in comment #0, there are other benefits. The fast/harness/sample-fail-mismatch-reftest.html[1][2] layout test tests that an image-only failure occurs when the reftest and its expected mismatch baseline provide equal output. This test is marked as WontFix on both Chromium and GTK (and probably other ports as well) but isn't run even though it's integral to checking the correct harness behavior on which plenty of tests rely. I remember one occurrence of problems in harness that were indicated by this test unexpectedly passing (but it wasn't reported due to an otherwise unrelated bug), so not running this test can backfire. > That's a good counterexample. > Also, the Chromium port currently marks over 6700 tests as WontFix. That's ~20% of all the tests the port collects. IMO that's way too much to ignore and not run. > IIRC, a large chunk of these are the sputnik tests, which we don't run because v8 runs them as part of their own test suite. Otherwise most of the tests we mark WontFix are for features that are either port-specific or that we have no intention of implementing. (In reply to comment #5) > > I'd very like to see at least a compromise of this in the form of an opt-in configuration to run tests marked as WontFix. > > I'd still prefer a possibility of an expanded modifier set, where the WontFix modifier would only indicate the non-fixable nature of the test while the extra modifier would indicate the actual expected failure. Using only the WontFix modifier could still just skip the test, as is done now. That's an interesting suggestion; it's actually basically compatible with the existing syntax (since no modifiers implies Skip). I will take a look at that and if there's no real complications that that introduces, that seems like a reasonable thing to do. 798447 7 zan 2013-01-01 09:41:21 -0800 (In reply to comment #6) > (In reply to comment #4) > > Also, the Chromium port currently marks over 6700 tests as WontFix. That's ~20% of all the tests the port collects. IMO that's way too much to ignore and not run. > > > > IIRC, a large chunk of these are the sputnik tests, which we don't run because v8 runs them as part of their own test suite. Otherwise most of the tests we mark WontFix are for features that are either port-specific or that we have no intention of implementing. Yeah, the sputnik tests make up for a large part of that enormous list of WontFix tests on Chromium port. I wasn't aware of that. Marking them as WontFix makes sense for the Chromium port, as does the comment in TestExpectations of moving them to the JSC test suite. 807667 8 182843 zan 2013-01-15 13:55:01 -0800 Created attachment 182843 Patch 807670 9 182843 dpranke 2013-01-15 13:59:41 -0800 Comment on attachment 182843 Patch Looks great, thanks for working on this! 807672 10 182843 zan 2013-01-15 14:04:29 -0800 Comment on attachment 182843 Patch Clearing flags on attachment: 182843 Committed r139786: <http://trac.webkit.org/changeset/139786> 807673 11 zan 2013-01-15 14:04:34 -0800 All reviewed patches have been landed. Closing bug. 182843 2013-01-15 13:55:01 -0800 2013-01-15 14:04:29 -0800 Patch bug-105860-20130115225205.patch text/plain 4679 zan U3VidmVyc2lvbiBSZXZpc2lvbjogMTM5Nzc0CmRpZmYgLS1naXQgYS9Ub29scy9DaGFuZ2VMb2cg Yi9Ub29scy9DaGFuZ2VMb2cKaW5kZXggMjFjMGJmMzlkODU4ODliZjE2YTE5Zjg3ZGU4OWEzMGZj ZGFlYTFlMi4uNmU4YzVhYjZjZTcyOTcxZDMyZDViMmY1NzE2OWYyOTAzMjJjZGNmYSAxMDA2NDQK LS0tIGEvVG9vbHMvQ2hhbmdlTG9nCisrKyBiL1Rvb2xzL0NoYW5nZUxvZwpAQCAtMSwzICsxLDI2 IEBACisyMDEzLTAxLTE1ICBaYW4gRG9iZXJzZWsgIDx6ZG9iZXJzZWtAaWdhbGlhLmNvbT4KKwor ICAgICAgICBUZXN0cyB3aXRoIFdvbnRGaXggZXhwZWN0YXRpb24gYXJlIChpbmRpcmVjdGx5KSBz a2lwcGVkCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0x MDU4NjAKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBP bmx5IHNraXAgdGhlIFdvbnRGaXggZXhwZWN0YXRpb24gaWYgdGhlcmUgYXJlIG5vIG90aGVyIGxh eW91dCB0ZXN0CisgICAgICAgIGV4cGVjdGF0aW9ucyAobGlrZSBGYWlsdXJlLCBJbWFnZU9ubHlG YWlsdXJlKSBsaXN0ZWQgZm9yIHRoZSB0ZXN0LgorICAgICAgICBUaGlzIG1ha2VzIGl0IHBvc3Np YmxlIHRvIHJ1biBhbiBleHBlY3RlZCBmYWlsdXJlIGZvciB3aGljaCB0aGVyZQorICAgICAgICBp cyBubyByZWFzb24gb3IgaW50ZXJlc3QgdG8gZml4LiBUaGVyZSBhcmUgc29tZSBwb3J0cyB0aGF0 IHdvdWxkIGxpa2UKKyAgICAgICAgdG8gZXhlcmNpc2UgdGhpcyBiZWhhdmlvciBhcyB3ZWxsIGFz IHNwZWNpZmljIHRlc3QgY2FzZXMgdGhhdCBzaG91bGQgdXNlCisgICAgICAgIHN1Y2ggZXhwZWN0 YXRpb25zLgorCisgICAgICAgICogU2NyaXB0cy93ZWJraXRweS9sYXlvdXRfdGVzdHMvbW9kZWxz L3Rlc3RfZXhwZWN0YXRpb25zLnB5OgorICAgICAgICAoVGVzdEV4cGVjdGF0aW9uUGFyc2VyLl90 b2tlbml6ZV9saW5lKTogT25seSBhZGQgdGhlIFNLSVAgbW9kaWZpZXIgZm9yIGEKKyAgICAgICAg V09OVEZJWCB0ZXN0IGlmIHRoZXJlIGFyZSBubyBleHBlY3RhdGlvbnMgbGlzdGVkLiBUaGUgd2Fy bmluZyBpcyBhZGp1c3RlZAorICAgICAgICB0byByZWZsZWN0IHRoZSBuZXcgYmVoYXZpb3IuCisg ICAgICAgICogU2NyaXB0cy93ZWJraXRweS9sYXlvdXRfdGVzdHMvbW9kZWxzL3Rlc3RfZXhwZWN0 YXRpb25zX3VuaXR0ZXN0LnB5OiBBZGQKKyAgICAgICAgYW5kIGFkanVzdCB0ZXN0IGNhc2VzIGZv ciB0aGVzZSBjaGFuZ2VzLgorICAgICAgICAoRXhwZWN0YXRpb25TeW50YXhUZXN0cy50ZXN0X3dv bnRmaXgpOgorICAgICAgICAoU2VtYW50aWNUZXN0cy50ZXN0X3NraXBfYW5kX3dvbnRmaXgpOgor CiAyMDEzLTAxLTE1ICBUaW0gJ21pdGhybycgQW5zZWxsICA8bWl0aHJvQG1pdGhpcy5jb20+CiAK ICAgICAgICAgUmVuYW1pbmcgQ2xlYW5Xb3JraW5nRGlyZWN0b3J5IHN0ZXAgdG8gRGlzY2FyZExv Y2FsQ2hhbmdlcyB0byBtYWtlCmRpZmYgLS1naXQgYS9Ub29scy9TY3JpcHRzL3dlYmtpdHB5L2xh eW91dF90ZXN0cy9tb2RlbHMvdGVzdF9leHBlY3RhdGlvbnMucHkgYi9Ub29scy9TY3JpcHRzL3dl YmtpdHB5L2xheW91dF90ZXN0cy9tb2RlbHMvdGVzdF9leHBlY3RhdGlvbnMucHkKaW5kZXggYTZm ZmRjYjVmNTJjYjBmYzZmMzEyNDE1NDczNDFhODgxYTA3NDIzZS4uZGE4ZWJiYjYzNmY1ZGM3ZWVl NDc1MDBiNjBiZjc5Y2E0ZmE1ZGZmMSAxMDA2NDQKLS0tIGEvVG9vbHMvU2NyaXB0cy93ZWJraXRw eS9sYXlvdXRfdGVzdHMvbW9kZWxzL3Rlc3RfZXhwZWN0YXRpb25zLnB5CisrKyBiL1Rvb2xzL1Nj cmlwdHMvd2Via2l0cHkvbGF5b3V0X3Rlc3RzL21vZGVscy90ZXN0X2V4cGVjdGF0aW9ucy5weQpA QCAtMzQ2LDEyICszNDYsMTIgQEAgY2xhc3MgVGVzdEV4cGVjdGF0aW9uUGFyc2VyKG9iamVjdCk6 CiAgICAgICAgICAgICBlbGlmIHN0YXRlIG5vdCBpbiAoJ25hbWVfZm91bmQnLCAnZG9uZScpOgog ICAgICAgICAgICAgICAgIHdhcm5pbmdzLmFwcGVuZCgnTWlzc2luZyBhICJdIicpCiAKLSAgICAg ICAgaWYgJ1dPTlRGSVgnIGluIG1vZGlmaWVycyBhbmQgJ1NLSVAnIG5vdCBpbiBtb2RpZmllcnM6 CisgICAgICAgIGlmICdXT05URklYJyBpbiBtb2RpZmllcnMgYW5kICdTS0lQJyBub3QgaW4gbW9k aWZpZXJzIGFuZCBub3QgZXhwZWN0YXRpb25zOgogICAgICAgICAgICAgbW9kaWZpZXJzLmFwcGVu ZCgnU0tJUCcpCiAKICAgICAgICAgaWYgJ1NLSVAnIGluIG1vZGlmaWVycyBhbmQgZXhwZWN0YXRp b25zOgogICAgICAgICAgICAgIyBGSVhNRTogVGhpcyBpcyByZWFsbHkgYSBzZW1hbnRpYyB3YXJu aW5nIGFuZCBzaG91bGRuJ3QgYmUgaGVyZS4gUmVtb3ZlIHdoZW4gd2UgZHJvcCB0aGUgb2xkIHN5 bnRheC4KLSAgICAgICAgICAgIHdhcm5pbmdzLmFwcGVuZCgnQSB0ZXN0IG1hcmtlZCBTa2lwIG9y IFdvbnRGaXggbXVzdCBub3QgaGF2ZSBvdGhlciBleHBlY3RhdGlvbnMuJykKKyAgICAgICAgICAg IHdhcm5pbmdzLmFwcGVuZCgnQSB0ZXN0IG1hcmtlZCBTa2lwIG11c3Qgbm90IGhhdmUgb3RoZXIg ZXhwZWN0YXRpb25zLicpCiAgICAgICAgIGVsaWYgbm90IGV4cGVjdGF0aW9uczoKICAgICAgICAg ICAgIGlmICdTS0lQJyBub3QgaW4gbW9kaWZpZXJzIGFuZCAnUkVCQVNFTElORScgbm90IGluIG1v ZGlmaWVycyBhbmQgJ1NMT1cnIG5vdCBpbiBtb2RpZmllcnM6CiAgICAgICAgICAgICAgICAgbW9k aWZpZXJzLmFwcGVuZCgnU0tJUCcpCmRpZmYgLS1naXQgYS9Ub29scy9TY3JpcHRzL3dlYmtpdHB5 L2xheW91dF90ZXN0cy9tb2RlbHMvdGVzdF9leHBlY3RhdGlvbnNfdW5pdHRlc3QucHkgYi9Ub29s cy9TY3JpcHRzL3dlYmtpdHB5L2xheW91dF90ZXN0cy9tb2RlbHMvdGVzdF9leHBlY3RhdGlvbnNf dW5pdHRlc3QucHkKaW5kZXggYjNiMGFlYTZhNjhjZjljYzBhMDUxZDJhYmEyMWQxMjE0MjliMzFh My4uZWFlMjU0Mzc1NGRiMjk5ZTcyNDgwNDczYjRiMDBkMzQzMTBjYjMyYyAxMDA2NDQKLS0tIGEv VG9vbHMvU2NyaXB0cy93ZWJraXRweS9sYXlvdXRfdGVzdHMvbW9kZWxzL3Rlc3RfZXhwZWN0YXRp b25zX3VuaXR0ZXN0LnB5CisrKyBiL1Rvb2xzL1NjcmlwdHMvd2Via2l0cHkvbGF5b3V0X3Rlc3Rz L21vZGVscy90ZXN0X2V4cGVjdGF0aW9uc191bml0dGVzdC5weQpAQCAtMzU2LDYgKzM1Niw4IEBA IGNsYXNzIEV4cGVjdGF0aW9uU3ludGF4VGVzdHMoQmFzZSk6CiAKICAgICBkZWYgdGVzdF93b250 Zml4KHNlbGYpOgogICAgICAgICBzZWxmLmFzc2VydF90b2tlbml6ZV9leHAoJ2Zvby5odG1sIFsg V29udEZpeCBdJywgbW9kaWZpZXJzPVsnV09OVEZJWCcsICdTS0lQJ10sIGV4cGVjdGF0aW9ucz1b J1BBU1MnXSkKKyAgICAgICAgc2VsZi5hc3NlcnRfdG9rZW5pemVfZXhwKCdmb28uaHRtbCBbIFdv bnRGaXggSW1hZ2VPbmx5RmFpbHVyZSBdJywgbW9kaWZpZXJzPVsnV09OVEZJWCddLCBleHBlY3Rh dGlvbnM9WydJTUFHRSddKQorICAgICAgICBzZWxmLmFzc2VydF90b2tlbml6ZV9leHAoJ2Zvby5o dG1sIFsgV29udEZpeCBQYXNzIEZhaWx1cmUgXScsIG1vZGlmaWVycz1bJ1dPTlRGSVgnXSwgZXhw ZWN0YXRpb25zPVsnUEFTUycsICdGQUlMJ10pCiAKICAgICBkZWYgdGVzdF9ibGFua19saW5lKHNl bGYpOgogICAgICAgICBzZWxmLmFzc2VydF90b2tlbml6ZV9leHAoJycsIG5hbWU9Tm9uZSkKQEAg LTM5MSwxNiArMzkzLDE2IEBAIGNsYXNzIFNlbWFudGljVGVzdHMoQmFzZSk6CiAgICAgICAgIHNl bGYuYXNzZXJ0RXF1YWwobGluZS53YXJuaW5ncywgWydUZXN0IGxhY2tzIEJVRyBtb2RpZmllci4n XSkKIAogICAgIGRlZiB0ZXN0X3NraXBfYW5kX3dvbnRmaXgoc2VsZik6Ci0gICAgICAgICMgU2tp cCBhbmQgV29udEZpeCBhcmUgbm90IGFsbG93ZWQgdG8gaGF2ZSBvdGhlciBleHBlY3RhdGlvbnMg YXMgd2VsbCwgYmVjYXVzZSB0aG9zZQorICAgICAgICAjIFNraXAgaXMgbm90IGFsbG93ZWQgdG8g aGF2ZSBvdGhlciBleHBlY3RhdGlvbnMgYXMgd2VsbCwgYmVjYXVzZSB0aG9zZQogICAgICAgICAj IGV4cGVjdGF0aW9ucyB3b24ndCBiZSBleGVyY2lzZWQgYW5kIG1heSBiZWNvbWUgc3RhbGUgLgog ICAgICAgICBzZWxmLnBhcnNlX2V4cCgnZmFpbHVyZXMvZXhwZWN0ZWQvdGV4dC5odG1sIFsgRmFp bHVyZSBTa2lwIF0nKQogICAgICAgICBzZWxmLmFzc2VydFRydWUoc2VsZi5fZXhwLmhhc193YXJu aW5ncygpKQogCiAgICAgICAgIHNlbGYucGFyc2VfZXhwKCdmYWlsdXJlcy9leHBlY3RlZC90ZXh0 Lmh0bWwgWyBDcmFzaCBXb250Rml4IF0nKQotICAgICAgICBzZWxmLmFzc2VydFRydWUoc2VsZi5f ZXhwLmhhc193YXJuaW5ncygpKQorICAgICAgICBzZWxmLmFzc2VydEZhbHNlKHNlbGYuX2V4cC5o YXNfd2FybmluZ3MoKSkKIAogICAgICAgICBzZWxmLnBhcnNlX2V4cCgnZmFpbHVyZXMvZXhwZWN0 ZWQvdGV4dC5odG1sIFsgUGFzcyBXb250Rml4IF0nKQotICAgICAgICBzZWxmLmFzc2VydFRydWUo c2VsZi5fZXhwLmhhc193YXJuaW5ncygpKQorICAgICAgICBzZWxmLmFzc2VydEZhbHNlKHNlbGYu X2V4cC5oYXNfd2FybmluZ3MoKSkKIAogICAgIGRlZiB0ZXN0X3Nsb3dfYW5kX3RpbWVvdXQoc2Vs Zik6CiAgICAgICAgICMgQSB0ZXN0IGNhbm5vdCBiZSBTTE9XIGFuZCBleHBlY3RlZCB0byBUSU1F T1VULgo=