-
Notifications
You must be signed in to change notification settings - Fork 149
refactor: migrate tests to Vitest + TS #1202
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
refactor: migrate tests to Vitest + TS #1202
Conversation
…e-ts-refactor-redone
Co-authored-by: j-k <[email protected]> Signed-off-by: Juan Escalada <[email protected]>
Co-authored-by: j-k <[email protected]> Signed-off-by: Juan Escalada <[email protected]>
Co-authored-by: j-k <[email protected]> Signed-off-by: Juan Escalada <[email protected]>
Co-authored-by: j-k <[email protected]> Signed-off-by: Juan Escalada <[email protected]>
e46cfac to
4b9b89b
Compare
4b9b89b to
61d349f
Compare
|
@kriswest I think this is ready for another look - hoping to get the v2 release ready soon! |
kriswest
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.
LGTM - thanks for the logging cleanup and email comparison fix. Still had a couple of type overrides I wasn't sure you needed... worth leaving a comment if it is needed.
The only lingering issue I see is the proxy tests. Logging errors on start/stop might be worth doing (as it appears not to have stopped...). If that doesn't turn up the answer then it should go on an issue so we can get this merged.
|
@kriswest Thanks for taking another look! Hoping it's ready to go now 😃 |
Co-authored-by: Kris West <[email protected]> Signed-off-by: Juan Escalada <[email protected]>
Co-authored-by: Kris West <[email protected]> Signed-off-by: Juan Escalada <[email protected]>
…itest-migration-from-service
coopernetes
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.
Just a few misc comments. With all the tests passing in CI and using all the vitest mocking facilities, this overall LGTM. Note I didn't do a fine grained review.
A few things we can track in separate issues related to e2e testing, integration testing and removing some of the weird test setups where shared state is making these layers difficult to test. I would also recommend splitting up the Express app layers so that they can import each other & more easily be mocked.
Fixes #1150, built on top of #1166.
Not all tests were perfectly refactored from Chai/Mocha -> Vitest. Some required partial or total rewriting to get them to play nice.
Some tests, specifically the proxy-related tests (
proxy.test.ts,testProxy.test.tsand most importantlytestProxyRoute.test.ts) have been very problematic for some reason - the tests intestPush.test.tsseem to fail in the CI when some of the proxy tests execute prior. They fail on the first execution locally, and then pass without issue (maybe a caching/cleanup problem).Coverage has dropped somewhat to 80.5%, but we should see an increase after removing unused/untested code such as plugin stuff.
Note that the CI is failing due to the new Blue Oak license.