mocha

mocha copied to clipboard

(edited for clarity)

Test cleanup seems to be a weak spot; I've got some very timing-sensitive tests that -- when they fail -- cause subsequent tests that should pass to fail, with stack traces that point to the wrong issue. I'm trying to make sure I'm not missing some obvious way of handling these.

Let's say I have these tests in a single suite:

test("A", function(done) {
  this.timeout(1000);
  setTimeout(function() {
    assert(false);
    done();
  }, 2000);
});

test("B", function(done) {
  this.timeout(10000);
  setTimeout(function() {
    done();
  }, 3000);
});

A will timeout and fail. B will start, and though it would pass, A's timer will fire and A's assertion will cause B to fail (with a stack trace that references A).

The only way I've found to get around this (and the problem exists for any delayed event in a test, whether that test times out or finishes in a separate test branch) is to do something like the following for A:

test("A", function(done) {
  var _runnable = this.runnable();
  this.timeout(1000);
  setTimeout(function() {
    if (!_runnable.timedOut && !_runnable.duration) {
      assert(false);
      done();
    }
  }, 2000);
});

.. is that the preferred method of handling this issue?

Just tested and it's still an issue:

$ cat test.js
var assert = require('assert');

it("A", function(done) {
  this.timeout(1000);
  setTimeout(function() {
    assert(false);
    done();
  }, 2000);
});

it("B", function(done) {
  this.timeout(10000);
  setTimeout(function() {
    done();
  }, 3000);
});

$ ./bin/mocha test.js


  ․․

  0 passing (2s)
  2 failing

  1)  A:
     Error: timeout of 1000ms exceeded. Ensure the done() callback is being called in this test.
      at null.<anonymous> (/Users/danielstjules/git/mocha/lib/runnable.js:170:19)
      at Timer.listOnTimeout [as ontimeout] (timers.js:110:15)

  2)  B:
     Uncaught AssertionError: false == true
      at null._onTimeout (/Users/danielstjules/git/mocha/test.js:6:5)
      at Timer.listOnTimeout [as ontimeout] (timers.js:110:15)

Your solution would work, but I think it's a symptom of a larger a problem.

I am a bot that watches issues for inactivity. This issue hasn't had any recent activity, and I'm labeling it stale. In 14 days, if there are no further comments or activity, I will close this issue. Thanks for contributing to Mocha!

Blast from the past. Forgot this was filed, but it's still an issue.

~/tmp $ cat ./test.js
var assert = require('assert');

it("A", function(done) {
  this.timeout(1000);
  setTimeout(function() {
    assert(false);
    done();
  }, 2000);
});

it("B", function(done) {
  this.timeout(10000);
  setTimeout(function() {
    done();
  }, 3000);
});

~/tmp $ ./node_modules/.bin/mocha --version
4.0.1
~/tmp $ node --version
v8.6.0
~/tmp $ ./node_modules/.bin/mocha test.js


  1) A
  2) B

  0 passing (2s)
  2 failing

  1) A:
     Error: Timeout of 1000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.


  2) B:

      Uncaught AssertionError [ERR_ASSERTION]: false == true
      + expected - actual

      -false
      +true

      at Timeout._onTimeout (test.js:6:5)



~/tmp $

I'm not sure this is actually fixable; in general, it's pretty hard (even moreso the wider the range of environments supported) to programmatically determine which function started a given piece of asynchronous code that threw an exception.

.. which is fine, I just don't want it closed by a bot.

Fair enough! (The bot went a little overkill on old issues as we're updating some labels and it filters based on labels. Sorry about that.)

It occurs to me that this would be a great use case for cancellation semantics -- stopping actions if a timeout is hit... Obviously, some actions have timeouts chosen by the caller of the action, but the lack of a standard way to allow a consumer to control it makes it difficult or impossible to integrate any arbitrary API's cancellability with something like Mocha, as far as I've heard. (This philosophical musing brought to you by my seeing people discuss things like promises but with corrections for common errors and obscure edge cases.)

If it's not clear, I'm not asking for people to focus on this issue -- this is a hard problem (as in "possibly unsolveable at the moment," not "difficult"). I'm 100% behind a decision to close the issue, but it would be nice to document it, and document a work-around (which -- I think -- is something like what I showed above).

EDIT: I spoke just slightly too soon! There's a very easy way to confound this fix -- if a test fails in a previous run and fails in the current run as well, it will be blocked and never complete on a subsequent run. I'll leave this post up but this is not a valid fix for the issue. I worked around my problem by chucking the whole reusable-mocha-instance notion and rebuilding it from scratch every time. Not ideal, but much better than a promise never resolving.

---

Adding to this for posterity -- I ran into this issue while designing testing for selenium. If a test times out while an await is pending, mocha will grab its neck and fall over when the promise resolves. I worked around this by wrapping my test functions in this function, based on @aahoughton's workaround. As far as I can tell, this solves the weird async issues entirely, as well as across multiple runs when invoking mocha programatically with cleanReferencesAfterRun turned off.

I don't pretend it's perfect -- it's very possible that there's a way to confound this fix, but it seems to work for my needs.

// if an async returns after the method times out, 
// it will cause a "done() called multiple times" error.
// In the context of a running webdriver, this is potentially very bad.
// We need to be able to quit the driver gracefully under any circumstances.
// This allows methods to time out while an async is pending,
// and will avoid calling done twice in this case.
function asyncSafety ( fn ) {
  return function ( done ) {
    let runnable = this.runnable();

    fn.bind( this )().then( ( res ) => {
      // for successful I think we only need timedOut? not sure though, might have side effects
      // when the duration check is added it will get stuck and never complete on a second runthrough
      if( !runnable.timedOut ) { 
        done( res );
      }
    }).catch( ( err ) => {
      if( !runnable.timedOut && !runnable.duration ) {
        done( err );
      }
    });
  }
}

Here's how it might be used:

describe( 'foo', function () {
  it( 'should not die on async weirdness', asyncSafety( async function () {
    await someAsyncMethod();
  }));
});

Yeah, this seems like a pretty big structural change as Scott mentions. Plus it hasn't received very much enthusiasm from users in the > decade since it was posted. Closing as wontfix / aged away.

If someone wants something like this, please file a new issue with our new templates that describes in detail what should be changed/improved.

Thanks all!