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

Use node:test and node:assert/strict #9649

Merged
merged 6 commits into from
Jan 12, 2024
Merged

Use node:test and node:assert/strict #9649

merged 6 commits into from
Jan 12, 2024

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Jan 9, 2024

Changes

For @astrojs/upgrade only for now as an experiment. There are quite some quirks with node:test:

  1. Passing test glob patterns in Node <21 is broken. You can't pass trailing arguments like --test-name-pattern with it. Test runner cannot find tests in multiple levels when using ** in tha path nodejs/node#50287
  2. Running .only tests requires the --test-only flag. If you have nested tests, e.g. describe > it, you need both to be describe.only and it.only :((
  3. Test arguments are really long and hard to use/remember

Hence I created a wrapper astro-scripts test CLI which should simplify these. Except no2's nested --only issue.

Testing

Tested manually with node 18.18.1 and node 20.6.1. Node 20 has slightly better test report description.

Performance-wise, running time pnpm test:

  • Before: pnpm test 0.89s user 0.21s system 167% cpu 0.659 total
  • After: pnpm test 0.54s user 0.08s system 89% cpu 0.687 total

Docs

n/a. internal change.

Copy link

changeset-bot bot commented Jan 9, 2024

⚠️ No Changeset found

Latest commit: 812cc47

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Member

@Princesseuh Princesseuh left a comment

Choose a reason for hiding this comment

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

I'm for this! I think node:test is likely to become better "quickly" and will in the future have a fair ecosystem around it, by being the built-in option. I trust in Node 🫡

Copy link
Member

@ematipico ematipico left a comment

Choose a reason for hiding this comment

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

How would it work if I wanted to run a single test? node test ../path/to/file.test.js?

@bluwy
Copy link
Member Author

bluwy commented Jan 12, 2024

Almost yeah! It's --test, so it should be node --test ../path/to/file.test.js. It's optional to use astro-scripts test since it's just a wrapper around it to workaround quirks and set proper defaults. But you can also if you want to, e.g. astro-scripts test ../path/to/file.test.js --match "filter test name", or shorter pnpm test -- -m "filter test name". I think I'll document this in the contributing guide before merging.

@bluwy bluwy merged commit 71db79e into main Jan 12, 2024
4 checks passed
@bluwy bluwy deleted the use-node-test branch January 12, 2024 07:53
ematipico pushed a commit that referenced this pull request Jan 12, 2024
ematipico added a commit that referenced this pull request Jan 17, 2024
* feat(i18n): disable redirect

* feat(i18n): add option to disable redirect to default language

* chore: add schema validation

* docs

* changeset

* Update packages/astro/src/core/config/schema.ts

Co-authored-by: Bjorn Lu <[email protected]>

* chore: address feedback

* fix test

* Update .changeset/cyan-grapes-suffer.md

Co-authored-by: Sarah Rainsberger <[email protected]>

* Update packages/astro/src/@types/astro.ts

Co-authored-by: Sarah Rainsberger <[email protected]>

* Fix discord fetch code (#9663)

* Force re-execution of Partytown's head snippet on view transitions (#9666)

* Remove the header script before a view transition takes place to force a reload on the next page

* Add changeset

* Save another char

* [ci] format

* fix(assets): Implement all hooks in the passthrough image service (#9668)

* fix(assets): Implement all hooks in the passthrough image service

* chore: changeset

* refactor(toolbar): Rename every internal reference of overlay/plugins to toolbar/apps (#9647)

* refactor(toolbar): Rename every internal reference of overlay/plugins to toolbar/apps

* refactor: rename vite plugin

* fix: update import

* nit: add setting fallback

* Disable file watcher for internal one-off vite servers (#9665)

* Use node:test and node:assert/strict (#9649)

* [ci] format

* fix(i18n): emit an error when the index isn't found (#9678)

* fix(i18n): emit an error when the index isn't found

* changeset

* Update .changeset/proud-guests-bake.md

Co-authored-by: Sarah Rainsberger <[email protected]>

* rename

* Update packages/astro/src/core/errors/errors-data.ts

Co-authored-by: Florian Lefebvre <[email protected]>

---------

Co-authored-by: Sarah Rainsberger <[email protected]>
Co-authored-by: Florian Lefebvre <[email protected]>

* feat(i18n): add option to disable redirect to default language

* chore: rebase

* Update packages/astro/src/@types/astro.ts

Co-authored-by: Bjorn Lu <[email protected]>

* lock file update

---------

Co-authored-by: Bjorn Lu <[email protected]>
Co-authored-by: Sarah Rainsberger <[email protected]>
Co-authored-by: Martin Trapp <[email protected]>
Co-authored-by: Martin Trapp <[email protected]>
Co-authored-by: Erika <[email protected]>
Co-authored-by: Bjorn Lu <[email protected]>
Co-authored-by: Florian Lefebvre <[email protected]>
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.

3 participants