fastify / pre-commit

Fork of https://github.com/observing/pre-commit
MIT License
9 stars 5 forks source link

fix: migrate test suite from tap to node:test #76

Closed nimesh0505 closed 1 month ago

nimesh0505 commented 1 month ago

This PR replaces tap with node:test and introduces c8 for code coverage.

Checklist

simoneb commented 1 month ago

Do we need c8 or can we use the built-in coverage? https://nodejs.org/api/test.html#collecting-code-coverage

nimesh0505 commented 1 month ago

Do we need c8 or can we use the built-in coverage? https://nodejs.org/api/test.html#collecting-code-coverage

@simoneb Thanks for suggesting the use of Node.js's built-in coverage. After consideration, I think we should stick with c8 for now. Here's why:

  1. The --experimental-test-coverage flag is still experimental, which could lead to instability or changes in future Node.js versions.
  2. As a widely-used package, @fastify/pre-commit should prioritize stability for its users.
  3. c8 is a mature, stable solution that we know works well across different Node.js versions.

However, I agree it's a good idea to eventually move to the built-in coverage. I suggest we:

  1. Keep using c8 for now.
  2. May be create an issue to track the eventual migration to Node.js's built-in coverage.
  3. Revisit this once the coverage feature in Node.js becomes stable (likely when they remove the experimental prefix).

This way, we maintain stability for our users while staying prepared to adopt the built-in solution when it's ready. What do you think?

simoneb commented 1 month ago

Sounds good, I brought it up in case other maintainers have an opinion on this

mcollina commented 1 month ago

Use c8 for now.