🚀 Feature: runner.on('end', ...) doesn't respect pending Promises

[dalegaspi]

my current annoyance with runner.on('end', ...) in my custom reporter is that mocha doesn't wait for any async/promises to complete. my reporter writes to an xlsx file and this gets aborted on the end handler, prompting me to add the dreaded setTimeout kludge--e.g.:

var reportDone = false; // set to true in an async xlsx writer when it's done.
function isReportDone() {
    if (!reportDone) {
        setTimeout(1000, isReportDone())
    }
}

// ...

runner.on('end', function() {
  // ...
  setTimeout(1000, isReportDone());
})

with a test suite, end() handler can be optionally passed with done callback which prevents mocha from ending the suite until done() is called.

i propose that runner.on('end', ...) could optionally have the same structure:

runner.on('end', function(done) { 
  // ...
  aPromiseCalled.then(function() {
     console.log('really done!');
     done();
  });
});

with the same behavior as end() test suite: prevent the reporter to end until done() callback is called, thereby not requiring the setTimeout kludgy fix.

[boneskull]

Hmm... I don't quite understand why this is necessary. Can you show more code?

[ScottFreeCode]

Could this be because _mocha#L467 isn't wrapped in process.on('exit', like _mocha#L461 is (both passed to run at _mocha#L458 based on a condition that I can't figure out what would set it)?

[dalegaspi]

@boneskull, here is a simple example derived from the minimalistic sample in mocha.js documentation on third-party reporters:

var Promise = require('bluebird')

function MyReporter(runner) {
  // ...
  runner.on('end', function() {
        winston.info('[SUMMARY] -- %d/%d', passes, passes + failures);

        Promise.delay(500).then(function() {
            console.log('i am spartacus') // this is not called unless ...
        })

        // ...this next line is uncommented
        //setTimeout(1000, function() {})
        process.exit(failures);
    });
}

[boneskull]

@ScottFreeCode That program.exit is the --no-exit option, which sounds applicable here. @dalegaspi Please see if running Mocha with --no-exit gives better results, and is a viable workaround for the time being.

--no-exit may result in a hanging process in case of an uncaught exception or when used with --bail, depending on how well your tests and sources (and Mocha, for that matter) recover gracefully.

[boneskull]

So it's weird to use an EventEmitter in this manner:

runner.on('end', function (done) {
  someAsyncStuff(function(err) {
    done(err);
  });
});

Not that the handler shouldn't be allowed to do async work, but that the EventEmitter--in this case, the Runner instance--shouldn't be waiting on its listeners to complete. The idea, of course, is that the EventEmitter will emit an event without concern as to what is listening.

Any given reporter is not necessarily the only listener on the Runner's events. This adds not-insignificant complexity, because:

• For every event, before emitting, the Runner needs to inspect its listeners via EventEmitter#listeners().
  • Take a count of how many listeners request a done function. This may even be impossible without breaking things, because:
    • Not every event has the same amount of arguments; e.g. pass will emit a Test instance, but fail will emit a Test and an Error.
    • There's no guarantee any given reporter's listener for any given event has an arity equal to the argument count.
  • Once we have that, the Runner may emit its event
  • When a listener calls done(), subtract from the count, and finally move on when the count is 0, and all listeners have completed.

On a large suite of tests, this could impact performance. We shouldn't abuse EventEmitters in this manner.

I'm open to other solutions, but here's one using the same semaphore-like idea:

• The Runner has an runningJobs count property.
• Instead of a done callback, the Runner instance can expose a method, enqueueJob(func).
• enqueueJob() will immediately increment runningJobs.
• Function func is executed thereafter, and is passed a standard Node.js-style callback.
• This callback decrements runningJobs.
• mocha's "exit" behavior would then wait for runningJobs to reach 0 before exiting.

This way, we can wait on internal operations before exiting, but not any operations which might come from tests, nor code under test.

The above implementation seems more straightforward if we used Promises. Portability concerns would mean we'd have to add a shim or compatible library. Mocha doesn't use them internally, so there's no precedence for this.

I can't envision having the time to do this, but if someone wants to send a PR along these lines--at minimum, which doesn't abuse events--that'd be helpful.

[dalegaspi]

@boneskull the --no-exit works without having to call setTimeout() as long as i don't make an explicit call to process.exit(failures) in the 'end' handler...which, i guess, is fine for our purpose.

[Nitive]

For everybody who faced the same issue and found this issue: as a workaround you can convert async calls to sync via deasync

[yunnysunny]

In my condition, I need to send the result of mocha to emails after run all test in our continuous integration flow. I wrote a reporter to do this. But the end listenter can not wait for the sending finish, exit at once when end function is called.

We can't use the option --no-exit, it will hang our CI till its timeout. So I think giving an done callback to end listener is a good idea to resolve it.

[Amr-afaqy]

Kindly refer to this alternative solution #https://github.com/cypress-io/cypress/issues/7139#issuecomment-1074747820