-
Notifications
You must be signed in to change notification settings - Fork 181
test: use node test runner to assert expected cases #538
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
Conversation
|
π¬ Hmm... I'm not sure coverage reports are generated as noted in codecov/feedback#107 for Node 20? Perhaps this is saved as a draft PR for now, or coverage is checked otherwise in the meantime. Open to suggestions! |
package.json
Outdated
| "lint:fix": "biome check --write", | ||
| "lint": "biome check", | ||
| "test": "c8 mocha test/*.spec.js", | ||
| "test": "node --test --experimental-test-coverage test/*.spec.js", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome π― it would allow us to reduce our dependencies
My only concern here is that --experimental-test-coverage is still experimental and subject to breaking changes and potential removal, I haven't been able to find a stable target release date for it.
I'm not opposed to testing this out on this project but we need to be ready to potentially roll this back if the feature changes or gets removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@WilliamBergamin I share this concern but think adjusting with those changes might be nice to prefer fewer packages installed π
A commit 41a3be0 was added for generating coverage reports for the @codecov steps which might help avoid regression in PRs too! IIRC this'll be running after a merge to main but I'll report back πΎ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ποΈβπ¨οΈ Hmm a fast follow was needed to upload the coverage report instead of test reports! #539
|
@WilliamBergamin I share the excitement - these are nice defaults to use! Thanks so much for the review too π’ β¨ |
Summary
This PR uses the
node:testpackage to assert expected cases in place of the amazingmochaandchaipackages β π«‘Notes
Also removed are the kind packages adjacent: π
@types/chai@types/mochac8mocha-suppress-logsCoverage is changed from
c8to--experimental-test-coveragewhich is checking both "src" and "test" directories but updates for #487 might bring options to change this also π§ͺRequirements