t.throws should return the error (or a promise that resolves with the rejection)

[jamestalmage]

From #492

test(t => {
  t.plan(2);
  return shouldRejectWithFoo().catch(reason => {
    t.is(reason.message, 'HelloMessage') // Prefer t.throws if all you care about is the message
    t.is(reason.foo, 'bar');
  });
});

test(t => {
  t.plan(2);
  try {
    shouldThrow();
  } catch (err) {
    t.is(err.message, 'HelloMessage') // Prefer t.throws if all you care about is the message
    t.is(err.foo, 'bar');
  }
});

In most cases, you should prefer the t.throws() assertion, but this is an acceptable use since t.throws() only allows you to assert against the error's message property.

We could make that recommendation obsolete:

test(async t => {
  var err = await t.throws(shouldRejectWithFoo, 'HelloMessage');
  t.is(err.foo, 'bar');
});

test(t => {
  var err = t.throws(shouldThrow, 'HelloMessage');
  t.is(err.foo, 'bar');
});

See also: #468 (it also changes the behavior of t.throws).

[kasperlewau]

Been putting in a little bit of work into this, and I just wanted to get some clarification on the intent before finishing up and posting a PR;

test(t => {
  const err = t.throws(function () { throw new Error('foo'); });
  t.is(err.message, 'foo');
});

test(t => {
  const err = await t.throws(function () { return Promise.reject(new Error('foo')); });
  t.is(err.message, 'foo');
});

Regardless of what one passes as the second/third argument to t.throws, you always want to get back the (or in the case of string, an) Error with the expected message. Is that a correct assessment or am I (miles) off?

I must admit, I'm a little confused by your example - are you expecting a property on the given Error object of foo that should equal the expected message?

---

I haven't taken #468 into consideration just yet - but I'd like to get cracking on that soon-ish as well. Babysteps.

[novemberborn]

@kasperisager does this help?

test(t => {
  const expected = new Error()
  const actual = t.throws(() => { throw expected })
  t.is(actual, expected)
})

test(t => {
  const expected = new Error()
  const actual = t.throws(() => Promise.reject(expected)
  t.is(actual, expected)
})

In other words t.throws() should return whatever was thrown (or, if a rejected promise was returned, the rejection reason).

[kasperlewau]

Sorry @novemberborn - if your poke was meant for me, I'm afraid you mistyped ever so slightly. Didn't get any notification(s). Regardless, you did make it clearer - tyvm!

In my feature branch - t.throws returns whatever was thrown in synchronous assertions, so that's pretty much covered.

In the case of Promise.reject I've got something somewhat working. I say somewhat, because it doesn't actually become the rejection reason - the result of Test.prototype.exit is decorated with a message property (the message of the rejection, defaults to '' as per Error). This "had" to be done in order to not break the entire promise test suite, what with the expectations on result.

Haven't posted it for review just yet as I know that isn't what the issue description calls for - but if one were to resolve in exit with the plain rejection of the Promise, we'd lose out on all of the default result properties from my (so far) limited findings.

Sidenote; First attempt(s) at handling this solely in .throws were futile 😄

---

tl;dr; I'm handling rejected promises by returning the result of t.throws to Test.prototype.exit, conditionally assigning RejectionError.message to the result of the assertion, if that makes any sense. I'll gladly post what I have so as to give a clearer picture of what's going on.

---

test(t => {
  const expected = 'foo'
  const actual = t.throws(() => Promise.reject(new Error(expected))
  t.is(actual.message, expected)
})

Not quite there yet I'd say.

[kasperlewau]

On that note; If we're fine with dropping the properties received from executing _result() (that would be 'passed', 'result' and 'reason') - I can sort that out by switching some of my code around & removing some assertions out of test/promise.

[kasperlewau]

Q: Do you reckon this behaviour should apply to an Observable as well? I made some changes in my branch to return the full Promise rejection which caused a bunch of failures in test/observable.

test(t => {
  const expected = 'foo';
  const actual = t.throws(() => new Observable(function (o) { o.error(new Error('foo'); }));
  t.is(actual.message, 'foo');
});

[novemberborn]

@kasperlewau,

Sorry @novemberborn - if your poke was meant for me, I'm afraid you mistyped ever so slightly. Didn't get any notification(s). Regardless, you did make it clearer - tyvm!

Too many Kaspers already! 😜

---

Looking at @jamestalmage's original example again I believe the intent is that if t.throws() passes, then the error should be returned. This then allows you to apply further assertions to the error. If it fails then your test has already failed. There's no more need to access the error object.

That would imply wrapping the call to fn in https://github.com/sindresorhus/ava/blob/f77ded959a0e7de6e9bf65d8c2fd6625bc59ba47/lib/assert.js#L99 to capture the thrown error and returning it after the assert.throws() call. If assert.throws() itself throws then the assertion has failed and no error is returned (because an AssertionError is thrown). There may not even be an error in case fn didn't throw one.

You'd need to return x.throws in the promise-handling code as well for it to bubble up correctly.

No changes should be needed to Test.prototype.exit.

#468 is related but should not have any bearing on this issue.

[kasperlewau]

@novemberborn One can never have too many Kaspers! : )

---

Your suggestion sounds close to what I've done so far.

This commit handles the wrapping of fn to return the thrown Error. Simple enough, but it could probably be cleaned up ever so slightly.

This other commit handles Promise bubbling out of assert.throws to the caller. I've tried to keep changes to .exit to a minimum, but I have yet to figure out how to avoid it entirely.

---

I can go ahead and post a [WIP] PR, perhaps that is a better way to go about this rather than referring to individual commits living in my fork?

[novemberborn]

Yea please open a PR.