Bug 58509

Summary: Add a sample test case for GTest framework
Product: WebKit Reporter: Dmitry Lomov <dslomov>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, dslomov, eric, levin, timothy, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on: 58507    
Bug Blocks: 21010    
Attachments:
Description Flags
Example test case for GTest in JavaScriptCore
levin: review-
Example test case for GTest in JavaScriptCore
levin: review-
Example test case for GTest in JavaScriptCore
none
Example test case for GTest in JavaScriptCore none

Description Dmitry Lomov 2011-04-13 20:14:32 PDT
Add a sample test case and a runner for WebKit and GTest framework
Comment 1 Dmitry Lomov 2011-04-13 20:42:33 PDT
Created attachment 89523 [details]
Example test case for GTest in JavaScriptCore
Comment 2 David Levin 2011-04-14 13:27:10 PDT
Comment on attachment 89523 [details]
Example test case for GTest in JavaScriptCore

View in context: https://bugs.webkit.org/attachment.cgi?id=89523&action=review

Just a few suggestions.

> Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:3435
> +			buildSettings = {

Let's try to trim this down.

> Source/JavaScriptCore/wtf/tests/RunAllWtfTests.cpp:35
> +int main(int argc, char **argv) {

The { needs to be on the next line.

> Source/JavaScriptCore/wtf/tests/RunAllWtfTests.cpp:36
> +    std::cout << "Running main() from gtest_main.cc\n";

Avoid stream io for consistency for the rest of WebKit.

> Source/JavaScriptCore/wtf/tests/StringTests.cpp:41
> +TEST_F(CStringTest, TestLength) {

{ on next line.

> Source/ThirdParty/gtest/xcode/gtest.xcodeproj/project.pbxproj:581
> +			developmentRegion = English;

Remove this to keep us the same as upstream.
Comment 3 Dmitry Lomov 2011-04-14 14:41:07 PDT
Created attachment 89654 [details]
Example test case for GTest in JavaScriptCore

Code review feedback addressed
Comment 4 David Levin 2011-04-14 14:49:34 PDT
Comment on attachment 89654 [details]
Example test case for GTest in JavaScriptCore

View in context: https://bugs.webkit.org/attachment.cgi?id=89654&action=review

Your change is inverted. Other than that and the minor issue below. It looks fine.

> ChangeLog:194
> +2011-03-28  Adam Barth  <abarth@webkit.org>

Get rid of this change.
Comment 5 Dmitry Lomov 2011-04-14 14:54:00 PDT
Created attachment 89660 [details]
Example test case for GTest in JavaScriptCore

Reversed the diff direction + removed typo in ChangeLog - sorry about that!
Comment 6 WebKit Review Bot 2011-04-14 14:56:21 PDT
Attachment 89660 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/wtf/tests/RunAllWtfTests.cpp:31:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/JavaScriptCore/wtf/tests/RunAllWtfTests.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
Source/JavaScriptCore/wtf/tests/StringTests.cpp:32:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 3 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Dmitry Lomov 2011-04-14 15:05:19 PDT
Created attachment 89664 [details]
Example test case for GTest in JavaScriptCore

Try n+1 - addressing style issues (check-webkit-style still complains about absence of a primary headers)
Comment 8 David Levin 2011-04-14 15:08:40 PDT
Yea!
Comment 9 WebKit Review Bot 2011-04-14 15:11:09 PDT
Attachment 89664 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/JavaScriptCore/ChangeLog', u'Source..." exit_code: 1

Source/JavaScriptCore/wtf/tests/RunAllWtfTests.cpp:32:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Source/JavaScriptCore/wtf/tests/StringTests.cpp:33:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 WebKit Commit Bot 2011-04-15 07:51:41 PDT
The commit-queue encountered the following flaky tests while processing attachment 89664 [details]:

http/tests/xmlhttprequest/cross-origin-no-authorization.html bug 33357 (author: ap@webkit.org)
The commit-queue is continuing to process your patch.
Comment 11 WebKit Commit Bot 2011-04-15 07:55:35 PDT
Comment on attachment 89664 [details]
Example test case for GTest in JavaScriptCore

Clearing flags on attachment: 89664

Committed r83974: <http://trac.webkit.org/changeset/83974>
Comment 12 WebKit Commit Bot 2011-04-15 07:55:43 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Timothy Hatcher 2011-04-15 11:53:10 PDT
What is this and why do we need it?

This GTest stuff causes an error on systems that don't have $(DEVELOPER_SDK_DIR)/MacOSX10.4u.sdk. It should not have an SDK and use the current system.
Comment 14 Eric Seidel (no email) 2011-04-15 11:55:17 PDT
The desire for WebCore unit testing goes waay waay back.  See bug 21010.  That said, I know nothing about this current effort.
Comment 15 Timothy Hatcher 2011-04-15 12:07:48 PDT
Filed bug 58679