Mocha: Impact of `setImmediate` between hooks on Promises

[meeber]

Prerequisites

• [x] Checked that your issue isn't already filed by cross referencing issues with the common mistake label
• [x] Checked next-gen ES issues and syntax problems by using the same environment and/or transpiler configuration without Mocha to ensure it isn't just a feature that actually isn't supported in the environment in question or a bug in your code.
• [x] 'Smoke tested' the code to be tested by running it outside the real test suite to get a better sense of whether the problem is in the code under test, your usage of Mocha, or Mocha itself
• [x] Ensured that there is no discrepancy between the locally and globally installed versions of Mocha. You can find them with:
  node node_modules/.bin/mocha --version(Local) and mocha --version(Global). We recommend avoiding the use of globally installed Mocha.

Description

I'm opening this issue as a result of an investigation into https://github.com/domenic/chai-as-promised/issues/173. However, there's a lot of unrelated noise in that thread, so I'll demonstrate the issue here independently of chai-as-promised.

Hooks typically wait until the next tick of the event loop before proceeding, per these lines. As a result, if a Promise with no rejection handler is declared in a hook prior to the test in which the rejection handler is added to the Promise, then a warning is typically generated about an unhandled promise rejection, followed by another warning about asynchronous handling of the promise rejection.

I used the word "typically" twice above because what I described doesn't apply to the final beforeEach prior to the test; unlike the other hooks, the final beforeEach proceeds immediately to the test, without waiting for the next tick of the event loop.

Steps to Reproduce

Example 1:

let promise;

describe("mySuite", function () {
  before(function () {
    promise = Promise.reject(new Error());
  });

  it("myTest", function () {
    promise.catch(function () {});
  });
});

Example 2:

let promise;

describe("mySuite", function () {
  beforeEach(function () {  // This is the only different line
    promise = Promise.reject(new Error());
  });

  it("myTest", function () {
    promise.catch(function () {});
  });
});

Expected behavior: Although the tests are meaningless, ideally I'd expect neither of the examples to generate any unhandled promise rejection warnings, or at least for both examples to result in the same behavior.

Actual behavior: Example 1 generates an unhandled promise rejection and Example 2 doesn't.

Example 1 output:

mocha

mySuite
(node:14219) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): Error
(node:14219) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
✓ myTest
(node:14219) PromiseRejectionHandledWarning: Promise rejection was handled asynchronously (rejection id: 1)

1 passing (11ms)

Example 2 output:

mocha

mySuite
✓ myTest

1 passing (7ms)

Reproduces how often: 100%

Versions

Node v8.5.0
Mocha v3.5.3

Additional Information

Changing this line to favor process.nextTick over global.setImmediate fixes the issue so that neither example generates a warning, and the Mocha test suite still passes. But I don't know what other consequences there are.

I think that in most situations this issue isn't a problem. Most promises being tested in the wild have (or should have) rejection handlers already registered, so no warning would be generated in those cases, despite the next tick between hooks. I just started digging in order to understand what was happening with https://github.com/domenic/chai-as-promised/issues/173. The example in that issue is completely contrived, as are the examples I provided here.

[ScottFreeCode]

This definitely ought to be consistent, at a minimum...

I don't happen to be an expert in the difference between setImmediate, nextTick and setTimeout(..., 0), but the only thing I can think of that it may affect is promise implementations in browsers where process isn't available (except as a shim with not quite the same underlying event loop nitty gritty details as Node). Getting the behavior consistent at least within an environment would be nice; getting it consistent across environments would be even nicer, although it's also conceivable that this sort of promise handling might be useful in obscure cases.

Anybody up for testing before/after the change against various browser promise implementations?

[boneskull]

Consistency around the hook behavior is needed, but more importantly users just need to know what to expect. It doesn't work quite right atm, and there's an open discussion (though dormant) about how various failure conditions in the hooks should impact the tests.

I'm 99% sure the intent of setImmediate(fn) was "execute this before setTimeout(fn) would."

So yeah, bug. Thanks :+1:

[overlookmotel]

I've come up against the same issue, but not related to promises. I'm trying to do set-up in a beforeEach hook and then run tests within a nested describe block, and the correct functioning of the tests relies on there not having been a tick between the execution of beforeEach and it.

Here's an example (again, rather contrived). I'd expect all 3 of these tests to pass. But the third fails, due to the introduction of a tick:

const expect = require('chai').expect;

describe('Simple', function() {
    beforeEach(function() {
        this.ticked = false;
        setImmediate(() => this.ticked = true);
    });

    it('should not have ticked', function() {
        // This test passes
        expect(this.ticked).to.be.false;
    });
});

describe('Nested describe', function() {
    describe('Nested', function() {
        beforeEach(function() {
            this.ticked = false;
            setImmediate(() => this.ticked = true);
        });

        it('should not have ticked', function() {
            // This test passes
            expect(this.ticked).to.be.false;
        });
    });
});

describe('Nested describe with beforeEach in lower level', function() {
    beforeEach(function() {
        this.ticked = false;
        setImmediate(() => this.ticked = true);
    });

    describe('Nested', function() {
        it('should not have ticked', function() {
            // This test fails
            expect(this.ticked).to.be.false;
        });
    });
});

I'd have expected no difference between the 3.

I believe the cause is the call to Runner.immediately() in these lines.

What is the rationale for waiting a tick before executing hooks? Is it necessary?

[overlookmotel]

@boneskull Any thoughts on this? Is the tick before executing hooks necessary?

[boneskull]

It's probably necessary, but I couldn't tell you why. Mocha does this in other places to compensate for the lack of tail call support in most JS runtimes & versions. have a look at the history...

[overlookmotel]

@boneskull OK thanks. I'll look at the history and see what I can work out.

[overlookmotel]

As far as I can see, the introduction of the tick happened back in 2011 (!) in order to prevent over-long stack traces. https://github.com/mochajs/mocha/commit/484432a15ad85c0b8260ddf8d88422f94c577629

I can see the argument for this, but it's kind of a hack. And it makes it almost impossible to use mocha for testing constructs like Promises where whether something happens in same or next tick is part of what the tests are meant to check.

I can see 3 possible solutions:

1. Accept that what happens in hooks is going to appear in stack traces (i.e. remove the tick).
2. Implement an alternative way of trimming stack traces, so original problem the tick was introduced to fix remains fixed, but the tick can be removed.
3. Introduce a command line flag to instruct mocha not to introduce these additional ticks when the user needs that.

@boneskull I accept perhaps this is a fairly niche case so maybe not worthwhile making major changes to accommodate. But how do you feel about the command line flag?

[ScottFreeCode]

And it makes it almost impossible to use mocha for testing constructs like Promises where whether something happens in same or next tick is part of what the tests are meant to check.

My gut instinct here is that if the behavior occurring within a tick is what's being tested then it belongs inside the test. E.g. instead of this:

describe("my promise", function() {
  var myPromise
  before|beforeEach(function() {
    myPromise = MyPromise.resolve(42)
  })
  it("should wait a tick to resolve", function() {
    assert(myPromise.isPending())
  })
  it("should use the resolved value", function() {
    return myPromise.then(function(value) {
      assert(value === 42)
    })
  })
  it("should have whatever other special behavior", function() {
    return doWhateverWith(myPromise)
  })
})

...do this instead (although it looks a little awkward):

describe("my promise", function() {
  var myPromise
  before|beforeEach(function() {
    myPromise = MyPromise.resolve(42)
  })
  it("should wait a tick to resolve", function() {
    // test how quickly it resolves -- so not reusing the usual value for the suite in this test case
    assert(MyPromise.resolve().isPending())
  })
  it("should use the resolved value", function() {
    return myPromise.then(function(value) {
      assert(value === 42)
    })
  })
  it("should have whatever other special behavior", function() {
    return doWhateverWith(myPromise)
  })
})

A much stronger case, in my opinion, is https://github.com/domenic/chai-as-promised/issues/173#issuecomment-330409841 -- which essentially means that there is no way to set up a rejected promise before the test (once Node starts treating unhandled rejections the same way it treats unhandled exceptions). Is there some workaround -- some way to define what the promise will be/do but not eagerly run it? Maybe something like this?

describe("my promise", function() {
  var makeMyPromise
  before(function() { // equivalent to beforeEach due to thunk
    makeMyPromise = function() { return MyPromise.reject(new Error("example")) }
  })
  // or
  var makeMyPromise
  before(function() {
    // WARNING: in this version the *first* test must handle the rejection,
    // and all subsequent tests are allowed to ignore it even if they shouldn't!
    var cache
    makeMyPromise = function() {
      if (!cache) { cache = MyPromise.reject(new Error("example")) }
      return cache
    }
  })
  // or
  function makeMyPromise() { // equivalent to beforeEach due to thunk
    return MyPromise.reject(new Error("example"))
  }
  // end or branches
  it("should wait a tick to complete", function() {
    var promise = makeMyPromise()
    assert(promise.isPending())
    // the end value/result of this promise is not relevant in this test: hack to suppress rejection warning/error
    promise.catch(function() {})
    // note that in the caching version,
    // provided the below test for `catch`ing is moved up to be the first test,
    // this can be simplified to `assert(makeMyPromise().isPending())`;
    // but that version will prevent any warnings about any subsequent test that forgets to handle it!
  })
  it("should use the rejected error", function() {
    return makeMyPromise().catch(function(error) {
      assert(error.message === "example")
    })
  })
  it("should have whatever other special behavior", function() {
    return doWhateverWith(makeMyPromise())
  })
})

There's a downside that one has to know about the problem and this solution. Might be other downsides I'm missing at this point. This also exposes an inherent conflict between "if I create a new promise for each test then every test must handle the rejection even if it's irrelevant" and "if I reuse a promise then the first test handling its rejection would allow the rest to ignore it whether they should or not"*; but I am pretty sure that issue is going to be present regardless of how the promise is created or whether and where it's stored.

*(EDITTED TO ADD: you could bypass the need for the first test to be the one that handles the rejection if you immediately .catch(function() {}) the promise to suppress the error -- without saving the caught version so you can still examine the rejection in the original -- in fact, the real danger I'm getting at is that even if you use the first-test-catches approach instead of that obvious hack it has the same effect on the rest of the tests as the hack would.)

[overlookmotel]

@ScottFreeCode Your solution of using makeMyPromise() works. However, that adds an extra line of "boiler plate" code to every test. The purpose of beforeEach hooks to precisely to avoid repeated set-up code in every test.

I guess everyone has their own style of writing tests. Personally, I like a large number of very short tests, each of which tests one specific aspect of the subject under test. So with that style, adding an extra line to every single test makes for a major bloat.

In my opinion, the introduction of this tick is unnecessary and a bit of a hack just to reduce the length of stack traces. Removing it would solve both my issue and @meeber's.

But I acknowledge, as I said before, that these are perhaps niche cases, so it's hard to justify making radical changes in mocha to cater to them. But I'd be interested in @boneskull's opinion.

[overlookmotel]

@ScottFreeCode By the way, I'm sorry I didn't realise that you were a mocha maintainer, hence me asking for @boneskull's opinion. I'm interested in both your opinions...

As a short-term fix for my issue, I've published a fork as mocha-no-hooks-tick which removes the tick. But of course it'd be better to effect a change in mocha itself if possible.

[boneskull]

@overlookmotel I'm trying to get this fixed now.

[boneskull]

cc @raymondfeng

[boneskull]

I think my original comment was incorrect; it seems setImmediate is what we want to be doing. Either way, it needs to be consistent.

[boneskull]

I've created #3631 which addresses the inconsistency, but it does not consume process.nextTick(); instead it uses setImmediate() in both cases. Please discuss further in that issue. cc @overlookmotel @meeber @raymondfeng

[mhofman]

I actually just got bit by this behavior as well.

Regarding the original report, I don't see a problem with before and beforeEach being inconsistent with each other.
before is executed once before everything, and unless the first test declared after the before hook uses the promise, then the test may fail, even if there weren't any setImmediate between any hooks and tests. E.g. of a contrived version of what I mean:

describe("mySuite", function () {
  let rejected;
  before(function () {
    rejected = Promise.reject(new Error());
  });

  it("some long test", async function () {
    await sleep(5);
  });

  it("some test", function () {
    doSomething(rejected);
  });
});

beforeEach is a different story however and I believe should be executed in the same turn of the event loop as the test itself, because it sets up the environment for it. It doesn't have to be synchronous, it could be a different micro task, but it has to be the same task.
I'm writing some tests for a WeakRefs shim. Basically the proposal mandates that a WeakRef stays consistent within a task job, but of course makes no guarantee across jobs, i.e. an object cannot be collected in the middle of a task, but may be after the completion of the current task.
Because of mocha's behavior I cannot create the WeakRef in a beforeEach hook and use it immediately in a test.

describe("mySuite", function () {
  let weakRef;
  beforeEach(function () {
    weakRef = new WeakRef({});
  });

  it("some test", function () {
    // May or may not fail if gc decided to run between the 2 task jobs
    expect(weakRef.deref()).to.be.ok;
  });
});

However reverting to the previous behavior is not sufficient. I also have nested describe tests like @overlookmotel above. Those would also need to be executed in the same task.

Would it not be possible to run beforeEach hooks, if not synchronously to avoid their stack trace from showing up, at least in a micro task?

async function runTest() {
  for (const hook of beforeEachHooks) await hook();
  await test();
  for (const hook of afterEachHooks) await hook();
}

Btw, there is however an inconsistance with the browser runner, which most of the time runs hooks in the same turn. It stems from the immediately override there:
```javascript
var immediateQueue = [];
var immediateTimeout;

function timeslice() {
var immediateStart = new Date().getTime();
while (immediateQueue.length && new Date().getTime() - immediateStart < 100) {
immediateQueue.shift()();
}
if (immediateQueue.length) {
immediateTimeout = setTimeout(timeslice, 0);
} else {
immediateTimeout = null;
}
}

/**

• High-performance override of Runner.immediately.
  */

Mocha.Runner.immediately = function(callback) {
immediateQueue.push(callback);
if (!immediateTimeout) {
immediateTimeout = setTimeout(timeslice, 0);
}
};```