Bug 59561 - Switch TestWebKitAPI to GTest
Summary: Switch TestWebKitAPI to GTest
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 60522 60533
Blocks:
  Show dependency tree / graph
 
Reported: 2011-04-26 16:57 PDT by Dmitry Lomov
Modified: 2011-05-11 16:53 PDT (History)
8 users (show)

See Also:

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 an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Dmitry Lomov 2011-04-26 16:57:55 PDT 
Placeholder bug capturing the experiment of moving TestWebKitAPI over to GTest
Comment 1 Dmitry Lomov 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!
Comment 2 Dmitry Lomov 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.
Comment 3 Sam Weinig 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.
Comment 4 Dmitry Lomov 2011-05-06 14:32:52 PDT 
Created attachment 92640 [details]
TestWebKitAPI switched to GTest for Mac and windows
Comment 5 WebKit Commit Bot 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
Comment 6 Dmitry Lomov 2011-05-09 16:24:43 PDT 
Created attachment 92881 [details]
Patch rebased
Comment 7 Dmitry Lomov 2011-05-09 16:50:54 PDT 
Created attachment 92886 [details]
Added Sam as a reviewer to rebased patch
Comment 8 WebKit Commit Bot 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
Comment 9 David Levin 2011-05-09 17:19:03 PDT 
Committed as http://trac.webkit.org/changeset/86108
Comment 10 WebKit Review Bot 2011-05-09 17:50:43 PDT 
http://trac.webkit.org/changeset/86108 might have broken WinCairo Debug (Build)
Comment 11 David Levin 2011-05-09 17:58:45 PDT 
Rolled out due to Cairo build break.
Comment 12 Sam Weinig 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.
Comment 13 Sam Weinig 2011-05-09 22:24:59 PDT 
Actually rolled out in r86130.
Comment 14 Sam Weinig 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.
Comment 15 David Levin 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.
Comment 16 Dmitry Lomov 2011-05-10 16:30:07 PDT 
Created attachment 93043 [details]
Updated patch

- Fixes Makefiles
- Removes MacOSX 10.4 SDK dependency
- Fixes wincairo
Comment 17 Dmitry Lomov 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
Comment 18 Adam Roben (:aroben) 2011-05-11 06:41:15 PDT 
Comment on attachment 93047 [details]
Updated patch

Don't forget about TestWebKitAPI/win/main.cpp!
Comment 19 Dmitry Lomov 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.
Comment 20 Adam Roben (:aroben) 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.
Comment 21 Dmitry Lomov 2011-05-11 09:50:34 PDT 
Created attachment 93135 [details]
Brings parity to Mac and Linux versions of TestWebKitAPI executable
Comment 22 WebKit Commit Bot 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
Comment 23 David Levin 2011-05-11 16:53:10 PDT 
Committed as http://trac.webkit.org/changeset/86287