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

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

Merged
merged 3 commits into from
Sep 6, 2024

Conversation

nimesh0505
Copy link
Contributor

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

Checklist

@nimesh0505 nimesh0505 marked this pull request as draft September 5, 2024 07:58
@nimesh0505 nimesh0505 marked this pull request as ready for review September 5, 2024 07:59
@nimesh0505 nimesh0505 marked this pull request as draft September 5, 2024 08:14
@simoneb
Copy link

simoneb commented Sep 5, 2024

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

@nimesh0505
Copy link
Contributor Author

nimesh0505 commented Sep 5, 2024

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
Copy link

simoneb commented Sep 5, 2024

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

@mcollina
Copy link
Member

mcollina commented Sep 5, 2024

Use c8 for now.

test.js Outdated
t.plan(1)
t.strictSame(typeof Hook, 'function')
test('pre-commit', async (t) => {
await t.test('is exported as a function', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you change to use the built-in t.plan and t.assert?

@nimesh0505 nimesh0505 marked this pull request as ready for review September 6, 2024 11:24
@simoneb simoneb merged commit a625b62 into fastify:main Sep 6, 2024
15 checks passed
@nimesh0505 nimesh0505 deleted the fix/migrate-to-node-test branch September 6, 2024 13:37
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 this pull request may close these issues.

4 participants