Skip to content

test: port server.test.js to node:test#5783

Merged
Eomm merged 3 commits into
fastify:mainfrom
pmarchini:test/port-tap-tests-to-node-test-runner
Oct 27, 2024
Merged

test: port server.test.js to node:test#5783
Eomm merged 3 commits into
fastify:mainfrom
pmarchini:test/port-tap-tests-to-node-test-runner

Conversation

@pmarchini

Copy link
Copy Markdown
Contributor

server.test.js porting from tap to node:test

Checklist

Comment thread test/server.test.js Outdated

test('abort signal', t => {
t.test('listen should not start server', t => {
describe('abort signal', () => {

@jsumners jsumners Oct 26, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We don't need to use describe.

Suggested change
describe('abort signal', () => {
test('abort signal', async () => {

Also, each subtest should be like:

await t.test('listen should not start server', (t, end) => { end() })

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hey @jsumners, no problem at all, I'm pushing the changes 😁
Could I ask about the rationale behind this choice? 😁

I ask because, IMHO, the describe / test / it approach is cleaner than the await t.test.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We don't use it anywhere else. And I find it noisy.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It makes perfect sense, I was just curious 🚀
Thanks for your review!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

pushed the changes in the meantime!

@pmarchini pmarchini requested a review from jsumners October 26, 2024 14:59

@jsumners jsumners left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to me.

@github-actions

Copy link
Copy Markdown

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.

@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Oct 28, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants