Skip to content

Improve error stack traces for async errors #542

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

Merged

Conversation

rahulchavan30
Copy link
Contributor

What:
Improve error stack traces for async errors

Why:
Improve error stack traces for async errors

How:
-changed the on timeout function

Checklist:

i have made changes suggested by KCD , please let me know if i need to do any more changes will be happy to do ... this is my first PR to Open source

@codesandbox-ci
Copy link

codesandbox-ci bot commented May 4, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 3b780fa:

Sandbox Source
nostalgic-glade-b3vkk Configuration

@codecov
Copy link

codecov bot commented May 4, 2020

Codecov Report

Merging #542 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #542   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           23        23           
  Lines          429       438    +9     
  Branches       103       105    +2     
=========================================
+ Hits           429       438    +9     
Impacted Files Coverage Δ
src/config.js 100.00% <ø> (ø)
src/wait-for.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7afa997...3b780fa. Read the comment docs.

@marcosvega91
Copy link
Member

marcosvega91 commented May 4, 2020

Hi @rahulchavan30 good job for your first contribution 🥳 . As I understand this behavior should be enabled based on a config parameter.
I think that you can add a new property in src/config.js and use it in wait-for.js.

Maybe It's good to add also a test case.

Last thing, to make the PR visibile in the issue you can add a reference like #539.

Regards
Marco

@rahulchavan30
Copy link
Contributor Author

hi macro ,
Thanks for the update ... if i understand clearly i need to do the following ,

  1. add a config parameter showOriginalStackTrace .
  2. by default showOriginalStackTrace will be false and when its false i need to return
    onDone(lastError || timedOutError, null) if its true i need to return
    let error = timedOutError
    if (lastError) {
    error = lastError
    const userStackTrace = timedOutError.stack.split('\n').slice(1).join('\n')
    error.stack = ${error.stack.split('\n')[0]}\n${userStackTrace}
    }
    onDone(error, null);
    i will mae these changes and update the PR if you agree ..

@marcosvega91
Copy link
Member

Hi @rahulchavan30 You're welcome 😸
Yes that's right.
You can put only this

const userStackTrace = timedOutError.stack.split('\n').slice(1).join('\n')
error.stack = ${error.stack.split('\n')[0]}\n${userStackTrace}

Around the new if. I think it's more readable

@rahulchavan30
Copy link
Contributor Author

Thanks macro will do the changes and submit ...

@rahulchavan30
Copy link
Contributor Author

Hi Macro , please bear with me , i have made the necessary changes but when i run npm test update
i get an error

i have changed the code to the following

function onTimeout() {
let error = timedOutError
let showOriginalStackTrace = getConfig().showOriginalStackTrace
if (lastError) {
error = lastError
if(showOriginalStackTrace){
const userStackTrace = timedOutError.stack.split('\n').slice(1).join('\n')
error.stack = ${error.stack.split('\n')[0]}\n${userStackTrace}
}
}
onDone(error, null)
}

but i get this code coverage error wait-for.js | 94.74 | 94.12 | 100 | 94.29 | 71-72
can you help me out with the test i need to satisfy the code coverage ?

@rahulchavan30
Copy link
Contributor Author

Quick update i added a new test case and was able to get the code coverage passing

kentcdodds
kentcdodds previously approved these changes May 4, 2020
Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Just one suggestion. Otherwise this looks awesome.

Copy link
Contributor Author

@rahulchavan30 rahulchavan30 left a comment

Choose a reason for hiding this comment

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

c

@kentcdodds kentcdodds force-pushed the pr/myFirstContribution branch from 79a2967 to 3bec76b Compare May 5, 2020 17:03
kentcdodds
kentcdodds previously approved these changes May 5, 2020
Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

This is looking good. I'm going to make a PR for docs and then merge if nobody has any objections to this.

I have verified that this works in my own project and the stack trace is WAY better. Thanks!

@kentcdodds kentcdodds merged commit d3287a1 into testing-library:master May 5, 2020
@kentcdodds
Copy link
Member

@all-contributors please add @rahulchavan30 for code and tests

@allcontributors
Copy link
Contributor

@kentcdodds

I've put up a pull request to add @rahulchavan30! 🎉

@kentcdodds
Copy link
Member

🎉 This PR is included in version 7.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants