Bug 60862 – Convert api tester over to using gtest expectations directly

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sam Weinig]

Convert api tester over to using gtest expectations directly

[Sam Weinig]

Created attachment 93595 [details]
Patch

[Sam Weinig]

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).

[WebKit Review Bot]

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.

[Sam Weinig]

Created attachment 93597 [details]
Patch

[WebKit Review Bot]

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.

[Darin Adler]

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!

[David Levin]

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.)

[David Levin]

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

[Sam Weinig]

Created attachment 93665 [details]
Patch

[Sam Weinig]

This version fixes the expected/actual ordering issue and adds NULL/NON_NULL macros for convenience.

[David Levin]

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)

[Sam Weinig]

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

[Sam Weinig]

Committed r86602: <http://trac.webkit.org/changeset/86602>