chaijs / chai

BDD / TDD assertion framework for node.js and the browser that can be paired with any testing framework.
https://chaijs.github.io
MIT License
8.15k stars 698 forks source link

Add support for Promise #1570

Open BryanHuntNV opened 11 months ago

BryanHuntNV commented 11 months ago

It appears that chai-as-promised does not work with chai v5 and is no longer under development. Given the prevalence of programming with Promise, would it make sense to include the functionality of chai-as-promised into chai proper?

43081j commented 11 months ago

that is what i'm leaning towards

think it makes sense to have native support for things like toThrow on a promise, etc

keithamus commented 10 months ago

An innate problem with adding promises to core is that all existing assertions synchronously throw, and there is no very clear signal as to what would then turn that into an asynchronous throw. chai-as-promised adds the eventually chainable property, which we could add but the await keyword is something that developers are much more likely to be aware of.

One way we could go about this change is to make all assertions return a Promise, but that's a big breaking change (though arguably one that gives us future flexibility for all kinds of async stuff). Another solution could be to extensively document how one would unpack promises within the existing assertions (which typically looks like expect(await ...), but the far bigger problem today exists with how to properly handle rejections which involves some code gymnastics.

BryanHuntNV commented 10 months ago

FWIW, my typical use-case for chai-as-promised (using mocha) is:

return expect(testClass.testFunction(args)).to.eventually.be.rejectedWith(Error);
43081j commented 10 months ago

i recall in some other test libraries, some smarts were done to change the return type

e.g.


expect(someSyncThing).to.equal(10);

await expect(someAsyncThing).to.equal(10);

and basically have a conditional return:

function equal(expectation: unknown): T extends PromiseLike<unknown> ? Promise<void> : void;

but that may make my fancy types im doing in the typescript branch a bit more difficult 😬

since i'm not sure we can do a type guard return of a promise

perrin4869 commented 7 months ago

I would also vote in favor of adding the eventually, become, rejectedWith, etc APIs found in chai-as-promised, I've used those since forever ago, and it's the one thing preventing me from upgrading to v5

mscharley commented 7 months ago

As much as I'm all for getting these into chai itself, chai-as-promised works perfectly fine for me at least with chai v5.

mscharley commented 7 months ago

Hmm, now I look at the original issue, I can't remember why I ended up subscribed to this ticket. I just updated a project to chai v5 which was using chai-as-promised and it seems to be working fine.

// mocha.env.js
const { use } = await import("chai");
use((await import("chai-as-promised")).default);
file: ["mocha.env.js"]

And then just go about using chai-as-promised how you normally would. I just wrote up a few intentionally broken tests to make sure the matchers are working properly and everything seems fine for ESM projects at least.

import { expect } from "chai";

describe("promise tests", () => {
  it("should succeed", async () => {
    await expect(Promise.resolve(10)).to.eventually.eq(10);
  });

  it("should catch rejections", async () => {
    await expect(Promise.reject(new Error("Oh no!"))).to.eventually.be.rejectedWith("Oh no!");
  });

  it("should fail", async () => {
    await expect(Promise.resolve(10)).to.eventually.eq(20);
  });

  it("should fail to catch rejections", async () => {
    await expect(Promise.reject(new Error("Oh no!"))).to.eventually.be.rejectedWith("Oops!");
  });
});
  promise tests
    ✔ should succeed
    ✔ should catch rejections
    1) should fail
    2) should fail to catch rejections
perrin4869 commented 7 months ago

yeah, I expect the code is compatible, and if you force it, npm will allow you to run chai-as-promised with chai@5, but it's not allowed as per chai-as-promised with the peer dependencies it has set.

One way we could go about this change is to make all assertions return a Promise, but that's a big breaking change (though arguably one that gives us future flexibility for all kinds of async stuff).

I don't like this idea at all to be honest 😅 But having built in constructs to run assertions on promises seems like a good candidate to be built in to be honest, considering that at this point, Promises are the base of all async JS APIs.

mscharley commented 7 months ago

yeah, I expect the code is compatible, and if you force it, npm will allow you to run chai-as-promised with chai@5, but it's not allowed as per chai-as-promised with the peer dependencies it has set.

Yeah, I did notice that after I posted but didn't know what to make of it. npm definitely didn't complain at all for me. npm peer dependencies working as intended yet again, I guess. I'd definitely still prefer to see it in chai directly, but maybe a short-term solution is just republishing the original package with an updated peer dependency so that people aren't blocked while a longer term solution is worked out.

43081j commented 7 months ago

so it sounds like chai-as-promised just needs the peer dependency semver changing to include 5?

im not sure how active @domenic is anymore in that repo, so we might struggle to update it, but could be worth opening a PR

ultimately, i think this issue still stands as we should really natively support promises in chai IMO

domenic commented 7 months ago

I'd be happy to transfer the chai-as-promised repo to the chaijs GitHub organization and the same for the npm package.

keithamus commented 7 months ago

Thanks @domenic we’d be happy to take it! I’ve invited you to the GitHub org (I actually thought you were already a member) so you can initiate the transfer.

keithamus commented 6 months ago

Thanks @domenic for transferring the repo! If you have time could you please also go to https://www.npmjs.com/package/chai-as-promised/access and invite the user https://www.npmjs.com/~chaijs to be able to publish packages. This account is the account we use for publishing all chai packages. Thanks!

perrin4869 commented 6 months ago

Sorry for the (mostly) unrelated comment, but I noticed that @domenic is also the maintainer of sinon-chai, which I'm also using. Would that plugin also be welcome in this org? I could also help maintain it if there is not enough throughput

keithamus commented 6 months ago

Happy to house any and all chai plugins under the org, existing maintainers remain as such.

domenic commented 6 months ago

Transferred and access given for both chai-as-promised and sinon-chai.