[webkit-reviews] review denied: [Bug 84446] [GTK] Build and run TestWebKitAPI WebKit2 unit tests : [Attachment 138083] Patch

[bugzilla-daemon at webkit.org]

Philippe Normand <pnormand at igalia.com> has denied Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 84446: [GTK] Build and run TestWebKitAPI WebKit2 unit tests
https://bugs.webkit.org/show_bug.cgi?id=84446

Attachment 138083: Patch
https://bugs.webkit.org/attachment.cgi?id=138083&action=review

------- Additional Comments from Philippe Normand <pnormand at igalia.com>
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?