Bug 84325 - [GTK] Build and run TestWebKitAPI unit tests
Summary: [GTK] Build and run TestWebKitAPI unit tests
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks: 84446
  Show dependency treegraph
 
Reported: 2012-04-19 02:51 PDT by Carlos Garcia Campos
Modified: 2012-04-24 09:48 PDT (History)
4 users (show)

See Also:


Attachments
Patch (13.23 KB, patch)
2012-04-19 02:59 PDT, Carlos Garcia Campos
mrobinson: review-
pnormand: commit-queue-
Details | Formatted Diff | Diff
Updated patch to try to fix the build (13.20 KB, patch)
2012-04-19 09:40 PDT, Carlos Garcia Campos
pnormand: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2012-04-19 02:51:30 PDT
It's specially useful to write and try unit tests when adding new API to WebKit2 C API.
Comment 1 Carlos Garcia Campos 2012-04-19 02:59:01 PDT
Created attachment 137866 [details]
Patch

For now only WTF unit tests are built to make the patch simpler.
Comment 2 Philippe Normand 2012-04-19 03:04:17 PDT
Comment on attachment 137866 [details]
Patch

Attachment 137866 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12434168
Comment 3 Carlos Garcia Campos 2012-04-19 03:15:06 PDT
(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 4 Martin Robinson 2012-04-19 06:14:17 PDT
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.
Comment 5 Martin Robinson 2012-04-19 06:15:17 PDT
(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 6 Martin Robinson 2012-04-19 06:21:34 PDT
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.
Comment 7 Carlos Garcia Campos 2012-04-19 06:27:07 PDT
(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
Comment 8 Carlos Garcia Campos 2012-04-19 06:28:56 PDT
(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.
Comment 9 Carlos Garcia Campos 2012-04-19 09:40:07 PDT
Created attachment 137912 [details]
Updated patch to try to fix the build
Comment 10 Philippe Normand 2012-04-19 16:44:14 PDT
(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.
Comment 11 Carlos Garcia Campos 2012-04-19 23:29:09 PDT
(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.
Comment 12 Philippe Normand 2012-04-20 10:47:08 PDT
> 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.
Comment 13 Carlos Garcia Campos 2012-04-23 23:50:23 PDT
(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 14 Philippe Normand 2012-04-24 07:51:46 PDT
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?
Comment 15 Carlos Garcia Campos 2012-04-24 08:02:30 PDT
(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 16 Martin Robinson 2012-04-24 08:38:20 PDT
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__
Comment 17 Philippe Normand 2012-04-24 08:49:36 PDT
(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.
Comment 18 Carlos Garcia Campos 2012-04-24 09:48:43 PDT
Committed r115075: <http://trac.webkit.org/changeset/115075>