72001 2011-11-09 23:39:08 -0800 [Chromium] only run media GPU layout tests on platforms supporting accelerated compositing 2011-11-10 18:05:06 -0800 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 scherkus scherkus abarth dglazkov dpranke jamesr ojan tony webkit.review.bot oldest_to_newest 499682 0 scherkus 2011-11-09 23:39:08 -0800 [Chromium] only run media GPU layout tests on platforms supporting accelerated compositing 499686 1 114443 scherkus 2011-11-09 23:41:46 -0800 Created attachment 114443 Patch 499688 2 scherkus 2011-11-09 23:44:13 -0800 Reason being that the following line in test_expectations.txt: WONTFIX SKIP GPU GPU-CG LEOPARD : media = PASS TIMEOUT FAIL ...doesn't seem to do the trick with respect to skipping the tests. Furthermore if I've been debugging the layout test results correctly it also looks like it's marking all media tests on 10.5 (non-GPU) as PASS/FAIL. 500003 3 tony 2011-11-10 09:36:33 -0800 (In reply to comment #2) > Reason being that the following line in test_expectations.txt: > WONTFIX SKIP GPU GPU-CG LEOPARD : media = PASS TIMEOUT FAIL > > ...doesn't seem to do the trick with respect to skipping the tests. Furthermore if I've been debugging the layout test results correctly it also looks like it's marking all media tests on 10.5 (non-GPU) as PASS/FAIL. Can we figure out why the above doesn't work? 500034 4 scherkus 2011-11-10 10:33:04 -0800 (In reply to comment #3) > (In reply to comment #2) > > Reason being that the following line in test_expectations.txt: > > WONTFIX SKIP GPU GPU-CG LEOPARD : media = PASS TIMEOUT FAIL > > > > ...doesn't seem to do the trick with respect to skipping the tests. Furthermore if I've been debugging the layout test results correctly it also looks like it's marking all media tests on 10.5 (non-GPU) as PASS/FAIL. > > Can we figure out why the above doesn't work? Turns out it _does_ work but that more-specific expectations will cause the test to run on 10.5 GPU. Take a look at flakiness results for media/audio-controls-rendering.html: http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20GPU%20Mesa%20-%20chromium.org&tests=media%2Faudio-controls-rendering&showExpectations=true It was previously marked as: BUGWK70748 CPU : media/audio-controls-rendering.html = IMAGE ...which caused it to override the SKIP and start running on Mac 10.5 GPU. After rebaselining and removing that expectation the test is now skipped again. Anyone know if that's the intended behaviour? 500039 5 ojan 2011-11-10 10:37:32 -0800 It is certainly the intended behavior that more specific listings override less specific ones. We could take a queue from CSS and add something like "!important" lol. 500041 6 scherkus 2011-11-10 10:40:08 -0800 (In reply to comment #5) > It is certainly the intended behavior that more specific listings override less specific ones. We could take a queue from CSS and add something like "!important" lol. Yeah that's what I figured and certainly makes sense to me! So... are we happy with my patch or should I investigate a different avenue? Having the tests potentially run isn't fun because it generates noise on the flakiness dashboard as well as increases test cycle time. 500043 7 tony 2011-11-10 10:40:26 -0800 This specific case seems like a bug since the SKIP line refers to GPU and the more specific line refers to CPU. I wouldn't expect CPU : media/audio-controls-rendering.html = IMAGE to override WONTFIX SKIP GPU GPU-CG LEOPARD : media = PASS TIMEOUT FAIL 500046 8 scherkus 2011-11-10 10:44:29 -0800 (In reply to comment #7) > This specific case seems like a bug since the SKIP line refers to GPU and the more specific line refers to CPU. I wouldn't expect > CPU : media/audio-controls-rendering.html = IMAGE > to override > WONTFIX SKIP GPU GPU-CG LEOPARD : media = PASS TIMEOUT FAIL Check out the diff: https://trac.webkit.org/changeset/99789/trunk/LayoutTests/platform/chromium/test_expectations.txt It looks like it was listed as: BUGWK70748 CPU : media/audio-controls-rendering.html = IMAGE BUGWK70748 GPU : media/audio-controls-rendering.html = IMAGE So in this case I guess GPU was being overridden. 500048 9 tony 2011-11-10 10:48:28 -0800 (In reply to comment #8) > (In reply to comment #7) > > This specific case seems like a bug since the SKIP line refers to GPU and the more specific line refers to CPU. I wouldn't expect > > CPU : media/audio-controls-rendering.html = IMAGE > > to override > > WONTFIX SKIP GPU GPU-CG LEOPARD : media = PASS TIMEOUT FAIL > > Check out the diff: > https://trac.webkit.org/changeset/99789/trunk/LayoutTests/platform/chromium/test_expectations.txt > > It looks like it was listed as: > BUGWK70748 CPU : media/audio-controls-rendering.html = IMAGE > BUGWK70748 GPU : media/audio-controls-rendering.html = IMAGE > > So in this case I guess GPU was being overridden. If you changed: BUGWK70748 GPU : media/audio-controls-rendering.html = IMAGE to: BUGWK70748 GPU SNOWLEOPARD WIN LINUX : media/audio-controls-rendering.html = IMAGE I think it would fix your problem since it doesn't overlap the previous SKIP line. 500049 10 scherkus 2011-11-10 10:54:28 -0800 (In reply to comment #9) > (In reply to comment #8) > > (In reply to comment #7) > > > This specific case seems like a bug since the SKIP line refers to GPU and the more specific line refers to CPU. I wouldn't expect > > > CPU : media/audio-controls-rendering.html = IMAGE > > > to override > > > WONTFIX SKIP GPU GPU-CG LEOPARD : media = PASS TIMEOUT FAIL > > > > Check out the diff: > > https://trac.webkit.org/changeset/99789/trunk/LayoutTests/platform/chromium/test_expectations.txt > > > > It looks like it was listed as: > > BUGWK70748 CPU : media/audio-controls-rendering.html = IMAGE > > BUGWK70748 GPU : media/audio-controls-rendering.html = IMAGE > > > > So in this case I guess GPU was being overridden. > > If you changed: > BUGWK70748 GPU : media/audio-controls-rendering.html = IMAGE > to: > BUGWK70748 GPU SNOWLEOPARD WIN LINUX : media/audio-controls-rendering.html = IMAGE > > I think it would fix your problem since it doesn't overlap the previous SKIP line. With updating test_expectations.txt being a collaborative effort is it reasonable to expect folks to notice situations like this and write fine-grained expectations? The person who added this expectation would have to be aware that compositing isn't not enabled on LEOPARD. 500050 11 ojan 2011-11-10 10:57:08 -0800 (In reply to comment #10) > With updating test_expectations.txt being a collaborative effort is it reasonable to expect folks to notice situations like this and write fine-grained expectations? > > The person who added this expectation would have to be aware that compositing isn't not enabled on LEOPARD. Not at all reasonable IMO. I don't see a better solution though other than generally reducing the complexity of the test_expectations format. I guess your proposal here to bake it into run-webkit-tests is one solution. That carries the problem of making it harder to know why a test is/isn't being run because now you need to know both how test_expectations works and what run-webkit-tests hard-codes. Definitely open to suggestions on how to make this easier to maintain/understand. It's crazy confusing right now. 500052 12 scherkus 2011-11-10 10:59:39 -0800 (In reply to comment #11) > (In reply to comment #10) > > With updating test_expectations.txt being a collaborative effort is it reasonable to expect folks to notice situations like this and write fine-grained expectations? > > > > The person who added this expectation would have to be aware that compositing isn't not enabled on LEOPARD. > > Not at all reasonable IMO. I don't see a better solution though other than generally reducing the complexity of the test_expectations format. I guess your proposal here to bake it into run-webkit-tests is one solution. That carries the problem of making it harder to know why a test is/isn't being run because now you need to know both how test_expectations works and what run-webkit-tests hard-codes. > > Definitely open to suggestions on how to make this easier to maintain/understand. It's crazy confusing right now. To be clear I'm not 100% thrilled with it either -- it took me a good deal of grepping just to find out how media tests were getting run on GPU bots in the first place! GPU tests are definitely a little strange as they only run small subset but share the test_expectations.txt file. 500056 13 tony 2011-11-10 11:05:41 -0800 From offline discussion with Ojan: Part of the problem is it's hard to figure out which lines in test_expectations.txt apply to a specific config. One solution would be to linkify the expectations on the flakiness dashboard and clicking on it would show you which lines apply to that test on that builder. 500376 14 dpranke 2011-11-10 16:02:17 -0800 The fact that we are subsetting the list of tests running on the GPU bots inside the chromium_gpu.py code is arguably a misfeature, but it does solve the problem you've run into (and may be why I've left that in until now rather than just WONTFIX SKIPping all of the directories). I certainly agree that the test_expectations files are fragile and hard to maintain by hand, but I suspect that any format that supports all of the different uses will be at least as complicated. One possible option would be to make any directories marked as SKIP override expected results for specific files inside the directory; I am not aware of any case in practice where we want to skip all but X files in a dir, and if we did, we could support that by requiring the user to explicitly list all of the tests to skip. We could also build up better tools for explaining what tests run where and what the expectations are. It is actually possible to extract that from NRWT now given the appropriate command line flags, but it's not easy to do so (using the --print trace* options) Maybe we need a "webkit-patch print-expectations" command that duplicates what we print with --print trace and iterates over all of the ports or something? 500379 15 114443 dpranke 2011-11-10 16:03:16 -0800 Comment on attachment 114443 Patch R+'ing this change; it sounds like it will produce the intended result most easily. Scherkus, it's up to you if you want to land it or just fall back to expectations-based skipping. 500387 16 ojan 2011-11-10 16:10:09 -0800 (In reply to comment #14) > One possible option would be to make any directories marked as SKIP override expected results for specific files inside the directory; I am not aware of any case in practice where we want to skip all but X files in a dir, and if we did, we could support that by requiring the user to explicitly list all of the tests to skip. This makes sense to me. "SKIP cannot be overridden" seems reasonable. It does further complicate things though. :( 500440 17 114443 scherkus 2011-11-10 17:46:11 -0800 Comment on attachment 114443 Patch Clearing flags on attachment: 114443 Committed r99920: <http://trac.webkit.org/changeset/99920> 500441 18 scherkus 2011-11-10 17:46:16 -0800 All reviewed patches have been landed. Closing bug. 500442 19 scherkus 2011-11-10 17:47:39 -0800 Thanks for the review Dirk! Do you want me to file follow up bugs for some of the ideas discussed here, or should we agree on a reasonable path forward first? 500443 20 tony 2011-11-10 17:48:40 -0800 We probably want to remove the SKIP line from test_expectations to avoid confusion. 500445 21 dpranke 2011-11-10 17:53:58 -0800 (In reply to comment #19) > Thanks for the review Dirk! > > Do you want me to file follow up bugs for some of the ideas discussed here, or should we agree on a reasonable path forward first? Having a 'webkit-patch print-expectations' seems like a good idea; please feel free to file a bug for that. I'm less certain about the SKIP overrides more-specific-paths, but feel free to file a bug to track better ideas for that. 500452 22 scherkus 2011-11-10 18:05:06 -0800 Done: https://bugs.webkit.org/show_bug.cgi?id=72079 https://bugs.webkit.org/show_bug.cgi?id=72080 114443 2011-11-09 23:41:46 -0800 2011-11-10 17:46:11 -0800 Patch bug-72001-20111109234144.patch text/plain 1827 scherkus U3VidmVyc2lvbiBSZXZpc2lvbjogOTk4MTcKZGlmZiAtLWdpdCBhL1Rvb2xzL0NoYW5nZUxvZyBi L1Rvb2xzL0NoYW5nZUxvZwppbmRleCBkNzZjMWVjZGIwYTZlMjk5NmRiOTJjYTEyMWU4MTBjZjYx ZDM4YmIyLi5hYjMyNjZmYzViNDY2MzAxMzVkN2U2NzA0N2Y1Y2ZmODNmMGI4NDcyIDEwMDY0NAot LS0gYS9Ub29scy9DaGFuZ2VMb2cKKysrIGIvVG9vbHMvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTIg QEAKKzIwMTEtMTEtMDkgIEFuZHJldyBTY2hlcmt1cyAgPHNjaGVya3VzQGNocm9taXVtLm9yZz4K KworICAgICAgICBbQ2hyb21pdW1dIG9ubHkgcnVuIG1lZGlhIEdQVSBsYXlvdXQgdGVzdHMgb24g cGxhdGZvcm1zIHN1cHBvcnRpbmcgYWNjZWxlcmF0ZWQgY29tcG9zaXRpbmcKKyAgICAgICAgaHR0 cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTcyMDAxCisKKyAgICAgICAgUmV2 aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgKiBTY3JpcHRzL3dlYmtpdHB5L2xh eW91dF90ZXN0cy9wb3J0L2Nocm9taXVtX2dwdS5weToKKwogMjAxMS0xMS0wOSAgS2V2aW4gT2xs aXZpZXIgIDxrZXZpbm9AdGhlb2xsaXZpZXJzLmNvbT4KIAogICAgICAgICBbd3hdIFVucmV2aWV3 ZWQgYnVpbGQgZml4LiBVcGRhdGUgcHJvamVjdCBmaWxlcyBhbmQKZGlmZiAtLWdpdCBhL1Rvb2xz L1NjcmlwdHMvd2Via2l0cHkvbGF5b3V0X3Rlc3RzL3BvcnQvY2hyb21pdW1fZ3B1LnB5IGIvVG9v bHMvU2NyaXB0cy93ZWJraXRweS9sYXlvdXRfdGVzdHMvcG9ydC9jaHJvbWl1bV9ncHUucHkKaW5k ZXggZTgwNWU2ODcwMjNmNzZiYzE4YmNmY2JlZTg3ZjgyNzgxMWM5OGJjMi4uNzI5ZTkxYTg4ZWYw ZjdjMmVkYWRlOTE2OWI0YjhmZTgwMGMxNjIyNCAxMDA2NDQKLS0tIGEvVG9vbHMvU2NyaXB0cy93 ZWJraXRweS9sYXlvdXRfdGVzdHMvcG9ydC9jaHJvbWl1bV9ncHUucHkKKysrIGIvVG9vbHMvU2Ny aXB0cy93ZWJraXRweS9sYXlvdXRfdGVzdHMvcG9ydC9jaHJvbWl1bV9ncHUucHkKQEAgLTc4LDcg Kzc4LDE1IEBAIGRlZiBfc2V0X2dwdV9vcHRpb25zKHBvcnQsIGdyYXBoaWNzX3R5cGU9J2dwdScp OgogCiBkZWYgX3Rlc3RzKHBvcnQsIHBhdGhzKToKICAgICBpZiBub3QgcGF0aHM6Ci0gICAgICAg IHBhdGhzID0gWydjb21wb3NpdGluZycsICdwbGF0Zm9ybS9jaHJvbWl1bS9jb21wb3NpdGluZycs ICdtZWRpYScsICdhbmltYXRpb25zLzNkJ10KKyAgICAgICAgcGF0aHMgPSBbJ2NvbXBvc2l0aW5n JywgJ3BsYXRmb3JtL2Nocm9taXVtL2NvbXBvc2l0aW5nJywgJ2FuaW1hdGlvbnMvM2QnXQorCisg ICAgICAgIGlmIG5vdCBwb3J0Lm5hbWUoKS5zdGFydHN3aXRoKCdjaHJvbWl1bS1ncHUtbWFjLWxl b3BhcmQnKToKKyAgICAgICAgICAgICMgT25seSBydW4gdGVzdHMgcmVxdWlyaW5nIGFjY2VsZXJh dGVkIGNvbXBvc2l0aW5nIG9uIHBsYXRmb3JtcyB0aGF0CisgICAgICAgICAgICAjIHN1cHBvcnQg aXQuCisgICAgICAgICAgICAjIEZJWE1FOiB3ZSBzaG91bGQgYWRkIHRoZSBhYm92ZSBwYXRocyBo ZXJlIGFzIHdlbGwgYnV0IGxldCdzIHRlc3QKKyAgICAgICAgICAgICMgdGhlIHdhdGVycyB3aXRo IG1lZGlhIGZpcnN0LgorICAgICAgICAgICAgcGF0aHMgKz0gWydtZWRpYSddCisKICAgICAgICAg aWYgbm90IHBvcnQubmFtZSgpLnN0YXJ0c3dpdGgoJ2Nocm9taXVtLWdwdS1jZy1tYWMnKToKICAg ICAgICAgICAgICMgQ2FudmFzIGlzIG5vdCB5ZXQgYWNjZWxlcmF0ZWQgb24gdGhlIE1hYywgc28g dGhlcmUncyBubyBwb2ludAogICAgICAgICAgICAgIyBpbiBydW5uaW5nIHRoZSB0ZXN0cyB0aGVy ZS4K