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.
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?
@Adam, would you please give comment?
> Does it sound good? Sounds fine. Pls post a patch.
Created attachment 132778 [details] patch v1
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 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 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 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 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.
(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.
Created attachment 132945 [details] patch v2
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.
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 on attachment 132985 [details] patch v3 Clearing flags on attachment: 132985 Committed r111642: <http://trac.webkit.org/changeset/111642>
All reviewed patches have been landed. Closing bug.
(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.
(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.
(In reply to comment #17) Seems that has been fixed in http://trac.webkit.org/changeset/111662
(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.