Bug 59561 – Switch TestWebKitAPI to GTest

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Dmitry Lomov]

Placeholder bug capturing the experiment of moving TestWebKitAPI over to GTest

[Dmitry Lomov]

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]

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]

(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]

Created attachment 92640 [details]
TestWebKitAPI switched to GTest for Mac and windows

[WebKit Commit Bot]

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]

Created attachment 92881 [details]
Patch rebased

[Dmitry Lomov]

Created attachment 92886 [details]
Added Sam as a reviewer to rebased patch

[WebKit Commit Bot]

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]

Committed as http://trac.webkit.org/changeset/86108

[WebKit Review Bot]

http://trac.webkit.org/changeset/86108 might have broken WinCairo Debug (Build)

[David Levin]

Rolled out due to Cairo build break.

[Sam Weinig]

(In reply to comment #11)
> Rolled out due to Cairo build break.

It doesn't seem to have been.

[Sam Weinig]

Actually rolled out in r86130.

[Sam Weinig]

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]

(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]

Created attachment 93043 [details]
Updated patch

- Fixes Makefiles
- Removes MacOSX 10.4 SDK dependency
- Fixes wincairo

[Dmitry Lomov]

Created attachment 93047 [details]
Updated patch

- Fixes Makefiles
- Fixes WinCairo build
- Removes MacOS X 10.4 SDK dependency

[Adam Roben (:aroben)]

Comment on attachment 93047 [details]
Updated patch

Don't forget about TestWebKitAPI/win/main.cpp!

[Dmitry Lomov]

(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)]

(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]

Created attachment 93135 [details]
Brings parity to Mac and Linux versions of TestWebKitAPI executable

[WebKit Commit Bot]

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]

Committed as http://trac.webkit.org/changeset/86287