Bug 119797 – grammar-markers.html and grammar-markers-hidpi.html pass even if element does not have grammar marker

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Grzegorz Czajkowski]

After tenth attempts of verifying the grammar marker, the tests call 'notifyDone' even if grammar marker was not be found.

if (hasMarker || --retry == 0)
    testRunner.notifyDone();

That's why those tests pass on platforms that doesn't implement grammar checking.

[Grzegorz Czajkowski]

Created attachment 208723 [details]
proposed patch

[Darin Adler]

Comment on attachment 208723 [details]
proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=208723&action=review

> LayoutTests/editing/spelling/grammar-markers-hidpi.html:36
> +    if (hasMarker && window.testRunner)
> +        testRunner.notifyDone();

Doesn’t make sense. If we never find the marker we still need to mark the test done, otherwise we’ll hang until we time out. We should write out whether we found the marker, not hang when we didn’t find it.

[Grzegorz Czajkowski]

Created attachment 209197 [details]
apply Darin's review

[Ryosuke Niwa]

Comment on attachment 209197 [details]
apply Darin's review

View in context: https://bugs.webkit.org/attachment.cgi?id=209197&action=review

> LayoutTests/editing/spelling/grammar-markers-hidpi.html:44
> +    if (hasMarker)
> +        testPassed('text : "' + target.innerHTML + '" has grammar marker under a word "has"');
> +    else
> +        testFailed('text : "' + target.innerHTML + '" does not have grammar marker under a word "has"');

Why don't we simply use shouldBeTrue?  You can even use indexOf and length of "has" instead of magic numbers 4 and 3 to be more descriptive.

[Build Bot]

Comment on attachment 209197 [details]
apply Darin's review

Attachment 209197 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1520427

New failing tests:
editing/spelling/grammar-markers.html
editing/spelling/grammar-markers-hidpi.html

[Build Bot]

Created attachment 209250 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.4

[Grzegorz Czajkowski]

(In reply to comment #4)
> (From update of attachment 209197 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=209197&action=review
> 
> > LayoutTests/editing/spelling/grammar-markers-hidpi.html:44
> > +    if (hasMarker)
> > +        testPassed('text : "' + target.innerHTML + '" has grammar marker under a word "has"');
> > +    else
> > +        testFailed('text : "' + target.innerHTML + '" does not have grammar marker under a word "has"');
> 
> Why don't we simply use shouldBeTrue?  You can even use indexOf and length of "has" instead of magic numbers 4 and 3 to be more descriptive.

That's good idea! I'll apply it before submitting.
Mac Ews showed that the modified tests start failing. The pixel tests have a grammar marker under a word "has". It seems that the tests are still wrong. I'll check them as well.

[Grzegorz Czajkowski]

Created attachment 209646 [details]
Patch

[Grzegorz Czajkowski]

Comment on attachment 209197 [details]
apply Darin's review

View in context: https://bugs.webkit.org/attachment.cgi?id=209197&action=review

> LayoutTests/editing/spelling/grammar-markers-hidpi.html:34
>      var hasMarker = internals.hasGrammarMarker(target, 4, 3);

This method is able to check grammar marker for document object only. Changed this in next patch - it's needed to pass on Mac.

[Ryosuke Niwa]

Comment on attachment 209646 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=209646&action=review

> LayoutTests/editing/spelling/grammar-markers-expected.txt:9
> +On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
> +
> +
> +PASS successfullyParsed is true
> +
> +TEST COMPLETE
> +PASS hasMarker is true

You're not setting jsTestIsAsync to true.  TEST COMPLETE should show up at the end.

> LayoutTests/editing/spelling/grammar-markers-hidpi.html:12
> +    

Whitespace.

> LayoutTests/editing/spelling/grammar-markers-hidpi.html:27
> +        

Whitespace.

> LayoutTests/editing/spelling/grammar-markers-hidpi.html:29
> +        

Whitespace.

> LayoutTests/editing/spelling/grammar-markers-hidpi.html:33
> +    

Whitespace

[Grzegorz Czajkowski]

Created attachment 209742 [details]
apply Ryosuke's review.

[WebKit Commit Bot]

Comment on attachment 209742 [details]
apply Ryosuke's review.

Clearing flags on attachment: 209742

Committed r154675: <http://trac.webkit.org/changeset/154675>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.