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 from tap to node test and c8 #248

Merged
merged 4 commits into from
Sep 15, 2024

Conversation

nimesh0505
Copy link
Contributor

Checklist

This PR replaces tap with node:test for testing and adds c8 for coverage.

@nimesh0505 nimesh0505 changed the title fix: migrate from jest to node test and c8 fix: migrate from test to node test and c8 Sep 11, 2024
@nimesh0505 nimesh0505 changed the title fix: migrate from test to node test and c8 fix: migrate from tap to node test and c8 Sep 11, 2024
package.json Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
Copy link
Member

@gurgunday gurgunday left a comment

Choose a reason for hiding this comment

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

If the taprc file did not contain disable-coverage: true, please enable c8 coverage; for how, see my suggested change

const fp = require('../plugin')

test('webpack removes require.main.filename', (t) => {
test('webpack removes require.main.filename', async (t) => {
Copy link
Member

Choose a reason for hiding this comment

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

Why have all of the test function been refactored to async functions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jsumners Thank you for your question. You are right to notice that all test functions have been converted to async functions. While it's not strictly necessary for the migration from tap to node:test, I chose to do so due to these reasons:

  1. Consistency - It provides a uniform pattern across all tests.
  2. Future-proofing - It allows for easier integration of async operations in the future if needed.
  3. Minimal impact: The change doesn't negatively affect the tests' functionality or performance.

However, if the team prefers to keep the tests as close to the original implementation as possible, we can certainly revert this change and only use async functions where necessary.

Copy link
Member

Choose a reason for hiding this comment

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

Unless there is a need for the functions to be async, please do not make them so.

gurgunday
gurgunday previously approved these changes Sep 13, 2024
Copy link
Member

@gurgunday gurgunday left a comment

Choose a reason for hiding this comment

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

lgtm

no strong opinions about #248 (comment)

@Uzlopak
Copy link
Contributor

Uzlopak commented Sep 14, 2024

I agree with @jsumners . It doesnt make any sesnse and has no benefit of making functions async if there is nothing to be awaited etc.. So yes, please remove unnecesasry async.

@simoneb
Copy link
Contributor

simoneb commented Sep 15, 2024

comments addressed

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@simoneb simoneb merged commit 9481e92 into fastify:master Sep 15, 2024
11 checks passed
@simoneb simoneb deleted the migrate-tap-to-node-test branch September 15, 2024 10:54
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.

6 participants