-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
Support custom inline snapshot matchers #9278
Support custom inline snapshot matchers #9278
Conversation
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 is really nice, thanks! Didn't turn out as hacky as I feared
@@ -104,7 +105,9 @@ export default class SnapshotState { | |||
this._dirty = true; | |||
if (options.isInline) { | |||
const error = options.error || new Error(); | |||
const lines = getStackTraceLines(error.stack || ''); | |||
const lines = getStackTraceLines( | |||
removeLinesBeforeExternalMatcherTrap(error.stack || ''), |
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.
Should getTopFrame
do this work internally instead of the caller having to do it? It seems like the behaviour we'd always want, no?
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 think the stack trace containing __EXTERNAL_MATCHER_TRAP__
will already got removed by getStackTraceLines
?
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.
Should we instead modify getStackTraceLines
? Or both?
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.
hmm, good point. Is it enough to just change https://github.com/facebook/jest/blob/9ac2dcd55c0204960285498c590c1aa7860e6aa8/packages/jest-message-util/src/index.ts#L49 to not filter out this new capture line? If not I think the approach you have here makes sense
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 tried, it works fine in this context, but it would break in formatStackTrace
.
If we omit all stack traces before the trap, then the custom matcher won't correctly format the stack trace. It's covered here in an e2e test.
I believe that we have 2 options:
- Leave it be like this to only remove stack traces from the caller
- Add logic in
formatStackTrace
to un-skip the trap.
I think (1) is more resilient while (2) feel more error-prone. Wonder what's your opinion on this?
Co-Authored-By: Simen Bekkhus <sbekkhus91@gmail.com>
Co-Authored-By: Simen Bekkhus <sbekkhus91@gmail.com>
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.
Wonderful, thank you! Just updating the changelog and we should be good then
@SimenB Awesome! Thanks a lot! Should I also update the doc to include a section about custom inline snapshot matcher? |
A docs update would be wonderful, yes! |
/cc @azz |
Thanks so much for fixing this @kevin940726 🥇 |
This looks amazing, It's been on my watchlist for a year! |
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Fix #8181.
Summary
Supports custom inline snapshot matchers, currently using them isn't possible.
Test plan
How
The implementation is a bit of a hack, I'm not sure if it's suitable for production but it's the only better way I can think of for now.
To summarize the issue to those who is not familiar with how inline snapshots works, here is a simplified breakdown.
snapshots
stack trace is generated upon runtime call, by callingnew Error().stack
saveSnapshotsForFile
function, it takes thesnapshots
coming from the previous step and thesourceFilePath
createInsertionParser
createFormattingParser
There's a tweet mentioned in the original issue stating that we should need to be able to figure out that the call is coming from a custom matcher and skip those frames in order for this to work. So this is exactly what I've done. I create a no-op trap for every external matcher, wrapping them with a function call so that it would show up in the stack trace.
The next step is to modify the logic where we get the top frame. We remove all stack trace lines before the trap function we just made (if it exists), so that when we try to get the top frame it won't get the native
toMatchInlineSnapshot
but the customtoMatchCustomInlineSnapshot
.We also have to update the babel traverse logic in
createFormattingParser
, or else it won't correctly indent the snapshots. Currently it only looks for matcher namedtoMatchInlineSnapshot
, I modify it to record all the matcher names we've seen in the stack traces, and check them one by one in the parser.I added an e2e test case to demonstrate the feature, please tell me if you have any other idea for tests, or edge cases I'm missing.
I'm also aware of that #7792 is near to be finished, this PR takes it into account and should be able to work just fine when it landed.
I'll update the CHANGELOG file once if you think it's a good idea :)