Bug 60862

Summary: Convert api tester over to using gtest expectations directly
Product: WebKit Reporter: Sam Weinig <sam>
Component: New BugsAssignee: Sam Weinig <sam>
Status: RESOLVED FIXED    
Severity: Normal CC: dslomov, levin, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch levin: review+

Description Sam Weinig 2011-05-15 17:42:58 PDT
Convert api tester over to using gtest expectations directly
Comment 1 Sam Weinig 2011-05-15 17:45:30 PDT
Created attachment 93595 [details]
Patch
Comment 2 Sam Weinig 2011-05-15 17:46:59 PDT
I am new to this gtest thing, so I am not sure how idiomatic these changes are.  I couldn't find a good way to state the expectation that a pointer was not null for instance, so I am just using EXPECT_TRUE(pointer).
Comment 3 WebKit Review Bot 2011-05-15 17:47:23 PDT
Attachment 93595 [details] did not pass style-queue:

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

Tools/TestWebKitAPI/PlatformUtilities.h:57:  The parameter name "string" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Sam Weinig 2011-05-15 17:49:14 PDT
Created attachment 93597 [details]
Patch
Comment 5 WebKit Review Bot 2011-05-15 17:51:09 PDT
Attachment 93597 [details] did not pass style-queue:

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

Tools/TestWebKitAPI/PlatformUtilities.h:57:  The parameter name "string" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Darin Adler 2011-05-15 20:20:33 PDT
Comment on attachment 93597 [details]
Patch

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

> Tools/TestWebKitAPI/PlatformUtilities.h:58
>  std::string toSTD(WKStringRef string);
> +std::string toSTD(WKRetainPtr<WKStringRef> string);
> +std::string toSTD(const char* string);

That StyleQueue is so smart. We should remove those names!
Comment 7 David Levin 2011-05-15 21:46:53 PDT
Comment on attachment 93597 [details]
Patch

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

> Tools/TestWebKitAPI/PlatformUtilities.h:63
> +static inline ::testing::AssertionResult assertWKStrigEqual(const char* expected_expression, const char* actual_expression, T expected, U actual)

typo: Strig

I'm surprised that EXPECT_STREQ just didn't work but this is perfect.

> Tools/TestWebKitAPI/Tests/WebKit2/AboutBlankLoad.cpp:38
> +    EXPECT_WK_STREQ(Util::MIMETypeForWKURLResponse(response), "text/html");

This is fine. The one odd thing about this way of doing it is that your expected and actual are reversed, so if there is a mismatch, then it will say
EXPECT: "foo"
ACTUAL: "text/html"

I always find the ordering of this to be odd, but I think it was done to be consistent with what other pre-existing test frameworks were doing like JUnit.

(This ordering issue to true throughout these changes.)
Comment 8 David Levin 2011-05-15 21:49:11 PDT
(In reply to comment #2)
> I couldn't find a good way to state the expectation that a pointer was not null for instance, so I am just using EXPECT_TRUE(pointer).

That's perfect. I think it was debated at one point about adding something specifically for this but I think they didn't see enough value over just plain EXPECT_TRUE. (Actually, I'm not 100% sure of the reasons, but I could probably find out why there isn't a specific EXPECT for this if you want to know.)
Comment 9 Sam Weinig 2011-05-16 10:26:29 PDT
Created attachment 93665 [details]
Patch
Comment 10 Sam Weinig 2011-05-16 10:27:46 PDT
This version fixes the expected/actual ordering issue and adds NULL/NON_NULL macros for convenience.
Comment 11 David Levin 2011-05-16 10:37:40 PDT
Comment on attachment 93665 [details]
Patch

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

> Tools/TestWebKitAPI/PlatformUtilities.h:63
> +static inline ::testing::AssertionResult assertWKStrigEqual(const char* expected_expression, const char* actual_expression, T expected, U actual)

typo: strig

I'm surprised that EXPECT_STREQ doesn't work for you. (I see you used it with char* inside of TEST(WebKit2, WKString)
Comment 12 Sam Weinig 2011-05-16 11:36:03 PDT
(In reply to comment #11)
> (From update of attachment 93665 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=93665&action=review
> 
> > Tools/TestWebKitAPI/PlatformUtilities.h:63
> > +static inline ::testing::AssertionResult assertWKStrigEqual(const char* expected_expression, const char* actual_expression, T expected, U actual)
> 
> typo: strig
> 
> I'm surprised that EXPECT_STREQ doesn't work for you. (I see you used it with char* inside of TEST(WebKit2, WKString)

EXPECT_STREQ only works with char*, the idea of this is you can pass a WKStringRef or a WKRetainPtr<WKStringRef> as either parameter.
Comment 13 Sam Weinig 2011-05-16 13:28:25 PDT
Committed r86602: <http://trac.webkit.org/changeset/86602>