-
Notifications
You must be signed in to change notification settings - Fork 1k
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
chore(core): switch to vitest #10362
Conversation
@Josh-Walker-GM This looks like it passed CI, any large considerations to take before we merge this? |
@peterp Nope I think it's good to go now. I've fixed the conflicts and such. If it passes CI this time I'll be happy for it to get merged. Want to sanity check and approve it before I merge? |
Yup! Awesome! I'll check this out and wrap my head around it! |
@@ -0,0 +1,21 @@ | |||
import { build, defaultBuildOptions } from '@redwoodjs/framework-tools' | |||
|
|||
// Build 'dist/bins' |
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.
Nit-picky for sure, but aren't we actually building src/bins
(and outputting to dist/bins
)?
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.
It is just because you added an import
that we now need to build? Can you use require
instead, so we don't have to build?
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 happy to correct the comment.
Vitest doesn't natively intercept require
which means if we still want to use require
we'd need to look to some additional library. I think I'd rather build the files - keeps the source style consistent with the rest of the code and no additional dependency we're not already familiar with. See: vitest-dev/vitest#3134.
As we're getting increasingly close to removing webpack this test and so the need for vitest at all in this package will go away.
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.
Right. I forgot about vitest not working with require
natively. Building makes sense. Then my next questions/suggestion/idea is: Can we build to dist/config
instead? But still not include it in release of course.
No longer needed after #10867 |
This PR updated the
@redwoodjs/core
package to use vitest instead of jest for the unit testing.This one is a little more crazy than the others due to some things I've had to work around. As a result it also switches out the build process for this package to use esbuild like our other packages. This was needed to ensure I could avoid certain syntax in the code to allow testing but ultimately still build the same dist that is shipped to users.
It passes CI just fine and I of course did adhoc local testing too.