-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Experiment: Running tests with Bun #84242
Conversation
Link to live branch is being generated... |
This PR modifies the release build for the following Calypso Apps: For info about this notification, see here: PCYsg-OT6-p2
To test WordPress.com changes, run |
This PR does not affect the size of JS and CSS bundles shipped to the user's browser. Generated by performance advisor bot at iscalypsofastyet.com. |
Runs part of the tests in the codebase that contain '.test.' in their file name with Bun.
746bf5e
to
b2c7c7c
Compare
This is a super cool experiment. 5 times faster is pretty compelling. I'll mostly play devil's advocate in this comment because the performance boost speaks for itself. I tried I already made some comments here p1P4aZ-5z1-p2#comment-30638 where I'm worried about Bun not yet having the track record for stability and maintenance. Not that it won't eventually get that reputation, but it just doesn't have it yet. I like the premise of this hackathon project: experiment with Bun on something less mission-critical first. On the other hand, failing tests block the deployment pipeline and cause queues in the #calypso channel. When we need to quickly revert a PR, tests failing for random reasons (not related to our code) could be quite a problem. I worry a little bit about the proliferation of tools. When switching between different projects I have to switch between pnpm, yarn and npm. I'm worried that using Bun for tests will feel a bit like this. I can see we're trying to ameliorate this issue by installing bun transparently as a project dependency. But that's not 100% smooth. I get this error when installing dependencies:
Magical things are harder to debug, and re-writing I'm also not getting a clean pass result like the screenshot in the OP. So accessor properties are not supported yet. Yarn doesn't support bun yet. I also heard that Bun doesn't support Windows yet. It's starting to sound like some of the points that this scathing review of the Bun release made: https://dev.to/thejaredwilcurt/bun-hype-how-we-learned-nothing-from-yarn-2n3j Their point was that we can just stick with Node because Bun will pressure the Node team to make their app faster (the same way Yarn pressured npm to improve). And so if we wait a year Node will catch up by itself and we won't have to experience the pain of migrating. Some thoughts for the hackathon team to ponder π I also really love snappy dev tools. So, you know I'm torn. |
Support References This comment is automatically generated. Please do not edit it.
|
It's not obvious from the PR summary, but that's our recommendation based on these explorations. The performance gains look promising, and we should keep following this project as it matures. We shouldn't attempt this migration now, though.
I read this post yesterday as I was researching this and wanted to link it in response. π |
Another thought about Bun is that it might never make sense for the production Node server -- reliability and stability are way more important there. But dev tools, we can probably accept some uncertainty if it's almost always a faster overall experience!
One of my thoughts around this is that if the configuration and our team (@Automattic/team-calypso-frameworks, @Automattic/team-calypso-platform) can absorb maintenance and β¨ magic β¨, then it's ok to have some degree of fragmentation... assuming the test lib works mostly the same and there isn't much context switch going between projects with with/without Bun for testing. As one example, when we upgrade Yarn to version 4 or Node to version 20, developers should mostly not notice -- it mostly all happens automagically when they run
and a very small point on this is that Calypso development already doesn't support Windows (given a fair number of bash scripts used), and I'm not sure it will unless there's a clear need. (Though that's very different for Gutenberg!)
yeah, this would definitely be important to fix first! So for me personally (not sure if the rest of Calypso team agrees!) I like the idea of continuing to experiment with this if Bun overall makes the unit testing experience much better. Especially since we want to focus more on unit/integration tests rather than e2es! |
You make a very good point about yarn and Node version updates. You folks to a great job of abstracting those issues from us, so maybe Bun would be similar.
I think you can run Bash on Windows since a while ago? I wish I had a Windows box to try it out. It's good to be open source in principle, even if in practice there aren't any other Calypso's running in the wild.
One of the theoretical advantages of unit tests over e2e is dev's will discover test failures early, before pushing. However I still find myself first alerted to unit test failures after creating my PR :D And it's probably due to how slow they are so I'm not always running |
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.
So for me personally (not sure if the rest of Calypso team agrees!) I like the idea of continuing to experiment with this if Bun overall makes the unit testing experience much better. Especially since we want to focus more on unit/integration tests rather than e2es!
Agreed. I like the approach where we specifically have two different file naming schemes for Bun and Jest and if we end up adopting Bun for the long run, we can decide which runner to run them with by just picking the names, potentially making a migration easier.
I'd love to see the branch green before we can start considering it more seriously though. Might need a rebase and/or some dependency or install tinkering. I've seen a few workarounds suggested around the Bun + Yarn incompatibility in yarnpkg/yarn#891
Closing since I won't be able to continue working on this soon. @tyxla feel free to reopen if your team wants to pick it up again. |
Proposed Changes
Runs part of the tests in the codebase that contain
.test.
in their file name with Bun. The rest can be handled with the existing system and Jest. Since a lot of the test files produce errors due to limited Bun support or adaptations that need to be added to port them, we can use this naming scheme for gradual migration:.test.
to its name.Here are some sample results for a subset of files where it works without modifications:
Comparison
Here is the comparison on a subset of tests in
client
that contain.test.js
in their filename. In this particular sample Bun is over 5 times faster.Jest
Bun
Remaining test files
Here is a list of files that need to be renamed and fixed in some cases to port them to Bun. You can read more about the current Bun limitations that might prevent us from migrating all of the existing Calypso tests in this post: pdt2If-Zp-p2
List of 1060 files
Testing Instructions
Run
yarn test:bun
Pre-merge Checklist