Bug 140036

Summary: Simplify WebKitTestRunner timeout tracking
Product: WebKit Reporter: Alexey Proskuryakov <ap>
Component: Tools / TestsAssignee: Alexey Proskuryakov <ap>
Status: RESOLVED FIXED    
Severity: Normal CC: cgarcia, clopez, kbalazs, ossy
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
proposed patch
none
proposed patch
none
proposed patch
none
proposed patch darin: review+

Alexey Proskuryakov
Reported 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.
Attachments
proposed patch (9.15 KB, patch)
2015-01-01 16:54 PST, Alexey Proskuryakov
no flags
proposed patch (10.34 KB, patch)
2015-01-01 23:53 PST, Alexey Proskuryakov
no flags
proposed patch (10.40 KB, patch)
2015-01-02 09:50 PST, Alexey Proskuryakov
no flags
proposed patch (10.41 KB, patch)
2015-01-02 10:48 PST, Alexey Proskuryakov
darin: review+
Alexey Proskuryakov
Comment 1 2015-01-01 16:54:34 PST
Created attachment 243875 [details] proposed patch
Alexey Proskuryakov
Comment 2 2015-01-01 23:53:37 PST
Created attachment 243882 [details] proposed patch With Efl and Gtk build fixes.
Csaba Osztrogonác
Comment 3 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.
Csaba Osztrogonác
Comment 4 2015-01-02 01:56:27 PST
It's strange how GTK built it. I thought GTK uses GCC 4.8 too.
Alexey Proskuryakov
Comment 5 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.
Darin Adler
Comment 6 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.
Alexey Proskuryakov
Comment 7 2015-01-02 10:48:04 PST
Created attachment 243901 [details] proposed patch
Darin Adler
Comment 8 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.
Darin Adler
Comment 9 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!
Alexey Proskuryakov
Comment 10 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.
Note You need to log in before you can comment on or make changes to this bug.