-
-
Notifications
You must be signed in to change notification settings - Fork 76
chore: migrate to node.js native test runner #517
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
owenvoke
left a comment
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.
I'm personally in favour of this partly just due to the pretty big reduction in dependencies. 👀 I didn't know that Node had it's own test runner for a while, that's pretty neat. 👍🏻
However, I guess if people are used to working with the current setup and it's not broken, perhaps we should stick to it. 🤷🏻 But will leave to other maintainers with more familiarity with the project to approve. 👍🏻
To me reducing the amount of dependencies has much more value than the argument to stick to a 3rd party test library because people are used to it. Another argument to switch to the Node test runner, is that I see the CI is much quicker. |
10e3bd0 to
6942b61
Compare
|
I'll return to this once the Cloudflare issues etc. are all over. I'm not sure what's wrong, but for me the tests are passing intermittently at the moment.
The issue appears to be related to either slower networks or when the connection lives for longer. This makes it sound like a timeout or race condition to me, but I don't see any obvious problems for now. But it's probably not a good time to try and fix network related tests while half the internet is on fire anyway. |
|
Rebased and resolved merge conflicts, and tests are passing now! 🎉 Not 100% sure if the failures before were due to the outages or not, but CI did pass twice in a row, so I'll just assume that it was. Of course, I'm happy to take a deeper look if the issue comes up again! |
I've seen other PRs failing as well the last day(s) (#490 for example, but as well the Dependabot PRs). |
|
Yeah sure! I'll get this merged then, and if we see any issues with Edit: I can also confirm that the CI run for |
Description
Node.js has added a standard library for Node.js unit testing, which has become more mature over the years, including a test runner, mocking support, coverage, etc.
See: https://nodejs.org/en/learn/test-runner/using-test-runner
I propose we migrate from mocha/sinon/should/nyc to just the standard library, which provides everything we need without adding literally 170 npm dependencies to the project.
node_modulesnot declared inpackage.json)node:test, whereas testing in the Node.js ecosystem has traditionally been very fragmented.All tests have been kept intact, this just migrates from one library to the standard library. I've been using
node:testin all of my other projects as well, and it's pretty great imo.Checklist
Please review this checklist before submitting a pull request.
npm run test:all)