Add assert method for rejection of promises

[rwjblue]

This would support the following:

QUnit.test('does stuff', function(assert) {
  assert.throws(() => {
    return new Promise((resolve, reject) => {
      setTimeout(reject, 10, new Error('NOOOOOOOO!!!!!');
    });
  }, /NOOOOOO/);
});

---

An alternative would be to add a assert.rejects, but I feel like transparently supporting this case in assert.throws is more aligned with how we handle things in other areas.

[trentmwillis]

I'm 👍 on this. Should include a docs update with it as well.

[Krinkle]

I like the idea of consistency here (return to assert.throws like we return to QUnit.test), but I'm also not sure whether using the throws callback for this is useful.

It's there today because QUnit needs to trigger the code and catch an exception. When dealing with async code or promises, this isn't needed. What about passing the promise directly?

QUnit.test('example', function (assert) {
  let result = asyncFunction();
  assert.throws(result, /NOOOOOO/, 'message');
});

QUnit.test('example 2', function (assert) {
  let result = new Promise( (resolve, reject) => {
    setTimeout(reject, 10, new Error('NOOOOOOOO!!!!!'));
  });
  assert.throws(result, /NOOOOOO/, 'message');
});

[trentmwillis]

After thinking about this a big more, I'm actually unsure that augmenting assert.throws is the proper way to validate these scenarios.

In particular, I am concerned that by wrapping promises in assert.throws it appears to be synchronous in regards to the test execution. Secondly, it can potentially obfuscate the fact that you are dealing with promises at all. Finally, (this is a minor point) it also doesn't make much sense semantically as the Promise isn't technically "throwing", it is being rejected.

With those in mind, is there a compelling reason to need this feature instead of simply using .catch() and performing assertions within the catch block?

[rwjblue]

Hmm. I think that same logic applies to assert.throws with synchronous errors. It's roughly the same problem, and you could definitely make the case that there really isn't a reason that you couldn't just wrap your assertions in a try { } catch(e) { } block.

Ultimately, the reason we need support for rejected promise assertions is the same reason we need assert.throws. The simple thing (that you described) isn't enough. You have to add assertions to both .then (assert.notOk) and to the .catch, you have to have logic to ensure the rejection reason to matches your expected message or regular expression, you have to have logic to ensure the test properly waits for promise completion (either assert.async() or return the promise).

tldr; it is very error prone to do the right thing.

test('...', function(assert) {
  return somethingReturningAPromise()
    .then(() => assert.notOk(true, 'should have rejected with "some message"'))
    .catch((reason) => assert.equal(reason.message, "some message"));
});

^ is annoying code to write, and this is why we have libraries. 😃

[platinumazure]

Could we create new APIs for promises around asserting that a promise resolves or rejects (possibly supporting a predicate that could be given the resolve/reject arguments and could further validate the result)? That way we can just keep the APIs for promise rejection and exception catching separate.

[trentmwillis]

So that makes sense, but I think I'd prefer a separate helper for these scenarios. Maybe assert.rejects? My primary concern in the above is being explicit about asynchronicity. For example:

assert.throws(() => foo());
assert.throws(() => bar());
assert.throws(baz);

It is not possible to tell which of the above is asynchronous, which, as I think we've seen in the Ember community, can lead to an awful lot of confusion. It also means that when a Promise is returned we will either have to automatically pause test execution or return a Promise. The former is likely to be relatively confusing as we couldn't prevent execution of code coming after the throws and the latter leads to inconsistent return values.

I'd much rather have something like the following:

return assert.rejects(promise, /value to compare/, 'message'); // returns a promise

[platinumazure]

@trentmwillis I don't know if you were replying to my comment or that of @rwjblue, but I basically like your last proposed API. The only issue I have with it is it seems the regular expression would have to be run against the first reject argument, where there could be multiple. So I would want to add a callback form that could take all rejection arguments. But otherwise I really like that API.

[rwjblue]

Just to be clear, I'm not really saying that I think assert.throws should do more things, I'm just saying that it should be easier to make assertions around API's that might reject a promise.

[trentmwillis]

@platinumazure, sorry, was replying to @rwjblue (timing issues). Also, my understanding is that Promises can only be resolved/rejected with a single value.

@rwjblue, okay 👍, so it sounds like adding a new API to specifically improve Promise ergonomics is inline with both of our thinking.

[rwjblue]

@trentmwillis - Generally speaking that API seems good to me. Though we should discuss the exact interface (e.g. should the return value always resolve?)...

[platinumazure]

Also, my understanding is that Promises can only be resolved/rejected with a single value.

Oops @trentmwillis, you're right. My mistake.

[rwjblue]

Right, they only have one value but it isn't obvious what property folks are wanting to assert against. Oftentimes the rejection value may not be an error instance.

[rwjblue]

FWIW, I implemented assert.rejects recently here. The internals are based on what is done in assert.throws but augmented/tweaked to handle promises.

QUnit.assert.rejects = function(callback, expected, message) {
  this.test.ignoreGlobalErrors = true;

  let actual;
  let result = false;

  let done = this.async();

  return Promise.resolve()
    .then(callback)
    .then(null, reason => {
      actual = reason;
    })
    .finally(() => {
      if (actual) {
        const expectedType = typeof expected;

        // We don't want to validate thrown error
        if (!expected) {
          result = true;
          expected = null;

          // Expected is a regexp
        } else if (expected instanceof RegExp) {
          result = expected.test(errorString(actual));

          // Expected is a constructor, maybe an Error constructor
        } else if (expectedType === 'function' && actual instanceof expected) {
          result = true;

          // Expected is an Error object
        } else if (expectedType === 'object') {
          result =
            actual instanceof expected.constructor &&
            actual.name === expected.name &&
            actual.message === expected.message;

          // Expected is a validation function which returns true if validation passed
        } else if (expectedType === 'function' && expected.call(null, actual) === true) {
          expected = null;
          result = true;
        }
      }

      this.pushResult({
        result,
        actual,
        expected,
        message,
      });

      this.test.ignoreGlobalErrors = false;
    })
    .finally(() => {
      this.test.ignoreGlobalErrors = false;
      done();
    });
};

@trentmwillis - Does this seem like a reasonable path forward?

[rwjblue]

Hopefully that seemed good, I submitted #1238 with the implementation and docs...

[Krinkle]

TL;DR: LGTM!

I agree the current workaround is worth trying to avoid by providing a built-in convenience method. (The workaround being the idea of returning the promise to QUnit.test after swapping the meaning of resolved and rejected.)

At first, I wasn't sure whether assert is the best object for the method to exist on. We do have other assert methods that don't (immediately) assert something (such as async, and expect). But if the method merely provides the inverted promise with an asynchronous assertion chained, that would create a potential usability hazard as the user would need to make sure to still either return the result from assert.reject to QUnit.test or manually attach assert.async to it.

However, I see now that the implementation goes beyond inverting the promise and attaching an assertion to the chain, it also adds async tracking. That was the missing puzzle piece. It does make for a more complex method, but I think in this case that overhead is worth it and makes for a very intuitive method.

One thing to keep in mind here is that this further increases the number of async-tracking methods we have in the public API.

Projects aiming to reduce or avoid multiple async things during tests, will need to keep in mind that assert.rejects is now one more additional pattern to look out for as being indicative of an asynchronous test (e.g. as part of code review). I don't expect that to be a problem, but something to keep in mind (adding complexity is never cost-free).

[platinumazure]

Projects aiming to reduce or avoid multiple async things during tests, will need to keep in mind that assert.rejects is now one more additional pattern to look out for as being indicative of an asynchronous test (e.g. as part of code review). I don't expect that to be a problem, but something to keep in mind (adding complexity is never cost-free).

Thanks for this tip, I'll have to see if I need to make any changes in eslint-plugin-qunit for this.

That said, I think the implementation of assert.rejects seems to manage its async test nature internally and does not tend to leak that async state. Is that a safe assumption (that the async test callback should always be invoked)? (@rwjblue?)

[rwjblue]

That said, I think the implementation of assert.rejects seems to manage its async test nature internally and does not tend to leak that async state.

There is definitely still async involved (which I think folks will understand with the verbiage used here?) that folks should be aware of (e.g. no guaranteed ordering of the rejection assertions), but yes, the idea here is definitely to hide away the manual shenanigans that folks have to do today when testing rejections.