Bug 110447 – REGRESSION(r143470): editing/spelling/spelling-changed-text.html fails on Qt, GTK, EFL

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

[Zoltan Arvai]

After r143470 editing/spelling/spelling-changed-text.html fails on Qt, GTK, EFL.

--- /ramdisk/qt-linux-release/build/layout-test-results/editing/spelling/spelling-changed-text-expected.txt
+++ /ramdisk/qt-linux-release/build/layout-test-results/editing/spelling/spelling-changed-text-actual.txt
@@ -3,8 +3,8 @@
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
 
 
-PASS window.getSelection().toString() is " Is it broken?"
+FAIL Spellcheck timed out
 PASS successfullyParsed is true
 
 TEST COMPLETE
-
+Spell cheher. Is it broken?

[Tony Chang]

We should probably skip this test on Qt, GTK, EFL.

[Rouslan Solomakhin]

Working on a patch.

[Rouslan Solomakhin]

Are other platforms ok?

[Rouslan Solomakhin]

Created attachment 189577 [details]
Patch

[Tony Chang]

Comment on attachment 189577 [details]
Patch

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

> LayoutTests/platform/efl/TestExpectations:1865
> +# Need to add "cheher" misspelling to tests.
> +webkit.org/b/110503 editing/spelling/spelling-changed-text.html [ Skip ]

Looks like we should use Failure since it's not timing out on EFL:
http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Release%20WK2/builds/5137/steps/layout-test/logs/stdio

The benefit of Failure is you can see the diff on the waterfall:
http://build.webkit.org/results/GTK%20Linux%2064-bit%20Release%20WK2%20(Tests)/r143631%20(4487)/editing/spelling/spelling-changed-text-pretty-diff.html

Looks like there's some testRunner method that isn't implemented.

> LayoutTests/platform/gtk/TestExpectations:1446
> +# Need to add "cheher" misspelling to tests.
> +webkit.org/b/110501 editing/spelling/spelling-changed-text.html [ Skip ]

This one also looks like Failure: http://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release%20WK2%20%28Tests%29/builds/4487/steps/layout-test/logs/stdio

[Tony Chang]

(In reply to comment #3)
> Are other platforms ok?

You can see the bots here: http://build.webkit.org/ .  Find me on IRC if you want some help on how to find the specific links I linked to.

[Rouslan Solomakhin]

Created attachment 189612 [details]
Patch

[Tony Chang]

Comment on attachment 189612 [details]
Patch

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

The change to editing/spelling/spelling-changed-text.html looks good.

> LayoutTests/platform/efl/TestExpectations:1824
> -webkit.org/b/108370 editing/spelling/spelling-with-underscore-selection.html [ Skip ]
> -webkit.org/b/108370 editing/spelling/spelling-with-whitespace-selection.html [ Skip ]
> +webkit.org/b/108370 editing/spelling/spelling-double-clicked-word.html [ Failure ]
> +webkit.org/b/108370 editing/spelling/spelling-double-clicked-word-with-underscores.html [ Failure ]
> +webkit.org/b/108370 editing/spelling/spelling-exactly-selected-multiple-words.html [ Failure ]

I would make the Skip -> Failure change in a separate patch.  My only worry is that some of these tests are timing out rather than failing, and in that case, we probably don't want to run the test.

[Rouslan Solomakhin]

Created attachment 189629 [details]
Patch

[Rouslan Solomakhin]

Comment on attachment 189629 [details]
Patch

Separated the change from [ Skip ] to [ Failure ] into a different patch for landing later.

[WebKit Review Bot]

Comment on attachment 189629 [details]
Patch

Clearing flags on attachment: 189629

Committed r143668: <http://trac.webkit.org/changeset/143668>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Zoltan Arvai]

On Qt and GTK have this error now:

--- /ramdisk/qt-linux-release/build/layout-test-results/editing/spelling/spelling-changed-text-expected.txt
+++ /ramdisk/qt-linux-release/build/layout-test-results/editing/spelling/spelling-changed-text-actual.txt
@@ -3,8 +3,8 @@
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
 
 
-PASS window.getSelection().toString() is " Is it broken?"
+FAIL Spellcheck timed out
 PASS successfullyParsed is true
 
 TEST COMPLETE
-
+Spell wellcome. Is it broken?

[Zoltan Arvai]

Skipped on Qt in http://trac.webkit.org/changeset/143716.

Probably we should reopen this bug, but for some reason i can't.

[Zoltan Arvai]

Changing to resolved -> unconfirmed -> new to reopen.

[Rouslan Solomakhin]

(In reply to comment #13)
> On Qt and GTK have this error now:
> 
> --- /ramdisk/qt-linux-release/build/layout-test-results/editing/spelling/spelling-changed-text-expected.txt
> +++ /ramdisk/qt-linux-release/build/layout-test-results/editing/spelling/spelling-changed-text-actual.txt
> @@ -3,8 +3,8 @@
>  On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
> 
> 
> -PASS window.getSelection().toString() is " Is it broken?"
> +FAIL Spellcheck timed out
>  PASS successfullyParsed is true
> 
>  TEST COMPLETE
> -
> +Spell wellcome. Is it broken?

Timeout happens because Qt and GTK do not mark "wellcome" as misspelled. Which word will Qt and GTK mark as misspelled?

[Rouslan Solomakhin]

How come Qt and GTK EWS is green for the patch?

[Rouslan Solomakhin]

Browsing through Tools/DumpRenderTree/qt and Tools/DumpRenderTree/gtk, it seems that Qt and Gtk will not mark any words misspelled. I am marking editing/spelling/spelling-changed-text.html as [ Failure ]. Sounds good?

[Zoltan Arvai]

Sounds ok for me on Qt.

[Rouslan Solomakhin]

<rouslan> mrobinson: qt folks want to disable my test on their platforms. should i do the same on gtk?
<mrobinson> rouslan: Were you thinking of using a different word? If you like I can test it locally to ensure that it passes.
<rouslan> mrobinson: that would be lovely! here's a list of a few words: "foo", "baz", "chello", "zz"
 mrobinson: i am reading from Tools/DumpRenderTree/chromium/TestRunner/src/MockSpellCheck.cpp basically 
<mrobinson> rouslan: I'll compile ToT and run the test locally. It might take 30 minutes or so.
<rouslan> mrobinson: thank you! please take your time

[Rouslan Solomakhin]

<mrobinson> rouslan: I'm fine with skipping it for now, but it worries me that the test seems to fail so readily on other platforms

[Rouslan Solomakhin]

Created attachment 189799 [details]
Patch

[Rouslan Solomakhin]

Comment on attachment 189799 [details]
Patch

This patch disables editing/spelling/spelling-changed-text.html on Qt and GTK platforms.

Note that bug http://webkit.org/b/108370 is for making sure that the same tests can pass on all platforms.

[WebKit Review Bot]

Comment on attachment 189799 [details]
Patch

Rejecting attachment 189799 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-03', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 189799, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:

fatal: read error: Connection reset by peer
Died at Tools/Scripts/update-webkit line 151.

Failed to run "['Tools/Scripts/update-webkit', '--chromium', '--force-update']" exit_code: 2

Updating OpenSource
fatal: read error: Connection reset by peer
Died at Tools/Scripts/update-webkit line 151.

Failed to run "['Tools/Scripts/update-webkit', '--chromium', '--force-update']" exit_code: 2
Updating OpenSource
fatal: read error: Connection reset by peer
Died at Tools/Scripts/update-webkit line 151.

Full output: http://queues.webkit.org/results/16709249

[Rouslan Solomakhin]

Commit queue denied because of failure to run "update-webkit". Was the git server down?

[WebKit Review Bot]

Comment on attachment 189799 [details]
Patch

Clearing flags on attachment: 189799

Committed r143788: <http://trac.webkit.org/changeset/143788>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Roger Fong]

This test also fails on AppleWin port.
skipped: r144516

[Rouslan Solomakhin]

(In reply to comment #28)
> This test also fails on AppleWin port.
> skipped: r144516

Roger: Should we open a different ticket to expect this test to fail on AppleWin?

[Roger Fong]

(In reply to comment #29)
> (In reply to comment #28)
> > This test also fails on AppleWin port.
> > skipped: r144516
> 
> Roger: Should we open a different ticket to expect this test to fail on AppleWin?

Don't really know the history of this bug but the results are:
-PASS window.getSelection().toString() is " Is it broken?"
+FAIL Spellcheck timed out
 PASS successfullyParsed is true

 TEST COMPLETE
-
+Spell cheher. Is it broken?

Was the fix to implement something in the test runner?
If all the other ports are now fixed then I guess we should file a separate bug. If all the ports also skipped this test then I think we should just add AppleWin to the title of this bug.

[Rouslan Solomakhin]

(In reply to comment #30)
> +Spell cheher. Is it broken?

This should have changed to "Spell wellcome. Is it broken?" Spellcheck times out when "cheher" is not marked as a misspelling, because some test runners do not mark it as a misspelling. All test runners should mark the word "wellcome" as a misspelling. I think. Qt and Gtk layout tests still expect this test to fail, however, because the test uses some features that are not implemented in Qt and Gtk test runners yet. For example, the test uses asynchronous spellcheck.

[Grzegorz Czajkowski]

Unskipped for EFL at http://trac.webkit.org/changeset/146189