Bug 120202 - [GTK] [WK2] Split up TestWebKitWebView into smaller executables
Summary: [GTK] [WK2] Split up TestWebKitWebView into smaller executables
Status: RESOLVED DUPLICATE of bug 126284
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brian Holt
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-08-23 03:05 PDT by Brian Holt
Modified: 2014-02-13 03:31 PST (History)
6 users (show)

See Also:


Attachments
Patch (89.95 KB, patch)
2013-08-23 07:24 PDT, Brian Holt
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Holt 2013-08-23 03:05:04 PDT
In https://bugs.webkit.org/show_bug.cgi?id=117689 TestWebKitWebView is failing because of multiple separate issues making it hard to resolve.
Comment 1 Brian Holt 2013-08-23 07:24:27 PDT
Created attachment 209459 [details]
Patch
Comment 2 WebKit Commit Bot 2013-08-23 07:26:15 PDT
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 3 Brian Holt 2013-08-30 08:46:05 PDT
mrobinson: Do you support the idea of splitting up the monolithic TestWebKitWebView into smaller executables? It will increase build and link times marginally but it would mean that we can run some tests instead of skipping all of them.
Comment 4 Martin Robinson 2013-09-01 17:21:53 PDT
I'm okay with splitting it up, but I'd prefer that the tests were written in a way that they were order independent.
Comment 5 Brian Holt 2013-09-02 01:49:41 PDT
(In reply to comment #4)
> I'm okay with splitting it up, but I'd prefer that the tests were written in a way that they were order independent.

I think for the most part they are order-independent.  Two exceptions that come to mind are that failure tests must precede success tests for authentication (because of libsoup) and that the custom-charset test causes problems with subsequent page loads.
Comment 6 Martin Robinson 2013-09-02 09:44:55 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > I'm okay with splitting it up, but I'd prefer that the tests were written in a way that they were order independent.
> 
> I think for the most part they are order-independent.  Two exceptions that come to mind are that failure tests must precede success tests for authentication (because of libsoup) and that the custom-charset test causes problems with subsequent page loads.

These kind of issues make it difficult to randomize test order. Why do libsoup authentication failures cause followup tests to fail. To me these kind of things seem like real bugs in the test.
Comment 7 Brian Holt 2013-09-02 09:55:22 PDT
(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > I'm okay with splitting it up, but I'd prefer that the tests were written in a way that they were order independent.
> > 
> > I think for the most part they are order-independent.  Two exceptions that come to mind are that failure tests must precede success tests for authentication (because of libsoup) and that the custom-charset test causes problems with subsequent page loads.
> 
> These kind of issues make it difficult to randomize test order. Why do libsoup authentication failures cause followup tests to fail. To me these kind of things seem like real bugs in the test.

I think you may have misunderstood my comment, I probably didn't explain the exception regarding authentication very well: what I meant was that you can do all the authentication failure tests in any random order, but as soon as you successfully authenticate libsoup will never challenge the user agent again for that session (see http://trac.webkit.org/browser/trunk/Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp#L1197).

The custom-charset bug is a real issue that I hope to address.

In principle though, all of these tests *should be* order independent, and if we find tests that are not then bugs should be raised.
Comment 8 Martin Robinson 2013-09-02 10:18:03 PDT
(In reply to comment #7)

> I think you may have misunderstood my comment, I probably didn't explain the exception regarding authentication very well: what I meant was that you can do all the authentication failure tests in any random order, but as soon as you successfully authenticate libsoup will never challenge the user agent again for that session (see http://trac.webkit.org/browser/trunk/Source/WebCore/platform/network/soup/ResourceHandleSoup.cpp#L1197).

Oh yes. Really good point!

> The custom-charset bug is a real issue that I hope to address.
> 
> In principle though, all of these tests *should be* order independent, and if we find tests that are not then bugs should be raised.

Okay. So the benefit of splitting the tests is that you can unskip some now and some later?
Comment 9 Brian Holt 2013-09-03 02:24:47 PDT
> Okay. So the benefit of splitting the tests is that you can unskip some now and some later?

Exactly.  Its just to prevent the case where a single test failure results in the entire (large) suite being skipped, so we can unskip some now and continue running at least some tests.
Comment 10 Brian Holt 2013-09-10 08:30:10 PDT
Any other comments or or reviews?
Comment 11 Brian Holt 2013-09-30 08:31:02 PDT
After a lot of reflection and diving into these tests, it seems that splitting TestWebKitWebView into smaller executables won't solve the problem with the timeouts and doesn't provide the real long-term fix of being able to skip individual tests when they fail.

Instead, I propose to abandon this patch and rather to comment out the individual tests that are failing in the and remove this from the skippedTests list.  What do you think?
Comment 12 Mario Sanchez Prada 2013-09-30 08:39:23 PDT
(In reply to comment #11)
> After a lot of reflection and diving into these tests, it seems that splitting TestWebKitWebView into smaller executables won't solve the problem with the timeouts and doesn't provide the real long-term fix of being able to skip individual tests when they fail.
> 
> Instead, I propose to abandon this patch and rather to comment out the individual tests that are failing in the and remove this from the skippedTests list.  What do you think?

As much as I don't like to have to comment out individual tests from the test suite, I'd personally rather have that now than having the entire test suite skipped, which seems to be the case.

And regarding to the split, I agree that doing that just for the sake of "fixing" this does not make much sense since it's not fixing the issue either (and so I'd rather comment those tests out). However, it might be something worth considering anyway (regardless of this issue) due to the size of the TestWebKitWebView test suite, and because some test cases (specially those related to the authentication API) seem to be quite self-contained anyway.

So, my vote goes for skipping those tests now (if others agree), so we can start running at least part of the TestWebKitWebView test suite, and think about the split as a completely unrelated thing.

My 2 cents.
Comment 13 Carlos Garcia Campos 2013-09-30 08:46:39 PDT
I think it's fine to keep the tests skipped if they fail on the bots. I always run the unit tests locally with gtester directly or with run-gtk-tests --no-xvfb --skipped=ignore so that xvfb is not used and skipped tests are ignored (and then executed)
Comment 14 Brian Holt 2014-01-02 03:42:45 PST
I guess this is no longer required since https://bugs.webkit.org/show_bug.cgi?id=126284?
Comment 15 Carlos Garcia Campos 2014-01-02 04:38:46 PST
(In reply to comment #14)
> I guess this is no longer required since https://bugs.webkit.org/show_bug.cgi?id=126284?

Oops, I'm sorry, I had forgotten this bug when I did the split :-(
Comment 16 Brian Holt 2014-01-02 05:35:22 PST
(In reply to comment #15)
> (In reply to comment #14)
> > I guess this is no longer required since https://bugs.webkit.org/show_bug.cgi?id=126284?
> 
> Oops, I'm sorry, I had forgotten this bug when I did the split :-(

No problem, its achieved the same thing.  I'll close this one.
Comment 17 Brian Holt 2014-01-02 05:36:00 PST

*** This bug has been marked as a duplicate of bug 126284 ***
Comment 18 Csaba Osztrogonác 2014-02-13 03:31:22 PST
Comment on attachment 209459 [details]
Patch

Cleared review? from attachment 209459 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).