-
-
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
Fix custom async matcher stack traces #7652
Conversation
Fixes a bug causing custom async matcher stack traces to be lost. The issue was caused by the JestAssertionError being created after the promise for the async matcher resolved - by which point the stack containing the correct stack trace (pointing to the line where matcher was called) has been discarded. The issue was fixed by passing an error that is initialized before the promise resolves.
Adds a test to verify the async custom matcher stack trace fix.
@SimenB I'm thinking that I'm going to need to update the removeInteralStackEntries function to strip out the async/generator lines. What do you think? You can see the full test failure on CircleCI. |
Thanks!
I think an easier solution is to just use |
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 great! Left an inline comment for a super minor thing, just fixing that (and making sure CI pass) and we can merge this 🙂
@SimenB Thanks for the prompt feedback! I implemented your suggestions for the e2e test. However, I'm not sure if I quite understand what you mean in regards to using
|
Uses the new wrap utility in e2e snapshot for custom matcher stack traces.
@SimenB When you have a chance, can you please take a look at my previous comment? |
Sorry!
The point is to verify that the stack is correct when a matcher returns a Promise. So |
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.
Thank you!
I was just about to message you to let you know that I updated the PR, but I see that you've already merged it. Thanks for the help Simen! |
|
||
return asyncResult | ||
.then(aResult => processResult(aResult)) | ||
.then(aResult => processResult(aResult, asyncError)) |
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'm having some issues with this code, 8 months on. I'm almost certainly not understanding correctly, but it seems to me....
-
asyncError
is always truthy here. Even though this is the normal codepath that any async matcher will follow -
In
processResult
we just test the truthiness ofasyncError
to decide whether an error was thrown. -
That means we end up
throw
ing a normal{ message, pass }
assertion result, which is not what is supposed to happen.
But.... what that suggests to me is that async custom would just be fully broken all the time, which.... I mean, I assume they're tested? So I assume I'm missing something?
But for me, they are in fact broken all the time.
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.
What issue are you having? The same one that I fixed in the description or a different one? It's been a while since I worked on this, so I don't remember what we're supposed to be throwing for point 3. Can you remind me what we're supposed to be throwing instead?
asyncError
will indeed always be defined if we're using async matchers. However, the only time we do something with the asyncError
is when the assertion fails because of this conditional here.
A little bit of context that I'm not sure you're aware of, but I think might be helpful. We always need to pass in an error to preserve the stack trace when we're working with async code. If we don't, then the stack trace is lost when we hit the next tick in the event loop. That's why the async error is always being initialized and passed in. It's just not used unless we hit an error state. Unless I'm misunderstanding what you're saying?
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. |
Summary
Fixes a bug causing custom async matcher stack traces to be lost. The issue was caused by the JestAssertionError being created after the promise for the async matcher resolved - by which point the stack containing the correct stack trace (pointing to the line where matcher was called) has been discarded. The issue was fixed by passing an error that is initialized before the promise resolves.
Test plan
I added an E2e test with a snapshot that ensures that the code fence and stack trace are displayed properly.
Before this fix, running
yarn jest
for a test that had a failing assertion from async matcher produced output like this:After the fix, the output looks like this:
Fixes #7066