Test CI with both Node.js and Bun #352
Conversation
There was a problem hiding this comment.
@pillarjs/express-tc This project is JS-only, no deps, nothing runtime specific (since it's designed to be used everywhere). Based on past discussions we would want to merge this (and any other runtimes people want to test on) but my personal experience is to keep the test suite minimal and instead target ES version supports vs runtimes.
| @@ -26,6 +30,8 @@ const { | |||
| } = require("path-to-regexp"); | |||
| ``` | |||
|
|
|||
| This library is compatible and tested with Node.js 16+ and Bun 1.2+. | |||
There was a problem hiding this comment.
I'd prefer to remove this, it's all just JS so it's compatible with anything that supports ES2015 basically. These just happen to be where tests are run.
| @@ -0,0 +1,3 @@ | |||
| [test] | |||
| coverage = true | |||
| coverageReporter = ["text", "lcov"] | |||
@ctcpip when we’ve discussed it in the past I thought you were on the side of testing everything that’s supported. The package supports everything that can run ES2015, including browsers, Cloudflare workers, etc so it seems like this is an improvement toward that goal. |
That's not exactly my position. My position is that if we are officially and deliberately supporting something, then we should have tests for those runtimes, environments, engines, etc. Similarly, I wouldn't expect to see tests for every JS engine out there either. The ADR on this is not yet merged, but right now contains the following:
|
|
Thank you for the contribution. Im +1 to keeping a minimal test suite Is there a specific problem this PR would solve for consumers or maintainers? If so, Im open to learning more. Absent a clear problem, Im not seeing value in the additional workflow complexity |
|
@ctcpip I did try my best to get that language removed with expressjs/discussions#323 (comment) 🤷 This package does support other runtimes than it's tested on, but lack of CI is due to redundancy and maintenance overhead. @Electroid I'm going to close this out for now since this is a simple package and supports any ES2015 environment, so it shouldn't need a dedicated test suite for this. Let me know if you disagree though. This is currently enforced through TypeScript with |
This adds support to run tests in CI using Bun, and keeps Node.js support. It makes running tests 35% faster.
Since Bun supports Jest & Vitest, you'll notice there are no code changes! It just works. (and I did test the Github Action on a fork to verify that it indeed does work)
This also changes
npm installtobun install, but just in CI. Developers can continue to use npm, yarn, etc.I also updated various GitHub Action steps to use the newer versions (e.g.
actions/checkout@v2->actions/checkout@v4)