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 traces in .throws assertions #2021

Merged
merged 3 commits into from
Jan 27, 2019
Merged

Conversation

dflupu
Copy link
Contributor

@dflupu dflupu commented Jan 18, 2019

Fixes #1372

Copy link
Member

@novemberborn novemberborn left a comment

Choose a reason for hiding this comment

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

The code's great!

The code snippet is no longer showing the assertion though. It shows the error being constructed. I think we might have to track both the assertion and the error stack and use them appropriately.

It's getting late here so let me know if you want some pointers. You seem to be pretty good at finding your way around the codebase though!

@dflupu
Copy link
Contributor Author

dflupu commented Jan 20, 2019

My thought process was along the lines of: whenever an error is thrown during an ava test, the stack trace always shows where the error was constructed, with the assertion being at the bottom of the trace. If an error is thrown inside a .notThrows, I think that it is fair to treat it as any other unexpected error. Same with errors of different types inside a .throws.

Do you disagree? Is there any situation in which both traces would be required?

@novemberborn
Copy link
Member

The stack traces shown are fine, but we also show where the assertion took place. Compare:

screen shot 2019-01-21 at 10 11 41

screen shot 2019-01-21 at 10 11 17

I think we should show the code snippet for t.throws() not new Error().

@dflupu
Copy link
Contributor Author

dflupu commented Jan 22, 2019

I've updated the PR. The output should be closer to what you had in mind.
Let me know if you see a cleaner way of implementing this. Note that the trace created in the AssertionError constructor doesn't always contain the frame where the assertion was made.

Copy link
Member

@novemberborn novemberborn left a comment

Choose a reason for hiding this comment

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

My thinking was we'd store two different "stack" properties. One to determine the source location, the other belonging to thrown errors / rejection reasons. The stack extracted in trySerializeError would first use the latter, and if missing fall back to the former. I think that'll work out better than setting a boolean flag.

lib/serialize-error.js Outdated Show resolved Hide resolved
@novemberborn novemberborn merged commit ad087f2 into avajs:master Jan 27, 2019
@novemberborn
Copy link
Member

Thanks @dflupu!

@sindresorhus
Copy link
Member

@dflupu Thank you!

You need to submit the PR URL to IssueHunt to claim the bounty ;)

@dflupu
Copy link
Contributor Author

dflupu commented Jan 27, 2019

@sindresorhus No worries, I'll be sure to submit it. I prefer to wait until the PR is accepted/merged because it feels like bad manners to submit before. Might be mostly because of the useless bot post when you do submit.

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

Successfully merging this pull request may close these issues.

3 participants