Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open to migrating to node:assert? #2165

Closed
cjihrig opened this issue Jan 16, 2025 · 2 comments · Fixed by #2167
Closed

Open to migrating to node:assert? #2165

cjihrig opened this issue Jan 16, 2025 · 2 comments · Fixed by #2167

Comments

@cjihrig
Copy link
Contributor

cjihrig commented Jan 16, 2025

Currently, this repo uses chai and chai-as-promised for assertions in tests. This works fine.

However, I'd like to propose moving to node:assert if others are OK with it. The reasons:

  • Fewer dependencies. node:assert is a Node core module. We could drop @types/chai, @types/chai-as-promised, chai, and chai-as-promised.
  • Some of chai's assertions are footguns. For example, expect(opts.headers.Authorization).to.be.undefined;. You can remove the undefined part of this assertion and nothing will change. The hapi ecosystem actually forked chai, primarily over this issue.
  • The linter complains about these same assertions.
  • We currently configure chai-as-promised inconsistently. We don't notice it because mocha runs all of the tests in the same process, but there are places where we use chai-as-promised without configuring it in the current file. Obviously this alone is not a reason to migrate off of it, but it's worth noting.

The downsides:

  • node:assert uses a different syntax. Instead of expect(emptyConfig.getCurrentCluster()).to.equal(null);, it would be written as strictEqual(emptyConfig.getCurrentCluster(), null);. I know some people have a strong preference for expect() style assertions.
  • Someone would have to do the work of migrating. I volunteer to do it if no one is opposed.
@mstruebing
Copy link
Member

I would be open for it!
Having fewer dependencies is always desired in my opinion.
Also, most of your arguments speak for a migration and the downsides you are calling out are not really downsides in my opinion.

@brendandburns
Copy link
Contributor

I'm fine with it if someone else does the work :) (and thank you!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants