Make `t.throws()` accept async function as parameter #1650

oantoro · 2018-01-23T14:19:47Z

When an async callback is given to t.throws(), it will execute the function and put the returned Promise to promise variable. The rest is same as processing Promise.
This commit introduce new private function isAsync() inside asserts.wrapAssertions()

Refs: #1371

oantoro · 2018-01-23T14:24:40Z

Hello, I am new here 😄
Need suggestions about this pull request
Thank you

oantoro · 2018-01-23T14:45:57Z

Broken on node 4 and 6

novemberborn · 2018-01-23T17:33:04Z

Broken on node 4 and 6

async is only supported in Node.js 8.

I don't think it matters whether the function is truly an AsyncFunction. Instead we should check the return value of fn and if it's a promise or an observable wait for it to resolve, like we already do when you pass a promise or observable directly.

oantoro · 2018-01-23T22:35:59Z

Instead we should check the return value of fn and if it's a promise or an observable wait for it to resolve, like we already do when you pass a promise or observable directly.

@novemberborn So we need to try to execute the fn inside a try {} catch(e) {}, right?

novemberborn · 2018-01-24T10:58:52Z

So we need to try to execute the fn inside a try {} catch(e) {}, right?

@okyantoro close. The way you're doing it now you're calling fn() twice if it doesn't return a promise/observable. It should only be called once.

I think the trick is to capture fn()'s return value, and then when coreAssert.throws() itself, well, throws, see if a promise/observable was returned and run the promise handling routine (which should be extracted into a function, and you'd have to convert the observable to a promise as well).

This isn't great because of how we wrap coreAssert.throws(). #1047 contains the proposal that removes this dependency, but it's not a deal breaker for this feature.

oantoro · 2018-01-24T14:38:01Z

@novemberborn I realize that calling fn() twice is dangerous, especially when the fn() it self modify outer states.
I try different approach like what you suggested and call the fn() only once.

novemberborn

Looking good @okyantoro, almost there!

t.notThrows() should be updated so that if the function returns a rejected promise, the assertion still fails.

The documentation for t.throws() and t.notThrows() should be updated to reflect the new behavior.

Similarly the TypeScript definition needs to be updated. I don't think the current Flow types prohibit this behavior but the documentation should be updated there too.

Let me know if you get stuck anywhere. The type definitions may be a bit daunting if you haven't used TypeScript or Flow before.

novemberborn · 2018-01-24T16:07:58Z

lib/assert.js

+			};
+
+			if (promise) {
+				return handlePromise.call(this, promise);


handlePromise is an arrow function, so .call(this isn't necessary. Just do return handlePromise(promise).

novemberborn · 2018-01-24T16:08:09Z

lib/assert.js

+						return handlePromise.call(this, maybePromise);
+					}
+					if (isObservable(maybePromise)) {
+						return handlePromise.call(this, observableToPromise(maybePromise));


Could you add a test for this code path?

sure, we can use already provided library zen-observable in package.json

novemberborn

(Just updating the PR state to reflect that it needs a bit more work before it's merged.)

oantoro · 2018-01-26T03:09:17Z

hi @novemberborn I'll continue this on saturday or sunday. Thank you for the suggestions and help

oantoro · 2018-01-26T12:41:55Z

hello @novemberborn
In notThrows(), the fn() is executed inside coreAssert.doesNotThrow() directly. Is it okay to wrap the fn() by using arrow function so we can check the returned value of fn()?

novemberborn · 2018-01-26T13:25:42Z

In notThrows(), the fn() is executed inside coreAssert.doesNotThrow() directly. Is it okay to wrap the fn() by using arrow function so we can check the returned value of fn()?

@okyantoro yup.

oantoro · 2018-01-26T22:56:17Z

@novemberborn when I try to add function signature for function that return promise, I get the following error:

Error: test/flow-types/regression-1148.js.flow:6
  6: 	t.throws(() => { throw new Error(); });
     	^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ call of method `throws`. Method cannot be called on any member of intersection type
  6: 	t.throws(() => { throw new Error(); });
     	^ intersection
  Member 1:
   91: type ContextualTestContext         = TestContext & { context: any; };
                                            ^^^^^^^^^^^ TestContext. See: index.js.flow:91
  Error:
    6: 	t.throws(() => { throw new Error(); });
       	^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ call of method `throws`. Method cannot be called on any member of intersection type
   91: type ContextualTestContext         = TestContext & { context: any; };
                                            ^^^^^^^^^^^ intersection. See: index.js.flow:91
    Member 1:
     85: type TestContext = AssertContext & {
                            ^^^^^^^^^^^^^ AssertContext. See: index.js.flow:85
    Error:
      6: 	t.throws(() => { throw new Error(); });
         	^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ call of method `throws`. Could not decide which case to select
      6: 	t.throws(() => { throw new Error(); });
         	^^^^^^^^ callable object type
      Case 2 may work:
       62: 		(value: () => PromiseLike<mixed>, error?: ErrorValidator, message?: string): Promise<any>;
           		^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ function type. See: index.js.flow:62
      But if it doesn't, case 3 looks promising too:
       63: 		(value: () => mixed, error?: ErrorValidator, message?: string): any;
           		^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ function type. See: index.js.flow:63
      Please provide additional annotation(s) to determine whether case 2 works (or consider merging it with case 3):
        6: 	t.throws(() => { throw new Error(); });
           	              ^ return
    Member 2:
                                            v
     85: type TestContext = AssertContext & {
     86: 	title: string;
     87: 	plan(count: number): void;
    ...:
     90: };
         ^ object type. See: index.js.flow:85
    Error:
      6: 	t.throws(() => { throw new Error(); });
         	  ^^^^^^ property `throws`. Property not found in
                                            v
     85: type TestContext = AssertContext & {
     86: 	title: string;
     87: 	plan(count: number): void;
    ...:
     90: };
         ^ object type. See: index.js.flow:85
  Member 2:
   91: type ContextualTestContext         = TestContext & { context: any; };
                                                          ^^^^^^^^^^^^^^^^^ object type. See: index.js.flow:91
  Error:
    6: 	t.throws(() => { throw new Error(); });
       	  ^^^^^^ property `throws`. Property not found in
   91: type ContextualTestContext         = TestContext & { context: any; };
                                                          ^^^^^^^^^^^^^^^^^ object type. See: index.js.flow:91

Error: test/flow-types/regression-1148.js.flow:9
  9: 	t.notThrows(() => { return; });
     	^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ call of method `notThrows`. Method cannot be called on any member of intersection type
  9: 	t.notThrows(() => { return; });
     	^ intersection
  Member 1:
   91: type ContextualTestContext         = TestContext & { context: any; };
                                            ^^^^^^^^^^^ TestContext. See: index.js.flow:91
  Error:
    9: 	t.notThrows(() => { return; });
       	^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ call of method `notThrows`. Method cannot be called on any member of intersection type
   91: type ContextualTestContext         = TestContext & { context: any; };
                                            ^^^^^^^^^^^ intersection. See: index.js.flow:91
    Member 1:
     85: type TestContext = AssertContext & {
                            ^^^^^^^^^^^^^ AssertContext. See: index.js.flow:85
    Error:
      9: 	t.notThrows(() => { return; });
         	^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ call of method `notThrows`. Could not decide which case to select
      9: 	t.notThrows(() => { return; });
         	^^^^^^^^^^^ callable object type
      Case 2 may work:
       68: 		(value: () => PromiseLike<mixed>, message?: string): Promise<void>;
           		^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ function type. See: index.js.flow:68
      But if it doesn't, case 3 looks promising too:
       69: 		(value: () => mixed, message?: string): void;
           		^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ function type. See: index.js.flow:69
      Please provide additional annotation(s) to determine whether case 2 works (or consider merging it with case 3):
        9: 	t.notThrows(() => { return; });
           	                 ^ return
    Member 2:
                                            v
     85: type TestContext = AssertContext & {
     86: 	title: string;
     87: 	plan(count: number): void;
    ...:
     90: };
         ^ object type. See: index.js.flow:85
    Error:
      9: 	t.notThrows(() => { return; });
         	  ^^^^^^^^^ property `notThrows`. Property not found in
                                            v
     85: type TestContext = AssertContext & {
     86: 	title: string;
     87: 	plan(count: number): void;
    ...:
     90: };
         ^ object type. See: index.js.flow:85
  Member 2:
   91: type ContextualTestContext         = TestContext & { context: any; };
                                                          ^^^^^^^^^^^^^^^^^ object type. See: index.js.flow:91
  Error:
    9: 	t.notThrows(() => { return; });
       	  ^^^^^^^^^ property `notThrows`. Property not found in
   91: type ContextualTestContext         = TestContext & { context: any; };
                                                          ^^^^^^^^^^^^^^^^^ object type. See: index.js.flow:91

Error: test/flow-types/regression-1148.js.flow:12
 12: 	const error = t.throws(() => { throw new Error(); });
     	              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ call of method `throws`. Method cannot be called on any member of intersection type
 12: 	const error = t.throws(() => { throw new Error(); });
     	              ^ intersection
  Member 1:
   91: type ContextualTestContext         = TestContext & { context: any; };
                                            ^^^^^^^^^^^ TestContext. See: index.js.flow:91
  Error:
   12: 	const error = t.throws(() => { throw new Error(); });
       	              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ call of method `throws`. Method cannot be called on any member of intersection type
   91: type ContextualTestContext         = TestContext & { context: any; };
                                            ^^^^^^^^^^^ intersection. See: index.js.flow:91
    Member 1:
     85: type TestContext = AssertContext & {
                            ^^^^^^^^^^^^^ AssertContext. See: index.js.flow:85
    Error:
     12: 	const error = t.throws(() => { throw new Error(); });
         	              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ call of method `throws`. Could not decide which case to select
     12: 	const error = t.throws(() => { throw new Error(); });
         	              ^^^^^^^^ callable object type
      Case 2 may work:
       62: 		(value: () => PromiseLike<mixed>, error?: ErrorValidator, message?: string): Promise<any>;
           		^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ function type. See: index.js.flow:62
      But if it doesn't, case 3 looks promising too:
       63: 		(value: () => mixed, error?: ErrorValidator, message?: string): any;
           		^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ function type. See: index.js.flow:63
      Please provide additional annotation(s) to determine whether case 2 works (or consider merging it with case 3):
       12: 	const error = t.throws(() => { throw new Error(); });
           	                            ^ return
    Member 2:
                                            v
     85: type TestContext = AssertContext & {
     86: 	title: string;
     87: 	plan(count: number): void;
    ...:
     90: };
         ^ object type. See: index.js.flow:85
    Error:
     12: 	const error = t.throws(() => { throw new Error(); });
         	                ^^^^^^ property `throws`. Property not found in
                                            v
     85: type TestContext = AssertContext & {
     86: 	title: string;
     87: 	plan(count: number): void;
    ...:
     90: };
         ^ object type. See: index.js.flow:85
  Member 2:
   91: type ContextualTestContext         = TestContext & { context: any; };
                                                          ^^^^^^^^^^^^^^^^^ object type. See: index.js.flow:91
  Error:
   12: 	const error = t.throws(() => { throw new Error(); });
       	                ^^^^^^ property `throws`. Property not found in
   91: type ContextualTestContext         = TestContext & { context: any; };
                                                          ^^^^^^^^^^^^^^^^^ object type. See: index.js.flow:91

or maybe I did something wrong when added the function signature.

sindresorhus · 2018-01-27T06:21:51Z

index.js.flow

@@ -56,12 +56,14 @@ type AssertContext = {
 	// Assert that value is not deep equal to expected.
 	notDeepEqual<U>(value: U, expected: U, message?: string): void;
 	// Assert that function throws an error or promise rejects.
+	// If the given function returns Promise, then the Promise will be evaluated instead.


Promise => a Promise

sindresorhus · 2018-01-27T06:21:56Z

index.js.flow

 	// @param error Can be a constructor, regex, error message or validation function.
 	throws: {
 		(value: PromiseLike<mixed>, error?: ErrorValidator, message?: string): Promise<any>;
 		(value: () => mixed, error?: ErrorValidator, message?: string): any;
 	};
 	// Assert that function doesn't throw an error or promise resolves.
+	// If the given function returns Promise, then the Promise will be evaluated instead.


Promise => a Promise

sindresorhus · 2018-01-27T06:23:15Z

readme.md

@@ -948,7 +948,7 @@ Assert that `value` is not deeply equal to `expected`. The inverse of `.deepEqua

 ### `.throws(function|promise, [error, [message]])`

-Assert that `function` throws an error, or `promise` rejects with an error.
+Assert that `function` throws an error, `promise` rejects with an error, or `function` that return rejected `promise`.


or `function` that return rejected `promise`.

⬇️

or `function` returns a rejected `promise`.

sindresorhus · 2018-01-27T06:25:57Z

readme.md

@@ -987,9 +997,19 @@ test('rejects', async t => {
 });
 ```

+Because a `function` that return a `promise` is allowed to be a parameter, then you can pass `async function` that throws exception as parameter:


This sentence needs to be better worded.

maybe we can make it simple like:

When testing an async function you must also wait for the assertion to complete:

@sindresorhus is it better?

sindresorhus · 2018-01-27T06:26:22Z

readme.md

 ### `.notThrows(function|promise, [message])`

-Assert that `function` does not throw an error or that `promise` does not reject with an error.
+Assert that `function` does not throw an error or that `promise` does not reject with an error. When you pass a `function` that return a `promise` as parameter, the returned promise will be evaluated instead.


return => returns

novemberborn

when I try to add function signature for function that return promise, I get the following error:

Odd! Perhaps try upgrading the Flow version used within AVA, perhaps it's a bug that has since been fixed.

This is almost there @okyantoro 👍 Don't forget to add pass and failure cases to https://github.com/avajs/ava/blob/a051d3e18dba92c893fddb08490a8627f586c231/test/assert.js#L624:L659 and https://github.com/avajs/ava/blob/master/test/assert.js#L701:L727.

novemberborn · 2018-01-27T13:23:13Z

index.js.flow

@@ -56,12 +56,14 @@ type AssertContext = {
 	// Assert that value is not deep equal to expected.
 	notDeepEqual<U>(value: U, expected: U, message?: string): void;
 	// Assert that function throws an error or promise rejects.
+	// If the given function returns a Promise, then the Promise will be evaluated instead.


Let's go with:

// Assert that the promise rejects, or the function throws or returns a rejected promise.

novemberborn · 2018-01-27T13:23:28Z

index.js.flow

 	// @param error Can be a constructor, regex, error message or validation function.
 	throws: {
 		(value: PromiseLike<mixed>, error?: ErrorValidator, message?: string): Promise<any>;
 		(value: () => mixed, error?: ErrorValidator, message?: string): any;
 	};
 	// Assert that function doesn't throw an error or promise resolves.
+	// If the given function returns a Promise, then the Promise will be evaluated instead.


Let's go with:

// Assert that the promise resolves, or the function doesn't throws or returns a resolved promise.

novemberborn · 2018-01-27T13:26:30Z

readme.md

+	await t.throws(async () => {
+		throw new TypeError('🦄');
+	});
+});


Perhaps replace this example with the async function one you added above, and remove it there.

novemberborn · 2018-01-27T13:27:29Z

readme.md

 ### `.notThrows(function|promise, [message])`

-Assert that `function` does not throw an error or that `promise` does not reject with an error.
+Assert that `function` does not throw an error or that `promise` does not reject with an error. When you pass a `function` that returns a `promise` as parameter, the returned promise will be evaluated instead.


Assert that function does not throw an error, promise does not reject with an error, or function returns a promise that does not reject with an error.

novemberborn · 2018-01-27T13:28:03Z

test/assert.js

@@ -678,6 +679,28 @@ test('.throws() returns the rejection reason of promise', t => {
 	});
 });

+test('.throws() returns the rejection reason of function that return rejected `Promise`', t => {


.throws() returns the rejection reason of a promise returned by the function

novemberborn · 2018-01-27T13:30:01Z

test/assert.js

+	});
+});
+
+test('.throws() throws exception from `Observable`', t => {


.throws() returns the error of an observable returned by the function

novemberborn · 2018-01-27T13:31:16Z

test/assert.js

@@ -732,6 +755,22 @@ test('.notThrows() returns undefined for a fulfilled promise', t => {
 	});
 });

+test('.notThrows() returns undefined for a function that return fulfilled promise', t => {


.notThrows() returns undefined for a fulfilled promise returned by the function

novemberborn · 2018-01-27T13:31:27Z

test/assert.js

+	});
+});
+
+test('.notThrows() returns undefined for a function that return `Observable`', t => {


.notThrows() returns undefined for an observable returned by the function

oantoro · 2018-01-28T09:56:37Z

hi @novemberborn
Thank you for reviewing
I think flow will not allow if we have two function signatures with subset type like:

(value: () => PromiseLike<mixed>, error?: ErrorValidator, message?: string): Promise<any>;
(value: () => mixed, error?: ErrorValidator, message?: string): any;

correct me if I am wrong.

Don't forget to add pass and failure cases to https://github.com/avajs/ava/blob/a051d3e18dba92c893fddb08490a8627f586c231/test/assert.js#L624:L659 and https://github.com/avajs/ava/blob/master/test/assert.js#L701:L727.

isn't it already covered by these lines:

test('.throws() returns the rejection reason of promise', t => {
	const expected = new Error();

	return assertions.throws(Promise.reject(expected)).then(actual => {
		t.is(actual, expected);
		t.end();
	});
});

test('.throws() returns the rejection reason of a promise returned by the function', t => {
	const expected = new Error();

	return assertions.throws(() => {
		return Promise.reject(expected);
	}).then(actual => {
		t.is(actual, expected);
		t.end();
	});
});

and

test('.notThrows() returns undefined for a fulfilled promise', t => {
	return assertions.notThrows(Promise.resolve(Symbol(''))).then(actual => {
		t.is(actual, undefined);
	});
});

test('.notThrows() returns undefined for a fulfilled promise returned by the function', t => {
	return assertions.notThrows(() => {
		return Promise.resolve(Symbol(''));
	}).then(actual => {
		t.is(actual, undefined);
	});
});

?

Thank you

novemberborn · 2018-01-28T15:05:38Z

I think flow will not allow if we have two function signatures with subset type like:

(value: () => PromiseLike, error?: ErrorValidator, message?: string): Promise;
(value: () => mixed, error?: ErrorValidator, message?: string): any;
correct me if I am wrong.

I don't know @okyantoro. Don't worry about it, we can always open an issue for somebody else to refine the signature.

Don't forget to add pass and failure cases to /test/assert.js@a051d3e#L624:L659 and /test/assert.js@master#L701:L727.

isn't it already covered by these lines:

Those tests are specifically about the expected pass/fail behavior, given all the ways the assertion can be used. The other tests are about their return value. They exercise the same code paths but they're a little different in intent. It's useful to see all possible behaviors in one place.

…ter. Refs: avajs#1371

oantoro · 2018-01-28T17:23:45Z

@novemberborn Oh I see. Updated. Is it correct?

novemberborn · 2018-01-29T12:21:12Z

@okyantoro this is great! I pushed some minor tweaks but will merge when CI passes.

Regarding my previous comment:

Those tests are specifically about the expected pass/fail behavior, given all the ways the assertion can be used. The other tests are about their return value. They exercise the same code paths but they're a little different in intent. It's useful to see all possible behaviors in one place.

Turns out I was wrong here, test/assert.js doesn't contain explicit assertions for promises and observables. These are in test/promise.js and test/observable.js. There's some legacy test organization in the way there 😄 I've made the necessary changes in the commit I just pushed.

oantoro · 2018-01-29T21:20:37Z

Thank you @novemberborn

oantoro force-pushed the async-throw branch from 2f27ea4 to 40945ec Compare January 24, 2018 08:16

oantoro force-pushed the async-throw branch from 40945ec to a40314e Compare January 24, 2018 14:34

oantoro force-pushed the async-throw branch from a40314e to 000826e Compare January 24, 2018 15:33

novemberborn reviewed Jan 24, 2018

View reviewed changes

novemberborn requested changes Jan 24, 2018

View reviewed changes

sindresorhus changed the title ~~assert: make t.throws() accept async function as parameter.~~ Make t.throws() accept async function as parameter Jan 25, 2018

oantoro force-pushed the async-throw branch from 000826e to d96ff68 Compare January 26, 2018 22:40

sindresorhus requested changes Jan 27, 2018

View reviewed changes

oantoro force-pushed the async-throw branch from d96ff68 to 211cd88 Compare January 27, 2018 07:29

novemberborn reviewed Jan 27, 2018

View reviewed changes

oantoro force-pushed the async-throw branch from 211cd88 to eac1ac3 Compare January 28, 2018 09:43

Make t.throws() and t.notThrows() accept async function as parame…

d451174

…ter. Refs: avajs#1371

oantoro force-pushed the async-throw branch from eac1ac3 to d451174 Compare January 28, 2018 17:03

Minor tweaks

ad010ad

novemberborn approved these changes Jan 29, 2018

View reviewed changes

novemberborn merged commit 40604ee into avajs:master Jan 29, 2018

oantoro deleted the async-throw branch January 29, 2018 21:20

novemberborn mentioned this pull request Feb 12, 2018

Unhandled rejection error on "t.throws()" test of trivial async function #1371

Closed

Make t.throws() accept async function as parameter #1650

Make t.throws() accept async function as parameter #1650

Conversation

oantoro commented Jan 23, 2018

oantoro commented Jan 23, 2018

oantoro commented Jan 23, 2018

novemberborn commented Jan 23, 2018

oantoro commented Jan 23, 2018

novemberborn commented Jan 24, 2018

oantoro commented Jan 24, 2018

novemberborn left a comment

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 left a comment

Choose a reason for hiding this comment

oantoro commented Jan 26, 2018

oantoro commented Jan 26, 2018

novemberborn commented Jan 26, 2018

oantoro commented Jan 26, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

novemberborn left a comment

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

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

Choose a reason for hiding this comment

oantoro commented Jan 28, 2018

novemberborn commented Jan 28, 2018

oantoro commented Jan 28, 2018

novemberborn commented Jan 29, 2018

oantoro commented Jan 29, 2018

Make `t.throws()` accept async function as parameter #1650

Make `t.throws()` accept async function as parameter #1650