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 #2320

Merged
merged 6 commits into from
Jul 12, 2023
Merged

Use node:test #2320

merged 6 commits into from
Jul 12, 2023

Conversation

remcohaszing
Copy link
Member

This replaces uvu with the node --test command and node:test module.

This replaces `uvu` with the `node --test` command and `node:test`
module.
@remcohaszing remcohaszing added 🕸️ area/tests This affects tests 🏗 area/tools This affects tooling 🤞 phase/open Post is being triaged manually labels Jul 4, 2023
@vercel
Copy link

vercel bot commented Jul 4, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
mdx ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 10, 2023 3:46pm

packages/esbuild/package.json Outdated Show resolved Hide resolved
packages/esbuild/test/index.test.js Outdated Show resolved Hide resolved
@@ -67,7 +67,7 @@
"scripts": {
"prepack": "npm run build",
"build": "tsc --build --clean && tsc --build && type-coverage",
"test-api": "uvu test \"\\.js$\"",
"test-api": "node --test",
Copy link
Member

Choose a reason for hiding this comment

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

I don’t get the need for the --test flag?

Copy link
Member Author

Choose a reason for hiding this comment

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

node --test discovers and runs test files. It’s the most elegant way to run them IMO.

Copy link
Member

Choose a reason for hiding this comment

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

node index.js does too? 😅

I feel like that’s much less “magic”. As it’s similar to how I run all other JS code?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, node index.js can run a single test file.

It matters as soon you have multiple test files (such as @mdx-js/mdx). At this point you need a tool to run all of these test files, which node --test provides. The alternative is to create a script that calls run from node:test, which is essentially what node --test does for you.

I like using node --test for everything, as it provides a single well-known entry point for running tests using the Node.js.

Copy link
Member

@wooorm wooorm Jul 6, 2023

Choose a reason for hiding this comment

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

There’s another alternative: use an index file that imports several files: https://github.com/micromark/micromark/blob/main/test/index.js. No tools needed.

The main reason that I like node:test is that any regular file is a test, nothing is injected into them or so which other test runners do, and any error is a correct error, and no flags are needed to use them

single well-known entry point for running tests using the Node.js.

To me that is solved by the test npm script?

Copy link
Member

@wooorm wooorm Jul 6, 2023

Choose a reason for hiding this comment

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

I also don’t really care to learn another resolution mechanism, to learn that oh wait it doesn’t look for .jsx files (so in those cases they have to be passed explicitly)

Copy link
Member Author

Choose a reason for hiding this comment

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

The difference is that node --test runs all test files in a separate process. Each test file is isolated. Also state from one file (e.g. a patched library or cwd) doesn’t leak into other test files. This is not the same as importing all test files.

Let’s say we have these files:

// a.js
import { beforeEach, test } from 'node:test'

beforeEach(() => {
  console.log('beforeEach a')
})

test(() => {})
// b.js
import { beforeEach, test } from 'node:test'

beforeEach(() => {
  console.log('beforeEach b')
})

test(() => {})
// index.js
import './test/a.js'
import './test/b.js'

node --test runs beforeEach before each test in the file it’s defined in.

$ node --test   
ℹ beforeEach a
✔ <anonymous> (1.686736ms)
ℹ beforeEach b
✔ <anonymous> (1.722439ms)
ℹ tests 2
ℹ pass 2
ℹ fail 0
ℹ cancelled 0
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 146.868511

node --test runs beforeEach before each test in all files imported.

$ node index.js 
beforeEach a
beforeEach b
beforeEach a
beforeEach b
✔ <anonymous> (1.814503ms)
✔ <anonymous> (0.353704ms)
ℹ tests 2
ℹ pass 2
ℹ fail 0
ℹ cancelled 0
ℹ skipped 0
ℹ todo 0
ℹ duration_ms 7.957022

Also the npm test script isn’t always used for running just tests. E.g. within the unified ecosystem it’s used to run testing, formatting, and linting.

I do agree it’s unfortunate node --test doesn’t support custom file extensions.

Copy link
Member

Choose a reason for hiding this comment

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

Well that’s kinda my point, I don’t want stuff like that, that behaves differently when run one way or another 🥲 Again, I like just regular files, that don’t interfere, and if weird process.cwd hacks are needed that they clean up themselves. That can be run fine with Node normally.

And I don’t want to an API with a giant surface. I want to throw errors if stuff is wrong. And in 4 years, when a new test framework is around, we can switch again easily.

Feels very similar to whether to write in .ts or .js files. I am not against types or tests, far from it. But it’s so much easier to maintain things for years if files are just plain javascript files.

within the unified ecosystem it’s used to run testing, formatting, and linting.

test-api and test-coverage do that, then.

You are sometimes going to need to pass other things to Node, with or without --test. So node --test doesn’t always run all the expected tests in the expected way. It sometimes needs to be configured. And that’s what npm scripts solve.

Copy link
Member

Choose a reason for hiding this comment

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

The difference is that node --test runs all test files in a separate process. Each test file is isolated. Also state from one file (e.g. a patched library or cwd) doesn’t leak into other test files. This is not the same as importing all test files.

On projects outside unified, I've tended to use the BDD API to handle this (describe blocks with it tests)
Which allows scoping before, after, beforeEach, and afterEach to a single test suite.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well that’s kinda my point, I don’t want stuff like that, that behaves differently when run one way or another 🥲 Again, I like just regular files, that don’t interfere, and if weird process.cwd hacks are needed that they clean up themselves. That can be run fine with Node normally.

This is exactly what I want. I want test files that don’t interfere with each other, so tests can be run in isolation. I.e. when running npm test and something goes wrong in test/compile.js, I want to be able to run node test/compile.js to troubleshoot the issue. This isn’t possible if it’s run via an import, because other tests might interfere.

I also like using describe to organize somewhat bigger test suites, but that’s not the same as running each test file in a separate process via node --test.

I will change it, because I’d rather move from uvu to node:test than block it over this, but I really think we should leverage node --test.

packages/loader/test/index.test.js Show resolved Hide resolved
Copy link
Member

@ChristianMurphy ChristianMurphy left a comment

Choose a reason for hiding this comment

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

Thanks @remcohaszing!
All for migrating to node:test. 👍
Once CI is passing and @wooorm's comments are addressed this will LGTM.

@wooorm
Copy link
Member

wooorm commented Jul 12, 2023

TLDR didn’t read everything but LGTM ;)

@wooorm wooorm merged commit ee176d7 into main Jul 12, 2023
9 checks passed
@wooorm wooorm deleted the node-test branch July 12, 2023 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🕸️ area/tests This affects tests 🏗 area/tools This affects tooling 🤞 phase/open Post is being triaged manually
Development

Successfully merging this pull request may close these issues.

3 participants