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

feat: use built in node:test runner in all template test implementations #682

Merged
merged 38 commits into from
Dec 31, 2023

Conversation

big-kahuna-burger
Copy link
Contributor

@big-kahuna-burger big-kahuna-burger commented Nov 27, 2023

Closes #632 #680

For all 4 variants, [cjs,esm] x [js, ts] I've used a locally linked cli to generate a project and have npm i && npm test pass.
Typescript variants have coverage enabled by default, cjs one using c8, esm one using --experimental-test-coverage flag. tsx has been added to devDeps, in order to support ts-esm variant.

Todos

There are two pending issues with this PR afaik:

  • test/templates/app-ts.test.ts is skipped instead of letting it break the complete suite. See more here . Any help would be nice as to what to do with it.

  • script unit:ts-cjs is set to always pass due to ||. This is a chicken-egg problem. Here a template test suite is being run, but that test suite also requires a fastify-cli/helper.js by name. So, I can't really install that as devDependency of self. I'm sure there is some trick to handle this. Any help appreciated again.

  • Skip running template suites on 14/16 <20

  • Node 20 on windows glob error

Checklist

.github/workflows/ci.yml Outdated Show resolved Hide resolved
* ci: only ts

* ci: no c8

* ci: c8 for javascript

* ci: no c8
@big-kahuna-burger
Copy link
Contributor Author

imho this can be reviewed now by maintainers.
All workflows are passing on my fork.

@mcollina @Fdawgs
PTAL

@big-kahuna-burger big-kahuna-burger changed the title feat: use built in node:test runner in all template test implementations feat: use built in node:test runner in all template test implementations Nov 28, 2023
@big-kahuna-burger big-kahuna-burger marked this pull request as draft November 29, 2023 06:30
@big-kahuna-burger
Copy link
Contributor Author

Converted to draft. Waiting for #681 to be merged so this can be rebased on top of it.

@big-kahuna-burger big-kahuna-burger marked this pull request as ready for review December 2, 2023 14:14
@big-kahuna-burger
Copy link
Contributor Author

Good for review again @mcollina.
Rebased on top of master, removed del-cli usage in favor of --noEmit flag given to tsc

suite-runner.js Outdated Show resolved Hide resolved
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina added the semver-major Issue or PR that should land as semver major label Dec 26, 2023
@mcollina
Copy link
Member

Could you remove node v14/v16 from CI? The code that this generates would never run there.

cc @Uzlopak @Fdawgs I'm not sure if we have a parameter for this

@Fdawgs
Copy link
Member

Fdawgs commented Dec 26, 2023

cc @Uzlopak @Fdawgs I'm not sure if we have a parameter for this

Shared workflow just needs to be updated to use v4, which dropped support for 14 and 16.

This was discussed previously in this PR here: #682 (comment)

@mcollina
Copy link
Member

I think we removd the v4 tag for now to avoid massive updates everywhere (by auto merge) that were not necessary.

@Kae7in
Copy link

Kae7in commented Dec 31, 2023

Any idea when this will get merged? I got a similar error to #680 when starting a project via fastify generate example --lang=ts --esm

@mcollina mcollina merged commit 196a9fb into fastify:master Dec 31, 2023
19 checks passed
@Kae7in
Copy link

Kae7in commented Dec 31, 2023

Thank you @mcollina!! That worked. 😄

@big-kahuna-burger big-kahuna-burger deleted the test-node-builtin branch December 31, 2023 19:45
@Eomm Eomm mentioned this pull request Jan 19, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-major Issue or PR that should land as semver major
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use Node.js built-in test runner in templates
4 participants