Bug 98366 – [GTK] accessibility/ignore-spacer-elements.html is failing

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Zan Dobersek]

accessibility/ignore-spacer-elements.html is failing on all GTK platforms.
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20webkit.org&tests=accessibility%2Fignore-spacer-elements.html

[Joanmarie Diggs]

Created attachment 169028 [details]
Patch

[Joanmarie Diggs]

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.

[WebKit Review Bot]

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

[Build Bot]

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

[Joanmarie Diggs]

Created attachment 169045 [details]
Patch

[chris fleizach]

Comment on attachment 169045 [details]
Patch

Your gtk test expectation is a failure. is that ok

[Joanmarie Diggs]

(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?

[WebKit Review Bot]

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

[Build Bot]

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

[Joanmarie Diggs]

Created attachment 169059 [details]
Patch

[Joanmarie Diggs]

(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.)

[chris fleizach]

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

[Mario Sanchez Prada]

(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.

[Joanmarie Diggs]

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!

[Joanmarie Diggs]

Created attachment 169157 [details]
Patch

[Joanmarie Diggs]

Comment on attachment 169157 [details]
Patch

Mario's suggestion works like a charm. (Thanks again!)

[chris fleizach]

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

[WebKit Review Bot]

Comment on attachment 169157 [details]
Patch

Clearing flags on attachment: 169157

Committed r131599: <http://trac.webkit.org/changeset/131599>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.