Bug 140036 - Simplify WebKitTestRunner timeout tracking
Summary: Simplify WebKitTestRunner timeout tracking
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alexey Proskuryakov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-01-01 16:46 PST by Alexey Proskuryakov
Modified: 2015-01-02 14:51 PST (History)
4 users (show)

See Also:


Attachments
proposed patch (9.15 KB, patch)
2015-01-01 16:54 PST, Alexey Proskuryakov
no flags Details | Formatted Diff | Diff
proposed patch (10.34 KB, patch)
2015-01-01 23:53 PST, Alexey Proskuryakov
no flags Details | Formatted Diff | Diff
proposed patch (10.40 KB, patch)
2015-01-02 09:50 PST, Alexey Proskuryakov
no flags Details | Formatted Diff | Diff
proposed patch (10.41 KB, patch)
2015-01-02 10:48 PST, Alexey Proskuryakov
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2015-01-01 16:46:53 PST
WebKitTestRunner has "short timeout" and "long timeout" for IPC, plus it has waitUntilDone timeout, plus run-webkit-tests itself has a timeout.

There are two reasons why WebKitTestRunner IPC timeouts are helpful:

1. If WebProcess freezes between tests, we want to detect that, and not attribute the freeze to the next test. Expecting a "start test" message to be handled quickly achieves this.

2. If WebProcess is so broken that it fails to launch, it's nice to say so, and not just fail with a generic timeout.

Both are handled by short timeout. Long timeout is only used when loading about:blank between tests, which I don't quite see the logic behind.
Comment 1 Alexey Proskuryakov 2015-01-01 16:54:34 PST
Created attachment 243875 [details]
proposed patch
Comment 2 Alexey Proskuryakov 2015-01-01 23:53:37 PST
Created attachment 243882 [details]
proposed patch

With Efl and Gtk build fixes.
Comment 3 Csaba Osztrogonác 2015-01-02 01:53:47 PST
(In reply to comment #2)
> Created attachment 243882 [details]
> proposed patch
> 
> With Efl and Gtk build fixes.

./../Tools/WebKitTestRunner/TestController.cpp:73:45: error: missing binary operator before token "("
 #if defined (__has_feature) && __has_feature(address_sanitizer)
                                             ^

Unfortunately GCC doesn't support short circuit evaluation,
so we need to use nested ifs for has_feature and has_include.
Comment 4 Csaba Osztrogonác 2015-01-02 01:56:27 PST
It's strange how GTK built it. I thought GTK uses GCC 4.8 too.
Comment 5 Alexey Proskuryakov 2015-01-02 09:50:24 PST
Created attachment 243895 [details]
proposed patch

OK, replaced with an ugly nested #if for now. I'm looking into adding a macro to WTF, but need to figure out how an existing ADDRESS_SANITIZER macro is used.
Comment 6 Darin Adler 2015-01-02 10:41:15 PST
Comment on attachment 243895 [details]
proposed patch

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

> Tools/WebKitTestRunner/TestController.cpp:78
> +#else

Missing #endif before this #else.
Comment 7 Alexey Proskuryakov 2015-01-02 10:48:04 PST
Created attachment 243901 [details]
proposed patch
Comment 8 Darin Adler 2015-01-02 10:52:09 PST
Comment on attachment 243901 [details]
proposed patch

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

I think it’s a little strange to compile in this 10 second vs. 5 second default timeout.

> Tools/WebKitTestRunner/TestController.cpp:81
> +#if defined(__has_feature)
> +#if __has_feature(address_sanitizer)
> +const double TestController::shortTimeout = 10.0;
> +#else
> +const double TestController::shortTimeout = 5.0;
> +#endif
> +#else
> +const double TestController::shortTimeout = 5.0;
> +#endif

I would have written:

    #if defined(__has_feature) && __has_feature(address_sanitizer)
    const double TestController::shortTimeout = 10;
    #else
    const double TestController::shortTimeout = 5;
    #endif

But maybe the nested syntax is needed to compile on compilers that don't have __has_feature?

> Tools/WebKitTestRunner/TestController.h:56
> +    constexpr static double noTimeout = -1;

Does this work on Windows? I had problems with constexpr on Windows.
Comment 9 Darin Adler 2015-01-02 10:52:53 PST
Comment on attachment 243901 [details]
proposed patch

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

>> Tools/WebKitTestRunner/TestController.cpp:81
>> +#endif
> 
> I would have written:
> 
>     #if defined(__has_feature) && __has_feature(address_sanitizer)
>     const double TestController::shortTimeout = 10;
>     #else
>     const double TestController::shortTimeout = 5;
>     #endif
> 
> But maybe the nested syntax is needed to compile on compilers that don't have __has_feature?

Oops, I see you discussing this earlier in this bug!
Comment 10 Alexey Proskuryakov 2015-01-02 14:51:12 PST
Committed <http://trac.webkit.org/r177870>. Removed the use of constexpr to be safe, as Windows EWS is not functional.