Bug 79859

Summary: Customize layout test timeout value for different ports.
Product: WebKit Reporter: Hao Zheng <zhenghao>
Component: Tools / TestsAssignee: Hao Zheng <zhenghao>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dpranke, jberlin, jnd, ojan, shezbaig.wk, webkit.review.bot
Priority: P3    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch v1
ojan: review-
patch v2
ojan: review-
patch v3 none

Description Hao Zheng 2012-02-28 19:34:43 PST
Some ports, like chromium-android port, essentially have less power than desktop ones. As a result, some tests that could normally pass on desktop with timeout value 6 sec timeout for these ports. We have to add SLOW for all those tests, so I think it would be more convenient if the timeout value could be customized.
Comment 1 Johnny(Jianning) Ding 2012-03-09 07:48:28 PST
Currently webkit use a option "--time-out-ms" to control the timeout for each test, if it is not specified in command line, the default value 6000ms (defined in Manager.DEFAULT_TEST_TIMEOUT_MS) will be used, and the test marked as "SLOW" uses 5 times of time-out-ms as the timeout for slow test.

However Webkit now supports multiple platforms (AKA: ports) and their computing power are not same, using same default value and multiplier for all platforms seems not proper.
I know we can set different timeout for different platform by leveraging option "--time-out-ms", but it is redundant to repeat passing the value to a platform you already know it always needs a special timeout rather than the default 6000ms. So I think it will be better if we can give each platform a ability to  customize its timeout/slow-timeout threshold.

The change seems pretty straightforward, the pseudo code in class Port looks like

Class Port(object)
  ...
  PLATFORM_DEFAULT_TIMEOUT_MS = 6000
  def get_default_timeout(for_slow_test):
    return x * LATFORM_DEFAULT_TIMEOUT_MS if for_slow_test else PLATFORM_DEFAULT_TIMEOUT_MS
  ...

Then each platform can have its own timeout/slow-timeout setting in its own Port implementation.

Does it sound good?
Comment 2 Johnny(Jianning) Ding 2012-03-12 14:47:03 PDT
@Adam, would you please give comment?
Comment 3 Adam Barth 2012-03-12 15:29:49 PDT
> Does it sound good?

Sounds fine.  Pls post a patch.
Comment 4 Johnny(Jianning) Ding 2012-03-20 01:47:38 PDT
Created attachment 132778 [details]
patch v1
Comment 5 Ojan Vafai 2012-03-20 10:33:29 PDT
Comment on attachment 132778 [details]
patch v1

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

> Tools/Scripts/webkitpy/layout_tests/port/base.py:88
> +    # The per-test timeout in milliseconds, if no --time-out-ms option was
> +    # given to run_webkit_tests. Subclass can override it in default_test_timeout_ms
> +    # according to the computing power on its platform.

This comment doesn't really add much value. It's pretty clear what DEFAULT_TEST_TIMEOUT_MS refers to.

> Tools/Scripts/webkitpy/layout_tests/port/base.py:93
> +    # The multiplier for per-test slow timeout. Subclass can override it in
> +    # default_slowtest_timeout_multiplier according to the computing power on its platform.
> +    DEFAULT_SLOWTEST_TIMEOUT_MULTIPLIER = 5

I don't really see the need for ports to make this configurable since it's a multiplier of the test timeout, which is configurable. Lets cut this out and add it in when we find a port actually needs it.

> Tools/Scripts/webkitpy/layout_tests/port/base.py:157
> +    def default_slowtest_timeout_multiplier(self):
> +        return self.DEFAULT_SLOWTEST_TIMEOUT_MULTIPLIER

Ditto: above, lets remove this.

> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:620
> +            kill_timeout_seconds = 3.0 * int(time_out_ms) / Port.DEFAULT_TEST_TIMEOUT_MS

Shouldn't this be using default_test_timeout_ms?
Comment 6 Tony Chang 2012-03-20 11:09:05 PDT
Comment on attachment 132778 [details]
patch v1

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

>> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:620
>> +            kill_timeout_seconds = 3.0 * int(time_out_ms) / Port.DEFAULT_TEST_TIMEOUT_MS
> 
> Shouldn't this be using default_test_timeout_ms?

This is how long to wait for a process to shutdown before we just kill the process.  We try to cleanly shutdown DRT, but if it doesn't stop after a few seconds, we just kill it.
Comment 7 Ojan Vafai 2012-03-20 11:17:58 PDT
Comment on attachment 132778 [details]
patch v1

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

>>> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:620
>>> +            kill_timeout_seconds = 3.0 * int(time_out_ms) / Port.DEFAULT_TEST_TIMEOUT_MS
>> 
>> Shouldn't this be using default_test_timeout_ms?
> 
> This is how long to wait for a process to shutdown before we just kill the process.  We try to cleanly shutdown DRT, but if it doesn't stop after a few seconds, we just kill it.

Right, but the number we divide by should be default_test_timeout_ms, not Port.DEFAULT_TEST_TIMEOUT_MS, right?
Comment 8 Tony Chang 2012-03-20 11:27:30 PDT
Comment on attachment 132778 [details]
patch v1

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

>>>> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:620
>>>> +            kill_timeout_seconds = 3.0 * int(time_out_ms) / Port.DEFAULT_TEST_TIMEOUT_MS
>>> 
>>> Shouldn't this be using default_test_timeout_ms?
>> 
>> This is how long to wait for a process to shutdown before we just kill the process.  We try to cleanly shutdown DRT, but if it doesn't stop after a few seconds, we just kill it.
> 
> Right, but the number we divide by should be default_test_timeout_ms, not Port.DEFAULT_TEST_TIMEOUT_MS, right?

Either seems OK in this case, I doubt it matters much.  If someone runs chromium-android with --time-out-ms=60000, we will wait 30s or 18s.  This was originally added because running DRT under valgrind takes a long time to shutdown, but I suspect either 30s or 18s is fine for android.
Comment 9 Ojan Vafai 2012-03-20 11:41:50 PDT
Comment on attachment 132778 [details]
patch v1

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

>>>>> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:620
>>>>> +            kill_timeout_seconds = 3.0 * int(time_out_ms) / Port.DEFAULT_TEST_TIMEOUT_MS
>>>> 
>>>> Shouldn't this be using default_test_timeout_ms?
>>> 
>>> This is how long to wait for a process to shutdown before we just kill the process.  We try to cleanly shutdown DRT, but if it doesn't stop after a few seconds, we just kill it.
>> 
>> Right, but the number we divide by should be default_test_timeout_ms, not Port.DEFAULT_TEST_TIMEOUT_MS, right?
> 
> Either seems OK in this case, I doubt it matters much.  If someone runs chromium-android with --time-out-ms=60000, we will wait 30s or 18s.  This was originally added because running DRT under valgrind takes a long time to shutdown, but I suspect either 30s or 18s is fine for android.

default_test_timeout_ms seems more correct to me. We already have access to the port object, so it's just a simple function call. You could imagine a future world in which the port's default_test_timeout_ms is smaller than Port.DEFAULT_TEST_TIMEOUT_MS, in which case, this would be doing the wrong thing.
Comment 10 Johnny(Jianning) Ding 2012-03-20 18:49:12 PDT
(In reply to comment #9)

> default_test_timeout_ms seems more correct to me. We already have access to the port object, so it's just a simple function call. You could imagine a future world in which the port's default_test_timeout_ms is smaller than Port.DEFAULT_TEST_TIMEOUT_MS, in which case, this would be doing the wrong thing.


If the input time_out_ms is smaller than the port's default_test_timeout_ms, which can lead same wrong. We probably should check it in the beginning.
Comment 11 Johnny(Jianning) Ding 2012-03-20 19:04:20 PDT
Created attachment 132945 [details]
patch v2
Comment 12 Ojan Vafai 2012-03-20 21:27:20 PDT
Comment on attachment 132945 [details]
patch v2

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

> Tools/Scripts/webkitpy/layout_tests/run_webkit_tests.py:165
> +    else:
> +        if int(options.time_out_ms) < port_default_timeout_ms:
> +            options.time_out_ms = str(port_default_timeout_ms)

You should be able to set the timeout to less than the port default.  I don't think we want this else statement. See my other coment below.

> Tools/Scripts/webkitpy/layout_tests/port/base.py:86
> +    DEFAULT_TEST_TIMEOUT_MS = 6 * 1000

Nit: I don't think we need this variable. You can just move the 6*1000 into default_test_timeout_ms

> Tools/Scripts/webkitpy/layout_tests/port/chromium.py:622
>          if time_out_ms and not self._no_timeout:
> -            # FIXME: Port object shouldn't be dependent on layout test manager.
> -            kill_timeout_seconds = 3.0 * int(time_out_ms) / Manager.DEFAULT_TEST_TIMEOUT_MS
> +            kill_timeout_seconds = 3.0 * int(time_out_ms) / self._port.default_test_timeout_ms()
>          else:
>              kill_timeout_seconds = 3.0

If you want, you can make sure that kill_timeout_seconds is at least 3.0 here. That would deal with the case of time_out_ms being less than default_test_timeout_ms.

Really this chunk of code is very confusing. Seems like we should always just set kill_timeout_seconds to 3.0.
Comment 13 Johnny(Jianning) Ding 2012-03-21 00:44:48 PDT
Created attachment 132985 [details]
patch v3

(In reply to comment #12)
> If you want, you can make sure that kill_timeout_seconds is at least 3.0 here. That would deal with the case of time_out_ms being less than default_test_timeout_ms.
> 
> Really this chunk of code is very confusing. Seems like we should always just set kill_timeout_seconds to 3.0.

I don't know the context of this chunk, but if time_out_ms is greater than default_test_timeout_ms, the kill_timeout_seconds is greater than 3.0
Patch3 is  going to make sure that kill_timeout_seconds is at least 3.0.
Comment 14 WebKit Review Bot 2012-03-21 19:26:37 PDT
Comment on attachment 132985 [details]
patch v3

Clearing flags on attachment: 132985

Committed r111642: <http://trac.webkit.org/changeset/111642>
Comment 15 WebKit Review Bot 2012-03-21 19:26:42 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Jessie Berlin 2012-03-22 08:47:29 PDT
(In reply to comment #15)
> All reviewed patches have been landed.  Closing bug.

This caused 2 python tests to start failing on the Lion Release testers:

http://build.webkit.org/builders/Lion%20Intel%20Release%20%28Tests%29/builds/6708
http://build.webkit.org/builders/Lion%20Intel%20Release%20%28Tests%29/builds/6709/steps/webkitpy-test/logs/stdio

Suppressing most webkitpy logging while running unit tests.
Skipping tests in the following modules or packages because they are really, really slow:
    webkitpy.common.checkout.scm.scm_unittest
    (https://bugs.webkit.org/show_bug.cgi?id=31818; use --all to include)

...........................................................................................................................................................................................................................................................................................................................................................................................................................................................................................E.E..........................................................................................................................................................................................................................................................................................this should be logged
..............................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................
======================================================================
ERROR: test_stop (webkitpy.layout_tests.port.chromium_unittest.ChromiumDriverTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Volumes/Data/slave/lion-intel-release-tests/build/Tools/Scripts/webkitpy/layout_tests/port/chromium_unittest.py", line 136, in test_stop
    self.driver.stop()
  File "/Volumes/Data/slave/lion-intel-release-tests/build/Tools/Scripts/webkitpy/layout_tests/port/chromium.py", line 627, in stop
    timeout_ratio = float(time_out_ms) / self._port.default_test_timeout_ms()
TypeError: unsupported operand type(s) for /: 'float' and 'Mock'

======================================================================
ERROR: test_two_drivers (webkitpy.layout_tests.port.chromium_unittest.ChromiumDriverTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Volumes/Data/slave/lion-intel-release-tests/build/Tools/Scripts/webkitpy/layout_tests/port/chromium_unittest.py", line 158, in test_two_drivers
    driver1.stop()
  File "/Volumes/Data/slave/lion-intel-release-tests/build/Tools/Scripts/webkitpy/layout_tests/port/chromium.py", line 627, in stop
    timeout_ratio = float(time_out_ms) / self._port.default_test_timeout_ms()
TypeError: unsupported operand type(s) for /: 'float' and 'Mock'

----------------------------------------------------------------------
Ran 1318 tests in 14.478s

http://build.webkit.org/builders/Lion%20Intel%20Release%20%28WebKit2%20Tests%29/builds/5606
http://build.webkit.org/builders/Lion%20Intel%20Release%20%28WebKit2%20Tests%29/builds/5607/steps/webkitpy-test/logs/stdio

Suppressing most webkitpy logging while running unit tests.
Skipping tests in the following modules or packages because they are really, really slow:
    webkitpy.common.checkout.scm.scm_unittest
    (https://bugs.webkit.org/show_bug.cgi?id=31818; use --all to include)

...........................................................................................................................................................................................................................................................................................................................................................................................................................................................................................E.E..........................................................................................................................................................................................................................................................................................this should be logged
..............................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................
======================================================================
ERROR: test_stop (webkitpy.layout_tests.port.chromium_unittest.ChromiumDriverTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Volumes/Data/slave/lion-intel-release-tests-wk2/build/Tools/Scripts/webkitpy/layout_tests/port/chromium_unittest.py", line 136, in test_stop
    self.driver.stop()
  File "/Volumes/Data/slave/lion-intel-release-tests-wk2/build/Tools/Scripts/webkitpy/layout_tests/port/chromium.py", line 627, in stop
    timeout_ratio = float(time_out_ms) / self._port.default_test_timeout_ms()
TypeError: unsupported operand type(s) for /: 'float' and 'Mock'

======================================================================
ERROR: test_two_drivers (webkitpy.layout_tests.port.chromium_unittest.ChromiumDriverTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Volumes/Data/slave/lion-intel-release-tests-wk2/build/Tools/Scripts/webkitpy/layout_tests/port/chromium_unittest.py", line 158, in test_two_drivers
    driver1.stop()
  File "/Volumes/Data/slave/lion-intel-release-tests-wk2/build/Tools/Scripts/webkitpy/layout_tests/port/chromium.py", line 627, in stop
    timeout_ratio = float(time_out_ms) / self._port.default_test_timeout_ms()
TypeError: unsupported operand type(s) for /: 'float' and 'Mock'

----------------------------------------------------------------------
Ran 1318 tests in 15.276s

FAILED (errors=2)


Please fix this, or this patch will have to be rolled out.
Comment 17 Johnny(Jianning) Ding 2012-03-22 09:20:50 PDT
(In reply to comment #16)

> Please fix this, or this patch will have to be rolled out.

Adding "mock_port.default_test_timeout_ms = lambda : 6 * 1000" after "mock_port = Mock()" can fix it. A patch will be posted soon.
Comment 18 Johnny(Jianning) Ding 2012-03-22 09:31:39 PDT
(In reply to comment #17)

Seems that has been fixed in http://trac.webkit.org/changeset/111662
Comment 19 Jessie Berlin 2012-03-22 09:42:39 PDT
(In reply to comment #18)
> (In reply to comment #17)
> 
> Seems that has been fixed in http://trac.webkit.org/changeset/111662

Ah, looks like the release builders got stuck, which is why it didn't look fixed yet. Apologies for not double checking.