Skip to content
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: don't assume stack is always a string #10697

Merged
merged 17 commits into from Oct 27, 2020
Merged

Conversation

snitin315
Copy link
Contributor

Summary

Fixes #10681

Test plan

Before

Screenshot at 2020-10-25 11-16-08

After

Screenshot at 2020-10-25 11-16-54

@snitin315 snitin315 changed the title fix(jest-jasmine2): handle case when stack is not string fix(jest-jasmine2): don't assume stack to be a string always Oct 25, 2020
@jeysal jeysal requested a review from SimenB October 25, 2020 18:44
@SimenB
Copy link
Member

SimenB commented Oct 25, 2020

Can you add an integration test which does throw {stack: 42} or something? I think we can make improvements to the stack, but seeing how it looks in both jasmine and circus first would be nice 🙂 Since what's thrown is not an error we'll have to point to the test definition, but that's better than nothing.

Adding a case here and adding some assertions somewhere around here should work out: https://github.com/facebook/jest/blob/2dafb09d51584d3785f3280f569784ec4334b5d8/e2e/__tests__/failures.test.ts#L30-L54

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@snitin315
Copy link
Contributor Author

I have added an integration test 💯 .

@SimenB
Copy link
Member

SimenB commented Oct 26, 2020

@snitin315 thanks! I pushed up one more failing test I'd like to se a better error for. I'll be happy to give you a hand if you get stuck attempting to fix it 👍

@snitin315
Copy link
Contributor Author

Looking into it 👍🏻

@SimenB
Copy link
Member

SimenB commented Oct 26, 2020

Thanks! That snapshot should look like the snapshot of Object thrown during test

@snitin315
Copy link
Contributor Author

Yeah, I got it.

@snitin315
Copy link
Contributor Author

snitin315 commented Oct 26, 2020

@SimenB it's like this now, Not sure why CI is failing for Node 10

Screenshot at 2020-10-26 18-40-36

@SimenB
Copy link
Member

SimenB commented Oct 26, 2020

Looks good! Not sure about node 10, but let's look into that after fixing the test for jest-circus. You can run that locally via JEST_CIRCUS=1 yarn jest failures.

@SimenB

This comment has been minimized.

@SimenB
Copy link
Member

SimenB commented Oct 27, 2020

@snitin315 I pushed the fix for Node 10. We just need to fix the test for Jest Circus and this should be ready to land 👍

@snitin315
Copy link
Contributor Author

@snitin315 I pushed the fix for Node 10. We just need to fix the test for Jest Circus and this should be ready to land

@SimenB Thanks!, I am working on Jest Circus I will push soon.

This was referenced Mar 12, 2021
@github-actions
Copy link

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.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Throwing a non-Error object with a property called "stack" causes test to hang
3 participants