Bug 48818 – Expectation of /editing/spelling/spelling-textarea.html is confusing

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

[Hajime Morrita]

the assertion function print "PASS" even if fail, that is confusing and should print "FAIL".

[Hajime Morrita]

Created attachment 72637 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 72637 [details]
Patch

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

> LayoutTests/editing/spelling/spelling-contenteditable.html:26
> +    // Test the above 'zz' is marked as misspelled.
> +    shouldBeTrue("layoutTestController.hasSpellingMarker(0, 2)");
>  }
>  
>  var successfullyParsed = true;

Does this even need to be a script test?  There seems to be only one test case in this.  We should make this a regular dump-as-text test or runDumpAsTextEditingTest test.

> LayoutTests/editing/spelling/spelling-textarea.html:23
> -    // Test the above 'zz' is marked as misspelled. We test it only on platforms that implement
> -    // textInputController.hasSpellingMarker() to avoid exceptions.
> -    logResult(layoutTestController.hasSpellingMarker(0, 2));
> +    // Test the above 'zz' is marked as misspelled.
> +    shouldBeTrue("layoutTestController.hasSpellingMarker(0, 2)");

Ditto for this test as well.  You could combine these two tests into one file.  Then it's perfectly valid to keep it as a script test.

[Hajime Morrita]

Created attachment 72918 [details]
Patch

[Hajime Morrita]

Hi Ryosuke, thank you for taking a look!

> Does this even need to be a script test?  There seems to be only one test case in this.  We should make this a regular dump-as-text test or runDumpAsTextEditingTest test.
Make these two test into one. 
On the name spelling-hasspellingmarker, these tests are introduced to check if 
LayoutTestController.hasSpellingMarker() works. The name reflects it.

[Ryosuke Niwa]

Comment on attachment 72918 [details]
Patch

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

> LayoutTests/editing/spelling/script-tests/spelling-hasspellingmarker.js:18
> +shouldBeTrue("hasMarked(\"<input id='test' type='text'>\");");

Is this a new test?  If so, you probably say why you're adding this.

> LayoutTests/editing/spelling/script-tests/spelling-hasspellingmarker.js:20
> +shouldBeFalse("hasMarked(\"<div id='test' contentEditable='true' spellcheck='false'></div>\");");

This seems to be new as well.

[Hajime Morrita]

Created attachment 73025 [details]
Patch

[Hajime Morrita]

> (From update of attachment 72918 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=72918&action=review
> 
> > LayoutTests/editing/spelling/script-tests/spelling-hasspellingmarker.js:18
> > +shouldBeTrue("hasMarked(\"<input id='test' type='text'>\");");
> 
> Is this a new test?  If so, you probably say why you're adding this.
Yes, it' new for make it more comprehensive. 
I added an explanation on the ChangeLog.

[Ryosuke Niwa]

Comment on attachment 73025 [details]
Patch

LGTM.

[Kent Tamura]

Comment on attachment 73025 [details]
Patch

Looks good except the ChangeLog diff doesn't start at the beginning of the file.

[Hajime Morrita]

Committed r71392: <http://trac.webkit.org/changeset/71392>