Changeset 177363 in webkit


Ignore:
Timestamp:
Dec 16, 2014 10:32:21 AM (9 years ago)
Author:
ap@apple.com
Message:

Sort out timeout implementations in DRT and WKTR
​https://bugs.webkit.org/show_bug.cgi?id=139671

Reviewed by Simon Fraser.

Test timeout implementation had many deficiencies, please see the bug for details.
Most notably, we shouldn't have the tool confused about timeouts vs. failures, and
[ Slow ] modifiers should work a lot better.

  • DumpRenderTree/TestRunner.cpp: (TestRunner::TestRunner):
  • DumpRenderTree/TestRunner.h: (TestRunner::setCustomTimeout):
  • DumpRenderTree/mac/DumpRenderTree.mm: (runTest):
  • DumpRenderTree/mac/TestRunnerMac.mm: (TestRunner::setWaitToDump):

DumpRenderTree already read the --timeout option from command line, and webkitpy
was already configured to pass it on Mac and iOS. Let's actually use it.
TestCommand already had the same 30 second default, so this doesn't change behavior
when DRT is ran manually without the option.
Windows DumpRenderTree will need to be fixed separately (that's easy).

  • DumpRenderTree/TestRunner.cpp: (TestRunner::waitToDumpWatchdogTimerFired()):

Don't print the timeout message to stdout to match WebKitTestRunner. It would be
slightly better to use stderr in both, as this is an out of band message, but
that's a larger refactoring, and the difference is minimal in practice.

  • Scripts/webkitpy/layout_tests/controllers/single_test_runner.py:

(SingleTestRunner.init): Ensure that script and tool timeouts are substantially
different. We want the tool to reliably detect timeouts that can be detected, and
not race with the script for that.

  • Scripts/webkitpy/port/base.py: (Port.default_timeout_ms): Don't make WebKit2

timeout longer than WebKit1 one, I doubt that this is necessary. Now that the value
is honored inmore cases, that could make tests run slower.

  • Scripts/webkitpy/port/driver.py:

(Driver.init):
(Driver.run_test):
(Driver.cmd_line):
(Driver._check_for_driver_timeout):
Detect tests that have the timeout output, and make these have the proper Timeout result.

  • Scripts/webkitpy/port/ios.py: (IOSSimulatorPort.default_timeout_ms): Remove an

incorrect recent change - 80 * 1000 is 80 seconds, not 80 milliseconds.

  • WebKitTestRunner/InjectedBundle/TestRunner.cpp:

(WTR::TestRunner::setCustomTimeout): Deleted.

  • WebKitTestRunner/InjectedBundle/TestRunner.h:

(WTR::TestRunner::setCustomTimeout):

  • WebKitTestRunner/InjectedBundle/efl/TestRunnerEfl.cpp:

(WTR::TestRunner::initializeWaitToDumpWatchdogTimerIfNeeded):

  • WebKitTestRunner/InjectedBundle/mac/TestRunnerMac.mm:

(WTR::TestRunner::initializeWaitToDumpWatchdogTimerIfNeeded):

  • WebKitTestRunner/InjectedBundle/win/TestRunnerWin.cpp:

(WTR::TestRunner::initializeWaitToDumpWatchdogTimerIfNeeded):

  • WebKitTestRunner/InjectedBundle/gtk/TestRunnerGtk.cpp:

(WTR::TestRunner::initializeWaitToDumpWatchdogTimerIfNeeded):
Updated to use a timeout passed from UI process, which used to be ignored.

  • WebKitTestRunner/TestController.cpp:

(WTR::TestController::TestController):
(WTR::TestController::runUntil):
(WTR::TestController::getCustomTimeout): Deleted.

  • WebKitTestRunner/TestController.h:

Delete unused m_timeout. First, it was always 0, and second, we don't need it at all.
Changed default message timeouts to match new run-webkit-tests timeout. These don't
affect ports where timeout is passed per test (shouldn't they all be like that?).

  • WebKitTestRunner/TestInvocation.cpp:

(WTR::TestInvocation::invoke):
(WTR::TestInvocation::setCustomTimeout): Deleted.

  • WebKitTestRunner/TestInvocation.h:

(WTR::TestInvocation::setCustomTimeout):
(WTR::TestInvocation::customTimeout):
Ditto.

Location:
trunk/Tools
Files:
19 edited

Legend:

Unmodified
Added
Removed

  • trunk/Tools/ChangeLog

    r177357 r177363  
     12014-12-15  Alexey Proskuryakov  <ap@apple.com>
     2
     3        Sort out timeout implementations in DRT and WKTR
     4        https://bugs.webkit.org/show_bug.cgi?id=139671
     5
     6        Reviewed by Simon Fraser.
     7
     8        Test timeout implementation had many deficiencies, please see the bug for details.
     9        Most notably, we shouldn't have the tool confused about timeouts vs. failures, and
     10        [ Slow ] modifiers should work a lot better.
     11
     12        * DumpRenderTree/TestRunner.cpp: (TestRunner::TestRunner):
     13        * DumpRenderTree/TestRunner.h: (TestRunner::setCustomTimeout):
     14        * DumpRenderTree/mac/DumpRenderTree.mm: (runTest):
     15        * DumpRenderTree/mac/TestRunnerMac.mm: (TestRunner::setWaitToDump):
     16        DumpRenderTree already read the --timeout option from command line, and webkitpy
     17        was already configured to pass it on Mac and iOS. Let's actually use it.
     18        TestCommand already had the same 30 second default, so this doesn't change behavior
     19        when DRT is ran manually without the option.
     20        Windows DumpRenderTree will need to be fixed separately (that's easy).
     21
     22        * DumpRenderTree/TestRunner.cpp: (TestRunner::waitToDumpWatchdogTimerFired()):
     23        Don't print the timeout message to stdout to match WebKitTestRunner. It would be
     24        slightly better to use stderr in both, as this is an out of band message, but
     25        that's a larger refactoring, and the difference is minimal in practice.
     26
     27        * Scripts/webkitpy/layout_tests/controllers/single_test_runner.py:
     28        (SingleTestRunner.__init__): Ensure that script and tool timeouts are substantially
     29        different. We want the tool to reliably detect timeouts that can be detected, and
     30        not race with the script for that.
     31
     32        * Scripts/webkitpy/port/base.py: (Port.default_timeout_ms): Don't make WebKit2
     33        timeout longer than WebKit1 one, I doubt that this is necessary. Now that the value
     34        is honored inmore cases, that could make tests run slower.
     35        * Scripts/webkitpy/port/driver.py:
     36        (Driver.__init__):
     37        (Driver.run_test):
     38        (Driver.cmd_line):
     39        (Driver._check_for_driver_timeout):
     40        Detect tests that have the timeout output, and make these have the proper Timeout result.
     41
     42        * Scripts/webkitpy/port/ios.py: (IOSSimulatorPort.default_timeout_ms): Remove an
     43        incorrect recent change - 80 * 1000 is 80 seconds, not 80 milliseconds.
     44
     45        * WebKitTestRunner/InjectedBundle/TestRunner.cpp:
     46        (WTR::TestRunner::setCustomTimeout): Deleted.
     47        * WebKitTestRunner/InjectedBundle/TestRunner.h:
     48        (WTR::TestRunner::setCustomTimeout):
     49        * WebKitTestRunner/InjectedBundle/efl/TestRunnerEfl.cpp:
     50        (WTR::TestRunner::initializeWaitToDumpWatchdogTimerIfNeeded):
     51        * WebKitTestRunner/InjectedBundle/mac/TestRunnerMac.mm:
     52        (WTR::TestRunner::initializeWaitToDumpWatchdogTimerIfNeeded):
     53        * WebKitTestRunner/InjectedBundle/win/TestRunnerWin.cpp:
     54        (WTR::TestRunner::initializeWaitToDumpWatchdogTimerIfNeeded):
     55        * WebKitTestRunner/InjectedBundle/gtk/TestRunnerGtk.cpp:
     56        (WTR::TestRunner::initializeWaitToDumpWatchdogTimerIfNeeded):
     57        Updated to use a timeout passed from UI process, which used to be ignored.
     58
     59        * WebKitTestRunner/TestController.cpp:
     60        (WTR::TestController::TestController):
     61        (WTR::TestController::runUntil):
     62        (WTR::TestController::getCustomTimeout): Deleted.
     63        * WebKitTestRunner/TestController.h:
     64        Delete unused m_timeout. First, it was always 0, and second, we don't need it at all.
     65        Changed default message timeouts to match new run-webkit-tests timeout. These don't
     66        affect ports where timeout is passed per test (shouldn't they all be like that?).
     67
     68        * WebKitTestRunner/TestInvocation.cpp:
     69        (WTR::TestInvocation::invoke):
     70        (WTR::TestInvocation::setCustomTimeout): Deleted.
     71        * WebKitTestRunner/TestInvocation.h:
     72        (WTR::TestInvocation::setCustomTimeout):
     73        (WTR::TestInvocation::customTimeout):
     74        Ditto.
     75
    1762014-12-16  Grzegorz Czajkowski  <g.czajkowski@samsung.com>
    277

  • trunk/Tools/DumpRenderTree/TestRunner.cpp

    r176677 r177363  
    116116    , m_expectedPixelHash(expectedPixelHash)
    117117    , m_titleTextDirection("ltr")
     118    , m_timeout(0)
    118119{
    119120}
    … …  
    22582259{
    22592260    const char* message = "FAIL: Timed out waiting for notifyDone to be called\n";
    2260     fprintf(stderr, "%s", message);
    22612261    fprintf(stdout, "%s", message);
    22622262    notifyDone();

  • trunk/Tools/DumpRenderTree/TestRunner.h

    r176677 r177363  
    357357    bool hasPendingWebNotificationClick() const { return m_hasPendingWebNotificationClick; }
    358358
     359    void setCustomTimeout(int duration) { m_timeout = duration; }
     360
    359361private:
    360362    TestRunner(const std::string& testURL, const std::string& expectedPixelHash);
    … …  
    432434    static JSStaticValue* staticValues();
    433435    static JSStaticFunction* staticFunctions();
     436
     437    int m_timeout;
    434438};
    435439

  • trunk/Tools/DumpRenderTree/mac/DumpRenderTree.mm

    r176917 r177363  
    18641864
    18651865    gTestRunner = TestRunner::create(testURL, command.expectedPixelHash);
     1866    gTestRunner->setCustomTimeout(command.timeout);
    18661867    topLoadingFrame = nil;
    18671868#if !PLATFORM(IOS)

  • trunk/Tools/DumpRenderTree/mac/TestRunnerMac.mm

    r176677 r177363  
    647647}
    648648
    649 static const CFTimeInterval waitToDumpWatchdogInterval = 30.0;
    650 
    651649static void waitUntilDoneWatchdogFired(CFRunLoopTimerRef timer, void* info)
    652650{
    … …  
    657655{
    658656    m_waitToDump = waitUntilDone;
    659     if (m_waitToDump && shouldSetWaitToDumpWatchdog())
    660         setWaitToDumpWatchdog(CFRunLoopTimerCreate(kCFAllocatorDefault, CFAbsoluteTimeGetCurrent() + waitToDumpWatchdogInterval, 0, 0, 0, waitUntilDoneWatchdogFired, NULL));
     657    if (m_waitToDump && m_timeout && shouldSetWaitToDumpWatchdog())
     658        setWaitToDumpWatchdog(CFRunLoopTimerCreate(kCFAllocatorDefault, CFAbsoluteTimeGetCurrent() + m_timeout / 1000.0, 0, 0, 0, waitUntilDoneWatchdogFired, NULL));
    661659}
    662660

  • trunk/Tools/Scripts/webkitpy/layout_tests/controllers/single_test_runner.py

    r173557 r177363  
    5656        self._results_directory = results_directory
    5757        self._driver = driver
    58         self._timeout = test_input.timeout
    5958        self._worker_name = worker_name
    6059        self._test_name = test_input.test_name
    … …  
    6261        self._reference_files = test_input.reference_files
    6362        self._stop_when_done = stop_when_done
     63
     64        self._timeout = test_input.timeout
     65        if self._timeout > 5000:
     66            # Timeouts are detected by both script and tool; tool detected timeouts are
     67            # better, because they contain partial output. Give the tool some time to
     68            # report the timeout instead of being killed.
     69            self._timeout = int(self._timeout) - 5000
    6470
    6571        if self._reference_files:

  • trunk/Tools/Scripts/webkitpy/port/base.py

    r177034 r177363  
    139139
    140140    def default_timeout_ms(self):
    141         if self.get_option('webkit_test_runner'):
    142             # Add some more time to WebKitTestRunner because it needs to syncronise the state
    143             # with the web process and we want to detect if there is a problem with that in the driver.
    144             return 80 * 1000
    145141        return 35 * 1000
    146142

  • trunk/Tools/Scripts/webkitpy/port/driver.py

    r177034 r177363  
    138138        self._crashed_pid = None
    139139
     140        self._driver_timed_out = False
     141
    140142        # WebKitTestRunner can report back subprocesses that became unresponsive
    141143        # This could mean they crashed.
    … …  
    173175        self.start(driver_input.should_run_pixel_test, driver_input.args)
    174176        test_begin_time = time.time()
     177        self._driver_timed_out = False
    175178        self.error_from_test = str()
    176179        self.err_seen_eof = False
    … …  
    185188        crashed = self.has_crashed()
    186189        timed_out = self._server_process.timed_out
     190        driver_timed_out = self._driver_timed_out
    187191        pid = self._server_process.pid()
    188192
    … …  
    214218        return DriverOutput(text, image, actual_image_hash, audio,
    215219            crash=crashed, test_time=time.time() - test_begin_time, measurements=self._measurements,
    216             timeout=timed_out, error=self.error_from_test,
     220            timeout=timed_out or driver_timed_out, error=self.error_from_test,
    217221            crashed_process_name=self._crashed_process_name,
    218222            crashed_pid=self._crashed_pid, crash_log=crash_log, pid=pid)
    … …  
    350354        if self._no_timeout:
    351355            cmd.append('--no-timeout')
    352         # FIXME: We need to pass --timeout=SECONDS to WebKitTestRunner for WebKit2.
    353356
    354357        cmd.extend(self._port.get_option('additional_drt_flag', []))
    … …  
    359362        cmd.append('-')
    360363        return cmd
     364
     365    def _check_for_driver_timeout(self, out_line):
     366        if out_line == "FAIL: Timed out waiting for notifyDone to be called":
     367            self._driver_timed_out = True
    361368
    362369    def _check_for_driver_crash(self, error_line):

  • trunk/Tools/Scripts/webkitpy/port/ios.py

    r177129 r177363  
    8383        if self.get_option('guard_malloc'):
    8484            return 350 * 1000
    85         if not self.get_option('webkit_test_runner'):
    86             # DumpRenderTree.app waits for the WebThread to run before dumping its output.  In practice
    87             # it seems sufficient to wait up to 80ms to ensure that the WebThread ran and hence output
    88             # for the test is dumped.
    89             return 80 * 1000
    9085        return super(IOSSimulatorPort, self).default_timeout_ms()
    9186

  • trunk/Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp

    r176011 r177363  
    5555
    5656namespace WTR {
    57 
    58 const double TestRunner::waitToDumpWatchdogTimerInterval = 30;
    5957
    6058PassRefPtr<TestRunner> TestRunner::create()
    … …  
    9492    , m_globalFlag(false)
    9593    , m_customFullScreenBehavior(false)
     94    , m_timeout(30000)
    9695    , m_databaseDefaultQuota(-1)
    9796    , m_databaseMaxQuota(-1)
    … …  
    163162
    164163    m_waitToDump = false;
    165 }
    166 
    167 void TestRunner::setCustomTimeout(int timeout)
    168 {
    169     m_timeout = timeout;
    170164}
    171165

  • trunk/Tools/WebKitTestRunner/InjectedBundle/TestRunner.h

    r176921 r177363  
    264264    bool callShouldCloseOnWebView();
    265265
    266     void setCustomTimeout(int duration);
     266    void setCustomTimeout(int duration) { m_timeout = duration; }
    267267
    268268    // Work queue.
    … …  
    281281
    282282private:
    283     static const double waitToDumpWatchdogTimerInterval;
    284 
    285283    TestRunner();
    286284

  • trunk/Tools/WebKitTestRunner/InjectedBundle/efl/TestRunnerEfl.cpp

    r161666 r177363  
    5454        return;
    5555
    56     m_waitToDumpWatchdogTimer = ecore_timer_loop_add(waitToDumpWatchdogTimerInterval,
    57                                                      waitToDumpWatchdogTimerCallback, 0);
     56    m_waitToDumpWatchdogTimer = ecore_timer_loop_add(m_timeout / 1000.0, waitToDumpWatchdogTimerCallback, 0);
    5857}
    5958

  • trunk/Tools/WebKitTestRunner/InjectedBundle/gtk/TestRunnerGtk.cpp

    r176921 r177363  
    5050
    5151    m_waitToDumpWatchdogTimer.scheduleAfterDelay("[WTR] waitToDumpWatchdogTimerCallback", [this] { waitToDumpWatchdogTimerFired(); },
    52         std::chrono::duration_cast<std::chrono::microseconds>(std::chrono::duration<double>(waitToDumpWatchdogTimerInterval)));
     52        std::chrono::duration_cast<std::chrono::microseconds>(std::chrono::duration<double>(m_timeout / 1000.0)));
    5353}
    5454

  • trunk/Tools/WebKitTestRunner/InjectedBundle/mac/TestRunnerMac.mm

    r161666 r177363  
    5353        return;
    5454
    55     m_waitToDumpWatchdogTimer = adoptCF(CFRunLoopTimerCreate(kCFAllocatorDefault, CFAbsoluteTimeGetCurrent() + waitToDumpWatchdogTimerInterval, 0, 0, 0, WTR::waitUntilDoneWatchdogTimerFired, NULL));
     55    CFTimeInterval interval = m_timeout / 1000.0;
     56    m_waitToDumpWatchdogTimer = adoptCF(CFRunLoopTimerCreate(kCFAllocatorDefault, CFAbsoluteTimeGetCurrent() + interval, 0, 0, 0, WTR::waitUntilDoneWatchdogTimerFired, NULL));
    5657    CFRunLoopAddTimer(CFRunLoopGetCurrent(), m_waitToDumpWatchdogTimer.get(), kCFRunLoopCommonModes);
    5758}

  • trunk/Tools/WebKitTestRunner/InjectedBundle/win/TestRunnerWin.cpp

    r161666 r177363  
    5757        return;
    5858
    59     m_waitToDumpWatchdogTimer = ::SetTimer(0, waitToDumpWatchdogTimerIdentifier, waitToDumpWatchdogTimerInterval * 1000, WTR::waitToDumpWatchdogTimerFired);
     59    m_waitToDumpWatchdogTimer = ::SetTimer(0, waitToDumpWatchdogTimerIdentifier, m_timeout, WTR::waitToDumpWatchdogTimerFired);
    6060}
    6161

  • trunk/Tools/WebKitTestRunner/TestController.cpp

    r177234 r177363  
    7171const unsigned TestController::w3cSVGViewHeight = 360;
    7272
    73 // defaultLongTimeout + defaultShortTimeout should be less than 80,
     73// defaultLongTimeout + defaultShortTimeout should be less than 35,
    7474// the default timeout value of the test harness so we can detect an
    7575// unresponsive web process.
    76 static const double defaultLongTimeout = 60;
    77 static const double defaultShortTimeout = 15;
     76// These values are only used by ports that don't have --timeout option passed to WebKitTestRunner.
     77static const double defaultLongTimeout = 25;
     78static const double defaultShortTimeout = 5;
    7879static const double defaultNoTimeout = -1;
    7980
    … …  
    111112    , m_useWaitToDumpWatchdogTimer(true)
    112113    , m_forceNoTimeout(false)
    113     , m_timeout(0)
    114114    , m_didPrintWebProcessCrashedMessage(false)
    115115    , m_shouldExitWhenWebProcessCrashes(true)
    … …  
    193193{
    194194    TestController::shared().handleUserMediaPermissionRequest(permissionRequest);
    195 }
    196 
    197 int TestController::getCustomTimeout()
    198 {
    199     return m_timeout;
    200195}
    201196
    … …  
    980975            timeout = m_longTimeout;
    981976            break;
    982         case CustomTimeout:
    983             timeout = m_timeout;
    984             break;
    985977        case NoTimeout:
    986978        default:

  • trunk/Tools/WebKitTestRunner/TestController.h

    r176011 r177363  
    6969   
    7070    // Runs the run loop until `done` is true or the timeout elapses.
    71     enum TimeoutDuration { ShortTimeout, LongTimeout, NoTimeout, CustomTimeout };
     71    enum TimeoutDuration { ShortTimeout, LongTimeout, NoTimeout };
    7272    bool useWaitToDumpWatchdogTimer() { return m_useWaitToDumpWatchdogTimer; }
    7373    void runUntil(bool& done, TimeoutDuration);
    … …  
    7575   
    7676    void configureViewForTest(const TestInvocation&);
    77 
    78     int getCustomTimeout();
    7977   
    8078    bool beforeUnloadReturnValue() const { return m_beforeUnloadReturnValue; }
    … …  
    232230    bool m_forceNoTimeout;
    233231
    234     int m_timeout;
    235 
    236232    bool m_didPrintWebProcessCrashedMessage;
    237233    bool m_shouldExitWhenWebProcessCrashes;

  • trunk/Tools/WebKitTestRunner/TestInvocation.cpp

    r176659 r177363  
    118118}
    119119
    120 void TestInvocation::setCustomTimeout(int timeout)
    121 {
    122     m_timeout = timeout;
    123 }
    124 
    125120static bool shouldLogFrameLoadDelegates(const char* pathOrURL)
    126121{
    … …  
    165160
    166161    WKContextPostMessageToInjectedBundle(TestController::shared().context(), messageName.get(), beginTestMessageBody.get());
    167 
    168     TestController::TimeoutDuration timeoutToUse = TestController::LongTimeout;
    169162
    170163    TestController::shared().runUntil(m_gotInitialResponse, TestController::ShortTimeout);
    … …  
    179172    WKPageLoadURL(TestController::shared().mainWebView()->page(), m_url.get());
    180173
    181     if (TestController::shared().useWaitToDumpWatchdogTimer()) {
    182         if (m_timeout > 0)
    183             timeoutToUse = TestController::CustomTimeout;
    184     } else
    185         timeoutToUse = TestController::NoTimeout;
    186     TestController::shared().runUntil(m_gotFinalMessage, timeoutToUse);
     174    TestController::shared().runUntil(m_gotFinalMessage, TestController::NoTimeout);
    187175
    188176    if (!m_gotFinalMessage) {

  • trunk/Tools/WebKitTestRunner/TestInvocation.h

    r169845 r177363  
    4545    void setIsPixelTest(const std::string& expectedPixelHash);
    4646
    47     void setCustomTimeout(int duration);
     47    void setCustomTimeout(int duration) { m_timeout = duration; }
     48    int customTimeout() const { return m_timeout; }
    4849
    4950    void invoke();
Note: See TracChangeset for help on using the changeset viewer.