Bug 84325 – [GTK] Build and run TestWebKitAPI unit tests

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

[Carlos Garcia Campos]

It's specially useful to write and try unit tests when adding new API to WebKit2 C API.

[Carlos Garcia Campos]

Created attachment 137866 [details]
Patch

For now only WTF unit tests are built to make the patch simpler.

[Philippe Normand]

Comment on attachment 137866 [details]
Patch

Attachment 137866 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12434168

[Carlos Garcia Campos]

(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.

[Martin Robinson]

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.

[Martin Robinson]

(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.

[Martin Robinson]

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.

[Carlos Garcia Campos]

(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

[Carlos Garcia Campos]

(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.

[Carlos Garcia Campos]

Created attachment 137912 [details]
Updated patch to try to fix the build

[Philippe Normand]

(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.

[Carlos Garcia Campos]

(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.

[Philippe Normand]

> 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.

[Carlos Garcia Campos]

(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.

[Philippe Normand]

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?

[Carlos Garcia Campos]

(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.

[Martin Robinson]

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__

[Philippe Normand]

(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.

[Carlos Garcia Campos]

Committed r115075: <http://trac.webkit.org/changeset/115075>