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

node:test support #308

Open
wants to merge 8 commits into
base: native-node-runner
Choose a base branch
from

Conversation

alcuadrado
Copy link
Contributor

@alcuadrado alcuadrado commented May 17, 2024

Hey @sz-piotr,

I continued your work on #305 in this PR.

Your previous TODO:

  • Support snapshots
  • Nice error printing: This depends on the reporter that you use. I added one that supports this to examples/example-node-runner
  • Plugin support for ESM (not sure why they don't work)

What does the last todo item mean? I'm willing to fix it if you explain it.

Also, can I add info about node:test to the readme and the docs?

I really really think this library + node:test are a great combo

node:test's default reporter is really limted, and doesn't print the
errors that earl generates in a nice way. I replaced it with a better
one, which behaves like mocha.
Copy link

changeset-bot bot commented May 17, 2024

🦋 Changeset detected

Latest commit: 9680303

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
earl Minor
website Patch

Not sure what this means? Click here to learn what changesets are.

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

Copy link

vercel bot commented May 17, 2024

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

Name Status Preview Updated (UTC)
earl-docs ✅ Ready (Inspect) Visit Preview May 18, 2024 0:01am

@@ -8,7 +8,7 @@ jobs:
CI:
strategy:
matrix:
node: ["16.x", "18.x"]
node: ["18.x", "20.x", "22.x"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason for the change is that 16 is EOL'ed, and I believe it doesn't support node:test.

Also added 20 and 22 to make sure it works with them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

node:test is much better in 22. It was more experimental in the previous versions.

@@ -10,11 +10,13 @@
"lint": "biome check ./src",
"lint:fix": "biome check --apply ./src",
"typecheck": "tsc --noEmit",
"test": "node --test --loader ts-node/esm src/*.test.ts",
"test": "node --test --test-reporter=@voxpelli/node-test-pretty-reporter --loader ts-node/esm src/*.test.ts",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This uses this other reporter, which takes advantage of the errors that earl generates to print a diff: https://github.com/voxpelli/node-test-pretty-reporter

I think this should be documented.

@@ -41,6 +42,6 @@ describe('stack traces for errors', () => {
return nestedGetControl()
}
const control = nestedValidator()
expect(control.file).to.equal(import.meta.url)
expect(control.file).to.equal(fileURLToPath(import.meta.url))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just a fix. I believe this changed in node 18/20, but import.meta.url is a file:// URL, and control.file is a path.

@@ -10,11 +10,14 @@
"lint": "biome check ./src",
"lint:fix": "biome check --apply ./src",
"typecheck": "tsc --noEmit",
"test": "node --test --loader ts-node/esm src/*.test.ts",
"test": "glob -c \"node --test --test-reporter=@voxpelli/node-test-pretty-reporter --loader ts-node/esm\" \"src/*.test.ts\"",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The need for glob is unfortunate. This is because npm scripts on windows don't work well with globs, and node:test before node 22 doesn't support glob patterns natively.

@alcuadrado
Copy link
Contributor Author

I understand what the last TODO item means now. I believe it may come from not using project references and tsc getting somewhat lost.

From a separate npm project things work as expected.

@alcuadrado
Copy link
Contributor Author

I understand what the last TODO item means now. I believe it may come from not using project references and tsc getting somewhat lost.

I can try to fix this, but would like to validate it with you first, as it would touch most tsconfig.json files.

@alcuadrado
Copy link
Contributor Author

alcuadrado commented May 17, 2024

I understand what the last TODO item means now. I believe it may come from not using project references and tsc getting somewhat lost.

I can try to fix this, but would like to validate it with you first, as it would touch most tsconfig.json files.

No, it's not that. The problem is that the example-plugin is CJS-only, so it imports the CJS version of earl, while the example imports the ESM one.

@sz-piotr
Copy link
Member

@alcuadrado Great work. Sorry I couldn't look at this earlier, I was really busy with conferences. I'll try to review this week and hopefully get this merged and released.

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.

None yet

2 participants