(test): migrate to node test runner

[ilteoood]

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]

God bless you @ilteoood, this one must have been a nightmare with all the tests.

[ilteoood]

@Fdawgs do you think there's something else I can do here?

[Copilot]

Pull Request Overview

This PR migrates the test suite from using the Tap test runner and simple-get to using Node’s built-in test runner and fetch. Key changes include:

• Converting tests from callback-based to async/await style
• Replacing simple-get HTTP calls with fetch
• Updating snapshot tests accordingly

Reviewed Changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.

File	Description
test/test.js	Migrated tests to async/await and node:test; replaced simple-get
test/test-import-engine.mjs	Updated to async test style and use fetch instead of callbacks
test/snap/test-ejs-with-snapshot.js	Adjusted asynchronous handling and snapshot assertions
benchmark.js	Replaced simple-get with fetch for the warmup request

[Fdawgs]

It's a big boofer but lgtm. Will need a few more extra pair of eyes as I no doubt glossed over some bits.

I know we don't track coverage in this repo's tests, but is coverage percentage still the same after migrating?

[simoneb]

I see that tap is still being used here. Was this intentional? If so, it would have been useful if you had mentioned it in the PR description

[ilteoood]

I mentioned it in one of the previous comments. It was not intended

[simoneb]

Ok, please consider that a reviewer will not be able to see resolved conversations unless they are explicitly opened, which is why I missed it.

[simoneb]

why is tap not being removed from dev dependencies?

[ilteoood]

On node 20 snapshots are not available yet, so I cannot remove it for those tests yet

[ilteoood]

#461 (comment)

[simoneb]

since there are no more assertions in callbacks, all the t,plan can be removed as they don't offer any value and just overspecify the tests. unless there is a specific reason why you left them in place, I would suggest to remove them from here and everywhere else you removed the assertions in callbacks (which is probably everywhere)

[ilteoood]

I left them just because everywhere else in the Fastify org they have been maintained even if the tests have been migrated. I'm ok with removing them, but I want to be 100% sure that it is something that we want to do, according also the amount of tests that I need to change

[simoneb]

yep fair point. I personally don't see any use in them if the tests are using async/await. the only reason for them to be there previously was to prevent the test from finishing until all the assertions running in callbacks had executed, hence no reason in my opinion to leave them there.

[ilteoood]

Let's see what @Eomm or @Fdawgs think about it. They have reviewed most of my PRs in other repos so I guess they are the best people to ask what they prefer

[gurgunday]

I agree with @simoneb but then again if I recall correctly there were maintainers who wanted to use them anyway so it might be better to wait for their input on this one :)

And like @ilteoood mentioned we currently use them in most places

[Eomm]

I think it is out of scope of the migration to the node test runner.
There are a lot of improvements we could write and removing the plan adds confusion to junior contributors IMHO.

For the logs:
In general I prefer to keep the plan because it "forces" the devs to STOP and THINK before writing the test.

since there are no more assertions in callbacks

Let's do an example: the double genPromise is a typo but:

• with plan: I can find it
• without plan: the test would be green

const { test } = require('node:test');
test('test', async (t) => {
  t.plan(1)

  async function genPromise () {
    t.assert.ok(true)
  }

  await genPromise()
  await genPromise()
})

Another example: If we have an assertion on a Fastify hook and, due to a bug, we can spot a double execution of that function.

---

Note: the subject test does not have assertion on a callback function executed by the framework itself, so it would be safe to remove it.. but then if someone adds a new assertion on a callback we (maintainers) should check if the plan is set... etc.. useless back and forth to save one line (?).

[simoneb]

I do not agree with this but I don't have a strong opinion either. Also, I see the value in consistency and if other repos are using the same approach, it does make sense to keep it instead of creating an exception.

I certainly do see an activity in the future of removing all t.plan from the whole codebase though. If you can spare writing unnecessary code, I'd say why not?

[simoneb]

It looks like we're all good. Thanks @ilteoood for the considerable time and effort that putting this together must have taken!

[simoneb]

funnily, art-template decided to do a release after 6 years, and it broke CI in main.

https://github.com/goofychris/art-template/issues/658