chore: Migrate to node:test by Rantoledo · Pull Request #5714 · fastify/fastify

Rantoledo · 2024-09-25T13:24:45Z

These change are related to the issue #5555

Recently I have been working on the transition from tap to node:test.
At first, I thought of combining all the tests and creating one PR, but I realized it was too much. First of all, because these are a lot of files, second, it is better if you take a look and say what you think about some of the changes before I continue to other files. If there is any feedback, it may have to be implemented in the following files.

So, I created this PR, with a sample of the files I changed, to get feedback, hoping that from here I'll get confirmation that I'm in the right direction, and from there we'll move on to the rest of the files.

It is important for me to emphasize that my approach in the implementation was to keep the integration as simple as possible, that is - not to change the logic of the test itself but only what is necessary so that the result of the test, and its behavior, will remain the same as it was.

This is also the approach that everyone who will work on this must adopt. I don't think we should be tempted to change tests or add stuff.

But the gradual change I propose can only be applied if it's possible to run all the tests together, without migrating completely to node:test. running npm run unit on my side worked and it seems like tap can handle migrated files.
I admit that I don't know tap in depth so I won't be able to know for sure if it's okay.

so let me know what you think of the changes. If it's good enough, I can continue to other files.
Then, we have to decide whether to merge and continue to more files in a separate PR.
If the changes are acceptable but you still want everything to be done in one PR.
I will continue to make changes to the following files in a similar way to what I did here, but it will take time to change all the files.
Personally, I think changing all files at once is way too much.

Checklist

run npm run test and npm run benchmark
tests and/or benchmarks are included
commit message and code follows the Developer's Certification of Origin
and the Code of conduct

jsumners · 2024-09-25T14:58:41Z

Recently I have been working on the transition from tap to node:test.

First, there isn't "a transition" as an overall ecosystem. If a project wants to do that, it is certainly within their purview. But tap is definitely not obsolete. That said, the Fastify project is working on migrating to node:test for various reasons.

Regarding the plan for doing so in this repository, please review #5683 (comment). We are not ready for converting individual test suites at this time. There is some prep-work that needs to be done first.

…equal

Rantoledo · 2024-09-25T15:21:03Z

First, there isn't "a transition" as an overall ecosystem. If a project wants to do that, it is certainly within their purview. But tap is definitely not obsolete.

I have no idea what you mean by that.

But tap is definitely not obsolete.

i don't remember saying it is...

Regarding the plan for doing so in this repository, please review #5683 (comment). We are not ready for converting individual test suites at this time. There is some prep-work that needs to be done first.

So should I close this PR? as I see it its probably not relevant

jsumners · 2024-09-26T10:49:40Z

First, there isn't "a transition" as an overall ecosystem. If a project wants to do that, it is certainly within their purview. But tap is definitely not obsolete.

I have no idea what you mean by that.

I read the statement as referring to some overall conversion across the ecosystem, not as specific to the Fastify project. There isn't a reference to an issue (e.g. #5555), nor any mention of "Fastify" in it. So I want to be clear that we are not advocating for anyone to migrate from tap to node:test other than ourselves.

So should I close this PR? as I see it its probably not relevant

No. We just need a different PR to land before this on in this repo, in my opinion.

Rantoledo · 2024-09-26T13:37:11Z

I read the statement as referring to some overall conversion across the ecosystem, not as specific to the Fastify project. There isn't a reference to an issue (e.g. #5555), nor any mention of "Fastify" in it. So I want to be clear that we are not advocating for anyone to migrate from tap to node:test other than ourselves.

Forgive me, I thought it's obvious that I'm referring to Fastify.
I didn't have any intention of saying something about tap nor encouraging people to migrate to node:test.
For the sake of clarity, I will add #5555 in my comment.

Rantoledo · 2024-09-27T13:01:05Z

recent changes:

use done() instead of wrapping fastify.inject with callback.
in case multiple inject in one test, use await.

Rantoledo · 2024-09-29T14:39:30Z

I think its best that I convert this PR to a draft and keep working on it, a couple of files at a time...

jsumners · 2024-10-02T13:33:18Z

a couple of files at a time...

Does that mean you intend to convert all files in this one PR?

Rantoledo · 2024-10-03T12:14:18Z

a couple of files at a time...

Does that mean you intend to convert all files in this one PR?

I think having all the files in one PR is too much as I said.
It's up to you guys to decide.
If you want, I can open this PR again and merge it. But we need to make sure running npm run test etc does work with the files that were changed (as I understand it, it does work, but I would like someone else who knows tap better take a look).

If you want one PR that's also fine, it's just going to take me some time to change all the files...

Let me know what you think.

jsumners · 2024-10-04T10:38:05Z

I'm just trying to understand your plan. I would not be in favor of converting everything in one PR. It would take too long and be too difficult to keep synchronized with ongoing changes from other PRs.

Rantoledo · 2024-10-04T12:11:03Z

I'm just trying to understand your plan. I would not be in favor of converting everything in one PR. It would take too long and be too difficult to keep synchronized with ongoing changes from other PRs.

My plan is to try and add more files until you guys decide whether or not you want it to be in one PR or more.
I understand that you're on board with this. But I saw another member that said it's best to have one PR.
if you want i can open this PR for a review, so we can merge it.

jsumners · 2024-10-05T11:32:44Z

Once #5720 lands, we will not need to convert everything in a single PR. The single PR suggestion is one that attempts to avoid issues with running the tests, in my opinion.

I want to see multiple PRs because:

Fewer changes are easier to review.
Fewer changes are less likely to encounter conflicts with other ongoing work.

Rantoledo · 2024-10-06T06:54:33Z

I agree.
So I'll stop adding files and open this PR as soon as #5720 lands.

jsumners · 2024-10-08T19:27:24Z

Yes, thank you.

Rantoledo · 2024-10-14T08:41:54Z

Opened this PR for a review again after #5720 merged.

jsumners · 2024-10-14T11:26:13Z

I just saw that this PR is 51 changed files. This is going to take a long time to review. In the future, let's keep it to ~5 files at a time.

mcollina

lgtm

jsumners

Thank you for all of the work.

Rantoledo · 2024-10-20T12:09:37Z

Thank you for all of the work.

Happy to help.

What's the best way to add more files? should I keep this branch and open a different PR? or should I just create a new branch?
@jsumners

jsumners · 2024-10-20T14:37:06Z

All PRs should branch off the current main (or target release line branch). So please open a new PR that starts from the current main.

github-actions · 2025-10-21T00:31:23Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

migrate-sample-tests-to-node-test

fc8e055

Rantoledo force-pushed the migrate-tap-to-node-test branch from c4511c1 to fc8e055 Compare September 25, 2024 13:25

gurgunday changed the title ~~Migrate to node:test~~ chore: Migrate to node:test Sep 25, 2024

jsumners reviewed Sep 25, 2024

View reviewed changes

Comment thread test/500s.test.js Outdated

replaced-deep-equal-to-deep-strict-equal-and-not-equal-to-not-strict-…

6cbecb0

…equal

Rantoledo requested review from climba03003 and jsumners September 25, 2024 16:58

remove-new-promise-use-done-instead-use-await-when-multiple-injects

5340980

Rantoledo marked this pull request as draft September 29, 2024 14:37

Rantoledo added 2 commits September 29, 2024 17:41

added-diagnostic-channel-and-bundler-tests

339c46e

added-esm-tests

06241cd

Rantoledo added 3 commits October 3, 2024 17:22

add-http-methods-tests

3a15d85

added-https-tests

9a5a62b

added-done-in-bundler

f8938ae

Rantoledo added 3 commits October 6, 2024 10:20

Merge branch 'main' into migrate-tap-to-node-test

8a65dae

changed-new-test-in-internals-initialconfig

d703276

Merge branch 'main' into migrate-tap-to-node-test

c8aa1fe

Merge branch 'main' into migrate-tap-to-node-test

7d5353e

Rantoledo marked this pull request as ready for review October 14, 2024 08:41

mcollina approved these changes Oct 15, 2024

View reviewed changes

Merge branch 'main' into migrate-tap-to-node-test

5242b34

jsumners approved these changes Oct 17, 2024

View reviewed changes

jsumners merged commit ef1c718 into fastify:main Oct 17, 2024

This was referenced Oct 21, 2024

test: Migrate http2 test to node:test #5757

Merged

test: removed second argument from assert fail method in tests #5761

Merged

github-actions Bot locked as resolved and limited conversation to collaborators Oct 21, 2025

Uh oh!

Conversation

Rantoledo commented Sep 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

jsumners commented Sep 25, 2024

Uh oh!

Uh oh!

Rantoledo commented Sep 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jsumners commented Sep 26, 2024

Uh oh!

Rantoledo commented Sep 26, 2024

Uh oh!

Rantoledo commented Sep 27, 2024

Uh oh!

Rantoledo commented Sep 29, 2024

Uh oh!

jsumners commented Oct 2, 2024

Uh oh!

Rantoledo commented Oct 3, 2024

Uh oh!

jsumners commented Oct 4, 2024

Uh oh!

Rantoledo commented Oct 4, 2024

Uh oh!

jsumners commented Oct 5, 2024

Uh oh!

Rantoledo commented Oct 6, 2024

Uh oh!

jsumners commented Oct 8, 2024

Uh oh!

Rantoledo commented Oct 14, 2024

Uh oh!

jsumners commented Oct 14, 2024

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

jsumners left a comment

Choose a reason for hiding this comment

Uh oh!

Rantoledo commented Oct 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jsumners commented Oct 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Oct 21, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Rantoledo commented Sep 25, 2024 •

edited

Loading

Rantoledo commented Sep 25, 2024 •

edited

Loading

Rantoledo commented Oct 20, 2024 •

edited

Loading

jsumners commented Oct 20, 2024 •

edited

Loading