-
Notifications
You must be signed in to change notification settings - Fork 46
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
Setup snapshot testing for editoast_derive using insta #9938
Conversation
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## dev #9938 +/- ##
===========================================
+ Coverage 38.21% 79.88% +41.66%
===========================================
Files 992 1054 +62
Lines 92158 105597 +13439
Branches 1186 759 -427
===========================================
+ Hits 35221 84353 +49132
+ Misses 56486 21202 -35284
+ Partials 451 42 -409
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
e2407b9
to
3e0bf44
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.
We have to "accept" diffs manually (either by renaming the file or by using cargo insta, their official interactive snapshot management CLI tool)
I actually don't see this as a drawback. Sure, it's more work for the developer, but we actually always do that even for "normal" unit tests: a expected output changed, we need to review it manually and validate that the new output is valid. So it's actually a good thing. Maybe you meant it's more cumbersome to do that operation of validating a different test output?
In general, this PR is going to bring much more to the experience of dealing with macros...
editoast/editoast_derive/src/snapshots/editoast_derive__error__tests__construction.snap
Show resolved
Hide resolved
editoast/editoast_derive/src/snapshots/editoast_derive__model__tests__construction.snap
Show resolved
Hide resolved
Great idea! Thanks for the PR. |
Thanks for the feedback! I've added |
We should probably add some documentation about working with insta, either in editoast README or in a new one for editoast_derive |
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 for digging up about the "macro into function" proposition, even if it ended up not satisfying. And thanks for the additional documentation. Great improvement for editoast_derive
in general! ❤️
Allows to easily test macro expansion, track changes, improves debuggability and ease review. HashMaps have been eliminated from `Model`'s internals to have a deterministic expansion. Otherwise snapshots would be flaky. Signed-off-by: Leo Valais <[email protected]>
Macros confuses `prettyplease` and we want snapshots as clean as possible. Signed-off-by: Leo Valais <[email protected]>
Not only does this reduces the complexity of batch operation expansion, it also allows `prettyplease` to format the code chunk in snapshots. Signed-off-by: Leo Valais <[email protected]>
closes #7467
Setup snapshot testing library
insta
in order to unit-test our derive macros. This approach has several benefits:Since we may soon have some extensive changes coming to our macros with the new error system, having tests and ease the review process are essential.
A few drawbacks I can think of:
HashMap
s are allowedcargo insta
, their official interactive snapshot management CLI tool)Important
Review this PR commit by commit to see how the snapshots evolve when the macro output changes.