| Summary: | grammar-markers.html and grammar-markers-hidpi.html pass even if element does not have grammar marker | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Grzegorz Czajkowski <g.czajkowski> | ||||||||||||
| Component: | Tools / Tests | Assignee: | Grzegorz Czajkowski <g.czajkowski> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | buildbot, commit-queue, rniwa | ||||||||||||
| Priority: | P2 | ||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||
| Hardware: | Unspecified | ||||||||||||||
| OS: | Unspecified | ||||||||||||||
| Attachments: |
|
||||||||||||||
Created attachment 208723 [details]
proposed patch
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. Created attachment 209197 [details]
apply Darin's review
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. 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 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
(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. Created attachment 209646 [details]
Patch
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. 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 Created attachment 209742 [details]
apply Ryosuke's review.
Comment on attachment 209742 [details] apply Ryosuke's review. Clearing flags on attachment: 209742 Committed r154675: <http://trac.webkit.org/changeset/154675> All reviewed patches have been landed. Closing bug. |
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.