unexpectedjs/unexpected

`instanceof` expect.fail() is not UnexpectedError

onetom opened this issue · 7 comments

Unlike the assertions thrown by the built-in Node.js assert, the error thrown by unexpected can not be detected using the instanceof operator.

The reason I'm looking into this, because I was trying to understand how node-tap distinguishes assertion errors from other errors.

I was expecting this script to yield true for both cases:

const expect = require('unexpected')
const UnexpectedError = require('unexpected/lib/UnexpectedError');
const assert = require('assert')
const L = console.log.bind(console)

try {
    assert.fail('asd')
} catch (e) {
    L(e instanceof assert.AssertionError)
}

try {
    expect.fail('qwe')
} catch (e) {
    L(e instanceof UnexpectedError)
}

but it resulted in:

true
false

Let me know if my expectation is unreasonable and recommend a better way of telling assertion errors apart from any other error.

Before I embarked on exploring unexpected I was under the false impression that JS runtimes provide some default, global AssertionError, which test frameworks can rely on for separating assertion failures from test code runtime errors. I wonder why did I think this... Maybe because other languages work like this? Is there any initiative for JavaScript to standardize an assertion mechanism?

The reason why this doesn't work is that we recently added a transpilation setup (since 10.34.2), so in newer versions you'd have to use:

const UnexpectedError = require('unexpected/build/lib/UnexpectedError');

But we don't consider the internal file names a stable part of the API, so that could change in the future.

Here's how we identify UnexpectedError instances in the library:

unexpected/lib/types.js

Lines 860 to 867 in 774539a

identify(value) {
return (
value &&
typeof value === 'object' &&
value._isUnexpected &&
this.baseType.identify(value)
);
},

The this.baseType.identify thing boils down to a generic Error instance detection, implemented here:

unexpected/lib/utils.js

Lines 120 to 123 in 774539a

typeof err === 'object' &&
(Object.prototype.toString.call(err) === '[object Error]' ||
err instanceof Error)
);

Is there any initiative for JavaScript to standardize an assertion mechanism?

Not that I know of. I'm also only aware of require('assert').AssertionError in node.js. We could try to use that when in node.js, but it'd be a bit weird as it isn't available in the browser where we'd then have another inheritance chain.

Chai has its own assertion-error module, which does not attempt to inherit from node's AssertionError:

const AssertionError = require('assert').AssertionError;
const expect = require('chai').expect;
try {
  expect(false).to.equal(true);
} catch (err) {
  console.log(err instanceof AssertionError); // false
}

Jest's expect implementation throws instances of its own JestAssertionError that aren't subclasses of node's AssertionError either.

expect.js produces regular Error instances.

So unfortunately it seems like there's no consensus around any of this stuff :(

@papandreou thanks for the quick feedback.
i need to give more thoughts to the topic, but some level of "fix" to this issue could simply be some documentation enhancement.

in the meantime, here are my initial results of integrating unexpected into node-tap:
https://github.com/enumatech/sprites/blob/f1fe6af1e84404f5ad6c70212bb43e0015278867/examples/paywall/test/custom-tap.js#L25-L63

i pondered a lot today about how should it work, because if i treat expectations async all the time, that leads to more verbose test code and chatty test output (because async node-tap tests are implemented as an implicit subtest wrapper; resulting in duplicate output and an extra nesting layer)

it was surprising to realize that a except can both throw an exception and reject the returned promise...

my latest idea is to detect synchronous assertions based on wether the promise returned by expect is already fulfilled or not. if it's pending, i would do the subtest wrapping, but if it's resolved, i wouldn't.
not sure how reliable is that approach... is there a way to query this more directly somehow?

it was surprising to realize that a except can both throw an exception and reject the returned promise...

Yeah, that was the compromise we ended up making when we introduced support for async assertions. At that time, async/await wasn't a thing yet, so taking the leap into "always async" didn't seem like an option. And it would probably still be too radical to break pretty much the only contract that all JS assertion libraries support: That an error results in an exception being thrown. We have experimented a bit with an alternative idea where the user wouldn't need to care, but I think it'll require buy-in from test runners to get off the ground.

my latest idea is to detect synchronous assertions based on wether the promise returned by expect is already fulfilled or not. if it's pending, i would do the subtest wrapping, but if it's resolved, i wouldn't.
not sure how reliable is that approach... is there a way to query this more directly somehow?

Unexpected's promises are based on bluebird 2.9.34, so they include support for the PromiseInspection interface. So you can:

(async () => {
  const promise = expect(setImmediate, 'to call the callback');
  console.log(promise.isPending()); // true
  console.log(promise.isFulfilled()); // false
  console.log(promise.isRejected()); // false

  await new Promise(resolve => setTimeout(resolve, 10));

  console.log(promise.isPending()); // false
  console.log(promise.isFulfilled()); // true
  console.log(promise.isRejected()); // false
})();

And you can extract the error from rejected promises using promise.reason().

Thanks, I think I know how to integrate it with node-tap seamlessly, then!

I've also tested native the Promise behaviour (in Node.js v11.5.0) to see if there is any case where I would misinterpret expect's behaviour:

const t = require('tap')
const expect = require('unexpected')

async function run() {
    await t.test('expect', async t => {
        const err = (msg = '<boom>') => Error(msg)
        const exn = () => { throw err() }

        await t.test('sync', async t => {
            t.ok(expect(123, 'to be', 123).isFulfilled())
            t.throws(() => expect(123, 'to be', 999))
            t.ok(expect(exn, 'to throw').isFulfilled())
            t.throws(() => expect(exn, 'not to throw'))
        })

        await t.test('async', async t => {
            const resolved = (x = 123) => Promise.resolve(x)
            const rejected = (x = err()) => Promise.reject(x)
            const nop = () => 0

            t.ok(expect(resolved, 'to be fulfilled').isPending())
            await t.resolves(expect(resolved, 'to be fulfilled'))

            t.ok(expect(resolved, 'to be rejected').catch(nop).isPending())
            await t.rejects(expect(resolved, 'to be rejected'))

            t.ok(expect(rejected, 'to be rejected').isPending())
            await t.resolves(expect(rejected, 'to be rejected'))

            t.ok(expect(rejected, 'to be fulfilled').catch(nop).isPending())
            await t.rejects(expect(rejected, 'to be fulfilled'))
        })
    })
}

run().catch(t.threw)

I can't resist promoting to try it with the unobtrusive Nix package manager:

curl https://nixos.org/nix/install | sh
nix-shell -p nodejs-11_x
mkdir /tmp/xxx; cd /tmp/xxx
npm init -y; time npm i -D tap unexpected
# copy the code to your clipboard
pbpaste >> test.js
node test.js

Just saying, because I think it's quite elegant (and very reproducible) :)

Seems like this got resolved, so I'll close. Please get in touch if you have any further questions :)