Return thrown error/rejected promise from t.throws #576

kasperlewau · 2016-02-29T12:39:14Z

If a throws assertion passes, we should return the reason for said pass. That entails;

A thrown Error.
A Promise rejection.
An Observable error.

Loosely based on the description of #493. I consider it to be loosely based on it, because of the following differences (in the Promised-land):

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

// Implemented
test(t => {
  const expected = new Error('foo');
  const actual = t.throws(() => Promise.reject(expected));
  t.is(expected, actual.reason); //true 
})

I hijacked the result.reason property because I wasn't fully comfortable in excluding the base result properties out of a successful throws assertion, which was heavily used in test/promise and test/observable. However this can be changed quite easily, to more closely reflect the initial desires of #493.

Haven't taken a stab at docs for the changes made yet, I'll get on top of that on my way home from work.

mention-bot · 2016-02-29T12:39:17Z

By analyzing the blame information on this pull request, we identified @ariporad, @SamVerschueren and @sotojuan to be potential reviewers

novemberborn · 2016-02-29T17:51:48Z

lib/assert.js

 			err = function (err) {
 				return err.message === errMsg;
 			};
 		}

 		assert.throws(fn, err, msg);
+
+		if (errMsg) {
+			return Error(errMsg);


errMsg is the expected message. The workaround above stems from #125 and was done because assert.throws doesn't take a string argument as the expected message, but we did want that behavior in AVA.

In other words it's not something to be returned.

Might actually be able to kill this if-block entirely given your suggestion at https://github.com/sindresorhus/ava/pull/576/files#r54448454. I'll give it a whirl!

Yes, you're returning a very different value here.

novemberborn · 2016-02-29T18:06:19Z

I don't understand why you're changing lib/test. What am I missing?

No need to exhaustively test .throws(). Just need to verify it returns the thrown error. You do need a test for when a rejected promise is returned though. You shouldn't need to change the observable and promise tests.

novemberborn · 2016-02-29T18:07:16Z

Cool getting there!

kasperlewau · 2016-02-29T18:19:35Z

I don't understand why you're changing lib/test. What am I missing?

This might be a case of me not writing the tests as one should to get it working as expected. But when going back and reverting the changes made to lib/test makes the following spec break;

// test/promise.js
test('throws becomes the promise rejection', function (t) {
    var rejection;
    ava(function (a) {
        a.plan(1);

        rejection = new Error('foo');
        var promise = Promise.reject(rejection);
        return a.throws(promise);
    }).run().then(function (result) {
        t.is(result.passed, true);
        t.is(result.result.assertCount, 1);
        t.is(result.reason, rejection);
        t.end();
    });
});

this line is the real deal-breaker in this specific case. Without it, I don't see how one would return the rejection reason to the callee.

Again, it's not that far-fetched that this test does not in fact belong here and I'm just doing it wrong.

On that note, my pizza just arrived. I'll be back in a bit 🍕

kasperlewau · 2016-03-01T07:57:34Z

Sorry, I phased out after having my supper last night (I blame Comedy Central). I got around to cleaning some stuff out, but I'm sort of at a loss on how to handle the test case for a Promise rejection.

So far I'm at this point;

test('.throws() becomes the rejection reason of promise', function (t) {
    var expected = new Error();

    assert.throws(Promise.reject(expected)).then(function (err) {
        assert.same(expected, err);
        t.end();
    });
});

But that's not actually testing the return value of throws. Still working on trying to figure that one out - I keep looking at test/promises and the way Promises are handled in there but, I haven't gotten it to work just yet.

kasperlewau · 2016-03-01T09:34:26Z

Alright, I reckon we're good to go there with the latest changes made.

lib/test changes are gone.
test/[promises|observables] changes are gone.
Promise rejection assertion is in place in test/assert
- Could potentially be cleaned up ever so slightly?
Code has been touched up as per @novemberborn's suggestions.

Let me know what you think, I've got an itch in my squash finger.

I guess a brief look from @jamestalmage would be warranted as well, given he's the author of #493.

vadimdemedes · 2016-03-01T12:16:28Z

To me, returning object with reason property is not logical. If t.throws() throws, everything is good and execution is continued, there's no "failure reason", because nothing failed. Why don't we just return error from t.throws()?

kasperlewau · 2016-03-01T12:24:38Z

@vdemedes Sorry, I should've probably updated the OP. Like I stated before, I wasn't all too comfy with overriding the return value of test.exit to reflect what happened with test.throws. I overcomplicated the matter.

The way it works now is this;

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

Whereas what I was doing in the beginning was something like so;

test(t => {
  const expected = new Error();
  return t.throws(Promise.reject(expected));
}).then(result) {
  // result.reason === expected
}

I was trying to cater to the test/promise suite, as opposed to testing throws where it should be tested (in test/assert). That whole snafu is gone now however, after touching up on the parts Mark mentioned.

I'll gladly edit the OP when I get a on the train. At work currently.

vadimdemedes · 2016-03-01T12:25:38Z

Oh great, so it does return thrown Error from t.throws(), right? No rush, thank you!

kasperlewau · 2016-03-01T12:27:28Z

Oh great, so it does return thrown Error from t.throws(), right?

It does indeed! :)

novemberborn · 2016-03-01T13:03:14Z

test/assert.js

@@ -182,6 +183,29 @@ test('.throws()', function (t) {
 	t.end();
 });

+test('.throws() becomes the thrown Error', function (t) {


returns rather than becomes. Could lowercase Error as well.

Would you like for me to change the subject of the Promise rejection while I'm at it? @novemberborn

'.throws() becomes the rejection reason of promise'
'.throws() returns the rejection reason of promise'

Yeah, '.throws() returns the rejection reason of promise' is a correct version indeed.

novemberborn · 2016-03-01T13:06:15Z

@kasperlewau cool! Just some simplifications in the test. Feel free to push up a squashed commit when that's fixed and I'll merge.

kasperlewau · 2016-03-01T14:07:22Z

@novemberborn Resolved, squashed, pushed. Let the CI wait commence.

novemberborn · 2016-03-01T14:50:08Z

test/assert.js

+		throw expected;
+	});
+
+	assert.same(actual, expected);


Oops wait. assert.same() means "deep equal". We want strict equality here. You should use assert.is(actual, expected) instead. Though the tests here are for AVA's assert module. Perhaps use t.is(actual, expected) which is from tap's assertion library (AVA's tests are run using tap).

Same below.

novemberborn · 2016-03-01T14:51:16Z

@kasperlewau OK just the tiniest issue with the assertion. Feel free to force push again and I'll merge.

kasperlewau · 2016-03-01T14:57:20Z

@novemberborn

Right-o! I'm still fairly new to ava (and tap for that matter). Coming from a chai background so I've hit the t.equal is not a function several times over now 😉

Off topic; same is a little bit far fetched when you want to perform a deep comparison. If you think twice about it, it makes all the more sense though. imho.

I'll get this sorted out!

kasperlewau · 2016-03-02T08:35:56Z

Rebasing and pushing in a jiffy.

Fixes #493

Return thrown error/rejected promise from t.throws

novemberborn · 2016-03-02T11:27:12Z

Thanks @kasperlewau!

kasperlewau · 2016-03-02T11:35:00Z

Glad I could help! : )

sindresorhus · 2016-03-02T12:47:59Z

Thank you! Keep'em coming :)

vadimdemedes · 2016-03-02T12:48:14Z

Thanks a lot!

PR #576 changed the `t.throws()` assertion to return the thrown error, or if asynchronous a promise for the rejection reason. Unfortunately this only worked for asynchronous errors. The tests cover the changes in `lib/assert.js` but `t.throws()` is an *enhanced* assertion. This commit ensures any values returned from `lib/assert.js` assertions are indeed returned by the corresponding `t.` assertions.

Scrum mentioned this pull request Feb 29, 2016

write tests posthtml/posthtml-cli#4

Closed

novemberborn reviewed Feb 29, 2016
View reviewed changes

kasperlewau changed the title ~~[WIP] t.throws return reason(s)~~ t.throws return thrown error/promise rejection Mar 1, 2016

kasperlewau changed the title ~~t.throws return thrown error/promise rejection~~ Return thrown error/rejected promise from t.throws Mar 1, 2016

novemberborn reviewed Mar 1, 2016
View reviewed changes

novemberborn mentioned this pull request Mar 1, 2016

[breaking] Don't convert non-Error promise rejection reason #582

Closed

sindresorhus force-pushed the master branch from c6214aa to 2f10448 Compare March 2, 2016 07:42

throws returns error or rejection reason of promise

e03b82c

Fixes #493

novemberborn added a commit that referenced this pull request Mar 2, 2016

Merge pull request #576 from kasperlewau/throws-return

829f91c

Return thrown error/rejected promise from t.throws

novemberborn merged commit 829f91c into avajs:master Mar 2, 2016

novemberborn mentioned this pull request Mar 17, 2016

Ensure t.throws() returns the error #651

Merged

kasperlewau mentioned this pull request Mar 29, 2016

t.throws shows undefined undefined undefined when a string is thrown #661

Closed

kasperlewau deleted the throws-return branch June 13, 2016 07:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return thrown error/rejected promise from t.throws #576

Return thrown error/rejected promise from t.throws #576

kasperlewau commented Feb 29, 2016

mention-bot commented Feb 29, 2016

novemberborn Feb 29, 2016

kasperlewau Feb 29, 2016

novemberborn Feb 29, 2016

kasperlewau Feb 29, 2016

novemberborn commented Feb 29, 2016

novemberborn commented Feb 29, 2016

kasperlewau commented Feb 29, 2016

kasperlewau commented Mar 1, 2016

kasperlewau commented Mar 1, 2016

vadimdemedes commented Mar 1, 2016

kasperlewau commented Mar 1, 2016

vadimdemedes commented Mar 1, 2016

kasperlewau commented Mar 1, 2016

novemberborn Mar 1, 2016

kasperlewau Mar 1, 2016

kasperlewau Mar 1, 2016

vadimdemedes Mar 1, 2016

novemberborn commented Mar 1, 2016

kasperlewau commented Mar 1, 2016

novemberborn Mar 1, 2016

novemberborn commented Mar 1, 2016

kasperlewau commented Mar 1, 2016

kasperlewau commented Mar 2, 2016

novemberborn commented Mar 2, 2016

kasperlewau commented Mar 2, 2016

sindresorhus commented Mar 2, 2016

vadimdemedes commented Mar 2, 2016

Return thrown error/rejected promise from t.throws #576

Return thrown error/rejected promise from t.throws #576

Conversation

kasperlewau commented Feb 29, 2016

mention-bot commented Feb 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

novemberborn commented Feb 29, 2016

novemberborn commented Feb 29, 2016

kasperlewau commented Feb 29, 2016

kasperlewau commented Mar 1, 2016

kasperlewau commented Mar 1, 2016

vadimdemedes commented Mar 1, 2016

kasperlewau commented Mar 1, 2016

vadimdemedes commented Mar 1, 2016

kasperlewau commented Mar 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

novemberborn commented Mar 1, 2016

kasperlewau commented Mar 1, 2016

Choose a reason for hiding this comment

novemberborn commented Mar 1, 2016

kasperlewau commented Mar 1, 2016

kasperlewau commented Mar 2, 2016

novemberborn commented Mar 2, 2016

kasperlewau commented Mar 2, 2016

sindresorhus commented Mar 2, 2016

vadimdemedes commented Mar 2, 2016