Bug 102184 - Reduce XHR timeout tests execution time
Summary: Reduce XHR timeout tests execution time
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dominik Röttsches (drott)
Depends on:
Reported: 2012-11-13 23:16 PST by Dominik Röttsches (drott)
Modified: 2012-11-27 05:28 PST (History)
2 users (show)

See Also:

Patch (10.65 KB, patch)
2012-11-27 04:43 PST, Allan Sandfeld Jensen
ossy: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dominik Röttsches (drott) 2012-11-13 23:16:57 PST
The XHR timeout tests use values of 2000, 3000 and 5000ms. This results in a pretty high total execution time.
Let's tune this to lower values.
Comment 1 Allan Sandfeld Jensen 2012-11-27 04:43:39 PST
Created attachment 176235 [details]
Comment 2 Dominik Röttsches (drott) 2012-11-27 05:00:36 PST
Comment on attachment 176235 [details]

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

Nice, thanks for doing this useful work. (Disclaimer: I am not a reviewer, just doing an informal review since I adapted the tests originally). Other than those minor suggestions, looks good to me.

> LayoutTests/ChangeLog:8
> +        Reduce all timeouts by five.

by five -> to a fifth of the previous value. But aren't you reducing them only to a third?

> LayoutTests/http/tests/resources/load-and-stall.cgi:25
> +            Time::HiRes::sleep($stallFor)

Did you check whether the other tests using load-and-stall.cgi continue to work fine? I'd recommend to put this change into a separate bug and land this first, see whether the bots stay green. Once it's in the tree, land the rest of the change. This would help avoiding someone rolling out the whole time adjustment change.

> LayoutTests/http/tests/xmlhttprequest/timeout/xmlhttprequest-timeout.js:14
> +var timeNormalLoad = 1000;

I'd spell these capitalized to indicate they're constant, e.g. TIME_NORMAL_LOAD, TIME_LATE_TIMEOUT.

> LayoutTests/http/tests/xmlhttprequest/timeout/xmlhttprequest-timeout.js:17
> +var timeTimeout = 400;

As mentioned below when commenting on usage of timeDelay/TIME_DELAY, you could have an extra timeout value here: TIME_REGULAR_TIMEOUT and TIME_SYNC_TIMEOUT.

> LayoutTests/http/tests/xmlhttprequest/timeout/xmlhttprequest-timeout.js:21
> + * This should point to a resource that responds after a delay.

You could make this comment include the variable, e.g. "This should point to a resource that responds after a delay of TIME_XHR_LOAD_EVENT."

> LayoutTests/http/tests/xmlhttprequest/timeout/xmlhttprequest-timeout.js:270
> +      req.timeout = timeDelay;

timeDelay has a different meaning in the other cases it's used, i.e. it describes when to do a late update to the timer. I'd say we should use a separate constant here, something like TIME_SYNC_TIMEOUT.

> LayoutTests/http/tests/xmlhttprequest/timeout/xmlhttprequest-timeout.js:284
> +    req.timeout = timeDelay;

Comment 3 Csaba Osztrogonác 2012-11-27 05:00:52 PST
Comment on attachment 176235 [details]

220 secs -> 48 secs, nice progression ;)
Comment 4 Csaba Osztrogonác 2012-11-27 05:04:32 PST
I just saw Dominik's comment, it sounds reasonable. Could you fix them before landing?
Comment 5 Dominik Röttsches (drott) 2012-11-27 05:09:54 PST
(In reply to comment #2)
> (From update of attachment 176235 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176235&action=review

> > LayoutTests/ChangeLog:8
> > +        Reduce all timeouts by five.
> by five -> to a fifth of the previous value. But aren't you reducing them only to a third?

Sorry, I checked again, a fifth it is - for the important values. I was mislead by the line that removes the testharness.js timeout factor "3 *".
Comment 6 Allan Sandfeld Jensen 2012-11-27 05:28:49 PST
Committed r135852: <http://trac.webkit.org/changeset/135852>