-
-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Maximum call stack issue for large test suite #1749
Conversation
7592eb7
to
5bc244a
Compare
anyone at home? |
// test suite don't do any immediate recursive loops. Thus, | ||
// allowing a JS runtime to breathe. | ||
if (self._grep !== self._defaultGrep) { | ||
Runner.immediately(function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can be shortened to Runner.immediately(next);
this will need tests |
you mention in microsoft/TypeScript#3511 that tests are 25% slower if have you tried increasing the maximum call stack size of your runtime? |
@boneskull that is only when grepping or when My first implementation was running Don't know the reason why you don't want to merge? I think you think my solution will increase slowness without grepping too?
This is not an ideal solution. Only a duck tape. Besides how do I do it with mocha bin? |
Doesn't current test covering this? I'm not sure if we want a perf test that will create 10000 test suites. |
@boneskull I just fixed the next call argument. |
thoughts? |
@boneskull can this be merged? |
Run or at least I think that's the right option. note the I'm looking at the typescript repo and I see stuff like 2000 test files in a single directory (or at least I think they are test files, but whatever's going on here is incredibly complex), 15000 fixtures in another... I think I'm going to need more information.
@travisjeffery I'd like your opinion on this. I don't like this is going to make all usages of cc @mochajs/mocha |
@tinganho Also, please be mindful that you're coming off as kind of pushy. |
Hi @boneskull sorry for being pushy.
Travis is saying that there are 35259 tests. Though the node runtimes have some room for pauses some times.
We have a test harness that converts baseline files and compares it with the result. So I don't know if directories are relevant. So every test is dynamically being read in and the test harness calls the necessary
We use it primarily on NodeJS. This is the relevant lines that causes the issue. |
I just ran to test the slowness again. One using No You can test it out:
|
I guess the node version fixed the problem. Though, the problem is that I didn't note which version I used in the old test. |
So there's no longer an issue after upgrading? |
Here is my results after 3 rounds of testing: No Runner.immediately 4.36 4.26 4.35 No runner.immediately runs faster. With I'm using Macbook Pro 2.6Ghz Intel Core i5. Node v0.12.5. |
I'm not sure if there is an upgrade issue. I didn't do a serious testing in my first test, where I mentioned 25% slowness. I can run on node v0.10 and compare later and see if there are any big diff. |
2f458ab
to
2952eca
Compare
Did some timing tests myself as well, and couldn't see any significant change. This branch seems consistently slower, but also only within the 1-2% range, which I'd consider negligible. @tinganho thanks for your work on this, seems like a sensible change. Also pretty incredible that you guys have 35259 over at Microsoft/TypeScript! @mochajs/mocha any objections to merging this? |
nope, don't care about a 2% slowdown |
I tried, but can't figure out how to merge this. test output (verbatim):
|
@tinganho please do a favor and rebase onto |
@boneskull I've merged master into the branch and created a PR from my fork at #1785, and all seems to work fine. |
@jbnicolai I had the wrong indentation on my commits. I just fixed and squashed everything down to one commit. |
👍 @boneskull can you confirm that it all works for you? I'm having no issues, so am all for merging :) |
Maximum call stack issue for large test suite
The TypeScript repo is running into some scalability issues with Mocha. We pinned down the problem in this line of code https://github.com/mochajs/mocha/blob/master/lib/runner.js#L541. It does a huge recursive loop if there are is a large amount of test suites and thus causing a maximum call stack error.
This PR fixes that problem.
Related issue in TypeScript repo:
microsoft/TypeScript#3511