Bug 69409 – [GTK] [WebKit2] Make adding another unit test easier

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Martin Robinson]

The GLib testing framework is a bit verbose and it's difficult to share fixtures between test files. This bug tracks adding a bit of C++ sugar to creating unit tests.

[Martin Robinson]

Created attachment 109767 [details]
Work in progress patch

[Carlos Garcia Campos]

Comment on attachment 109767 [details]
Work in progress patch

View in context: https://bugs.webkit.org/attachment.cgi?id=109767&action=review

I like the idea of making test easier to write, I have some comments, though.

> GNUmakefile.am:211
> +include Source/WebKit2/UIProcess/API/gtk/tests/GNUmakefile.am

Why adding a new makefile for the tests?

> Source/WebKit2/UIProcess/API/gtk/tests/GNUmakefile.am:4
> +TEST_PROGS += \
> +	Programs/WebKit2APITests/TestWebKitWebContext \
> +	Programs/WebKit2APITests/TestWebKitWebView \
> +	Programs/WebKit2APITests/TestWebKitWebLoaderClient

Renaming the path and test names would break the bots, see bug https://bugs.webkit.org/show_bug.cgi?id=68992. so I would leave current names unless there's an important reason to rename them.

> Source/WebKit2/UIProcess/API/gtk/tests/GNUmakefile.am:35
> +noinst_LTLIBRARIES += Libraries/libWebKit2APITestCore.la

This library should probably be in WebCore or Tools, so that it can be used for wk1 tests too.

> Source/WebKit2/UIProcess/API/gtk/tests/GNUmakefile.am:38
> +	Source/WebKit2/UIProcess/API/gtk/tests/LoadTrackingTest.cpp \
> +	Source/WebKit2/UIProcess/API/gtk/tests/LoadTrackingTest.h \

These files are specific to the loading test, I think there should be only common classes in the library, since all tests will link ot it.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebLoaderClient.cpp:27
> +#include "config.h"
> +
> +#include "LoadTrackingTest.h"
> +#include <gtk/gtk.h>
> +#include <libsoup/soup.h>
> +#include <wtf/text/CString.h>
> +

I don't mind using C++ for unit tests, but I'm not sure about using internal API in the tests. I think tests should only use public gtk+ api.

[Carlos Garcia Campos]

Created attachment 109797 [details]
Another approach based on martin's

I think we can make it even simpler, we don't need a library, just a header file with a couple of macros.

[Martin Robinson]

I think I prefer my original approach in the end for a few reasons:

1. Having a generic fixture means that we can apply logic before and after all tests in all suites. The logic for the test path isn't duplicated in multiple places.

2. I think it's important that we push the most common fixtures now to other files so that they can be used in more than one place. If necessary simple fixtures can be kept inline near tests.

For an example, I could have easily extended the LoadTrackingTest fixture to also load alternate HTML when a failure happens. Then I could have used that fixture for multiple tests. This test probably belongs in TestWebKitWebView anyway.

In the second patch we have to duplicate the WebView and main loop creation. Seeing this in every single WebKit1 unit test file is one of the main reasons I set out to do this. We get to use the C++ class system to our advantage.

3. I think we should avoid having the main method as a macro. I dislike macros in general, and perhaps I'll look for a way to implement this via templates.

4. I renamed the test files to better match the other files in the source tree. I believe we should use WebKit naming conventions everywhere possible. The only other binaries we produce that are named like this are the WK1 tests.  I'm not sure it makes sense in this case. I'll upload a new patch which fixes build-webkit as well.

[Martin Robinson]

(In reply to comment #4)
> I'll upload a new patch which fixes build-webkit as well.

Sorry, I mean run-gtk-tests.

[Carlos Garcia Campos]

(In reply to comment #4)
> I think I prefer my original approach in the end for a few reasons:
> 
> 1. Having a generic fixture means that we can apply logic before and after all tests in all suites. The logic for the test path isn't duplicated in multiple places.

There's a generic fixture in my patch too with methods begin and end to do whatever before and after tests.

> 2. I think it's important that we push the most common fixtures now to other files so that they can be used in more than one place. If necessary simple fixtures can be kept inline near tests.
>
> For an example, I could have easily extended the LoadTrackingTest fixture to also load alternate HTML when a failure happens. Then I could have used that fixture for multiple tests. This test probably belongs in TestWebKitWebView anyway.

That's possible with my patch too, you can just add a new fixture for loading tests and make loader client fixture inherit from it. I merged them just to avoid having an unnecessary library and to make the patch simpler and smaller.
 
> In the second patch we have to duplicate the WebView and main loop creation. Seeing this in every single WebKit1 unit test file is one of the main reasons I set out to do this. We get to use the C++ class system to our advantage.

You are assuming all tests using a web view have a main loop too. 

> 3. I think we should avoid having the main method as a macro. I dislike macros in general, and perhaps I'll look for a way to implement this via templates.

I guess it's a matter of taste, I like macros :-) 

> 4. I renamed the test files to better match the other files in the source tree. I believe we should use WebKit naming conventions everywhere possible. The only other binaries we produce that are named like this are the WK1 tests.  I'm not sure it makes sense in this case. I'll upload a new patch which fixes build-webkit as well.

[Martin Robinson]

Created attachment 109808 [details]
My patch updated to fix run-gtk-tests

[Martin Robinson]

(In reply to comment #6)
 
> You are assuming all tests using a web view have a main loop too. 

This is the case in almost all of the WebKit1 unit tests. I think it will be even more likely now because more operations will need to be asynchronous in WebKit -- for instance getting page content. I also believe this will help us organize our tests by what unit they go with, versus what fixture they need.

I'm confidant that a more hierarchical design will make the barrier to adding a unit test smaller by allowing code reuse and reducing the noise in each test file. I think it's very important to make adding a unit test as easy as possible so we no longer shudder at having to write them.

[Martin Robinson]

Carlos, I examined your patch again and I think it is  not not so different from mine. I see a few main differences:

1. main() is inside a #define and depends on the fact that a test suite can only use one type of fixture. 
2. Tests are embedded in the fixtures.
3. The files use their original names.

Number 3 is not so important to me, but I do believe we should name the tests after the API they test - testloading -> testwebloaderclient, etc.

Number 1 and 2 are pretty important to me though. Fixtures should be independent of tests, we should be able to share them between suites and suites should be able to mix and match fixtures. That's the main reason I wanted to do this. Perhaps we can reach some sort of compromise on the rest?

[Carlos Garcia Campos]

(In reply to comment #9)
> Carlos, I examined your patch again and I think it is  not not so different from mine. I see a few main differences:

Yes, as I said, my patch is based on yours.

> 1. main() is inside a #define and depends on the fact that a test suite can only use one type of fixture. 
> 2. Tests are embedded in the fixtures.
> 3. The files use their original names.
> 
> Number 3 is not so important to me, but I do believe we should name the tests after the API they test - testloading -> testwebloaderclient, etc.
> 
> Number 1 and 2 are pretty important to me though. Fixtures should be independent of tests, we should be able to share them between suites and suites should be able to mix and match fixtures. That's the main reason I wanted to do this. Perhaps we can reach some sort of compromise on the rest?

Ok, I just wanted to avoid having another library if it is not really needed, and I didn't like having the main() in the library and the beforAll() afterAll() methods, I think we can have a class where befor/fater is just the constructor/destructor. But anyway, I don't want to block all new patches on this, and I agree on the idea of making adding new tests easier, so if xan and kov like your proposal it's fine with me.

[Martin Robinson]

Created attachment 110445 [details]
Patch

[Martin Robinson]

Created attachment 110472 [details]
Patch

[Martin Robinson]

(In reply to comment #12)
> Created an attachment (id=110472) [details]
> Patch

I updated the patch to fix some style issues and disconnect handlers signal destruction.

[Gustavo Noronha (kov)]

Comment on attachment 110472 [details]
Patch

This shall make us able to set aside laziness and add unit tests more often =)

[Martin Robinson]

Committed r97226: <http://trac.webkit.org/changeset/97226>