tape-catch

[tcurdt]

It's a bit unfortunate that one has to resort to use tape-catch to have all test throwing exception to fail (why isn't that the default?). Given that I am using blue-tape I can only seem to use either or.

[Raynos]

Its a bad default as it makes debugging harder.

a node script should exit when an exception is thrown.

tape is a node program.

[yoshuawuyts]

100% agreed on what @Raynos says. If you want to catch expected exceptions use t.throws:

const assert = require('assert')
const test = require('tape')

test('an exception', function (t) {
  t.plan(1)
  t.throws(compare.bind(null, 1, 2))
})

function compare (num, num) {
  assert.equal(num, num)
}

[tcurdt]

Hm. I had tests throwing exceptions and it was going unnoticed. I just noticed because the amount tests were less. Now maybe that's also because of blue-tape or how I am using it.
Exiting on exceptions would be just fine - just not quietly ignoring them.

So maybe this isn't really tape issue after all then. Seems like I need to dig deeper.

[Raynos]

This might be the unhandled rejection problem with promises

[robhaswell]

+1 what is the use-case for reporting a test which raised an exception as passing?

[ljharb]

@robhaswell testing what kind of exceptions your API throws and where is absolutely essential.

[robhaswell]

@ljharb Absolutely I agree, yet the current behaviour doesn't support that at all. Exceptions raised are silently ignored, how does it help test what is thrown? For that you can use t.throws(fn).

[ljharb]

Perhaps inside a promise - but that's how promises work. Otherwise, it will terminate the process.

[chrisnicola]

This is definitely related to the fact that Node currently doesn't handle promise rejections. With many tests now being written using async/await this creates basically useless test fail messages.

While this is caused by Node.js currently not handling Promise rejection (in 8 there is a warning that eventually this will just fail but it currently doesn't do that).

As a way to behave inline with expectations for tests, I would suggest that tape should check to see if the test function is async or a Promise an then call .catch() with a function that simply rethrows the error.

For those dealing with this problem right now, a simple global solution, is to require a test helper that includes the following line:

process.on('unhandledRejection', error => {
  // Won't execute
  console.log('unhandledRejection', error.test);
});

This will override Node's lack of proper error handling for Promise's. Quite frankly as this will eventually be the real behaviour of Node, I'd seriously consider just doing this all the time.

[ljharb]

node's not supposed to "handle" promise rejections; the developer is. The language spec requires you to be able to create unhandled rejections; there's nothing wrong with having unhandled rejections.

Thus, it definitely won't become the real behavior of node by default, as that will break real code and also violate the spec.

[chrisnicola]

So I'm definitely a bit inexperienced with Node, but that does not appear to be correct as far as the current deprecation warning indicates.

http://thecodebarbarian.com/unhandled-promise-rejections-in-node.js.html

(node:7741) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): Error: woops
(node:7741) DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

That seems to indicate that at some future Node version it will eventually fail if a promise rejection is unhandled.

Either way, this behaviour seems undesirable when writing unit tests to not throw unhandled promise rejections within an async marked tests so they can report an error and a stack trace.

As a result, everywhere we want to do the following:

test('Something async', async (t) => {
  const result = await doTheThing();
  t.assert(result);
})

If we want to get useful error reports for unexpected exceptions we have to write:

test('Something async', async (t) => {
  try {
    const result = await doTheThing();
  } catch (e) { throw e; }
  t.assert(result);
})

If we want to get a stack trace for an unexpected error. As I've discovered event a simple syntax error in the above example will result in a useless warning from Node.

I'll add that I disagree with the author's opinion that Node shouldn't change this and that unhandled promise rejections should, in fact, be silently handled.

As far as I know other languages like C# that implement async/await do fail normally, as did Bluebird as the author acknowledges. If one doesn't explicitly put a try/catch around await the program crashes normally. Which is what I believe the programmer should expect to happen.

Also my apologies if I've hijacked an entirely unrelated issue. On initial glance it looked as if it might be related.

[ljharb]

Yes, the deprecation warning was optimistic by a few vocal folks who dislike promises; actual discussions in node core have thankfully roughly agreed that there's no way it will happen that way.

Node definitely shouldn't change this - the JS language spec strictly requires that unhandled rejections be allowed to exist, without impacting the program.

---

The relation to this thread is simply that if tape chose to, when Promise was available, wrap every callback return value in Promise.resolve, and then used .then/.catch to determine if there were unhandled rejections, that using an async function as your test callback would work the way you expect.

node has had promises since 0.12; node < 4 is EOL; node 8 has async functions; node 8 will be LTS in October. The ecosystem has moved from "promises shouldn't be required", to "promises are expected to be polyfilled everywhere". As such, it's - recently - a much stronger argument imo for tape to support Promises in this fashion. However, adding such complexity to a simple library is not something that should be done lightly or quickly.

[Raynos]

Checking whether a return value is a promise by some mechanism and then handling it seems reasonable for async functions.

Given that async functions use the built in promise and not a third party library you can do a prototype check ( e.g. Promise.prototype.isPrototypeOf(returnVal) ). Adding a catch expression that will throw or fail the test block seems reasonable.

It's difficult to tell whether a test block should be failed or whether the process should exit. I would favor the latter since tape has a test interface that is used to write assertions.

If you dont like writing try { await a } catch (err) { assert.ifError(err) } you can use something like asde or any one of the other 100s of async/await helper functions.

[ljharb]

@Raynos that won't work cross-realm; the simplest robust way to do it is to unconditionally Promise.resolve; alternatively, we'd have to build a custom isPromise function that did a try/catch around a .call of a specific Promise prototype method - which seems much worse to me than an unconditional Promise.resolve.

The process should never exit when there's an unhandled rejection in a promise, per the JS language spec.

[Raynos]

The process should never exit when there's an unhandled rejection in a promise, per the JS language spec.

Yes the process should never exit.

But the testing framework can have a different approach, its not an unhandled rejection. The testing framework handles all rejections and exits the test program saying one of the test blocks failed.

it could continue running the other test blocks but test blocks in tape tend to be stateful and bailing on the first error in this case might make it easier to debug then continueing the program.

[ljharb]

That's a fair point; but "might make it easier" is where we disagree. It will make tests much cleaner if a rejection is seen as a failure rather than terminating early (just like how one test failure doesn't cause tape to exit, even when it would make other tests fail too)

[Raynos]

that makes sense. An async function is not run to completion so it makes sense that an await failing or a syntax error is reported as a failure instead of terminating early.

[chrisnicola]

@ljharb I agree with the test framework seeing it as a failure.

Though that does kind of make me wonder: If it makes sense for a test to fail and allow testing to continue in an async exception situation, why not also synchronous one?

Would it not desirable to express the result of an unhandled exception the same way regardless of the async/sync context?

[ljharb]

@chrisnicola that would be a reasonable choice for tape to make, but sync errors and promise rejections simply aren't the same thing, despite both the parallels and the insistence of some luminaries in the community. It's totally reasonable for a sync exception to terminate the process; it's unreasonable for a promise rejection to do so.

[chrisnicola]

I'll digress a bit just to argue that it is usually up to the author of an application to determine whether unexpected errors do or do not terminate their process. Most applications will explicitly handle and then log or report unexpected errors. Web server processes return a 500 error as well as logging them and do so without crashing. Handling unexpected errors is a pretty standard part of any application.

What I don't understand is why the JS standard would deem it desirable to silently swallow unexpected errors unless the author explicitly chose to define that behaviour. However I also get that's how it works right now and it isn't going to change anytime soon.

Coming back to the discussion at hand, for testing specifically, I see value in handling both unhandled rejections and exceptions the same way. That is record them as a test failure, print the stack indicating where the error happened and then continue running the rest of the tests.

[gabrielcsapo]

To be honest, the reason why I use tape is that it doesn't make any assumptions in the way of handling certain use cases differently then others. @chrisnicola

I think the point that @ljharb pointed out is that promises are built in a way that they don't create catastrophic crashes that sync operations can create.

If is outlined here and in many other places that state the problem is an issue with the language implementation, not tape.

[chrisnicola]

@gabrielcsapo I think we're getting a bit off topic here. The issue at hand is what is useful for the test function to do when unexpected errors happen. I'm not really here to discus language decisions of Javascript. I don't think you have to make any assumptions to decide to report the details of the error to the user here.

[refactorized]

It seems blue-tape currently supports tape-catch style execution out of the box now, solving the originally stated issue, at least.

[badeball]

a node script should exit when an exception is thrown.

tape is a node program.

Personally I don't get this argument. What isn't a program and therefore should everything behave the same way?

More practically, not exiting upon exception allows me to gather feedback about the rest of the program even though I made a mistake somewhere.

I test (amongst other things) to help me catch mistakes I make. I also expect myself to make them and seek to optimize with that in mind.

As for making Tape behave like EG. Mocha - I agree with @gabrielcsapo in that not making assumptions is a strength (or at least provide multiple levels of abstractions and the ability to opt-out). If one does want it to behave like Mocha, that is super-easy to achieve by one self without making changes to the library. We've done as shown below for my current project and it seems to work well so far.

import tape from "tape";

const testConstructorDecorator = (testConstructor) => (description, testFn) => {
  testConstructor(description, (t) => {
    Promise.resolve(testFn(t)).then(t.end, t.end);
  });
}

const test = Object.assign(testConstructorDecorator(tape), {
  only: testConstructorDecorator(tape.only),
  skip: testConstructorDecorator(tape.skip)
});

export { test };

It does a tad bit more than proposed in #441, like invoking t.end() on behalf on the user even in the case of a successful test.

[camsjams]

If you don't want to import tape-catch into your project, you can just use or modify this snippet I made to prevent uncaught exceptions from going unnoticed:

process.on('uncaughtException', (err) => {
  console.log('\x1b[31m%s\x1b[0m', '✘ Fatality! Uncaught Exception within unit tests, error thrown:');
  console.log(err);
  console.log('not ok 1');
  console.log('\x1b[31m%s\x1b[0m', 'Force-Exiting process ...');
  process.exit(1);
});

Note: You should use the above to prevent a build from passing on a CI tool. Coupled with code coverage, you can rely on the fact that some tests will not fail silently. Likewise, you will be able to find the source of the failure soon (like a circular reference that someone added in the latest build).

[ljharb]

In v5, rejected promises returned from tests will fail the test. I think this can be closed.