Bug 120202

Created attachment 209459 [details] Patch

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

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.

I'm okay with splitting it up, but I'd prefer that the tests were written in a way that they were order independent.

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

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

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

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

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

Any other comments or or reviews?

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?

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

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)

I guess this is no longer required since https://bugs.webkit.org/show_bug.cgi?id=126284?

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

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

*** This bug has been marked as a duplicate of bug 126284 ***

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