[Webkit-unassigned] [Bug 84446] [GTK] Build and run TestWebKitAPI WebKit2 unit tests

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=84446





--- Comment #3 from Carlos Garcia Campos <cgarcia at igalia.com>  2012-04-24 08:37:25 PST ---
(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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.