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

Attach a stack trace to the it() block promise error generated if pre… #302

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Attach a stack trace to the it() block promise error generated if pre… #302

wants to merge 1 commit into from

Conversation

alexjeffburke
Copy link
Member

…sent.

No idea why this never opened - but this is the change so far. If previously asked for any suggestions about the best way of testing this :)

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 97.902% when pulling 329caa6 on alexjeffburke:add-trace-on-promise-notify into 45d7e01 on unexpectedjs:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 97.902% when pulling 005f2ef on alexjeffburke:add-trace-on-promise-notify into 45d7e01 on unexpectedjs:master.

@@ -34,6 +41,9 @@ function registerAfterEachHook() {
displayName = currentSpec.fullName;
}
error = new Error(displayName + ': You have created a promise that was not returned from the it block');
if (pendingPromise.trace && pendingPromise.trace.stack) {
Copy link
Member

Choose a reason for hiding this comment

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

Isn't it pendingPromise._trace?

@papandreou
Copy link
Member

@alexjeffburke, are you still working on this? Did my feedback make any sense? :)

@alexjeffburke
Copy link
Member Author

@papandreou it completely did and thought I'd mention I've brought this back to life :)

Working on a small patch the records any pending promises in an array - but first, actually had a little problem with _trace. Struggled with the env incarnation required to get the trace - I seemed to end up with. promise that didn't have a top level _trace but one down the stack did. What are the latest env args - I was trying UNEXPECTED_FULL_TRACE and BLUEBIRD_DEBUG.

@alexjeffburke
Copy link
Member Author

Oh and re stack trimming, you'll have to point me at where/how that's done as I'm not familiar with it.

@coveralls
Copy link

coveralls commented May 29, 2016

Coverage Status

Coverage decreased (-0.03%) to 97.911% when pulling 9931c3c on alexjeffburke:add-trace-on-promise-notify into fc1ce8a on unexpectedjs:master.

@papandreou
Copy link
Member

papandreou commented May 30, 2016

What are the latest env args

UNEXPECTED_FULL_TRACE should be the only one relevant to users. It implies BLUEBIRD_DEBUG so you get the full async traces based on Bluebird's _trace captured at promise creation time.

What changed with #300 was that even when you don't specify UNEXPECTED_FULL_TRACE, Unexpected will attach a stack trace to all promises that it finds cannot be "oathbroken", ie. decidable synchronously, after evaluating an assertion. (Other promises that float around won't get _trace, though, so maybe that's what you ran into). In this case the _trace won't be completely correct, but the inaccuracy is cancelled out by the stack trimming code, which removes the Unexpected-internal stack frames anyway, so the end result is the same as with BLUEBIRD_DEBUG turned on, only cheaper :)

In your case I think UNEXPECTED_FULL_TRACE (or at least BLUEBIRD_DEBUG) is a necessity to be able to get to the bottom of this kind of error, though, as you really want to have _trace available on every promise, not just the ones that Unexpected decides to attach _trace to.

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