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
{build,run}-api-tests fixed, project files fixed (11.34 KB, patch)
2011-04-26 20:12 PDT, Dmitry Lomov
no flags
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-
Patch rebased (29.67 KB, patch)
2011-05-09 16:24 PDT, Dmitry Lomov
no flags
Added Sam as a reviewer to rebased patch (29.66 KB, patch)
2011-05-09 16:50 PDT, Dmitry Lomov
commit-queue: commit-queue-
Updated patch (39.82 KB, patch)
2011-05-10 16:30 PDT, Dmitry Lomov
dslomov: review-
dslomov: commit-queue-
Updated patch (38.48 KB, patch)
2011-05-10 16:40 PDT, Dmitry Lomov
no flags
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-
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
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
Note You need to log in before you can comment on or make changes to this bug.