-
Notifications
You must be signed in to change notification settings - Fork 10
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
Migrate from Mocha
to Jest
#706
Conversation
10c4af1
to
ec446c2
Compare
mySnapshotTest
to mocha-chai-jest-snapshot
mySnapshotTest
to mocha-chai-jest-snapshot
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.
Sorry for not getting back to you earlier, now that I look at it, it's pretty hard to review changes to snapshots when they are all packed into one file with no syntax highlight... I would like to have them in separate files with proper extensions (to allow syntax highlight). I did a little investigation and didn't find an easy way to do this.
There's a jest plugin that can help us: https://github.com/igor-dv/jest-specific-snapshot
However, I find Jest to be significantly slower than Vitest, so if we would migrate from mocha/chai - we probably should migrate to Vitest. And I think that jest-specific-snapshot might work with Vitest since it has a lot of interoperability with Jest, but that needs to be confirmed.
TL;DR, let's investigate how we can keep snapshot files as they currently are, maybe by using Vitest with jest-specific-snapshot plugin
I appreciate your efforts, and it's unfortunate that I didn't see this through at first! Please feel free to post your findings/design here first before taking the time to code, thank you!
I would like to try separate snapshots in separate files first. If everything works fine, maybe migrate Jest to Vitest. |
It seems that even if we separated snapshots into separate files, |
I see... I'd probably suggest creating a separate PR for #688 |
I've updated title and description for #705 to reflect our findings |
Jest Snapshot Language Support provides syntax highlighting for your Jest snapshots. |
I agree with you. It might be great if we first split snapshot files first, then go on to migrate to vest or syntax highlighting. |
There is one problem with using |
Cool :)
Thanks! |
I don't think it's worth it, we probably can just migrate these tests to jest instead of using mocha-chai-jest-snapshot. mocha-chai-jest-snapshot is kinda workaround to add snapshot testing for mocha/chai... |
I'll see what I can do. |
mySnapshotTest
to jest-specific-snapshots
Playing around, I found out that jest with typescript did not play well with ES modules. |
Yeah, I've had a lot of trouble setting up jest with ES Modules, but sometimes I was lucky... |
ec446c2
to
24d729b
Compare
I migrated the test to |
5a879c9
to
aa57209
Compare
mySnapshotTest
to jest-specific-snapshots
mySnapshotTest
to Jest
+ jest-specific-snapshots
e7fdc1a
to
95a911d
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.
Amazing job! I really appreciate your efforts. I've added some comments that I'd like to discuss/address before merging. Thank you!
95a911d
to
b6ec58f
Compare
All requested changes are resolved. |
ee812ec
to
062854a
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.
I do appreciate the investigation and effort that went into making sure that snapshots are in individual files, I've never dealt with this before and this is for sure very interesting knowledge about jest's inner workings. Now I can see that this requires much more complication than I originally anticipated. Custom resolver, for example, is very advanced, requires a deep understanding of jest inner-workings, and in case jest breaks backward compatibility in future updates - it'll be very challenging to work with. And we can't use TS yet which isn't good for code quality. And we probably can't really test this complex logic.
Now having this full picture thanks to your research, I think that having one huge file with snapshots isn't that big of a problem. The default approach is quite good for JSON and TS files: it doesn't add extra escaping for double quotes and jest extension in VS Code lets you fold it and does a pretty good job at highlighting. Markdown README will be a challenge as it can't be folded and it takes up many pages so it might be challenging to understand the context, like from which test this part of README is. But that shouldn't be a huge problem.
I'm pretty happy with all the changes in tests. Do you think that it'd make sense to bring all the snapshots back into one file?
Thank you!
Agreed! Thanks for your efforts. |
7560657
to
38146bd
Compare
38146bd
to
4f11686
Compare
mySnapshotTest
to Jest
+ jest-specific-snapshots
mySnapshotTest
to Jest
mySnapshotTest
to Jest
Mocha
to Jest
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 have generated all types using the master branch, committed, then generated using your branch and compared the difference, there was none, except for one package where revision got updated. So I'm quite certain this won't break anything and will be very happy to merge it as soon as you fix that little timeout issue. Thank you so much!
'v10', | ||
'v11', | ||
]); | ||
}, 0); // performs requests to the actual server |
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.
Turns out setting timeout to 0 doesn't disable timeouts in jest, please set it to 30000 instead, should give it enough time. It took just ~2 seconds for CI so that's why it didn't fail, but it tool just a little bit over the default 5s timeout on my computer (probably Australian internet to blame here)
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.
Fixed in a613f11
Actually, I think it's easier and quicker to just merge it and fix the test myself |
Thank you again for an amazing job! |
Closes #705.