Bug 84446 – [GTK] Build and run TestWebKitAPI WebKit2 unit tests

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

[Carlos Garcia Campos]

Bug #84325 adds support to build a and run TestWebKitAPI, but it only adds WTF unit tests. We should laso enable all WebKit2 tests.

[Carlos Garcia Campos]

Created attachment 138083 [details]
Patch

[Philippe Normand]

Comment on attachment 138083 [details]
Patch

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

Looks good in general but I'm afraid I missed some things, maybe Martin can do another pass? This patch is quite big :). Anyway, some questions follow:

> Tools/ChangeLog:19
> +        * TestWebKitAPI/JavaScriptTest.cpp: Use
> +        JavaScriptCore/JSContextRef.h instead of
> +        JavaScriptCore/JavaScriptCore.h since it includes JSStringRefCF.h
> +        unconditionally.

Would it make sense to fix JavaScriptCore.h instead?

> Tools/Scripts/run-gtk-tests:83
> +        SkippedTest("TestWebKitAPI/WebKit2/TestNewFirstVisuallyNonEmptyLayout",
> +                    "Test times out"),
> +        SkippedTest("TestWebKitAPI/WebKit2/TestNewFirstVisuallyNonEmptyLayoutForImages",
> +                    "Test times out"),
> +        SkippedTest("TestWebKitAPI/WebKit2/TestWKConnectionTest",
> +                    "Test times out"),
> +        SkippedTest("TestWebKitAPI/WebKit2/TestRestoreSessionStateContainingFormData",
> +                    "Session State is not implemented in GTK+ port"),
> +        SkippedTest("TestWebKitAPI/WebKit2/TestSpacebarScrolling",
> +                    "Test fails")

Can these failures be tracked in bugzilla if possible?

> Tools/Scripts/run-gtk-tests:201
> +        self._test_env["TEST_WEBKIT_API_WEBKIT2_INJECTED_BUNDLE_PATH"] = os.path.abspath(os.path.join(self._get_build_directory(), "Libraries"))

Damn I forgot the "get build directory" is duplicated between this script and common.py. It'd be great to fix that at some point, in another patch.

> Tools/Scripts/run-gtk-tests:265
> +        tester_command = [test, "--gtest_throw_on_failure"]

Maybe that change should go in the other patch?

> Tools/TestWebKitAPI/GNUmakefile.am:61
> +testwebkitapi_wtf_tests_cppflags = \

Ditto

> Tools/TestWebKitAPI/GNUmakefile.am:69
> +testwebkitapi_wtf_tests_ldadd = \

Ditto

> Tools/TestWebKitAPI/GNUmakefile.am:74
> +testwebkitapi_wtf_tests_ldflags = \

Ditto

> Tools/TestWebKitAPI/GNUmakefile.am:172
> -Programs_TestWebKitAPI_WTF_TestCheckedArithmeticOperations_CPPFLAGS = $(testwebkitapi_tests_cppflags)
> -Programs_TestWebKitAPI_WTF_TestCheckedArithmeticOperations_LDADD = $(testwebkitapi_tests_ldadd)
> -Programs_TestWebKitAPI_WTF_TestCheckedArithmeticOperations_LDFLAGS = $(testwebkitapi_tests_ldflags)
> +Programs_TestWebKitAPI_WTF_TestCheckedArithmeticOperations_CPPFLAGS = $(testwebkitapi_wtf_tests_cppflags)
> +Programs_TestWebKitAPI_WTF_TestCheckedArithmeticOperations_LDADD = $(testwebkitapi_wtf_tests_ldadd)
> +Programs_TestWebKitAPI_WTF_TestCheckedArithmeticOperations_LDFLAGS = $(testwebkitapi_wtf_tests_ldflags)

Ok I'm tired of Ditto, you get it I guess :)

> Tools/TestWebKitAPI/gtk/PlatformUtilitiesGtk.cpp:65
> +    return g_filename_to_utf8(value, -1, 0, &bytesWritten, 0);

I find it a bit strange to use g_filename_to_utf8() here. Maybe some day we'll have env variables not dealing with file paths. Or maybe it's just this function that should be renamed :)

> Tools/TestWebKitAPI/gtk/main.cpp:37
> +    bool passed = TestWebKitAPI::TestsController::shared().run(argc, argv);
> +
> +    return passed ? EXIT_SUCCESS : EXIT_FAILURE;

This can be one line maybe?

[Carlos Garcia Campos]

(In reply to comment #2)
> (From update of attachment 138083 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138083&action=review
> 
> Looks good in general but I'm afraid I missed some things, maybe Martin can do another pass? This patch is quite big :). Anyway, some questions follow:

Sure, thanks for reviewing!

> > Tools/ChangeLog:19
> > +        * TestWebKitAPI/JavaScriptTest.cpp: Use
> > +        JavaScriptCore/JSContextRef.h instead of
> > +        JavaScriptCore/JavaScriptCore.h since it includes JSStringRefCF.h
> > +        unconditionally.
> 
> Would it make sense to fix JavaScriptCore.h instead?

Well, it's a public header, I'm not sure we can change it without breaking existing apps.

> > Tools/Scripts/run-gtk-tests:83
> > +        SkippedTest("TestWebKitAPI/WebKit2/TestNewFirstVisuallyNonEmptyLayout",
> > +                    "Test times out"),
> > +        SkippedTest("TestWebKitAPI/WebKit2/TestNewFirstVisuallyNonEmptyLayoutForImages",
> > +                    "Test times out"),
> > +        SkippedTest("TestWebKitAPI/WebKit2/TestWKConnectionTest",
> > +                    "Test times out"),
> > +        SkippedTest("TestWebKitAPI/WebKit2/TestRestoreSessionStateContainingFormData",
> > +                    "Session State is not implemented in GTK+ port"),
> > +        SkippedTest("TestWebKitAPI/WebKit2/TestSpacebarScrolling",
> > +                    "Test fails")
> 
> Can these failures be tracked in bugzilla if possible?

Yes, I haven't opened bugs for them yet, because they don't actually exist. It's chicken-egg problem, so my idea was to update it once bugs are filed.

> > Tools/Scripts/run-gtk-tests:201
> > +        self._test_env["TEST_WEBKIT_API_WEBKIT2_INJECTED_BUNDLE_PATH"] = os.path.abspath(os.path.join(self._get_build_directory(), "Libraries"))
> 
> Damn I forgot the "get build directory" is duplicated between this script and common.py. It'd be great to fix that at some point, in another patch.
>

Sure.

> > Tools/Scripts/run-gtk-tests:265
> > +        tester_command = [test, "--gtest_throw_on_failure"]
> 
> Maybe that change should go in the other patch?

Yes, I realized when running wk2 tests, because wtf tests didn't fail.

> > Tools/TestWebKitAPI/GNUmakefile.am:61
> > +testwebkitapi_wtf_tests_cppflags = \
> 
> Ditto

In previous patch there are no other tests, I don't think it's a problem to changing it here, but I can do that in the other patch if you want.

> > Tools/TestWebKitAPI/GNUmakefile.am:69
> > +testwebkitapi_wtf_tests_ldadd = \
> 
> Ditto
> 
> > Tools/TestWebKitAPI/GNUmakefile.am:74
> > +testwebkitapi_wtf_tests_ldflags = \
> 
> Ditto
> 
> > Tools/TestWebKitAPI/GNUmakefile.am:172
> > -Programs_TestWebKitAPI_WTF_TestCheckedArithmeticOperations_CPPFLAGS = $(testwebkitapi_tests_cppflags)
> > -Programs_TestWebKitAPI_WTF_TestCheckedArithmeticOperations_LDADD = $(testwebkitapi_tests_ldadd)
> > -Programs_TestWebKitAPI_WTF_TestCheckedArithmeticOperations_LDFLAGS = $(testwebkitapi_tests_ldflags)
> > +Programs_TestWebKitAPI_WTF_TestCheckedArithmeticOperations_CPPFLAGS = $(testwebkitapi_wtf_tests_cppflags)
> > +Programs_TestWebKitAPI_WTF_TestCheckedArithmeticOperations_LDADD = $(testwebkitapi_wtf_tests_ldadd)
> > +Programs_TestWebKitAPI_WTF_TestCheckedArithmeticOperations_LDFLAGS = $(testwebkitapi_wtf_tests_ldflags)
> 
> Ok I'm tired of Ditto, you get it I guess :)

Yes.

> > Tools/TestWebKitAPI/gtk/PlatformUtilitiesGtk.cpp:65
> > +    return g_filename_to_utf8(value, -1, 0, &bytesWritten, 0);
> 
> I find it a bit strange to use g_filename_to_utf8() here. Maybe some day we'll have env variables not dealing with file paths. Or maybe it's just this function that should be renamed :)

I copied this from WTR

> > Tools/TestWebKitAPI/gtk/main.cpp:37
> > +    bool passed = TestWebKitAPI::TestsController::shared().run(argc, argv);
> > +
> > +    return passed ? EXIT_SUCCESS : EXIT_FAILURE;
> 
> This can be one line maybe?

Yes, I guess.

[Martin Robinson]

Comment on attachment 138083 [details]
Patch

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

>>> Tools/Scripts/run-gtk-tests:83
>>> +                    "Test fails")
>> 
>> Can these failures be tracked in bugzilla if possible?
> 
> Yes, I haven't opened bugs for them yet, because they don't actually exist. It's chicken-egg problem, so my idea was to update it once bugs are filed.

It might be good to at least understand why the spacebar test fails. With it failing, it's hard to know if the code you wrote to support sending key events is working properly.

> Tools/TestWebKitAPI/gtk/PlatformWebViewGtk.cpp:76
> +    gtk_main_do_event(event.get());
> +    event->key.type = GDK_KEY_RELEASE;
> +    gtk_main_do_event(event.get());

Is it okay for this to happen asynchronously or do the tests expect that it be finished by the time the method returns?

[Philippe Normand]

(In reply to comment #3)
> 
> > > Tools/TestWebKitAPI/GNUmakefile.am:61
> > > +testwebkitapi_wtf_tests_cppflags = \
> > 
> > Ditto
> 
> In previous patch there are no other tests, I don't think it's a problem to changing it here, but I can do that in the other patch if you want.
> 

It's not technically a problem but it contributes to make this patch bigger and slightly lower reviewer's motivation (at least in my very personal case) :). Anyway no big deal I guess.

> > > Tools/TestWebKitAPI/gtk/PlatformUtilitiesGtk.cpp:65
> > > +    return g_filename_to_utf8(value, -1, 0, &bytesWritten, 0);
> > 
> > I find it a bit strange to use g_filename_to_utf8() here. Maybe some day we'll have env variables not dealing with file paths. Or maybe it's just this function that should be renamed :)
> 
> I copied this from WTR
> 

Ok but nevertheless :) I was just a bit surprised by the name of this function vs its code using g_file API. It's probably to keep it as it is but FTR that name doesn't make much sense to me :)

[Carlos Garcia Campos]

(In reply to comment #4)
> (From update of attachment 138083 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138083&action=review
> 
> >>> Tools/Scripts/run-gtk-tests:83
> >>> +                    "Test fails")
> >> 
> >> Can these failures be tracked in bugzilla if possible?
> > 
> > Yes, I haven't opened bugs for them yet, because they don't actually exist. It's chicken-egg problem, so my idea was to update it once bugs are filed.
> 
> It might be good to at least understand why the spacebar test fails. With it failing, it's hard to know if the code you wrote to support sending key events is working properly.

There are other tests using simulateSpacebarKeyPress() that pass. The problem is that test doesn't fail but hangs. I'll work on every single test that don't pass as soon as this patch lands and bug reports are filed for them. 

> > Tools/TestWebKitAPI/gtk/PlatformWebViewGtk.cpp:76
> > +    gtk_main_do_event(event.get());
> > +    event->key.type = GDK_KEY_RELEASE;
> > +    gtk_main_do_event(event.get());
> 
> Is it okay for this to happen asynchronously or do the tests expect that it be finished by the time the method returns?

I'm not sure, why?

[Martin Robinson]

Comment on attachment 138083 [details]
Patch

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

>>> Tools/TestWebKitAPI/gtk/PlatformUtilitiesGtk.cpp:65
>>> +    return g_filename_to_utf8(value, -1, 0, &bytesWritten, 0);
>> 
>> I find it a bit strange to use g_filename_to_utf8() here. Maybe some day we'll have env variables not dealing with file paths. Or maybe it's just this function that should be renamed :)
> 
> I copied this from WTR

I think the reason this is so confusing is that it's doing two things (perhaps this is a problem with WebKitTestRunner too). The first thing it does is get a path from an environment variable. The second thing it does is convert the path from the system encoding to UTF-8. Perhaps this should be split into two functions.

const char* getEnvironmentVariable(const char* variableName);
String convertFilenameToString();

Then each call site could call both functions. The reasoning here is that for any environment variable that is not a path, converting from the system encoding is not necessarily the right thing to do.

 Note that I use a String for the return value of convertFilenameToString, to avoid encoding ambiguity. You should have access to String in this code.

[Martin Robinson]

(In reply to comment #6)

> > > Tools/TestWebKitAPI/gtk/PlatformWebViewGtk.cpp:76
> > > +    gtk_main_do_event(event.get());
> > > +    event->key.type = GDK_KEY_RELEASE;
> > > +    gtk_main_do_event(event.get());
> > 
> > Is it okay for this to happen asynchronously or do the tests expect that it be finished by the time the method returns?
> 
> I'm not sure, why?

Assuming that gtk_main_do_event works asynchronously (I believe it does, but perhaps I'm misremembering), there is a chance that the event may still be on the event queue. If the code that calls this method expects WebKit to have already processed the event, the test will likely fail.

[Carlos Garcia Campos]

(In reply to comment #8)
> (In reply to comment #6)
> 
> > > > Tools/TestWebKitAPI/gtk/PlatformWebViewGtk.cpp:76
> > > > +    gtk_main_do_event(event.get());
> > > > +    event->key.type = GDK_KEY_RELEASE;
> > > > +    gtk_main_do_event(event.get());
> > > 
> > > Is it okay for this to happen asynchronously or do the tests expect that it be finished by the time the method returns?
> > 
> > I'm not sure, why?
> 
> Assuming that gtk_main_do_event works asynchronously (I believe it does, but perhaps I'm misremembering), there is a chance that the event may still be on the event queue. If the code that calls this method expects WebKit to have already processed the event, the test will likely fail.

Ah, don't worry then, gtk_main_do_event processes the given event synchronously,

[Carlos Garcia Campos]

Created attachment 138595 [details]
Updated patch

Updated to apply on current git master now that bug #84325 landed, and address review comments

[Martin Robinson]

Comment on attachment 138595 [details]
Updated patch

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

> Tools/TestWebKitAPI/gtk/PlatformUtilitiesGtk.cpp:66
> +static char* getEnvironmentVariableAsUTF8String(const char* variableName)
> +{
> +    const char* value = g_getenv(variableName);
> +    if (!value) {
> +        g_printerr("%s environment variable not found\n", variableName);
> +        exit(1);
> +    }
> +    gsize bytesWritten;
> +    return g_filename_to_utf8(value, -1, 0, &bytesWritten, 0);
> +}

I guess you didn't want to split this?

[Carlos Garcia Campos]

(In reply to comment #11)
> (From update of attachment 138595 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138595&action=review
> 
> > Tools/TestWebKitAPI/gtk/PlatformUtilitiesGtk.cpp:66
> > +static char* getEnvironmentVariableAsUTF8String(const char* variableName)
> > +{
> > +    const char* value = g_getenv(variableName);
> > +    if (!value) {
> > +        g_printerr("%s environment variable not found\n", variableName);
> > +        exit(1);
> > +    }
> > +    gsize bytesWritten;
> > +    return g_filename_to_utf8(value, -1, 0, &bytesWritten, 0);
> > +}
> 
> I guess you didn't want to split this?

No, sorry, I forgot to reply to your comment. In this file this function is only used to get paths from the environment, and the only use of the convertFilenameToString() function would be from getEnvironmentVariable(). I agree with your reasoning, in general, but in this particular case I think we could just rename the function as something like getFilenameFromEnvironmentVariableAsUTF8(). 

Also note that the code expects a char *, so with your approach we would be converting from utf8 to utf16 to return a String and then again to utf8 to create the WKString.

[Carlos Garcia Campos]

Created attachment 138743 [details]
Updated patch

Renamed getEnvironmentVariableAsUTF8String to getFilenameFromEnvironmentVariableAsUTF8

[Martin Robinson]

Comment on attachment 138743 [details]
Updated patch

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

> Tools/TestWebKitAPI/gtk/PlatformUtilitiesGtk.cpp:48
> +    g_timeout_add(500, checkTestFinished, done);

This means that every test will be at least 500 milliseconds long, which means that 20 tests will take at least 10 seconds to run. Don't you think it makes more sense to make this g_idle_add or something similar? Windows and Mac do this test on literally every cycle of the main loop.

> Tools/TestWebKitAPI/gtk/PlatformWebViewGtk.cpp:121
> +    GtkWidget* viewWidget = GTK_WIDGET(m_view);
> +    if (!gtk_widget_get_realized(viewWidget))
> +        gtk_widget_show(m_window);
> +    doMouseButtonEvent(viewWidget, GDK_BUTTON_PRESS, x, y, 3);
> +    doMouseButtonEvent(viewWidget, GDK_BUTTON_RELEASE, x, y, 3);

Depending on the design of the context menu API, we may need to add some extra logic here. If we add a default action which opens the context menu on right clicks, we'll need to block that here in the future. It might be worth putting a small comment.

[Carlos Garcia Campos]

(In reply to comment #14)
> (From update of attachment 138743 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138743&action=review
> 
> > Tools/TestWebKitAPI/gtk/PlatformUtilitiesGtk.cpp:48
> > +    g_timeout_add(500, checkTestFinished, done);
> 
> This means that every test will be at least 500 milliseconds long, which means that 20 tests will take at least 10 seconds to run. Don't you think it makes more sense to make this g_idle_add or something similar? Windows and Mac do this test on literally every cycle of the main loop.

Yes, sure, I'll use a g_idle instead.

> > Tools/TestWebKitAPI/gtk/PlatformWebViewGtk.cpp:121
> > +    GtkWidget* viewWidget = GTK_WIDGET(m_view);
> > +    if (!gtk_widget_get_realized(viewWidget))
> > +        gtk_widget_show(m_window);
> > +    doMouseButtonEvent(viewWidget, GDK_BUTTON_PRESS, x, y, 3);
> > +    doMouseButtonEvent(viewWidget, GDK_BUTTON_RELEASE, x, y, 3);
> 
> Depending on the design of the context menu API, we may need to add some extra logic here. If we add a default action which opens the context menu on right clicks, we'll need to block that here in the future. It might be worth putting a small comment.

Context menu API doesn't affect the C API at all. C API users are supposed to add their own implementation of the ContextMenuClient.

[Carlos Garcia Campos]

Created attachment 138986 [details]
Updated patch

Rebased to apply on current git master. Use g_idle_add instead of g_timeout_add as suggested by Martin. And build a custom libmain instead of libgtestmain so that all tests use TestsController class that initializes WTF threading to fix bug #84868

[Philippe Normand]

Comment on attachment 138986 [details]
Updated patch

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

Alright, looks good! Please don't forget to file new bugs for the skipped tests.

> Tools/ChangeLog:18
> +        * TestWebKitAPI/JavaScriptTest.cpp: Use
> +        JavaScriptCore/JSContextRef.h instead of
> +        JavaScriptCore/JavaScriptCore.h since it includes JSStringRefCF.h
> +        unconditionally.

Maybe I'm wrong but I think this issue should still be fixed. JavaScriptCore.h shouldn't unconditionally include CF headers if it's meant to be used by non-mac ports.

[Carlos Garcia Campos]

Committed r115314: <http://trac.webkit.org/changeset/115314>