moll / js-must

An assertion library for JavaScript and Node.js with a friendly BDD syntax (awesome.must.be.true()). It ships with many expressive matchers and is test runner and framework agnostic. Follows RFC 2119 with its use of MUST. Good stuff and well tested.
Other
336 stars 35 forks source link

How to use `then` to test for rejection? #28

Closed julien-f closed 8 years ago

julien-f commented 9 years ago

I tried the following but it does not seem to work:

Promise.reject(new Error('foo')).must.then.throw('foo')
moll commented 9 years ago

Indeed, haven't added rejections yet. The throw matcher works only on functions, but a promise rejects with an error. That's why that fails. Promise support is transparent to matchers.

I was thinking of...

Promise.reject(new Error("Problem")).must.catch.and.be.an.error("Problem")

The error matcher would do what throw does, but directly on errors. I'm not sure what to call the rejection equivalent of then. Could also call it reject?

Promise.reject(new Error("Problem")).must.reject.and.be.an.error("Problem")

That would ask for aliasing then to resolve too, then:

Promise.resolved(42).must.resolve.and.equal(42)
moll commented 9 years ago

I'm slightly worried someone might think reject and resolve in the above example are matchers whereas they're just properties returning a modified version of Must to assert on. So one should never do Promise.resolved(42).must.resolve. Unless we make reject and resolve to also work as functions, akin to Must.prototype.a works.

julien-f commented 9 years ago

Agreed and I am not very fond of the and conjunction which imply dependent assertions.

Also, with promises (just like exceptions) it is possible to resolve to an error and reject with something else than an errors but maybe those are edge cases…

Maybe adding a resolve and a reject matchers which would behave like this:

describe('.reject()', function () {
  it('asserts for promise rejection', async function () {
    await Promise.reject().must.reject()
  })

  if('supports a callback to test the rejected value', async function () {
    Promise.reject(new Error('foo')).must.reject(function (value) {
       except(value).to.be.an.error('foo')
    })
  })
})

// For symmetry but redundant with then/eventually.
describe('.resolve()', function () {
  it('asserts for promise fulfillment', async function () {
    // Not really useful, we can simply await the promise but can
    // provide a better error message.
    await Promise.resolve().must.resolve()
  })

  if('supports a callback to test the resolved value', async function () {
    Promise.resolve('foo').must.resolve(function (value) {
      expect(value).to.equal('foo')
    })
  })
})
moll commented 9 years ago

We could also do both:

Promise.resolve(42).must.resolve()
Promise.reject(69).must.reject()
Promise.resolve(42).must.resolve.to.equal(42)
Promise.reject(69).must.reject.to.equal(69)

I'm not sure how useful resolve with a function is tho, as that's equivalent to:

Promise.resolve(42).then(function(value) { value.must.equal(42) })
;(await Promise.resolve(42)).must.equal(42)

Also, with promises (just like exceptions) it is possible to resolve to an error and reject with something else than an errors but maybe those are edge cases…

That's true. The way asserting on rejections would be implemented is just a wrapper that waits until a promise is rejected and then calls a matcher on it. That would work with whatever type the rejection reason happens to be.

julien-f commented 9 years ago

I'm not sure how useful resolve with a function is tho, as that's equivalent to:

Promise.resolve(42).then(function(value) { value.must.equal(42) })
;(await Promise.resolve(42)).must.equal(42)

Indeed except it might give a better error message in case of rejection, but it is a minor feature which might not be enough to justify it.

Promise.resolve(42).must.resolve()
Promise.reject(69).must.reject()
Promise.resolve(42).must.resolve.to.equal(42)
Promise.reject(69).must.reject.to.equal(69)

I like your proposal, and with the error matcher all cases are covered.

But do we agree that all cases with resolve() are unnecessary (redundant with then, etc.) and are only here for the sake of symmetry? Should it be implemented? Because it might confuse some users as they will have to choose between both styles. Maybe one style should be explicitly preferred in the documentation…

moll commented 9 years ago

Indeed, resolve would be equivalent to then or eventually. In addition to wrapping matchers for promise support, resolve and reject would have to be matchers themselves too for the above example (Promise.reject(69).must.reject()).

Given that this Promise stuff isn't still released, we can remove then and just leave resolve and reject. Or leave both. I've seen eventually used by Chai as Promised and Should.js, hence perhaps leave that in? What do you think?

moll commented 9 years ago

Having then and eventually matchers like resolve and reject would sound weird:

Promise.resolve(42).must.then()
Promise.resolve(42).must.eventually()
julien-f commented 9 years ago

Yes, then() and eventually() do not make sense as matchers but they add a lot of high level fluency used as conjunction (Promise.resolve('foo').must.eventually.equal('foo')).

Any ideas @JohnnyEstilles?

moll commented 9 years ago

I threw Must.prototype.reject with Must.prototype.resolve in there for everyone to give a go. They're not yet matchers, so Promise.resolve(42).must.resolve() won't work.

estilles commented 9 years ago

I haven't had a chance to test this. I will give it a whirl today. I'll give my two cents after I do.

moll commented 8 years ago

Thanks again everyone for their thoughts and help! I'll close this issue for now as we reached one milestone at least — basic generic promise support. It's also up with a stable NPM tag as v0.13.0.

We can continue chatting though. I presume everything has worked okay for @JohnnyEstilles. 8–)