Summary: | Convert api tester over to using gtest expectations directly | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sam Weinig <sam> | ||||||||
Component: | New Bugs | Assignee: | 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
Sam Weinig
2011-05-15 17:42:58 PDT
Created attachment 93595 [details]
Patch
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). 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.
Created attachment 93597 [details]
Patch
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 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 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.) (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.) Created attachment 93665 [details]
Patch
This version fixes the expected/actual ordering issue and adds NULL/NON_NULL macros for convenience. 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) (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. Committed r86602: <http://trac.webkit.org/changeset/86602> |