Bug 102184 – Reduce XHR timeout tests execution time

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Dominik Röttsches (drott)]

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.

[Allan Sandfeld Jensen]

Created attachment 176235 [details]
Patch

[Dominik Röttsches (drott)]

Comment on attachment 176235 [details]
Patch

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;

Ditto.

[Csaba Osztrogonác]

Comment on attachment 176235 [details]
Patch

220 secs -> 48 secs, nice progression ;)

[Csaba Osztrogonác]

I just saw Dominik's comment, it sounds reasonable. Could you fix them before landing?

[Dominik Röttsches (drott)]

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

[Allan Sandfeld Jensen]

Committed r135852: <http://trac.webkit.org/changeset/135852>