-
-
Notifications
You must be signed in to change notification settings - Fork 2k
refactor(store): migrate unit tests to Vitest #5036
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(store): migrate unit tests to Vitest #5036
Conversation
✅ Deploy Preview for ngrx-io ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for ngrx-site-v21 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
exequiel09
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.
Self-comment
|
Only type errors are left which I didn't touch and just appeared when migrated to
Thinking of how to fix this 🤔
|
|
Would love to ask for some feedback on the PR @markostanimirovic and @brandonroberts especially on the issues that arised upon migrating to |
2922a93 to
334ecd9
Compare
|
Hi @timdeschryver would love also your feedback here. Just a couple issues then I think it's good to go. Thank you. |
I will take a look at it later this week (probably at the end of it) |
8853f4b to
ea6e106
Compare
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.
The remaining CI errors are not caused by this PR => they should be resolved in #5044.
From looking at the PR, I think you addressed all of the issues yourself, or is there still something left where you're waiting for input?
The failing type tests can be fixed by updating the timeout to 8_000 within the describe block (this will be passed down to each test case), similar to what's done in #5044.
There are 2 errors that's related to vitest uncaught exceptions.
@timdeschryver I was looking for inputs on how to resolve this. Also, I tried to propose solutions in the comment (#5036 (comment)), that needs to be discussed since the solutions I have thought of might not be up to the team's liking. |
Oh sorry, I missed those comments while reviewing. |
Hey @timdeschryver I removed the "draft" status and changed the title to use the type |
2310a82 to
5a52eb9
Compare
|
@exequiel09 could you pull the latest main branch into your branch please? |
|
I just rebased the PR just moments ago based on the latest commits on the main branch. Did something change again or got merged? |
Thanks 👍 |
timdeschryver
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.
Thanks @exequiel09 , great work!
|
I see one of the issues exceeded 8000ms. That's why something failed. https://github.com/ngrx/platform/actions/runs/20258235781/job/58164627027?pr=5036#step:8:1558 |
36ea9a2 to
141f6a5
Compare
…previous test run
141f6a5 to
4bbea6a
Compare
timdeschryver
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.
This looks good to me.
The failing tests are caused by the "affected" tests that are run during the CI.








PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
The current behavior is that tests are running perfectly fine on
jest. This PR ensures that it runs smoothly onvitestwithout any errors.Closes #5018
What is the new behavior?
No new behavior. Just ensures to maintain parity when it runs using
vitestDoes this PR introduce a breaking change?
Other information