Maximum call stack issue for large test suite

[tinganho]

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

[tinganho]

anyone at home?

[boneskull]

can be shortened to Runner.immediately(next);

[boneskull]

this will need tests

[boneskull]

you mention in microsoft/TypeScript#3511 that tests are 25% slower if --grep is used. I imagine that's going to impact quite a few people. if left as is, I don't want to merge this.

have you tried increasing the maximum call stack size of your runtime?

[tinganho]

@boneskull that is only when grepping or when Runner.immediate is used. And you are already grepping the tests which will narrow it down so the 25% increased slowness will be negligible for majority of 99.99% of all cases.

My first implementation was running Runner.immediate on all tests. That's where I saw the increased slowness.

Don't know the reason why you don't want to merge? I think you think my solution will increase slowness without grepping too?

increasing the maximum call stack size of your runtime?

This is not an ideal solution. Only a duck tape. Besides how do I do it with mocha bin?

[tinganho]

this will need tests

Doesn't current test covering this? I'm not sure if we want a perf test that will create 10000 test suites.

[tinganho]

@boneskull I just fixed the next call argument.

[tinganho]

thoughts?

[tinganho]

@boneskull can this be merged?

[boneskull]

Besides how do I do it with mocha bin?

Run node --stack_size=9084327948237 /path/to/_mocha test_dir/

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.

• How many tests are you attempting to grep through?
• How many directories does it involve?
• How many directories actually contain the things you are grepping for?
• Are you running the tests in the browser, NodeJS, or both? Where is the problem?
• Instead of using/abusing --grep, would you consider a more granular organizational scheme, and simply specific directory paths instead?
• Would Support for --tags to include/exclude tests based on some optional tagging #1445 be helpful?

@travisjeffery I'd like your opinion on this. I don't like this is going to make all usages of --grep slower. I'm also a bit concerned about the solution itself; perhaps the better solution would be to run suites with some sort of generator instead of a recursive strategy (if that's possible).

cc @mochajs/mocha

[boneskull]

@tinganho Also, please be mindful that you're coming off as kind of pushy.

[tinganho]

Hi @boneskull sorry for being pushy.

How many tests are you attempting to grep through?

Travis is saying that there are 35259 tests. Though the node runtimes have some room for pauses some times.

How many directories does it involve?
How many directories actually contain the things you are grepping for?

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 describe and it functions. But when we develop we don't want to run all tests because it takes a long time so we use the mocha grep option. Right now, every time we do it it shows maximum call stack error, because of the recursive loop issue.

Are you running the tests in the browser, NodeJS, or both? Where is the problem?

We use it primarily on NodeJS.

This is the relevant lines that causes the issue.
https://github.com/Microsoft/TypeScript/blob/master/src/harness/compilerRunner.ts#L39-L384

[tinganho]

I just ran to test the slowness again.

One using Runner.immediate and one using the old implementation. I'm in Node 12.5.

No Runner.immediate got 4m 41s and with Runner.immediate got 4m 42s so no big difference this in time just one 1s.

You can test it out:

• clone the TS repo: https://github.com/Microsoft/TypeScript
• npm install -g jake
• run npm install
• run jake runtests notice the time
• go to node_modules/mocha/lib/runner.js
• Change the line self.runSuite(curr, next); to

Runner.immediately(function() {
    self.runSuite(curr, next);
});

• run jake runtests again and notice the time diff.

[tinganho]

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.

[boneskull]

So there's no longer an issue after upgrading?

[tinganho]

Here is my results after 3 rounds of testing:

No Runner.immediately 4.36 4.26 4.35
Runner.immediately 4.46 4.35 4.41

No runner.immediately runs faster. With Runner.immediately 3-4% slower.

I'm using Macbook Pro 2.6Ghz Intel Core i5. Node v0.12.5.

[tinganho]

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.

[jbnicolai]

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?

[boneskull]

nope, don't care about a 2% slowdown

[boneskull]

I tried, but can't figure out how to merge this. test output (verbatim):

$ npm test

> mocha@2.2.5 test /Volumes/alien/projects/boneskull/mocha
> make test-all











  ✓ should work

[boneskull]

@tinganho please do a favor and rebase onto master. you will need to run npm test and ensure everything is working, including lint checks. thanks!

[jbnicolai]

@boneskull I've merged master into the branch and created a PR from my fork at #1785, and all seems to work fine. npm test gives the expected output for me (lint, then a ton of tests)

[tinganho]

@jbnicolai I had the wrong indentation on my commits. I just fixed and squashed everything down to one commit.

[jbnicolai]

👍 @boneskull can you confirm that it all works for you? I'm having no issues, so am all for merging :)