Bug 110447 - REGRESSION(r143470): editing/spelling/spelling-changed-text.html fails on Qt, GTK, EFL
Summary: REGRESSION(r143470): editing/spelling/spelling-changed-text.html fails on Qt,...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-02-21 02:29 PST by Zoltan Arvai
Modified: 2013-03-19 01:19 PDT (History)
10 users (show)

See Also:


Attachments
Patch (3.88 KB, patch)
2013-02-21 12:40 PST, Rouslan Solomakhin
no flags Details | Formatted Diff | Diff
Patch (15.61 KB, patch)
2013-02-21 15:16 PST, Rouslan Solomakhin
no flags Details | Formatted Diff | Diff
Patch (8.22 KB, patch)
2013-02-21 16:13 PST, Rouslan Solomakhin
no flags Details | Formatted Diff | Diff
Patch (2.75 KB, patch)
2013-02-22 11:32 PST, Rouslan Solomakhin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zoltan Arvai 2013-02-21 02:29:42 PST
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?
Comment 1 Tony Chang 2013-02-21 12:03:15 PST
We should probably skip this test on Qt, GTK, EFL.
Comment 2 Rouslan Solomakhin 2013-02-21 12:16:53 PST
Working on a patch.
Comment 3 Rouslan Solomakhin 2013-02-21 12:17:33 PST
Are other platforms ok?
Comment 4 Rouslan Solomakhin 2013-02-21 12:40:36 PST
Created attachment 189577 [details]
Patch
Comment 5 Tony Chang 2013-02-21 13:11:03 PST
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
Comment 6 Tony Chang 2013-02-21 13:11:43 PST
(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.
Comment 7 Rouslan Solomakhin 2013-02-21 15:16:54 PST
Created attachment 189612 [details]
Patch
Comment 8 Tony Chang 2013-02-21 15:40:27 PST
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.
Comment 9 Rouslan Solomakhin 2013-02-21 16:13:08 PST
Created attachment 189629 [details]
Patch
Comment 10 Rouslan Solomakhin 2013-02-21 16:13:58 PST
Comment on attachment 189629 [details]
Patch

Separated the change from [ Skip ] to [ Failure ] into a different patch for landing later.
Comment 11 WebKit Review Bot 2013-02-21 16:58:04 PST
Comment on attachment 189629 [details]
Patch

Clearing flags on attachment: 189629

Committed r143668: <http://trac.webkit.org/changeset/143668>
Comment 12 WebKit Review Bot 2013-02-21 16:58:09 PST
All reviewed patches have been landed.  Closing bug.
Comment 13 Zoltan Arvai 2013-02-22 01:07:34 PST
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?
Comment 14 Zoltan Arvai 2013-02-22 05:33:56 PST
Skipped on Qt in http://trac.webkit.org/changeset/143716.

Probably we should reopen this bug, but for some reason i can't.
Comment 15 Zoltan Arvai 2013-02-22 05:51:33 PST
Changing to resolved -> unconfirmed -> new to reopen.
Comment 16 Rouslan Solomakhin 2013-02-22 08:05:56 PST
(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?
Comment 17 Rouslan Solomakhin 2013-02-22 08:06:44 PST
How come Qt and GTK EWS is green for the patch?
Comment 18 Rouslan Solomakhin 2013-02-22 08:36:14 PST
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?
Comment 19 Zoltan Arvai 2013-02-22 08:51:29 PST
Sounds ok for me on Qt.
Comment 20 Rouslan Solomakhin 2013-02-22 09:47:37 PST
<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
Comment 21 Rouslan Solomakhin 2013-02-22 11:28:42 PST
<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
Comment 22 Rouslan Solomakhin 2013-02-22 11:32:34 PST
Created attachment 189799 [details]
Patch
Comment 23 Rouslan Solomakhin 2013-02-22 11:34:41 PST
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.
Comment 24 WebKit Review Bot 2013-02-22 13:28:11 PST
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
Comment 25 Rouslan Solomakhin 2013-02-22 13:32:42 PST
Commit queue denied because of failure to run "update-webkit". Was the git server down?
Comment 26 WebKit Review Bot 2013-02-22 13:52:34 PST
Comment on attachment 189799 [details]
Patch

Clearing flags on attachment: 189799

Committed r143788: <http://trac.webkit.org/changeset/143788>
Comment 27 WebKit Review Bot 2013-02-22 13:52:40 PST
All reviewed patches have been landed.  Closing bug.
Comment 28 Roger Fong 2013-03-01 17:04:24 PST
This test also fails on AppleWin port.
skipped: r144516
Comment 29 Rouslan Solomakhin 2013-03-01 17:27:32 PST
(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?
Comment 30 Roger Fong 2013-03-01 17:35:09 PST
(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.
Comment 31 Rouslan Solomakhin 2013-03-02 09:33:51 PST
(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.
Comment 32 Grzegorz Czajkowski 2013-03-19 01:19:40 PDT
Unskipped for EFL at http://trac.webkit.org/changeset/146189