It's specially useful to write and try unit tests when adding new API to WebKit2 C API.
Created attachment 137866 [details] Patch For now only WTF unit tests are built to make the patch simpler.
Comment on attachment 137866 [details] Patch Attachment 137866 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12434168
(In reply to comment #2) > (From update of attachment 137866 [details]) > Attachment 137866 [details] did not pass gtk-ews (gtk): > Output: http://queues.webkit.org/results/12434168 I have no idea what that error means, it builds fine for me.
Comment on attachment 137866 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=137866&action=review > GNUmakefile.am:153 > + -DBUILDING_WEBKIT2__=1 Perhaps it would be better to use a WTF enable flag here like -DWTF_ENABLE_WEBKIT2.
(In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 137866 [details] [details]) > > Attachment 137866 [details] [details] did not pass gtk-ews (gtk): > > Output: http://queues.webkit.org/results/12434168 > > I have no idea what that error means, it builds fine for me. Something about your patch is breaking the thin archives build. The thin archives build greatly speeds up building the debug version of WebKit, so I think it's best that we don't break it. To test this locally just pass AR_FLAGS="cruT" when calling configure/autogen.
Comment on attachment 137866 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=137866&action=review > Source/ThirdParty/gtest/GNUmakefile.am:65 > +# #included by gtest-all.cc. Did you mean to have the second '#' here? > Tools/Scripts/run-gtk-tests:47 > + TEST_DIRS = [ "unittests", "WebKit2APITests", "TestWebKitAPI/WTF" ] I'm pretty sure > Tools/Scripts/run-gtk-tests:271 > + def _run_test_google(self, test): > + tester_command = [test] > + skipped_tests_cases = self._test_cases_to_skip(test) > + if skipped_tests_cases: > + tester_command.append("--gtest_filter=-%s" % ":".join(skipped_tests_cases)) > + > + return not self._create_process(tester_command, env=self._test_env).wait() > + > + def _run_test(self, test): > + if "unittests" in test or "WebKit2APITests" in test: > + return self._run_test_glib(test) > + > + if "TestWebKitAPI" in test: > + return self._run_test_google(test) > + > + return False > + The logic for running the API tests is already in Tools/Scripts/run-api-tests, so we get that script working instead probably.
(In reply to comment #6) > (From update of attachment 137866 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=137866&action=review > > > Source/ThirdParty/gtest/GNUmakefile.am:65 > > +# #included by gtest-all.cc. > > Did you mean to have the second '#' here? Yes, #include <foo.cc>, this is indeed copied from original gtest Makefile > > Tools/Scripts/run-gtk-tests:47 > > + TEST_DIRS = [ "unittests", "WebKit2APITests", "TestWebKitAPI/WTF" ] > > I'm pretty sure What? > > Tools/Scripts/run-gtk-tests:271 > > + def _run_test_google(self, test): > > + tester_command = [test] > > + skipped_tests_cases = self._test_cases_to_skip(test) > > + if skipped_tests_cases: > > + tester_command.append("--gtest_filter=-%s" % ":".join(skipped_tests_cases)) > > + > > + return not self._create_process(tester_command, env=self._test_env).wait() > > + > > + def _run_test(self, test): > > + if "unittests" in test or "WebKit2APITests" in test: > > + return self._run_test_glib(test) > > + > > + if "TestWebKitAPI" in test: > > + return self._run_test_google(test) > > + > > + return False > > + > > The logic for running the API tests is already in Tools/Scripts/run-api-tests, so we get that script working instead probably. GTK+ port doesn't use that script, we use run-gtk-tests
(In reply to comment #5) > (In reply to comment #3) > > (In reply to comment #2) > > > (From update of attachment 137866 [details] [details] [details]) > > > Attachment 137866 [details] [details] [details] did not pass gtk-ews (gtk): > > > Output: http://queues.webkit.org/results/12434168 > > > > I have no idea what that error means, it builds fine for me. > > Something about your patch is breaking the thin archives build. The thin archives build greatly speeds up building the debug version of WebKit, so I think it's best that we don't break it. To test this locally just pass AR_FLAGS="cruT" when calling configure/autogen. Thanks, I'll try.
Created attachment 137912 [details] Updated patch to try to fix the build
(In reply to comment #7) > (In reply to comment #6) > > (From update of attachment 137866 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=137866&action=review > > > > > > The logic for running the API tests is already in Tools/Scripts/run-api-tests, so we get that script working instead probably. > > GTK+ port doesn't use that script, we use run-gtk-tests I'm a bit worried to run both our API tests and the C API tests via a single script and it's not coherent from the POV of the developers using run-api-tests.
(In reply to comment #10) > (In reply to comment #7) > > (In reply to comment #6) > > > (From update of attachment 137866 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=137866&action=review > > > > > > > > > The logic for running the API tests is already in Tools/Scripts/run-api-tests, so we get that script working instead probably. > > > > GTK+ port doesn't use that script, we use run-gtk-tests > > I'm a bit worried to run both our API tests and the C API tests via a single script and it's not coherent from the POV of the developers using run-api-tests. What's the problem? we already run wk1 and wk2 api tests using a single script. It allows to use a common interface and provide great features like using the internal jhbuild, run Xvfb, skip tests and even particular tests cases, ... Regarding run-api-tests, I already agreed with Martin on keeping run-gtk-test run all tests, and the bots using run-gtk-test (so that we don't need to add yet another step to the build), and make run-api-test call run-gtk-test with an option so that TestWebKitAPI tests are run.
> Regarding run-api-tests, I already agreed with Martin on keeping run-gtk-test run all tests, and the bots using run-gtk-test (so that we don't need to add yet another step to the build), and make run-api-test call run-gtk-test with an option so that TestWebKitAPI tests are run. Ok I missed that discussion it seems... My only concern was for other devs external to our port using run-api-tests in their port finding odd to not use run-api-tests for gtk... Anyway, no big deal.
(In reply to comment #12) > > Regarding run-api-tests, I already agreed with Martin on keeping run-gtk-test run all tests, and the bots using run-gtk-test (so that we don't need to add yet another step to the build), and make run-api-test call run-gtk-test with an option so that TestWebKitAPI tests are run. > > Ok I missed that discussion it seems... > My only concern was for other devs external to our port using run-api-tests in their port finding odd to not use run-api-tests for gtk... > Anyway, no big deal. I tried to make run-api-tests call run-gtk-tests for Gtk but it's not that easy, so I'll do it in a separate patch to not make this patch more complex.
Comment on attachment 137912 [details] Updated patch to try to fix the build View in context: https://bugs.webkit.org/attachment.cgi?id=137912&action=review r=me but please address the following questions before landing: > ChangeLog:9 > + * GNUmakefile.am: Include makefiles to build gtest and > + TestWebKitAPI. Add BUILDING_WEBKIT2__ macro to compilation when What about -DWTF_ENABLE_WEBKIT2 like Martin suggested? > Tools/Scripts/run-gtk-tests:255 > + def _run_test_google(self, test): can this be renamed to _run_test_gtester or _run_gtester?
(In reply to comment #14) > (From update of attachment 137912 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=137912&action=review > > r=me but please address the following questions before landing: > > > ChangeLog:9 > > + * GNUmakefile.am: Include makefiles to build gtest and > > + TestWebKitAPI. Add BUILDING_WEBKIT2__ macro to compilation when > > What about -DWTF_ENABLE_WEBKIT2 like Martin suggested? ENABLE macros are of features, but we want to know whether we are building wk2 or not. WebKit2 is not a feature. > > Tools/Scripts/run-gtk-tests:255 > > + def _run_test_google(self, test): > > can this be renamed to _run_test_gtester or _run_gtester? gtester is actually the glib test command, it would be gtest. Since it's indeed confusing I decided to use glib and google instead of gtester and gtest.
Comment on attachment 137912 [details] Updated patch to try to fix the build View in context: https://bugs.webkit.org/attachment.cgi?id=137912&action=review > Tools/TestWebKitAPI/config.h:32 > -#if __APPLE__ > +#ifdef __APPLE__ Technically to maintain the semantics of this line, I think you need to use: #if defined(__APPLE__) && __APPLE__
(In reply to comment #15) > (In reply to comment #14) > > (From update of attachment 137912 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=137912&action=review > > > > r=me but please address the following questions before landing: > > > > > ChangeLog:9 > > > + * GNUmakefile.am: Include makefiles to build gtest and > > > + TestWebKitAPI. Add BUILDING_WEBKIT2__ macro to compilation when > > > > What about -DWTF_ENABLE_WEBKIT2 like Martin suggested? > > ENABLE macros are of features, but we want to know whether we are building wk2 or not. WebKit2 is not a feature. > Fair enough! > > > Tools/Scripts/run-gtk-tests:255 > > > + def _run_test_google(self, test): > > > > can this be renamed to _run_test_gtester or _run_gtester? > > gtester is actually the glib test command, it would be gtest. Since it's indeed confusing I decided to use glib and google instead of gtester and gtest. Oh yeah, my bad. Less confusing like this indeed.
Committed r115075: <http://trac.webkit.org/changeset/115075>