Bug 32216 - [Qt] Improve the autotests of QtWebkit
Summary: [Qt] Improve the autotests of QtWebkit
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Benjamin Poulain
URL:
Keywords: Qt
Depends on: 31590 32222 32280 32491
Blocks:
  Show dependency treegraph
 
Reported: 2009-12-07 02:34 PST by Benjamin Poulain
Modified: 2010-01-21 01:02 PST (History)
10 users (show)

See Also:


Attachments
Refactor tst_qgraphicswebview (3.82 KB, patch)
2009-12-07 04:36 PST, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Refactor tst_qgraphicswebview (3.95 KB, patch)
2009-12-07 05:05 PST, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Refactor tst_qgraphicswebview (3.93 KB, patch)
2009-12-07 05:24 PST, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Refactor the test of QWebElement (2.59 KB, patch)
2009-12-07 05:30 PST, Benjamin Poulain
kenneth: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Remove all the qWait() of the test of QWebFrame (5.75 KB, patch)
2009-12-07 05:47 PST, Benjamin Poulain
kenneth: review-
Details | Formatted Diff | Diff
Remove all the qWait() of the test of QWebFrame and fix the comment (6.08 KB, patch)
2009-12-07 06:21 PST, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Refactor the test of QWebElement (2.59 KB, patch)
2009-12-07 06:34 PST, Benjamin Poulain
kenneth: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Update the tests of QWebView to avoid the calls to qWait() (1.65 KB, patch)
2009-12-07 06:47 PST, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Update the test of QWebElement to avoid idle time of the tests (2.59 KB, patch)
2009-12-08 23:27 PST, Benjamin Poulain
no flags Details | Formatted Diff | Diff
Update the test of QWebPage to avoid qWait() (6.73 KB, patch)
2009-12-16 05:03 PST, Benjamin Poulain
hausmann: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Update the test of QWebPage to avoid qWait() (6.70 KB, patch)
2010-01-07 06:03 PST, Benjamin Poulain
hausmann: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Poulain 2009-12-07 02:34:42 PST
A significant amount of time is spend in the Qt autotest. Some of this time is lost in numerous "QTest::qWait(200)".

To increase the number of tests run per days, we should refactor the auto-test to use the latest addition of the testlib of Qt 4.6.
Comment 1 Benjamin Poulain 2009-12-07 04:36:11 PST
Created attachment 44402 [details]
Refactor tst_qgraphicswebview

Refactor tst_qgraphicswebview, gain a few millisecons from the qWait().
Comment 2 WebKit Review Bot 2009-12-07 04:38:21 PST
Attachment 44402 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/qt/tests/util.h:23:  Alphabetical sorting problem.  [build/include_order] [4]
WebKit/qt/tests/qgraphicswebview/tst_qgraphicswebview.cpp:26:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 2
Comment 3 Benjamin Poulain 2009-12-07 05:05:27 PST
Created attachment 44403 [details]
Refactor tst_qgraphicswebview

Change the include order of util.h.
I have no idea why it complains about the include order of the test itself.
Comment 4 WebKit Review Bot 2009-12-07 05:09:13 PST
Attachment 44403 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/qt/tests/qgraphicswebview/tst_qgraphicswebview.cpp:20:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1
Comment 5 Benjamin Poulain 2009-12-07 05:24:01 PST
Created attachment 44405 [details]
Refactor tst_qgraphicswebview

There is no way to please the checker, we don't need config.h or a main header here.
Comment 6 WebKit Review Bot 2009-12-07 05:24:58 PST
Attachment 44405 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/qt/tests/qgraphicswebview/tst_qgraphicswebview.cpp:20:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1
Comment 7 Benjamin Poulain 2009-12-07 05:30:35 PST
Created attachment 44406 [details]
Refactor the test of QWebElement
Comment 8 WebKit Review Bot 2009-12-07 05:35:24 PST
Attachment 44406 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/qt/tests/qwebelement/tst_qwebelement.cpp:21:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1
Comment 9 Benjamin Poulain 2009-12-07 05:47:04 PST
Created attachment 44408 [details]
Remove all the qWait() of the test of QWebFrame

With the patch, the test run in 3s instead of 8s :)
No way to reduce more, we are limited by cursorFlashTime().
Comment 10 WebKit Review Bot 2009-12-07 05:50:55 PST
style-queue ran check-webkit-style on attachment 44408 [details] without any errors.
Comment 11 Kenneth Rohde Christiansen 2009-12-07 06:08:01 PST
Comment on attachment 44408 [details]
Remove all the qWait() of the test of QWebFrame


>      // triple the flashing time, should at least blink twice already
> -    int delay = qApp->cursorFlashTime() * 3;
> +    int delay = qApp->cursorFlashTime() * 2;

Now the comment doesn't fit with the actual code.
Comment 12 WebKit Commit Bot 2009-12-07 06:13:53 PST
Comment on attachment 44405 [details]
Refactor tst_qgraphicswebview

Clearing flags on attachment: 44405

Committed r51761: <http://trac.webkit.org/changeset/51761>
Comment 13 WebKit Commit Bot 2009-12-07 06:20:49 PST
Comment on attachment 44406 [details]
Refactor the test of QWebElement

Rejecting patch 44406 from commit-queue.

Failed to parse ChangeLog: /Users/eseidel/Projects/CommitQueue/WebKit/qt/ChangeLog
Comment 14 Benjamin Poulain 2009-12-07 06:21:24 PST
Created attachment 44409 [details]
Remove all the qWait() of the test of QWebFrame and fix the comment

Nice catch Kenneth, I overlooked the comments.
Comment 15 WebKit Review Bot 2009-12-07 06:21:51 PST
style-queue ran check-webkit-style on attachment 44409 [details] without any errors.
Comment 16 Benjamin Poulain 2009-12-07 06:34:13 PST
Created attachment 44411 [details]
Refactor the test of QWebElement

The previous patch has been rejected by the commit bot. The only error I found is a whitespace at the end of a line. Here is the same patch without the whitespace. (Useless if you integrate 44406 manually).
Comment 17 WebKit Commit Bot 2009-12-07 06:34:57 PST
Comment on attachment 44409 [details]
Remove all the qWait() of the test of QWebFrame and fix the comment

Clearing flags on attachment: 44409

Committed r51763: <http://trac.webkit.org/changeset/51763>
Comment 18 WebKit Review Bot 2009-12-07 06:37:21 PST
Attachment 44411 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/qt/tests/qwebelement/tst_qwebelement.cpp:21:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1
Comment 19 Benjamin Poulain 2009-12-07 06:47:40 PST
Created attachment 44413 [details]
Update the tests of QWebView to avoid the calls to qWait()

The time taken by the test is ~6s instead of 18s.
Comment 20 Benjamin Poulain 2009-12-07 06:50:13 PST
There is one more test we need to update before we can close this task : tst_QWebPage.

We cannot do it right now because there is a regression in this test suite: tst_QWebPage::requestCache() fails.

As soon as we fix this test, we can update it.
Comment 21 WebKit Review Bot 2009-12-07 06:52:46 PST
style-queue ran check-webkit-style on attachment 44413 [details] without any errors.
Comment 22 WebKit Commit Bot 2009-12-07 07:24:49 PST
Comment on attachment 44411 [details]
Refactor the test of QWebElement

Rejecting patch 44411 from commit-queue.

Failed to parse ChangeLog: /Users/eseidel/Projects/CommitQueue/WebKit/qt/ChangeLog
Comment 23 WebKit Commit Bot 2009-12-07 07:31:49 PST
Comment on attachment 44413 [details]
Update the tests of QWebView to avoid the calls to qWait()

Clearing flags on attachment: 44413

Committed r51767: <http://trac.webkit.org/changeset/51767>
Comment 24 Eric Seidel (no email) 2009-12-07 11:30:51 PST
Comment on attachment 44411 [details]
Refactor the test of QWebElement

Not sure why the ChangeLog parse failed.  Lets roll again.  If it fails again we'll need to file a bug against bugzilla-tool
Comment 25 WebKit Commit Bot 2009-12-07 11:43:51 PST
Comment on attachment 44411 [details]
Refactor the test of QWebElement

Rejecting patch 44411 from commit-queue.

Failed to parse ChangeLog: /Users/eseidel/Projects/CommitQueue/WebKit/qt/ChangeLog
Comment 26 Eric Seidel (no email) 2009-12-08 12:02:14 PST
Comment on attachment 44411 [details]
Refactor the test of QWebElement

Looks like there is a trailing space on the date line.
+2009-12-07  Benjamin Poulain  <benjamin.poulain@nokia.com> 

I expect that's why it's failing to parse it.  We could make it have a smarter parser.
Comment 27 Benjamin Poulain 2009-12-08 23:27:45 PST
Created attachment 44512 [details]
Update the test of QWebElement to avoid idle time of the tests

The previous patch was rejected because of a space at the end of the first line of the changelog. This is the same patch without this space character.
Comment 28 Benjamin Poulain 2009-12-08 23:29:19 PST
(In reply to comment #26)
> (From update of attachment 44411 [details])
> Looks like there is a trailing space on the date line.
> +2009-12-07  Benjamin Poulain  <benjamin.poulain@nokia.com> 
> 
> I expect that's why it's failing to parse it.  We could make it have a smarter
> parser.

I have updated the patch to remove this trailing space. I need to be more careful when looking at the changelog with vim... :)
Comment 29 WebKit Review Bot 2009-12-08 23:32:36 PST
Attachment 44512 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/qt/tests/qwebelement/tst_qwebelement.cpp:21:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1
Comment 30 Benjamin Poulain 2009-12-08 23:32:41 PST
Comment on attachment 44413 [details]
Update the tests of QWebView to avoid the calls to qWait()

My bad, this patch is already in.
Comment 31 piet 2009-12-09 16:50:53 PST
I get a build error on Mac 10.6 after your commit in tst_qwebframe.cpp:

/Users/psaslawsky/WebKit/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:2722: error: ‘qWaitForWindowShown’ is not a member of ‘QTest’

I have qt4-mac v4.5.3 from DarwinPorts. Should I use qt4-mac-dev v4.6.0 instead?
Comment 32 Benjamin Poulain 2009-12-09 23:22:39 PST
(In reply to comment #31)
> I get a build error on Mac 10.6 after your commit in tst_qwebframe.cpp:
> 
> /Users/psaslawsky/WebKit/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:2722:
> error: ‘qWaitForWindowShown’ is not a member of ‘QTest’
> 
> I have qt4-mac v4.5.3 from DarwinPorts. Should I use qt4-mac-dev v4.6.0
> instead?

Yes, those commits use a new feature of Qt TestLib: http://doc.trolltech.com/4.6/qtest.html#qWaitForWindowShown
Comment 33 piet 2009-12-10 14:00:28 PST
(In reply to comment #32)
> Yes, those commits use a new feature of Qt TestLib:
> http://doc.trolltech.com/4.6/qtest.html#qWaitForWindowShown

Ok, I updated the page at https://trac.webkit.org/wiki/BuildingQtOnOSX to require 4.6 instead.
Comment 34 Eric Seidel (no email) 2009-12-11 15:54:55 PST
Comment on attachment 44512 [details]
Update the test of QWebElement to avoid idle time of the tests

Make sure to mark you patch attachments as "text/plain" and check the "patch" checkbox.   Otherwise you can use tools like "bugzilla-tool post-diff" or "bugzilla-tool post-commits" which will do that for you (see "bugzilla-tool help" for more info).
Comment 35 Eric Seidel (no email) 2009-12-11 15:55:54 PST
Comment on attachment 44512 [details]
Update the test of QWebElement to avoid idle time of the tests

I'm confused by waitForSignal being removed here?
Comment 36 Antonio Gomes 2009-12-12 04:53:17 PST
(In reply to comment #32)
> (In reply to comment #31)
> > I get a build error on Mac 10.6 after your commit in tst_qwebframe.cpp:
> > 
> > /Users/psaslawsky/WebKit/WebKit/qt/tests/qwebframe/tst_qwebframe.cpp:2722:
> > error: ‘qWaitForWindowShown’ is not a member of ‘QTest’
> > 
> > I have qt4-mac v4.5.3 from DarwinPorts. Should I use qt4-mac-dev v4.6.0
> > instead?
> 
> Yes, those commits use a new feature of Qt TestLib:
> http://doc.trolltech.com/4.6/qtest.html#qWaitForWindowShown

so qt autotests wont work with qt 4.5.x anymore ?
Comment 37 Kenneth Rohde Christiansen 2009-12-12 07:38:32 PST
We support the current Qt version plus the one in development, thus
4.6 and 4.7.

For our last release we removed support for 4.4 if you remember.

> so qt autotests wont work with qt 4.5.x anymore ?
Comment 38 Benjamin Poulain 2009-12-13 07:40:51 PST
(In reply to comment #35)
> (From update of attachment 44512 [details])
> I'm confused by waitForSignal being removed here?

I have moved waitForSignal() to WebKit/qt/tests/util.h in https://bug-32216-attachments.webkit.org/attachment.cgi?id=44405

This function was copied in each tests, it is now accessible via util.h.
Comment 39 WebKit Commit Bot 2009-12-14 12:42:48 PST
Comment on attachment 44512 [details]
Update the test of QWebElement to avoid idle time of the tests

Clearing flags on attachment: 44512

Committed r52111: <http://trac.webkit.org/changeset/52111>
Comment 40 WebKit Commit Bot 2009-12-14 12:42:55 PST
All reviewed patches have been landed.  Closing bug.
Comment 41 Benjamin Poulain 2009-12-14 13:28:25 PST
Reopening, there is one test left to fix.
Comment 42 Benjamin Poulain 2009-12-16 05:03:05 PST
Created attachment 44964 [details]
Update the test of QWebPage to avoid qWait()

This patch divide by two the time spent on this test. It still takes a lot of time (~25s), but we cannot avoid it (test of timeouts, and wait for asynchronous callback).
Comment 43 WebKit Review Bot 2009-12-16 05:05:23 PST
Attachment 44964 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:21:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1
Comment 44 Holger Freyther 2009-12-26 03:16:46 PST
(In reply to comment #43)
> Attachment 44964 [details] did not pass style-queue:
> 
> Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
> WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:21:  Found other header before
> WebCore config.h. Should be: config.h, primary header, blank line, and then
> alphabetically sorted.  [build/include_order] [4]
> Total errors found: 1

This error can be ignored as this is a test case using the public QtWebKit API.
Comment 45 Laszlo Gombos 2009-12-28 09:54:35 PST
(In reply to comment #37)
> We support the current Qt version plus the one in development, thus
> 4.6 and 4.7.
> 
> For our last release we removed support for 4.4 if you remember.

We only removed support for Qt 4.3 and older - see http://trac.webkit.org/changeset/51174.

I think we should maintain "limited support" for Qt 4.4 - see https://bugs.webkit.org/show_bug.cgi?id=30327.

As QtWebKit will start making more frequent and independent releases from Qt, I think we should change the "current Qt version plus the one in development" rule of thumb to something that relates to calender years (e.g. "Qt releases 2 years back" or so).
Comment 46 Simon Hausmann 2009-12-29 06:41:05 PST
Comment on attachment 44964 [details]
Update the test of QWebPage to avoid qWait()

Looks good to me.
Comment 47 WebKit Commit Bot 2009-12-29 06:45:11 PST
Comment on attachment 44964 [details]
Update the test of QWebPage to avoid qWait()

Rejecting patch 44964 from commit-queue.

Failed to run "['/Users/eseidel/Projects/CommitQueueSVN/WebKitTools/Scripts/svn-apply', '--reviewer', 'Simon Hausmann', '--force']" exit_code: 1
patching file WebKit/qt/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file WebKit/qt/tests/qwebpage/tst_qwebpage.cpp
Hunk #1 FAILED at 18.
Hunk #2 succeeded at 223 (offset -4 lines).
Hunk #3 succeeded at 471 (offset -4 lines).
Hunk #4 succeeded at 1550 (offset -4 lines).
Hunk #5 succeeded at 1625 (offset -4 lines).
Hunk #6 FAILED at 1718.
2 out of 6 hunks FAILED -- saving rejects to file WebKit/qt/tests/qwebpage/tst_qwebpage.cpp.rej

Full output: http://webkit-commit-queue.appspot.com/results/151527
Comment 48 Eric Seidel (no email) 2009-12-29 10:51:50 PST
Looks like the patch is out of date.  If you'd like this landed using the commit bot, we'll need an updated patch.
Comment 49 Eric Seidel (no email) 2010-01-06 23:40:26 PST
Ping?
Comment 50 Benjamin Poulain 2010-01-07 02:42:16 PST
(In reply to comment #49)
> Ping?

Sorry Eric, I'll look at that today.
Comment 51 Benjamin Poulain 2010-01-07 06:03:25 PST
Created attachment 46049 [details]
Update the test of QWebPage to avoid qWait()

Same patch, just updated so it merge with trunk
Comment 52 WebKit Review Bot 2010-01-07 06:04:23 PST
Attachment 46049 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/qt/tests/qwebpage/tst_qwebpage.cpp:21:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 1
Comment 53 Simon Hausmann 2010-01-16 00:34:06 PST
Comment on attachment 46049 [details]
Update the test of QWebPage to avoid qWait()

r=me
Comment 54 WebKit Commit Bot 2010-01-16 00:54:57 PST
Comment on attachment 46049 [details]
Update the test of QWebPage to avoid qWait()

Rejecting patch 46049 from commit-queue.

Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--reviewer', 'Simon Hausmann', '--force']" exit_code: 1
patching file WebKit/qt/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file WebKit/qt/tests/qwebpage/tst_qwebpage.cpp
Hunk #1 FAILED at 18.
Hunk #2 succeeded at 183 (offset -44 lines).
Hunk #3 succeeded at 431 (offset -44 lines).
Hunk #4 succeeded at 1510 (offset -44 lines).
Hunk #5 succeeded at 1585 (offset -44 lines).
Hunk #6 succeeded at 1681 (offset -41 lines).
1 out of 6 hunks FAILED -- saving rejects to file WebKit/qt/tests/qwebpage/tst_qwebpage.cpp.rej

Full output: http://webkit-commit-queue.appspot.com/results/190922
Comment 55 Simon Hausmann 2010-01-21 01:02:49 PST
Committed r53614: <http://trac.webkit.org/changeset/53614>