Summary: | [Qt] Improve the autotests of QtWebkit | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Benjamin Poulain <benjamin> | ||||||||||||||||||||||||
Component: | WebKit Qt | Assignee: | Benjamin Poulain <benjamin> | ||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||
Severity: | Normal | CC: | benjamin, commit-queue, eric, hausmann, kenneth, laszlo.gombos, piet.webkit, tonikitoo, webkit.review.bot, zecke | ||||||||||||||||||||||||
Priority: | P2 | Keywords: | Qt | ||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||||||
Bug Depends on: | 31590, 32222, 32280, 32491 | ||||||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||||||
Attachments: |
|
Description
Benjamin Poulain
2009-12-07 02:34:42 PST
Created attachment 44402 [details]
Refactor tst_qgraphicswebview
Refactor tst_qgraphicswebview, gain a few millisecons from the qWait().
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
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.
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
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.
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
Created attachment 44406 [details]
Refactor the test of QWebElement
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
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().
style-queue ran check-webkit-style on attachment 44408 [details] without any errors.
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 on attachment 44405 [details] Refactor tst_qgraphicswebview Clearing flags on attachment: 44405 Committed r51761: <http://trac.webkit.org/changeset/51761> 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
Created attachment 44409 [details]
Remove all the qWait() of the test of QWebFrame and fix the comment
Nice catch Kenneth, I overlooked the comments.
style-queue ran check-webkit-style on attachment 44409 [details] without any errors.
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 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> 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
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.
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. style-queue ran check-webkit-style on attachment 44413 [details] without any errors.
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 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 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 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 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. 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.
(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... :) 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 on attachment 44413 [details]
Update the tests of QWebView to avoid the calls to qWait()
My bad, this patch is already in.
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? (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 (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 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 on attachment 44512 [details]
Update the test of QWebElement to avoid idle time of the tests
I'm confused by waitForSignal being removed here?
(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 ? 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 ?
(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 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> All reviewed patches have been landed. Closing bug. Reopening, there is one test left to fix. 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).
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
(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. (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 on attachment 44964 [details]
Update the test of QWebPage to avoid qWait()
Looks good to me.
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 Looks like the patch is out of date. If you'd like this landed using the commit bot, we'll need an updated patch. Ping? (In reply to comment #49) > Ping? Sorry Eric, I'll look at that today. Created attachment 46049 [details]
Update the test of QWebPage to avoid qWait()
Same patch, just updated so it merge with trunk
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 on attachment 46049 [details]
Update the test of QWebPage to avoid qWait()
r=me
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 Committed r53614: <http://trac.webkit.org/changeset/53614> |