Bug 98366

Created attachment 169028 [details] Patch

Hey Chris. If you have a moment for a review it would be great. Very simple patch (unskipped a test, added a platform exception). However, to make the test more than just pass/fail I modified the original test to use shouldBe() with the childrenCount.

Comment on attachment 169028 [details] Patch Attachment 169028 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14396304 New failing tests: accessibility/ignore-spacer-elements.html

Comment on attachment 169028 [details] Patch Attachment 169028 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14400171 New failing tests: accessibility/ignore-spacer-elements.html

Created attachment 169045 [details] Patch

Comment on attachment 169045 [details] Patch Your gtk test expectation is a failure. is that ok

(In reply to comment #6) > (From update of attachment 169045 [details]) > Your gtk test expectation is a failure. is that ok I *believe* it is a failure in test output only: Officially 4 children are expected to "succeed" -- at least for the Mac port, I assume. But that is not correct for GTK which considers 4 a "failure" and 2 "success". I'd be delighted if there is a way to have it say "PASS" even though the results are different. I generated the test expectations using --new-baseline --add-platform-exceptions as described on http://trac.webkit.org/wiki/NewRunWebKitTests. And when I then run the test, it claims to have run as expected. If the hierarchy were to unexpectedly change and a different number of children were to appear, the regression would be caught. Do you have suggestions for how to make it officially not a failure?

Comment on attachment 169045 [details] Patch Attachment 169045 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14394304 New failing tests: accessibility/ignore-spacer-elements.html

Comment on attachment 169045 [details] Patch Attachment 169045 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14394309 New failing tests: accessibility/ignore-spacer-elements.html

Created attachment 169059 [details] Patch

(I just built the latest webkit on my Mac. The test was failing on the Mac due to a bogus newline I added. Hopefully this will pass EWS.)

Comment on attachment 169059 [details] Patch It doesn't seem like we should remove the Failure keyword if the GTK expected file says FAIL. There are a number of options (maybe more than this) a) Skip this test and add a copy of it just for GTK that has the right value b) add something in DRT to identify GTK builds and conditionalize the test c) change the output so that it doesn't just output childrenCount and instead outputs something more meaningful

(In reply to comment #12) > (From update of attachment 169059 [details]) > It doesn't seem like we should remove the Failure keyword if the GTK > expected file says FAIL. Agreed. > There are a number of options (maybe more than this) > a) Skip this test and add a copy of it just for GTK that has the right value I think just placing a copy of the .html file in platform/gtk will give priority to that test, avoiding the one in LayoutTests/accessibility from being executed. > b) add something in DRT to identify GTK builds and conditionalize the test We already have testRunner.platformName for this, which returns an string with the name of the platform ("gtk" for the GTK port). So, in case we went for this option, the test could be rewritten as this: var expectedCount = (testRunner.platform == "gtk") ? 2 : 4; if (container.childrenCount == expectedCount) { result.innerText += "Test passed\n"; } else { result.innerText += "Test failed\n"; } > c) change the output so that it doesn't just output childrenCount and instead outputs something more meaningful If my suggestion for b) works, I think it would be a better (or just a "good enough") approach.

Thanks guys! (In reply to comment #13) > (In reply to comment #12) > > There are a number of options (maybe more than this) > > a) Skip this test and add a copy of it just for GTK that has the right value > > I think just placing a copy of the .html file in platform/gtk will give priority to that test, avoiding the one in LayoutTests/accessibility from being executed. Personally I'm not jazzed about either of these. Ideally I'd like us to have a (clean) way to get a single set of cross-port tests that work for a11y. > > b) add something in DRT to identify GTK builds and conditionalize the test This I like. :) Better still given this: > We already have testRunner.platformName for this, which returns an string with the name of the platform ("gtk" for the GTK port). > > So, in case we went for this option, the test could be rewritten as this: > > var expectedCount = (testRunner.platform == "gtk") ? 2 : 4; > if (container.childrenCount == expectedCount) { > result.innerText += "Test passed\n"; > } else { > result.innerText += "Test failed\n"; > } Thanks Mario! I'll give that a shot. > > c) change the output so that it doesn't just output childrenCount and instead outputs something more meaningful > > If my suggestion for b) works, I think it would be a better (or just a "good enough") approach. Agreed. Given the platform differences with respect to including and ignoring objects, childrenCount strikes me as extremely relevant with respect to our current set of regression tests -- unless or until we create a full set of hierarchy-related, platform-specific tests, anyway. Thanks again guys!

Created attachment 169157 [details] Patch

Comment on attachment 169157 [details] Patch Mario's suggestion works like a charm. (Thanks again!)

Comment on attachment 169157 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=169157&action=review > LayoutTests/accessibility/ignore-spacer-elements.html:29 > + if (container.childrenCount == expectedCount) { this is a pretty bad test that only outputs Test passed with no indication of what might have failed. We should log a bug to rewrite it so it's a little more descriptive

Comment on attachment 169157 [details] Patch Clearing flags on attachment: 169157 Committed r131599: <http://trac.webkit.org/changeset/131599>

All reviewed patches have been landed. Closing bug.