[Breaking] support passing in an async function for the test callback #472

Raynos · 2019-06-20T11:43:09Z

This PR adds rudimentary support for async/await in a fashion
that breaks back compat as little as possible.

Currently tape has no opinions about exception handling.
If you throw in a test block it will go straight to the
uncaught handler and exit the process.

However newer versions of node & v8 added async functions.
These functions when thrown return a rejected promise.

Currently node logs unhandled rejections with a warning
and in future versions of node, unhandled rejections may get
forwarded to uncaught exceptions.

This patch modifies tape so that an unhandled rejection will be
marked as a TAP failure and fails the tests with exit code 1.

Previously on master it would log to STDERR and then continue
onwards running the rests of the tests.

For example :

var tape = require('tape')

tape('my test', function (t) {
  // this causes the tests to fail with exit code 1
  t.oktypo(true, 'my bad')
})

tape('my async test', async function (t) {
  // on master : this causes the tests to pass with exit code 0 
  //     and STDERR noise about unhandled rejections
  // in this PR : this causes the tests to fail with exit code 1
  t.oktypo(true, 'my bad')
})

This patch is optional, there is a different work around you can do

process.on('unhandledRejection', function (err) { throw err })

var tape = require('tape')

tape('my async test', async function (t) {
  // on master : this causes the tests to fail with exit code 1
  t.oktypo(true, 'my bad')
})

But adding that one liner to the top of every test file
can get quite annoying and EASY to forget :(

Back compat is broken for some cases, only when the user of tape mixes promises & callbacks & async functions in a single test block. If the async function returns before you call t.end() ( aka you do not await the code that calls t.end()

However no back compat breaks for users that

do not use async/await
do not use promises
uses async/await but only with promises
uses promises and no callbacks
uses async/await, promises & callbacks but always does await util.promisify((cb) { ...; t.end() }

This change makes tape work the same with synchronous and asynchronous functions. ``` test('my test', () => { throw new Error('oopsie') }) test('my async test', async () => { throw new Error('oopsie') }) ``` These two cases now have the same semantics which means you can safely use an async function because the unhandled rejection will be converted into a thrown exception. Failing a test when the return promise rejected will fail a test that was probably silently broken previously. Extra test cases have been added to reflect real world usage of tape with async functions which we preferably do not want to break.

Raynos · 2019-06-20T11:52:39Z

This is a much requested feature by my peers @juliangruber @maxharris9 @davidmarkclements

See related issues :

ljharb · 2019-06-20T14:31:25Z

in future versions of node, unhandled rejections may get forwarded to uncaught exceptions.

fwiw this won’t actually happen by default; unhandled rejections are a normal part of the language, and by default, the program needs to continue when one occurs. The node warning message is overly ambitious and thus misleading.

ljharb

Let’s add some tests for normal promises as well (we can shim them with es6-shim, and test them i all node versions)

ljharb · 2019-06-20T14:32:28Z

.eslintrc

@@ -7,4 +7,7 @@
            "named": "never",
        }],
    },
+    "parserOptions": {
+        "ecmaVersion": 2017


let’s enable this only for the tests that need it; otherwise breaking syntax could be added.

I'll see if I can opt in on a per file basis, maybe a nested .eslintrc file in one of the test folders is sufficient.

ljharb · 2019-06-20T14:33:15Z

lib/test.js

+        typeof Promise === 'function' &&
+        callbackReturn &&
+        typeof callbackReturn.then === 'function' &&
+        typeof callbackReturn.catch === 'function'


.catch isn’t required; any thenable should be supported.

Personally I only care about supporting async functions and not thenable returning functions.

However if you care strongly I can add support for thenable's

ljharb · 2019-06-20T14:33:38Z

lib/test.js

+        typeof callbackReturn.then === 'function' &&
+        typeof callbackReturn.catch === 'function'
+    ) {
+        callbackReturn.catch(function onError(err) {


since it might not be a real promise, we should wrap it in Promise.resolve first.

I don't understand why that matters in this concrete case. It might be a good practice in general but I think for this patch it's unnecessary.

ljharb · 2019-06-20T14:34:05Z

lib/test.js

+    ) {
+        callbackReturn.catch(function onError(err) {
+            nextTick(function rethrowError() {
+                throw err


Suggested change

throw err

throw err;

and throughout

ljharb · 2019-06-20T14:41:51Z

as-is, this is still a breaking change - anyone currently using an async function (or any promise-returning function) with an unhandled rejection will cause an error where none previously existed.

ljharb · 2019-06-20T14:43:16Z

lib/test.js

+        typeof callbackReturn.catch === 'function'
+    ) {
+        callbackReturn.catch(function onError(err) {
+            nextTick(function rethrowError() {


Im actually confused here - all this seems to be doing is ensuring that the returned promise can’t hook into node’s unhandledRejection event - it’s not actually failing the associated test and allowing tests to continue.

Raynos · 2019-06-20T14:59:32Z

Im actually confused here - all this seems to be doing is ensuring that the returned promise can’t hook into node’s unhandledRejection event - it’s not actually failing the associated test and allowing tests to continue.

All this does is make exceptions in async functions work. unhandled rejections from the async function passed to tape(fnName, fn) will go to uncaught exception and have the default process uncaught exception semantics.

fwiw this won’t actually happen by default; unhandled rejections are a normal part of the language, and by default, the program needs to continue when one occurs. The node warning message is overly ambitious and thus misleading.

I was at the openjs summit recently and spoke to some node core folks, it seems like we want stricter semantics around unhandled rejections in core, forwarding unhandled rejections to uncaught exceptions might be a change we see in future node versions.

Unhandled rejections are a bug and undefined behavior, they should be handled exactly like uncaught exceptions.

Raynos · 2019-06-20T15:00:53Z

as-is, this is still a breaking change - anyone currently using an async function (or any promise-returning function) with an unhandled rejection will cause an error where none previously existed.

If you have an async function that throws an exception its "silently" continueing. Your test suite is broken, it's just that the exitCode of the process does not reflect it.

This patch makes the exception you threw visible.

Technically it is a breaking change, it's a change in semantics. However it is a better user experience for exception handling in async functions and normal functions to be the same in tape

Raynos · 2019-06-20T15:05:01Z

anyone currently using an async function (or any promise-returning function) with an unhandled rejection will cause an error where none previously existed.

It should be noted that there's very few cases of this happening, if you have a thrown exception in an async function you are probably not calling t.end()

The behavior reported in #358 is more common for async functions with thrown exceptions, confusion about tests not ending and just hanging.

Tape will report "expected t.end() to be called but test not finished" if you have no handles open in your test process, but most realistic tests will actually leak sockets or timers or something that keeps the event loop open when you throw in an async function. This leads to the output of the process just hanging and tape will not report anything, and the process wont exit because you didnt teardown your http server or whatever.

ljharb · 2019-06-20T15:05:05Z

They’re not a bug, and they’re quite defined - if node exits on them by default, it’s violating the intent of the spec.

…est. Also, implicitly call `.end()` if not already called when a Promise is returned, because the promise itself marks the end of the test.

ljharb · 2019-06-24T06:45:58Z

I've rebased this, and added a commit that changes the behavior to:

fail the test when its callback returns a rejected thenable
call .end() for you when its callback returns a resolved thenable
fail when you've called .end() already before returning a resolved thenable (this part would be a breaking change for sure, even if the rest wouldn't be - so i'm open to making this silently work)

Thoughts?

lib/test.js

Raynos · 2019-06-24T10:34:58Z

fail when you've called .end() already before returning a resolved thenable (this part would be a breaking change for sure, even if the rest wouldn't be - so i'm open to making this silently work)

The existing tests I wrote had t.end() in them because that's a pattern people are already doing. The only test in which my patch is changing semantics is the async-error.js test.

I think failing on "double end" is too heavy of a breaking change for this patch. It requires fixing tests and code that "just works" before when you upgrade tape.

When we make a breaking change to report the rejected promise error at least you are fixing dubious tests that silently "failed" and are worth looking at and fixing. If you upgrade tape and it makes a breaking change I hope that the break will actually allow you identify tests that silently failed and improved your test suite.

You don't want to upgrade tape and just refactor tests that are "correct"

call .end() for you when its callback returns a resolved thenable

This is actually a breaking change that will break "correct" tests that have passed before, I'm going to push a test case onto this branch to demonstrate this, but here is an example

tape('my func', async function (t) {
  var res = await fetch('http://localhost:3000')
  var body = await res.json()
  t.equal(body, '"ok"')

  checkDb(function done(data) {
    t.equal(data, 'is good')
    t.end()
  })
})

I've seen tests in the wild where people are migrating their code to async/await. They start using for example node-fetch and using promises for HTTP but their database driver is still callbacks and they make callback related code in their tests.

This means they are using t.end() and if tape were to auto call t.end() when the async function completes this test would finish before the database read completed which leads to a t.equal() after t.end()

I strongly recommend this patch implements support for async functions and makes minimal breaking changes based on how people are using tape and async functions in the wild instead of implementing "proper promises" support.

fail the test when its callback returns a rejected thenable

My original patch has two test cases, sync-error.js and async-error.js ; I made the smallest change possible to handle the promise returned from an async function such that those two test cases behave the same ( complain on STDERR and exit with code 1 ).

Your change preserves the exit code 1 semantics but does mean that sync functions and async functions report their failures differently, both of them print the stack to stdout or stderr and both of them fail.

I'm ok with failing the test but I still prefer having identical behavior for sync and async errors. Note that adding a try / catch to make sync exceptions fail the test is probably a bad idea and out of scope for this PR.

Raynos · 2019-06-24T10:36:03Z

CI fails because the anonymous stack frame needs to be stripped, I have another test with an example on how to strip it.

I'm going to push a commit to add another test case and add t.end() back into the test cases.

juliangruber · 2019-06-24T10:38:06Z

This is actually a breaking change that will break "correct" tests that have passed before, I'm going to push a test case onto this branch to demonstrate this, but here is an example
tape('my func', async function (t) {
  var res = await fetch('http://localhost:3000')
  var body = await res.json()
  t.equal(body, '"ok"')

  checkDb(function done(data) {
    t.equal(data, 'is good')
    t.end()
  })
})

To me this should make the test fail as you're mixing control flow primitives. Once the async function completes, the test is done. Anything happening after that is unsafe territory. How would the test harness be able to tell it should wait for an explicit t.end() before starting to run the next test?

Raynos · 2019-06-24T10:39:20Z

How would the test harness be able to tell it should wait for an explicit t.end() before starting to run the next test?

For back compat reasons the test harness cannot implicitly call t.end() when the async function completes. You still have to manually call t.end() in an async function.

juliangruber · 2019-06-24T10:44:31Z

Ok. I'm not really interested in this patch unless it implicitly calls t.end() because then I'll need a tape wrapper anyway. tap and ava and mocha all do this, I hope tape can move into the future sooner rather than later...and we know we'll need a breaking change for this anyway, so why not do it right away.

Raynos · 2019-06-24T10:47:19Z

This patch is purely about fixing the exitCode of your test suite when using async functions so that you can trust CI and it doesn't "silently fail"

This patch is not about improved user experience

Accidentally using an async function with tape can lead to tests that fail but exit with exitCode 0.

I'm open to improving the user experience but that should be a seperate PR.

This one is for the bugfix of the exitcode, not for the feature of improved user experience with async functions.

ljharb · 2019-06-25T01:29:06Z

I don’t think we can fix one without providing the other; it’s not currently a bug because tape has no promise support, and a rejected promise is not an error per the intention of the JS spec.

Raynos · 2019-06-25T11:06:00Z

I don’t think we can fix one without providing the other; it’s not currently a bug because tape has no promise support, and a rejected promise is not an error per the intention of the JS spec.

The user experience for thrown exceptions in async functions is bad. The behavior is different from thrown exceptions in normal functions.

I would recommend you look at this from the perspective of exceptions in functions and not the behavior of promises themselves.

We can add a check to see if the fn is an AsyncFunction if you prefer.

Raynos · 2019-06-25T11:17:46Z

That being said I'm fine with supporting promises if that's what we need to get async functions to work

ljharb · 2019-06-25T14:41:46Z

There’s no way to determine reliably if something is an async function without parsing toString, and an async function is nothing more than a function that returns a Promise - async/await is promises, nothing more.

Raynos · 2019-06-25T15:00:16Z

Ok, I think this PR is ready for another review.

There were a few issues mentioned that are worth discussing

Can we implicitly call t.end() when the promise resolves ? This might be a breaking change, see test case that I've added.
Call t.fail(err) when the promise rejects or rethrow the error to uncaught exception ? I'm fine with either approach.

r0mflip · 2019-06-25T17:18:17Z

Can we implicitly call t.end() when the promise resolves ?

Explicit is 👌🏻

Call t.fail(err) when the promise rejects or rethrow the error to uncaught exception ? I'm fine with either approach.

Fail and call end.

test/async-await.js

ljharb · 2019-07-17T20:56:05Z

Let me double check to make sure all the non-major stuff that's ready is merged and released; and then we can just start bringing in semver-major stuff into master, including this one (i'll do a final rereview of this one as well first)

Raynos · 2019-08-08T14:53:53Z

@ljharb would you like some help rolling up a new major release ?

ljharb · 2019-08-08T16:27:56Z

Thanks, but i’m not quite ready yet :-) im making some changes in deep-equal first, so i can roll that into the major. Sometime next week i hope to have it all prepared.

Raynos · 2019-08-08T18:30:23Z

Sounds great, thanks for keeping me posted :)

ljharb · 2019-08-16T05:55:47Z

ftr: I'm waiting on a particular user to maybe hand over 4 package names; which I'd use to make robust brand checking methods to help add proper collection support to deep-equal, at which point i'd release a v2 of it, and then merge this along with updating that. Hopefully next week? I haven't forgotten!

Raynos · 2019-10-23T05:13:14Z

@ljharb friendly reminder :D

ljharb · 2019-10-29T23:01:47Z

Thanks, I haven't forgotten. I'm waiting on a few package names; once they're transferred I'll publish them, bump deep-equal's major, and then merge this and bump tape's major.

ljharb · 2019-12-18T01:29:17Z

I'll get all the pending breaking changes in, and publish a prerelease for v5 soon.

Raynos · 2019-12-18T10:53:28Z

Nice :) I will update a bunch of my apps & libraries to the pre-release once available.

- [Breaking] `error` should not emit `expected`/`actual` diags (#455) - [Breaking] support passing in an async function for the test callback (#472) - [Breaking] update `deep-equal` to v2 - [Deps] update `resolve` - [meta] change dep semver prefix from ~ to ^

Changes since v5.0.0-next.0: - [Breaking] fail any assertion after `.end()` is called (#489( - [Breaking] tests with no callback are failed TODO tests (#69) - [Breaking] equality functions: throw when < 2 arguments are provided - [Breaking] add "exports" to restrict public API - [Breaking] `throws`: bring into line with node’s `assert.throws` - [Breaking] use default `require.extensions` collection instead of the magic Array `['.js']` (#396) - [Fix] error stack file path can contain parens/spaces - [Refactor] make everything strict mode - [Refactor] Avoid setting message property on primitives; use strict mode to catch this (#490) - [Refactor] generalize error message from calling `.end` more than once - [Dev Deps] update `eslint` - [Tests] improve some failure output by adding messages - [Tests] handle stack trace variation in node <= 0.8 - [Tests] ensure bin/tape is linted - [Tests] Fail a test if its callback returns a promise that rejects (#441) - [eslint] fix remaining undeclared variables (#488) - [eslint] Fix leaking variable in tests - [eslint] fix object key spacing Changes since v4.12.1: - [Breaking] `error` should not emit `expected`/`actual` diags (#455) - [Breaking] support passing in an async function for the test callback (#472) - [Breaking] update `deep-equal` to v2 - [Deps] update `resolve` - [meta] change dep semver prefix from ~ to ^

Changes since v5.0.0-next.3: - [Fix] `.catch` is a syntax error in older browsers - [Refactor] remove unused code - [Deps] update `resolve` Changes since v4.13.0: - [Breaking] update `deep-equal` to v2 - [Breaking] fail any assertion after `.end()` is called (#489) - [Breaking] `error` should not emit `expected`/`actual` diags (#455) - [Breaking] support passing in an async function for the test callback (#472) - [Breaking] tests with no callback are failed TODO tests (#69) - [Breaking] equality functions: throw when < 2 arguments are provided - [Breaking] add "exports" to restrict public API - [Breaking] `throws`: bring into line with node’s `assert.throws` - [Breaking] use default `require.extensions` collection instead of the magic Array `['.js']` (#396) - [meta] change dep semver prefix from ~ to ^

Changes since v5.0.0-next.4: - [Breaking] only `looseEqual`/`deepEqual, and their inverses, are now non-strict. - [Breaking] make equality functions consistent: - [Breaking] `equal`: use `==`, not `===`, to match `assert.equal` - [Breaking] `strictEqual`: bring `-0`/`0`, and `NaN` into line with `assert` - [patch] Print name of test that didnt end (#498) - [Refactor] remove unused code - [Deps] update `resolve` Changes since v4.13.2: - [Breaking] only `looseEqual`/`deepEqual, and their inverses, are now non-strict. - [Breaking] make equality functions consistent: - [Breaking] `equal`: use `==`, not `===`, to match `assert.equal` - [Breaking] `strictEqual`: bring `-0`/`0`, and `NaN` into line with `assert` - [Breaking] update `deep-equal` to v2 - [Breaking] fail any assertion after `.end()` is called (#489) - [Breaking] `error` should not emit `expected`/`actual` diags (#455) - [Breaking] support passing in an async function for the test callback (#472) - [Breaking] tests with no callback are failed TODO tests (#69) - [Breaking] equality functions: throw when < 2 arguments are provided - [Breaking] add "exports" to restrict public API - [Breaking] `throws`: bring into line with node’s `assert.throws` - [Breaking] use default `require.extensions` collection instead of the magic Array `['.js']` (#396) - [meta] change dep semver prefix from ~ to ^

Raynos · 2020-04-24T09:34:46Z

I ran into the unhandled rejection issue with tape again.

I thought I fixed this almost a year ago 😞

Can we release v5 officially as latests ?

ljharb · 2020-04-24T20:11:10Z

tape@next has been available for awhile; I've run into a number of bugs, which have delayed the regular v5 from being released as I increased usage.

I'm now using v5.0.0-next.5 on 200+ of my projects, so I'm pretty confident that it's ready to go. I'll plan to release v5 later today or tomorrow.

Raynos · 2020-04-24T20:20:45Z

Awesome, thanks! I appreciate it.

ljharb · 2020-04-24T22:03:16Z

v5.0.0 is released.

Raynos · 2020-04-25T13:03:45Z

Nice I released @pre-bundled/tape v5.0.0 ( https://www.npmjs.com/package/@pre-bundled/tape/v/5.0.0 ).

🎉

3cp · 2020-04-28T03:53:42Z

Love the new feature! Should tape two more assertions to handle async throw? Like tape-promise does:

https://github.com/jprichardson/tape-promise#new-assertions

ljharb · 2020-04-28T03:56:04Z

@3cp a) that seems like it would conflict with "the test ends when the returned-to-tape promise is settled", b) let's discuss new features in new issues :-)

Raynos requested review from ljharb and a user June 20, 2019 11:54

ljharb requested changes Jun 20, 2019

View reviewed changes

ljharb reviewed Jun 20, 2019

View reviewed changes

[Breaking] if a test callback returns a rejected thenable, fail the t…

f248610

…est. Also, implicitly call `.end()` if not already called when a Promise is returned, because the promise itself marks the end of the test.

ljharb force-pushed the async-await branch from f013f18 to cffe4e5 Compare June 24, 2019 06:44

juliangruber reviewed Jun 24, 2019

View reviewed changes

lib/test.js Show resolved Hide resolved

Raynos changed the title ~~Support exceptions in async functions~~ [Bugfix]: Ensure the process exitCode is correct when an exception occurs in async functions Jun 24, 2019

r0mflip reviewed Jun 25, 2019

View reviewed changes

test/async-await.js Outdated Show resolved Hide resolved

ljharb force-pushed the async-await branch 2 times, most recently from bd77f63 to 197019c Compare December 16, 2019 22:44

ljharb approved these changes Dec 18, 2019

View reviewed changes

ljharb merged commit 5f88895 into master Dec 18, 2019

ljharb deleted the async-await branch December 18, 2019 01:29

ljharb mentioned this pull request Dec 20, 2019

Fail a test if its callback returns a promise that rejects #441

Merged

Raynos mentioned this pull request Apr 25, 2020

fix tape: test emitter in async function #503

Closed

3cp mentioned this pull request Apr 28, 2020

Suggest new assertions on async rejection #507

Open

donmccurdy mentioned this pull request Feb 8, 2021

CLI tests broken with dependency update. donmccurdy/glTF-Transform#180

Closed

chrisirhc mentioned this pull request Aug 17, 2021

Request for Comment: Attempt at removing react-palm for 1 task keplergl/kepler.gl#1577

Draft

4 tasks

[Breaking] support passing in an async function for the test callback #472

[Breaking] support passing in an async function for the test callback #472

Conversation

Raynos commented Jun 20, 2019 • edited Loading

Raynos commented Jun 20, 2019

ljharb commented Jun 20, 2019

ljharb 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

ljharb commented Jun 20, 2019

Choose a reason for hiding this comment

Raynos commented Jun 20, 2019

Raynos commented Jun 20, 2019

Raynos commented Jun 20, 2019

ljharb commented Jun 20, 2019

ljharb commented Jun 24, 2019

Raynos commented Jun 24, 2019

Raynos commented Jun 24, 2019

juliangruber commented Jun 24, 2019

Raynos commented Jun 24, 2019

juliangruber commented Jun 24, 2019

Raynos commented Jun 24, 2019

ljharb commented Jun 25, 2019

Raynos commented Jun 25, 2019

Raynos commented Jun 25, 2019

ljharb commented Jun 25, 2019

Raynos commented Jun 25, 2019

r0mflip commented Jun 25, 2019

ljharb commented Jul 17, 2019

Raynos commented Aug 8, 2019

ljharb commented Aug 8, 2019

Raynos commented Aug 8, 2019

ljharb commented Aug 16, 2019

Raynos commented Oct 23, 2019

ljharb commented Oct 29, 2019

ljharb commented Dec 18, 2019

Raynos commented Dec 18, 2019

Raynos commented Apr 24, 2020

ljharb commented Apr 24, 2020

Raynos commented Apr 24, 2020

ljharb commented Apr 24, 2020

Raynos commented Apr 25, 2020

3cp commented Apr 28, 2020

ljharb commented Apr 28, 2020

Raynos commented Jun 20, 2019 •

edited

Loading