Fixes harness bug

[tinganho]

Fixes #3273.

[tinganho]

I also don't think https://github.com/Microsoft/TypeScript/blob/master/src/harness/projectsRunner.ts#L419-L421 does as intended.
With the following test:

it('Errors in generated Dts files for (' + moduleNameToString(moduleKind) + '): ' + testCaseFileName, () => {
    console.log('it call.');
    if (!compilerResult.errors.length && testCase.declaration) {
        var dTsCompileResult = compileCompileDTsFiles(compilerResult);
        if (dTsCompileResult.errors.length) {
            Harness.Baseline.runBaseline('Errors in generated Dts files for (' + moduleNameToString(compilerResult.moduleKind) + '): ' + testCaseFileName, getBaselineFolder(compilerResult.moduleKind) + testCaseJustName + '.dts.errors.txt', () => {
                return getErrorsBaseline(dTsCompileResult);
            });
        }
    }
});
after(() => {
    console.log('Should have it calls above.');
    compilerResult = undefined;
});

it generates:

it call.
it call.
Should have it calls above.
Should have it calls above.

Whereas I think you intend to have the result:

it call.
Should have it calls above.
it call.
Should have it calls above.

After looking through Mocha source code. Each describe defines a new test suite in Mocha and it defines a test in the last test suite(latest describe call). All hooks after before etc. will be defined in the last test suite(latest describe call). And all test suites runs from top to bottom — left to right. So if you define a after hook higher up in one describe and you define tests in a nested describe. The outer after hook will be ran first before the tests — quite unintuitive situation. Though a reminder for mistakes.

[danquirk]

👍

[tinganho]

I think I have narrowed down the root cause:

Mocha seems to read in all it(...) calls and then execute them one by one. When defining with the grep option it greps the name in a gigantic for loop. There are simply too many tests cases for it to handle and doesn't allow the node runtime to breathe, so it exceeds the callstack limit.

Though when running without the grep options allows the node runtime to breathe, thus not causing a crash.

One solution is to grep in the test runners before Mocha greps them, then Mocha don't need to read in all test cases. Or simply patch Mocha with a setTimeout on their for loop.

Don't know why my fix fixes the problem though.

[danquirk]

After looking at your fix I assumed the fix had to be something of that nature (too many loose 'it' blocks) as that would also explain why we suddenly ran into it without any harness changes/regressions. I also don't fully understand why the fix works but presume with the 'it' blocks properly registered under 'describe's mocha may be doing a better job of only evaluating what needs to be evaluated.

One solution is to grep in the test runners before Mocha greps them, then Mocha don't need to read in all test cases. Or simply patch Mocha with a setTimeout on their for loop.

This is sort of how the harness used to work before we switched to mocha. We had a lot more machinery manually defining what subsets of tests to run and binding keywords to sets of tests (rather than a more flexible grep system).

[tinganho]

@danquirk just tested my hypothesis. Seems like adding process.nextTick fixes the problem: https://github.com/mochajs/mocha/blob/master/lib/runner.js#L541

process.nextTick(function() {
  self.runSuite(curr, next);
});

It's running a big recursion loop there. I can probably try to add a PR to Mocha instead.

(Though I still think some parts of the test runner are defining the wrong describe statement and after hooks like the code change I made and the comment I made)

[danquirk]

Awesome, nice work.

(Though I still think some parts of the test runner are defining the wrong describe statement and after hooks like the code change I made and the comment I made)

Yep, we should absolutely fix these types of cases regardless.

[tinganho]

Just added a PR into Mocha. You can track it here mochajs/mocha#1749. setImmediate and process.nextTick entails some perf cost around 25%. My PR special case setImmediate and process.nextTick only when used with the grep option. So for TypeScript, it will run the whole test suite at the same speed as before.

I will make a new PR to resolve the hooks issues.