WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
59561
Switch TestWebKitAPI to GTest
https://bugs.webkit.org/show_bug.cgi?id=59561
Summary
Switch TestWebKitAPI to GTest
Dmitry Lomov
Reported
2011-04-26 16:57:55 PDT
Placeholder bug capturing the experiment of moving TestWebKitAPI over to GTest
Attachments
Draft attempt
(10.31 KB, patch)
2011-04-26 17:13 PDT
,
Dmitry Lomov
no flags
Details
Formatted Diff
Diff
{build,run}-api-tests fixed, project files fixed
(11.34 KB, patch)
2011-04-26 20:12 PDT
,
Dmitry Lomov
no flags
Details
Formatted Diff
Diff
TestWebKitAPI switched to GTest for Mac and windows
(29.43 KB, patch)
2011-05-06 14:32 PDT
,
Dmitry Lomov
sam
: review+
commit-queue
: commit-queue-
Details
Formatted Diff
Diff
Patch rebased
(29.67 KB, patch)
2011-05-09 16:24 PDT
,
Dmitry Lomov
no flags
Details
Formatted Diff
Diff
Added Sam as a reviewer to rebased patch
(29.66 KB, patch)
2011-05-09 16:50 PDT
,
Dmitry Lomov
commit-queue
: commit-queue-
Details
Formatted Diff
Diff
Updated patch
(39.82 KB, patch)
2011-05-10 16:30 PDT
,
Dmitry Lomov
dslomov
: review-
dslomov
: commit-queue-
Details
Formatted Diff
Diff
Updated patch
(38.48 KB, patch)
2011-05-10 16:40 PDT
,
Dmitry Lomov
no flags
Details
Formatted Diff
Diff
Brings parity to Mac and Linux versions of TestWebKitAPI executable
(37.60 KB, patch)
2011-05-11 09:50 PDT
,
Dmitry Lomov
levin
: review+
commit-queue
: commit-queue-
Details
Formatted Diff
Diff
Show Obsolete
(7)
View All
Add attachment
proposed patch, testcase, etc.
Dmitry Lomov
Comment 1
2011-04-26 17:13:48 PDT
Created
attachment 91192
[details]
Draft attempt This is a very first draft. It switches the all tests to GTest, retaining the functionality. Output format is a bit different though, and test names use '.' instead of '/' (WTF.VectorBasic instead of WTF/VectorBasic). Only builds on Mac yet. As you see, the change is very small. I have not replaced all the assertion macros, merely redefined existing ones in terms of GTest. By a lucky coincidence, the TEST macro works unchanged. Here is some work items to be done before this can be landed: 0) Clean up project files on Mac 1) Build on Windows 2) Run on bots (what is the current story with TestWebKitAPI on bots? Are those tests run on bots on any platform?) Sam, is (1) a priority for you? After this lands, we might want to make tests more GTest-idiomatic by: a) Using GTest macros throughout b) Using GTest fixtures and SetUp/TearDown instead of statics Let me know what you think!
Dmitry Lomov
Comment 2
2011-04-26 20:12:05 PDT
Created
attachment 91220
[details]
{build,run}-api-tests fixed, project files fixed Fixed scripts. I timed run-api-tests and got exact same timings with GTest as before the switch.
Sam Weinig
Comment 3
2011-04-27 15:18:00 PDT
(In reply to
comment #1
)
> Created an attachment (id=91192) [details] > Draft attempt > > This is a very first draft. It switches the all tests to GTest, retaining the functionality. Output format is a bit different though, and test names use '.' instead of '/' (WTF.VectorBasic instead of WTF/VectorBasic). > Only builds on Mac yet. > > As you see, the change is very small. I have not replaced all the assertion macros, merely redefined existing ones in terms of GTest. By a lucky coincidence, the TEST macro works unchanged. > > Here is some work items to be done before this can be landed: > 0) Clean up project files on Mac > 1) Build on Windows > 2) Run on bots (what is the current story with TestWebKitAPI on bots? Are those tests run on bots on any platform?) > > Sam, is (1) a priority for you? > > After this lands, we might want to make tests more GTest-idiomatic by: > a) Using GTest macros throughout > b) Using GTest fixtures and SetUp/TearDown instead of statics > > Let me know what you think!
Looks great, we should definitely get this working on Windows before landing, but I think the way you have prioritized so far is great.
Dmitry Lomov
Comment 4
2011-05-06 14:32:52 PDT
Created
attachment 92640
[details]
TestWebKitAPI switched to GTest for Mac and windows
WebKit Commit Bot
Comment 5
2011-05-09 16:02:42 PDT
Comment on
attachment 92640
[details]
TestWebKitAPI switched to GTest for Mac and windows Rejecting
attachment 92640
[details]
from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-4', 'apply-..." exit_code: 2 Last 500 characters of output: Configurations/TestWebKitAPICommon.vsprops patching file Tools/TestWebKitAPI/Test.h patching file Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj patching file Tools/TestWebKitAPI/Tests/WTF/VectorBasic.cpp patching file Tools/TestWebKitAPI/TestsController.cpp patching file Tools/TestWebKitAPI/TestsController.h patching file Tools/TestWebKitAPI/mac/main.mm Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Sam Weinig', u'--force']" exit_code: 1 Full output:
http://queues.webkit.org/results/8652696
Dmitry Lomov
Comment 6
2011-05-09 16:24:43 PDT
Created
attachment 92881
[details]
Patch rebased
Dmitry Lomov
Comment 7
2011-05-09 16:50:54 PDT
Created
attachment 92886
[details]
Added Sam as a reviewer to rebased patch
WebKit Commit Bot
Comment 8
2011-05-09 16:53:08 PDT
Comment on
attachment 92886
[details]
Added Sam as a reviewer to rebased patch Rejecting
attachment 92886
[details]
from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-cq-sf', 'ap..." exit_code: 2 Last 500 characters of output: ts patching file Tools/TestWebKitAPI/Configurations/TestWebKitAPICommon.vsprops patching file Tools/TestWebKitAPI/Test.h patching file Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj patching file Tools/TestWebKitAPI/Tests/WTF/VectorBasic.cpp patching file Tools/TestWebKitAPI/TestsController.cpp patching file Tools/TestWebKitAPI/TestsController.h patching file Tools/TestWebKitAPI/mac/main.mm Failed to run "[u'/Projects/CommitQueue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1 Full output:
http://queues.webkit.org/results/8655707
David Levin
Comment 9
2011-05-09 17:19:03 PDT
Committed as
http://trac.webkit.org/changeset/86108
WebKit Review Bot
Comment 10
2011-05-09 17:50:43 PDT
http://trac.webkit.org/changeset/86108
might have broken WinCairo Debug (Build)
David Levin
Comment 11
2011-05-09 17:58:45 PDT
Rolled out due to Cairo build break.
Sam Weinig
Comment 12
2011-05-09 22:10:51 PDT
(In reply to
comment #11
)
> Rolled out due to Cairo build break.
It doesn't seem to have been.
Sam Weinig
Comment 13
2011-05-09 22:24:59 PDT
Actually rolled out in
r86130
.
Sam Weinig
Comment 14
2011-05-09 22:26:27 PDT
In the next iteration, please add both gtest to the Makefile based builds (add it to ./Makefile, see the reference to Source/ThirdParty/ANGLE in that file for how to do this) and please remove the reference to the 10.4 SDK from gtest.
David Levin
Comment 15
2011-05-10 05:12:29 PDT
(In reply to
comment #12
)
> (In reply to
comment #11
) > > Rolled out due to Cairo build break. > > It doesn't seem to have been.
Thanks for doing that. Sherrifbot failed to rolled it out. I tried to do it by hand but the git repository seems to be way out of date for some reason. (Still I can only get up to 86081.) Then I talked to Brett Fulgram in irc and he was going to fix the Cairo build (or I told him he could roll this out). I didn't notice the problem in the Makefile based builds. I should have updated this bug.
Dmitry Lomov
Comment 16
2011-05-10 16:30:07 PDT
Created
attachment 93043
[details]
Updated patch - Fixes Makefiles - Removes MacOSX 10.4 SDK dependency - Fixes wincairo
Dmitry Lomov
Comment 17
2011-05-10 16:40:10 PDT
Created
attachment 93047
[details]
Updated patch - Fixes Makefiles - Fixes WinCairo build - Removes MacOS X 10.4 SDK dependency
Adam Roben (:aroben)
Comment 18
2011-05-11 06:41:15 PDT
Comment on
attachment 93047
[details]
Updated patch Don't forget about TestWebKitAPI/win/main.cpp!
Dmitry Lomov
Comment 19
2011-05-11 09:07:16 PDT
(In reply to
comment #18
)
> (From update of
attachment 93047
[details]
) > Don't forget about TestWebKitAPI/win/main.cpp!
Oh right :) It actually just works - the only difference is that it does not get the added functionality of running all tests. I feel I should add it later - it is a big patch already - but let me know if you feel this is necessary.
Adam Roben (:aroben)
Comment 20
2011-05-11 09:08:08 PDT
(In reply to
comment #19
)
> (In reply to
comment #18
) > > (From update of
attachment 93047
[details]
[details]) > > Don't forget about TestWebKitAPI/win/main.cpp! > > Oh right :) > It actually just works - the only difference is that it does not get the added functionality of running all tests. I feel I should add it later - it is a big patch already - but let me know if you feel this is necessary.
You should either add it to both or to neither. Diverging this way is confusing.
Dmitry Lomov
Comment 21
2011-05-11 09:50:34 PDT
Created
attachment 93135
[details]
Brings parity to Mac and Linux versions of TestWebKitAPI executable
WebKit Commit Bot
Comment 22
2011-05-11 16:29:20 PDT
Comment on
attachment 93135
[details]
Brings parity to Mac and Linux versions of TestWebKitAPI executable Rejecting
attachment 93135
[details]
from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-3', 'apply-..." exit_code: 2 Last 500 characters of output: -api-tests patching file Tools/TestWebKitAPI/Configurations/TestWebKitAPICommon.vsprops patching file Tools/TestWebKitAPI/Test.h patching file Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj patching file Tools/TestWebKitAPI/Tests/WTF/VectorBasic.cpp patching file Tools/TestWebKitAPI/TestsController.cpp patching file Tools/TestWebKitAPI/TestsController.h Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'David Levin', u'--force']" exit_code: 1 Full output:
http://queues.webkit.org/results/8690438
David Levin
Comment 23
2011-05-11 16:53:10 PDT
Committed as
http://trac.webkit.org/changeset/86287
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug