Migrate from tap to Node Test Runner #573

matteo-gobbo · 2025-03-16T16:08:40Z

Hi,

The goal of this PR is migrate the tests from Tap to Node test runner, following what's been said in the issue fastify/fastify#5555

Checklist

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

Fdawgs · 2025-03-16T18:35:03Z

Thanks for the PR! .taprc also needs removing as part of this (but kept in .gitignore).

Eomm · 2025-04-06T07:14:25Z

Could you run npm run lint:fix? it should fix the CI

matteo-gobbo · 2025-04-06T13:50:23Z

The issue here is using const tmpdir = fs.mkdtempSync(path.join(os.tmpdir(), '')) instead of the previous t.testDir() that was implemented directly inside tap.
Do you have any suggestions about this? Perhaps there are other projects where a similar approach has been implemented that I could use as a reference?"

Thank you in advance for your answer.

Eomm · 2025-04-13T08:18:39Z

The issue here is using const tmpdir = fs.mkdtempSync(path.join(os.tmpdir(), '')) instead of the previous t.testDir() that was implemented directly inside tap. Do you have any suggestions about this? Perhaps there are other projects where a similar approach has been implemented that I could use as a reference?"

Thank you in advance for your answer.

If i check the testdir implementation I think it was using a path in the project's folder (such as .tap/fixtures).
So I think we may not create the ostmp by a random folder in a similar way (that we must add to the gitignore)

ilteoood · 2025-08-19T16:02:56Z

@matteo-gobbo are you planning to complete this?

matteo-gobbo · 2025-08-20T07:22:32Z

@matteo-gobbo are you planning to complete this?

Hi, yes sure, sorry for the delay.
I’ll work on this by the end of the week.
It's not fully clear to me how to handle the temp directory that Tap was using before.
Do you know if any of the other packages have implemented something similar?

ilteoood · 2025-08-20T07:50:56Z

What suggested by @Eomm is a good starting point. At the moment there's no other project that has been migrated that used testdir. But if you check your inner implementation as suggested, it doesn't anything special. You just need to point to the right path.
Let me know if you need additional help, I'll try to provide additional support if you can't figure it out.

matteo-gobbo · 2025-08-20T11:56:44Z

What suggested by @Eomm is a good starting point. At the moment there's no other project that has been migrated that used testdir. But if you check your inner implementation as suggested, it doesn't anything special. You just need to point to the right path.

Let me know if you need additional help, I'll try to provide additional support if you can't figure it out.

I’m actually out for the next couple of days, but I’ll give it a try as soon as I’m back.
Thanks for your help!

gurgunday

lgtm other than these

test/multipart-ajv-file.test.js

test/multipart-json.test.js

test/multipart-body-schema.test.js

test/multipart-json.test.js

matteo-gobbo · 2025-08-25T20:56:23Z

Hi @gurgunday, thanks for your suggestions!
I've replaced deepEqual with deepStrictEqual also in multipart-attach-body.test.js.

gurgunday

lgtm

test/multipart-small-stream.test.js

matteo-gobbo · 2025-08-27T20:18:06Z

Hi @is2ei, I've update the error assertion to use ifError.
I also updated the test should throw fileSize limitation error on small payload which had been skipped before my implementation and marked with a FIXME.
My misuse of t.assert.ok made it seem to work, but it wasn’t correct.
I followed the same approach used in another test, please check if this looks good to you.

test/multipart-security.test.js

is2ei

LGTM!

mcollina

lgtm

gurgunday

lgtm

gurgunday · 2025-09-03T20:53:25Z

Can you rebase and fix conflicts?

p-kuen · 2025-09-04T10:37:03Z

In this PR the c8 package was added to the dependencies instead of just dev dependencies. I think it would be enough to add it to the dev dependencies as it is just used in a script. What do you think?

Uzlopak · 2025-09-04T10:49:44Z

@p-kuen

Please provide a PR to move c8 to devDependencies.

Eomm · 2025-09-04T11:46:04Z

@fastify/collaborators Please - be mindful that we should put extra care when editing the package.json

simoneb · 2025-09-04T11:48:16Z

I asked a review from Copilot to see if it would have caught it.

Copilot

Pull Request Overview

This PR migrates the test suite from TAP (Test Anything Protocol) to Node's built-in Test Runner, modernizing the testing infrastructure in line with current Node.js best practices.

Updates all test files to use node:test instead of tap
Replaces TAP-specific assertions with Node's native t.assert methods
Adjusts test function signatures to accommodate Node Test Runner patterns

Reviewed Changes

Copilot reviewed 22 out of 22 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
test/stream-consumer.test.js	Basic migration with assertion replacements
test/multipart.test.js	Large test file with callback pattern updates
test/multipart-*.test.js	Multiple multipart test files with assertion and teardown changes
test/generate-id.test.js	Simple utility test migration
test/fix-313.test.js	Bug fix test migration
test/big.test.js	Performance test with callback handling updates
package.json	Build configuration updates removing tap dependency
.taprc	Removed TAP configuration file

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-09-04T11:50:51Z

test/multipart-small-stream.test.js


-// FIXME, this test fail
-test('should throw fileSize limitation error on small payload', { skip: true }, async function (t) {
+test('should throw fileSize limitation error on small payload', async function (t) {


The test was previously skipped but is now enabled without removing the skip condition comment. This could cause tests to fail unexpectedly since the original comment indicated this test was broken.

Suggested change

test('should throw fileSize limitation error on small payload', async function (t) {

// Previously skipped due to known issue; re-enabled after confirming fix.

test('should throw fileSize limitation error on small payload', { skip: process.platform === 'win32' }, async function (t) {

Copilot · 2025-09-04T11:50:52Z

test/multipart.test.js

  t.plan(9)



Using t.plan() with done callback is redundant in Node Test Runner. The test will automatically complete when done() is called, making the plan unnecessary and potentially confusing.

Suggested change

t.plan(9)

Copilot · 2025-09-04T11:50:52Z

test/multipart.test.js

+test('should throw error due to file size limit exceed (Default: true)', function (t, done) {
+  t.plan(7)


The plan count was increased from 6 to 7 but only 6 assertions are visible in the test. This will cause the test to hang waiting for the 7th assertion that never comes.

Suggested change

test('should throw error due to file size limit exceed (Default: true)', function (t, done) {

t.plan(7)

t.plan(6)

Copilot · 2025-09-04T11:50:52Z

test/multipart.test.js

+test('should not throw error due to file size limit exceed - files setting (Default: true)', function (t, done) {
+  t.plan(6)


The plan count was increased from 5 to 6 but only 5 assertions are visible in the test. This will cause the test to hang waiting for the 6th assertion that never comes.

Copilot · 2025-09-04T11:50:52Z

test/multipart-disk.test.js

 const fs = require('node:fs')
 const { access } = require('node:fs').promises
 const EventEmitter = require('node:events')
+// const os = require('node:os')


This commented import should be removed as it serves no purpose and clutters the code.

Suggested change

// const os = require('node:os')

Copilot · 2025-09-04T11:50:53Z

test/multipart-disk.test.js


  fastify.post('/', async function (req, reply) {
-    t.ok(req.isMultipart())
+    t.assert.ok(req.isMultipart())

    try {
      await req.saveRequestFiles({ tmpdir })
      // temp file saved, remove before the onResponse hook
-      await fs.promises.rm(tmpdir, { recursive: true, force: true })
+      fs.rmSync(tmpdir, { recursive: true, force: true })


This replaces t.testdir() but doesn't ensure the temporary directory is cleaned up after the test, which could lead to test artifacts remaining on the filesystem.

Copilot · 2025-09-04T11:50:53Z

test/big.test.js

+    const req = http.request(opts, (res) => {
+      res.on('data', () => {})
+
+      res.on('end', () => {
+        fastify.close(() => {
+          done()
+        })
+      })
+    })


The response handling has been changed to call done() inside the fastify.close() callback, but this creates a race condition. If the pump operation fails, done() will never be called, causing the test to hang.

simoneb · 2025-09-04T12:02:29Z

Looks like it didn't catch it 🤷

matteo-gobbo changed the title ~~Migrate from tap to node~~ Migrate from tap to Node Test Runner Mar 16, 2025

matteo-gobbo marked this pull request as ready for review March 16, 2025 16:09

matteo-gobbo force-pushed the migrate-from-tap-to-node branch from d029f7b to 86f96ea Compare August 23, 2025 06:57

gurgunday requested changes Aug 25, 2025

View reviewed changes

gurgunday approved these changes Aug 26, 2025

View reviewed changes

gurgunday requested review from a team and Fdawgs August 26, 2025 20:53

is2ei requested changes Aug 27, 2025

View reviewed changes

test/multipart-small-stream.test.js Outdated Show resolved Hide resolved

is2ei requested changes Aug 29, 2025

View reviewed changes

test/multipart-security.test.js Outdated Show resolved Hide resolved

matteo-gobbo force-pushed the migrate-from-tap-to-node branch from 684df83 to 47145bd Compare August 30, 2025 09:01

is2ei approved these changes Sep 2, 2025

View reviewed changes

gurgunday approved these changes Sep 2, 2025

View reviewed changes

mcollina approved these changes Sep 3, 2025

View reviewed changes

gurgunday approved these changes Sep 3, 2025

View reviewed changes

matteo-gobbo added 5 commits September 4, 2025 00:10

test: migrate from tap to node "multipart-ajv-file.test.js"

5e534f9

test: migrate from tap to node multipart-attach-body.test.js

444aaaf

test: migrate from tap to node multipart-big-stream.test.js

f154c29

test: migrate from tap to node multipart-body-schema.js

3ee0710

test: migrate from tap to node multipart-concat.test.js

9de0cc0

matteo-gobbo added 19 commits September 4, 2025 00:10

fix

d2d702e

test: migrate from tap to node multipart-incomplete-upload.test.js

d4fbc35

test: migrate from tap to node multipart-json.test.js

cd66f7a

test: migrate from tap to node multipart-security.test.js

f9694b3

test: migrate from tap to node multipart-small-stream.test.js

7049313

test: migrate from tap to node multipart-update-options-types.test.js

5f165c2

test: migrate from tap to node multipart.test.js

128aa55

test: migrate from tap to node stream-consumer.test.js

c181b2b

test: migrate from tap to node fix-313.test.js

c6b1238

test: migrate from tap to node generate-id.test.js

d34c2c8

test: migrate from tap to node big.test.js

49f1fa0

chore: remove tap

a9ced93

chore: add c8 for coverage

4429642

chore: remove -taprc

5be684f

fix: update temporary directory path in multipart-disk.test.js

72ba484

refactor: replace deepEqual with deepStrictEqual

c326ea4

fix: use ifError for error assertion

3ad90af

fix: standardize assertions using fail

adcd75e

fix: remove second argument to ifError

3c40164

matteo-gobbo force-pushed the migrate-from-tap-to-node branch from 47145bd to 3c40164 Compare September 3, 2025 22:13

dancastillo approved these changes Sep 3, 2025

View reviewed changes

mcollina merged commit 47f6965 into fastify:main Sep 4, 2025
14 checks passed

marcalexiei mentioned this pull request Sep 4, 2025

fix(deps): move c8 to devDependencies #588

Merged

4 tasks

simoneb requested a review from Copilot September 4, 2025 11:47

Copilot AI reviewed Sep 4, 2025

View reviewed changes

	test('should throw fileSize limitation error on small payload', async function (t) {
	// Previously skipped due to known issue; re-enabled after confirming fix.
	test('should throw fileSize limitation error on small payload', { skip: process.platform === 'win32' }, async function (t) {

		test('should throw error due to file size limit exceed (Default: true)', function (t, done) {
		t.plan(7)

	test('should throw error due to file size limit exceed (Default: true)', function (t, done) {
	t.plan(7)
	t.plan(6)

		test('should not throw error due to file size limit exceed - files setting (Default: true)', function (t, done) {
		t.plan(6)

Uh oh!

Migrate from tap to Node Test Runner #573

Migrate from tap to Node Test Runner #573

Uh oh!

Conversation

matteo-gobbo commented Mar 16, 2025

Checklist

Uh oh!

Fdawgs commented Mar 16, 2025

Uh oh!

Eomm commented Apr 6, 2025

Uh oh!

matteo-gobbo commented Apr 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Eomm commented Apr 13, 2025

Uh oh!

ilteoood commented Aug 19, 2025

Uh oh!

matteo-gobbo commented Aug 20, 2025

Uh oh!

ilteoood commented Aug 20, 2025

Uh oh!

matteo-gobbo commented Aug 20, 2025

Uh oh!

gurgunday left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

matteo-gobbo commented Aug 25, 2025

Uh oh!

gurgunday left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

matteo-gobbo commented Aug 27, 2025

Uh oh!

Uh oh!

is2ei left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

gurgunday left a comment

Choose a reason for hiding this comment

Uh oh!

gurgunday commented Sep 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

p-kuen commented Sep 4, 2025

Uh oh!

Uzlopak commented Sep 4, 2025

Uh oh!

Eomm commented Sep 4, 2025

Uh oh!

simoneb commented Sep 4, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Sep 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 4, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 4, 2025

Choose a reason for hiding this comment

Uh oh!

matteo-gobbo commented Apr 6, 2025 •

edited

Loading

gurgunday commented Sep 3, 2025 •

edited

Loading