Bug 79859 – Customize layout test timeout value for different ports.

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Hao Zheng]

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.

[Johnny(Jianning) Ding]

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?

[Johnny(Jianning) Ding]

@Adam, would you please give comment?

[Adam Barth]

> Does it sound good?

Sounds fine.  Pls post a patch.

[Johnny(Jianning) Ding]

Created attachment 132778 [details]
patch v1

[Ojan Vafai]

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?

[Tony Chang]

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.

[Ojan Vafai]

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?

[Tony Chang]

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.

[Ojan Vafai]

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.

[Johnny(Jianning) Ding]

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

[Johnny(Jianning) Ding]

Created attachment 132945 [details]
patch v2

[Ojan Vafai]

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.

[Johnny(Jianning) Ding]

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.

[WebKit Review Bot]

Comment on attachment 132985 [details]
patch v3

Clearing flags on attachment: 132985

Committed r111642: <http://trac.webkit.org/changeset/111642>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Jessie Berlin]

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

[Johnny(Jianning) Ding]

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

[Johnny(Jianning) Ding]

(In reply to comment #17)

Seems that has been fixed in http://trac.webkit.org/changeset/111662

[Jessie Berlin]

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