Last Comment Bug 662612 - rewrite test_websocket.html to be faster
: rewrite test_websocket.html to be faster
Status: RESOLVED FIXED
[inbound]
:
Product: Core
Classification: Components
Component: Networking: WebSockets (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla8
Assigned To: Patrick McManus [:mcmanus]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-07 12:59 PDT by Patrick McManus [:mcmanus]
Modified: 2011-08-04 03:11 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch 1 (12.71 KB, patch)
2011-08-01 15:10 PDT, Patrick McManus [:mcmanus]
no flags Details | Diff | Splinter Review
patch v2 (13.18 KB, patch)
2011-08-02 17:00 PDT, Patrick McManus [:mcmanus]
no flags Details | Diff | Splinter Review
patch 3 (12.51 KB, patch)
2011-08-03 06:14 PDT, Patrick McManus [:mcmanus]
bzbarsky: review+
Details | Diff | Splinter Review

Description Patrick McManus [:mcmanus] 2011-06-07 12:59:10 PDT
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.
Comment 1 Patrick McManus [:mcmanus] 2011-08-01 15:10:37 PDT
Created attachment 549942 [details] [diff] [review]
patch 1

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.
Comment 2 Patrick McManus [:mcmanus] 2011-08-02 17:00:33 PDT
Created attachment 550255 [details] [diff] [review]
patch v2
Comment 3 Boris Zbarsky [:bz] 2011-08-02 20:14:55 PDT
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....
Comment 4 Patrick McManus [:mcmanus] 2011-08-03 05:33:54 PDT
(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.
Comment 5 Patrick McManus [:mcmanus] 2011-08-03 06:14:05 PDT
Created attachment 550364 [details] [diff] [review]
patch 3

this one is fully event driven
Comment 6 Boris Zbarsky [:bz] 2011-08-03 08:25:05 PDT
> 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 7 Boris Zbarsky [:bz] 2011-08-03 10:24:11 PDT
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.
Comment 8 Patrick McManus [:mcmanus] 2011-08-03 20:54:29 PDT
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!
Comment 9 Boris Zbarsky [:bz] 2011-08-03 20:56:29 PDT
Mmmm.... the smell of 2+ minutes shaved off the test run time in the morning!
Comment 10 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-08-04 03:11:31 PDT
http://hg.mozilla.org/mozilla-central/rev/a8045a08cbc9

Note You need to log in before you can comment on or make changes to this bug.