Closed Bug 662612 Opened 13 years ago Closed 13 years ago

rewrite test_websocket.html to be faster

Categories

(Core :: Networking: WebSockets, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla8

People

(Reporter: mcmanus, Assigned: mcmanus)

Details

(Whiteboard: [inbound])

Attachments

(1 file, 2 obsolete files)

These old tests are one of the longest polls in the mochitest-1 tent, mostly due to being organized around very long timeouts (which can also lead to random oranges).

-------- Forwarded Message --------
> From: Boris Zbarsky <bzbarsky@mit.edu>
> To: dev-planning@lists.mozilla.org
> Subject: Re: Notes on ways to improve the build/automation turn around
> time
> Date: Fri, 27 May 2011 15:33:11 -0400
> Newsgroups: mozilla.dev.planning
> 
> On 5/27/11 3:21 PM, jmaher wrote:
> > 35404 INFO TEST-END | /tests/content/base/test/test_websocket.html |
> > finished in 128442ms
> > 36529 INFO TEST-END | /tests/content/base/test/
> > test_ws_basic_tests.html | finished in 116905ms
> 
> As I recall those are just buggy: they use large setTimeouts all over 
> and stuff.  Can we just fix them?

so I inherited these tests, but I run them all the time - so I know how
annoying they are :)

Improving them is on my list - but I'm actually battling with one of
them right now (that fails intermittently in conjunction with a feature
that is not enabled on a anything) so I certainly won't touch them until
that is figured out.
Attached patch patch 1 (obsolete) — Splinter Review
I took a pass at this - 
 * parallelized a bunch of tests with timeouts in them
 * lowered some timeouts that could be controlled by pref
 * made the test completion be a little more event driven in the "green" case instead of checking on everything after a 60 second timeout.

note that websockets has a rule about starting more than 1 connection at a time to the same host (the restriction is on the connection phase, not the established phase) so I used a variety of hostnames in the tests :)

it's not great but it seems to bring execution time down from 120-180 sec to 10-15 sec without reducing the testing surface.

I will put it through a fuller set of try runs.
Assignee: nobody → mcmanus
Status: NEW → ASSIGNED
Summary: rewerite test_websocket.html to be faster → rewrite test_websocket.html to be faster
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #549942 - Attachment is obsolete: true
Attachment #550255 - Flags: review?(bzbarsky)
Why do we want the finishWSTest thing running off a timer?  Could we not just call it from all the places where we set waitTest* to false and bail out from it if not all the waits are done or not all the tests have been started yet?

As written, this test looks like it will fail if it takes longer than 60s due to machine load, for example....
(In reply to comment #3)
> Why do we want the finishWSTest thing running off a timer?  Could we not
> just call it from all the places where we set waitTest* to false and bail
> out from it if not all the waits are done or not all the tests have been
> started yet?

> 
> As written, this test looks like it will fail if it takes longer than 60s
> due to machine load, for example....


it always ran off a timer to completion.. I set out to improve "firing too late", not "firing too early" :)

I can update that too, though.
Attached patch patch 3Splinter Review
this one is fully event driven
Attachment #550255 - Attachment is obsolete: true
Attachment #550255 - Flags: review?(bzbarsky)
Attachment #550364 - Flags: review?(bzbarsky)
> I set out to improve "firing too late", not "firing too early" :)

The difference is that "firing too late" makes the test run slowly, while "firing too early" makes randomorange. ;)
Comment on attachment 550364 [details] [diff] [review]
patch 3

Most of this looks great.  The nits:

>+  if (!ranAllTests)
>+  return false;

Fix the indent?  But also, the return value is ignored by all callers; why do we even need one?

>+  if (!(!waitTest2Part1  && 

  if (waitTest2Part1 || waitTest2Part2 || ....

and also fix the indentation here.

>-{
>+{ 

Don't add that stray space, please.

r=me with those dealt with.
Attachment #550364 - Flags: review?(bzbarsky) → review+
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
From OS X opt the changeset before this landed on inbound:
/tests/content/base/test/test_websocket.html | finished in 146648ms

With this changeset added:
/tests/content/base/test/test_websocket.html | finished in 9034ms

yea!
Mmmm.... the smell of 2+ minutes shaved off the test run time in the morning!
http://hg.mozilla.org/mozilla-central/rev/a8045a08cbc9
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: