Bug 46953 – Add simple API tester for WebKit2

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

[Sam Weinig]

Add simple API tester for WebKit2.

[Sam Weinig]

Created attachment 69411 [details]
Patch

[Sam Weinig]

<rdar://problem/8235234>

[WebKit Review Bot]

Attachment 69411 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKitTools/TestWebKitAPI/TestWebKitAPIPrefix.h:29:  Alphabetical sorting problem.  [build/include_order] [4]
WebKitTools/TestWebKitAPI/TestsController.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKitTools/TestWebKitAPI/Test.h:80:  TEST_CLASS_NAME is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
WebKitTools/TestWebKitAPI/Tests/WebKit2/BasicTest1.cpp:26:  Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKitTools/TestWebKitAPI/Tests/WebKit2/BasicTest1.cpp:31:  Alphabetical sorting problem.  [build/include_order] [4]
WebKitTools/TestWebKitAPI/Tests/WTF/Vector1.cpp:26:  Found other header before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
WebKitTools/TestWebKitAPI/Tests/WTF/Vector1.cpp:36:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebKitTools/TestWebKitAPI/Tests/WTF/Vector1.cpp:37:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 8 in 46 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

Created attachment 69413 [details]
Updated patch

[Adam Roben (:aroben)]

Comment on attachment 69413 [details]
Updated patch

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

I didn't look at the Perl very closely.

When should we integrate this into run-webkit-tests?

> WebKitTools/ChangeLog:24
> +        * TestWebKitAPI: Added.

Maybe TestWebKit2API would be better?

> WebKitTools/TestWebKitAPI/PlatformWebView.h:30
> +#if __APPLE__
> +#if __OBJC__

#ifdef is better if you want to turn on the "warn for undefined macros" warning someday.

> WebKitTools/TestWebKitAPI/PlatformWebView.h:38
> +typedef WKView* PlatformWKView;
> +typedef NSWindow* PlatformWindow;

Misplaced *s.

> WebKitTools/TestWebKitAPI/PlatformWebView.h:52
> +    PlatformWKView platformView() { return m_view; }

Can be a const member function.

> WebKitTools/TestWebKitAPI/ForwardingHeaders/wtf/ASCIICType.h:1
> +#include <JavaScriptCore/ASCIICType.h>

Can we just use WebCore's forwarding headers instead? They're copied into WebCore's PrivateHeaders.

> WebKitTools/TestWebKitAPI/Tests/WTF/Vector1.cpp:38
> +TEST(Vector1)
> +{
> +    Vector<int> intVector;
> +    TEST_ASSERT(intVector.isEmpty());
> +    TEST_ASSERT(intVector.size() == 0);
> +    TEST_ASSERT(intVector.capacity() == 0);
> +}

WTF isn't really part of the WebKit2 API. Why are we testing it here?

> WebKitTools/TestWebKitAPI/Tests/WebKit2/BasicTest1.cpp:52
> +    State* state = ((State*)clientInfo);

Extra parentheses here. And static_cast/const_cast would maybe be better.

> WebKitTools/TestWebKitAPI/Tests/WebKit2/BasicTest1.cpp:57
> +    TEST_ASSERT(state->didDecidePolicyForNavigationAction);
> +    TEST_ASSERT(!state->didCommitLoadForFrame);
> +
> +    state->didStartProvisionalLoadForFrame = true;

Should we assert that didStartProvisionalLoadForFrame is false before this?

An enum for the states would be nicer, since we always expect them to be called in the same order. It would make the order clearer!

> WebKitTools/TestWebKitAPI/Tests/WebKit2/BasicTest1.cpp:107
> +    PlatformWebView webView(pageNamespace.get());

Having PlatformWebView both here and in WebKitTestRunner makes me think that getting the view from the page (rather than vice-versa) might be better overall.

[Sam Weinig]

(In reply to comment #5)
> (From update of attachment 69413 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=69413&action=review
> 
> I didn't look at the Perl very closely.
> 
> When should we integrate this into run-webkit-tests?
> 
> > WebKitTools/ChangeLog:24
> > +        * TestWebKitAPI: Added.
> 
> Maybe TestWebKit2API would be better?

There is not much in here that has to be WebKit2 specific.  I have even provided a test case for WTF/Vector.h.

> 
> > WebKitTools/TestWebKitAPI/PlatformWebView.h:30
> > +#if __APPLE__
> > +#if __OBJC__
> 
> #ifdef is better if you want to turn on the "warn for undefined macros" warning someday.
> 
> > WebKitTools/TestWebKitAPI/PlatformWebView.h:38
> > +typedef WKView* PlatformWKView;
> > +typedef NSWindow* PlatformWindow;
> 
> Misplaced *s.
> 

Done.

> > WebKitTools/TestWebKitAPI/PlatformWebView.h:52
> > +    PlatformWKView platformView() { return m_view; }
> 
> Can be a const member function.

Done.

> 
> > WebKitTools/TestWebKitAPI/ForwardingHeaders/wtf/ASCIICType.h:1
> > +#include <JavaScriptCore/ASCIICType.h>
> 
> Can we just use WebCore's forwarding headers instead? They're copied into WebCore's PrivateHeaders.
> 

I am not sure how to do that. If that works, we should change both WebKitTestRunner and this.


> > WebKitTools/TestWebKitAPI/Tests/WTF/Vector1.cpp:38
> > +TEST(Vector1)
> > +{
> > +    Vector<int> intVector;
> > +    TEST_ASSERT(intVector.isEmpty());
> > +    TEST_ASSERT(intVector.size() == 0);
> > +    TEST_ASSERT(intVector.capacity() == 0);
> > +}
> 
> WTF isn't really part of the WebKit2 API. Why are we testing it here?

See above.

> 
> > WebKitTools/TestWebKitAPI/Tests/WebKit2/BasicTest1.cpp:52
> > +    State* state = ((State*)clientInfo);
> 
> Extra parentheses here. And static_cast/const_cast would maybe be better.

Done.

> > WebKitTools/TestWebKitAPI/Tests/WebKit2/BasicTest1.cpp:57
> > +    TEST_ASSERT(state->didDecidePolicyForNavigationAction);
> > +    TEST_ASSERT(!state->didCommitLoadForFrame);
> > +
> > +    state->didStartProvisionalLoadForFrame = true;
> 
> Should we assert that didStartProvisionalLoadForFrame is false before this?

Done.

> > WebKitTools/TestWebKitAPI/Tests/WebKit2/BasicTest1.cpp:107
> > +    PlatformWebView webView(pageNamespace.get());
> 
> Having PlatformWebView both here and in WebKitTestRunner makes me think that getting the view from the page (rather than vice-versa) might be better overall.

Perhaps.

[Sam Weinig]

Oops, didn't mean to close this.

[Sam Weinig]

Landed in r68910.

[WebKit Review Bot]

http://trac.webkit.org/changeset/68910 might have broken GTK Linux 32-bit Debug