Closed
Bug 662612
Opened 13 years ago
Closed 13 years ago
rewrite test_websocket.html to be faster
Categories
(Core :: Networking: WebSockets, defect)
Tracking
()
RESOLVED
FIXED
mozilla8
People
(Reporter: mcmanus, Assigned: mcmanus)
Details
(Whiteboard: [inbound])
Attachments
(1 file, 2 obsolete files)
12.51 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•13 years ago
|
||
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
Assignee | ||
Updated•13 years ago
|
Summary: rewerite test_websocket.html to be faster → rewrite test_websocket.html to be faster
Assignee | ||
Comment 2•13 years ago
|
||
Attachment #549942 -
Attachment is obsolete: true
Attachment #550255 -
Flags: review?(bzbarsky)
Comment 3•13 years ago
|
||
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....
Assignee | ||
Comment 4•13 years ago
|
||
(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.
Assignee | ||
Comment 5•13 years ago
|
||
this one is fully event driven
Attachment #550255 -
Attachment is obsolete: true
Attachment #550255 -
Flags: review?(bzbarsky)
Attachment #550364 -
Flags: review?(bzbarsky)
Comment 6•13 years ago
|
||
> 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•13 years ago
|
||
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+
Assignee | ||
Updated•13 years ago
|
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
Assignee | ||
Comment 8•13 years ago
|
||
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•13 years ago
|
||
Mmmm.... the smell of 2+ minutes shaved off the test run time in the morning!
Comment 10•13 years ago
|
||
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.
Description
•