Skip to content

Update tests to use node:test, remove tap #235

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 3 commits into from
Sep 15, 2024

Conversation

bencoder
Copy link
Contributor

Updates tests to use node:test
Remove tap
Converted tests to async/await style over callbacks and removed t.plan(x) statements where appropriate

Checklist

@Uzlopak
Copy link
Contributor

Uzlopak commented Sep 11, 2024

Yeah the windows test fails. Maybe simplest solution: change file extensions of all test files from .js to .test.js or maybe adding ./ to the npm script?!

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.

Hi, thanks a lot for the PR!

Please:

  1. Install c8
  2. Change the test:unit command to c8 --100 node --test
  3. As @Uzlopak said, for consistency, it would be great if you could add .test. to the test files

@bencoder bencoder requested a review from gurgunday September 11, 2024 12:03
Copy link
Member

@dancastillo dancastillo left a comment

Choose a reason for hiding this comment

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

Can you add back the t.plan to the tests node:test does support test count assertions ref

@bencoder
Copy link
Contributor Author

@dancastillo I've removed them where they are not adding anything - they only really make sense when you have assertions in callbacks or conditional statements - everywhere I've removed them has been where I've changed to using await so the assertions will always run and setting t.plan just seems like extra friction.

If they should be a standard for fastify tests though then I can add them back if necessary

@dancastillo
Copy link
Member

@bencoder That makes sense to me! As for the standard with Fastify tests, I believe we have them in place for most of our tests but like you mention likely for callbacks or conditionals

I'll defer it to @Uzlopak or @gurgunday to confirm whether it's a strict requirement or is okay

@jsumners
Copy link
Member

I would reserve plans for tests of asynchronous operations.

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

Copy link
Member

@dancastillo dancastillo left a comment

Choose a reason for hiding this comment

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

Looks like there is no error defined on await fastify.inject(...) response ref

So this below will never fail.

t.assert.ifError(response.error)

Can we update all similar asserts to below to validate we received a truthy response

  t.assert.ok(response)

…to `ok(response)` because response.error does not exist
Copy link
Member

@dancastillo dancastillo left a comment

Choose a reason for hiding this comment

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

LGTM

@dancastillo dancastillo merged commit f9bdf93 into fastify:master Sep 15, 2024
11 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants