Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

test: #5555 - move to node test runner #440

Open
wants to merge 32 commits into
base: master
Choose a base branch
from

Conversation

SamSalvatico
Copy link

Checklist

Moving the tests from tap to the node test runner, based on what requested in the following issue
Fastify #5555

@SamSalvatico SamSalvatico marked this pull request as ready for review January 17, 2025 18:18
@SamSalvatico
Copy link
Author

I did not move all the tests yet, only the ones under commonjs and typescript esm
Just to understand if you like how I’ve moved the tests and to avoid modifying too many files in the same PR, I’ll make the next changes in a subsequent PR

test/commonjs/autohooks-basic.js Outdated Show resolved Hide resolved
test/commonjs/error.js Outdated Show resolved Hide resolved
test/typescript-esm/forceESM.ts Outdated Show resolved Hide resolved
@jean-michelet
Copy link
Contributor

Thanks for your PR.

I’ll make the next changes in a subsequent PR

This plugin is a bit tricky regarding testing, we might discover later then the changes are difficult to apply.
So imho, this PR should demonstrate if we can do it or not.
Plus, refactoring tests is generally very risky, as we don't have feedback about how the changes have affected the project so we might need some more reviews.

@SamSalvatico
Copy link
Author

Thanks for your PR.

I’ll make the next changes in a subsequent PR

This plugin is a bit tricky regarding testing, we might discover later then the changes are difficult to apply. So imho, this PR should demonstrate if we can do it or not. Plus, refactoring tests is generally very risky, as we don't have feedback about how the changes have affected the project so we might need some more reviews.

Got it! So would you prefer that I try to update all the tests in this PR?

@jean-michelet
Copy link
Contributor

So would you prefer that I try to update all the tests in this PR

I think it's a good idea, you might want some more reviews before going forward.

@jean-michelet
Copy link
Contributor

@climba03003 @Fdawgs wdyt?

@dancastillo
Copy link
Member

since t.assert.plan was not added until node v22 wondering if we should use tspl or something similar, wdyt? @mcollina @gurgunday

@climba03003
Copy link
Member

I think it's a good idea, you might want some more reviews before going forward.

I believe we can do incremental one, but it should be green before land.

since t.assert.plan was not added until node v22 wondering if we should use tspl or something similar, wdyt?

If you were mentioning t.plan, it exists since 20.15.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants