Skip to content

Conversation

jzaefferer
Copy link
Contributor

Fixes #73

Been testing this with the testsuites in QUnit itself and jQuery UI. It launches just one worker per browser and reuses them as expected.

In addition to fixing various bugs in @dhimil's implementation, I've also made a useful improvement, by always setting config.build. The format is based on what Karma uses for local runs.

@dhimil's commit introduced a reuseWorker configuration option. I didn't see the point of making this configurable, so removed the option. If there's more than one test page, workers are automatically reused.

@rahulnwn
Copy link
Contributor

We need reuseWorker configuration option as many users run multiple tests in parallel with current implementation, making their test suite run faster. As this is a very specific issue, we would like to keep it backward compatible.

@nakula
Copy link
Contributor

nakula commented Sep 17, 2014

@rahulnwn I suppose lets do some numbers around this. overall reusing might be much faster than allocation and starting of new browser.

@jzaefferer
Copy link
Contributor Author

For projects like jQuery UI, where spawning a worker per test quickly exceeds the worker limit, this is required. Elsewhere, its still faster to reuse workers. For example, testing QUnit:

this branch (12 workers running 4 pages): 3m 14s (194s)
master (56 workers running 1 page each): 5m 47s (347s)

That's an 80% increase.

@nakula
Copy link
Contributor

nakula commented Sep 19, 2014

sounds good. @dhimil can you please run your tests as well? we will merge then

@dhimil
Copy link
Contributor

dhimil commented Sep 19, 2014

Yup. Will try to update over weekend.

On Fri, Sep 19, 2014 at 7:44 PM, nakula notifications@github.com wrote:

sounds good. @dhimil https://github.com/dhimil can you please run your
tests as well? we will merge then


Reply to this email directly or view it on GitHub
#93 (comment)
.

Dhimil Gosalia,
BrowserStack.

@jzaefferer
Copy link
Contributor Author

@dhimil @nakula it's been two weeks, any updates?

You could land this and bump the "minor" version, here going to 0.2.0. We can still improve it later.

@nakula
Copy link
Contributor

nakula commented Oct 1, 2014

apologies @jzaefferer . we were happy with implementation and @dhimil confirmed to me it has been merged and published. @rahulnwn will just do it. sorry for confusion

rahulnwn added a commit that referenced this pull request Oct 1, 2014
@rahulnwn rahulnwn merged commit 7bf042c into browserstack:master Oct 1, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reuse workers? Was: Error: Cannot queue more than 200 workers
4 participants