Remove proxy frames from stack traces and improve docs/tests #884

Merged
merged 8 commits into from Jan 3, 2017

Projects

None yet

4 participants

@meeber
Contributor
meeber commented Dec 11, 2016

This PR is intentionally split into a number of commits, each one of which has a description within the commit message.

Highlights of this PR:

  • Remove Chai's internal proxy frames from stack traces of failed property assertions (note: they were already correctly removed from the stack traces of failed method and chainable method assertions)
  • Improve tests and inline documentation related to Chai's stack trace wizardry
  • Remove dead code related to Chai's stack trace wizardry

Note that this doesn't address #878 regarding extra frames in stack traces when using the assert interface.

@meeber meeber changed the title from Fix stack to Remove proxy frames from stack traces and improve docs/tests Dec 11, 2016
@@ -58,7 +58,7 @@ var call = Function.prototype.call,
* @api public
*/
-module.exports = function (ctx, name, method, chainingBehavior) {
+module.exports = function addChainableMethod(ctx, name, method, chainingBehavior) {
@lucasfcosta
lucasfcosta Dec 18, 2016 Member

I'm a big fan of naming every function even if they're being assigned to a variable.
Bonus style points for this excellent practice.

+ // this assertion has been overwritten since overwriting a chainable
+ // method merely replaces the saved methods in `ctx.__methods` instead
+ // of completely replacing the overwritten assertion.
+ flag(this, 'ssfi', chainableMethodWrapper);
@lucasfcosta
lucasfcosta Dec 18, 2016 Member

Just out of curiosity, I've always wondered what does ssfi means, I'm not sure this is a good name since it's kind of "cryptic" (or maybe it just feels like it because I don't know what it means).

@meeber
meeber Dec 18, 2016 Contributor

According to this comment, it's "start stack function indicator". Also mentioned on http://chaijs.com/guide/plugins/. I don't feel strongly one way or another about renaming it.

@lucasfcosta
lucasfcosta Dec 18, 2016 Member

Hmm, since it's pretty specific I think there's nothing we can do about it.
Let's leave it that way.

Maybe a comment indicating what it means somewhere would also be nice, especially if we added a link to that comment, which was really useful for me when understanding how the whole stack trace manipulation works.

lib/chai/utils/proxify.js
@@ -64,7 +64,7 @@ module.exports = function proxify (obj, nonChainableMethodName) {
}
}
- return target[property];
+ return Reflect.get(target, property);
@shvaikalesh
shvaikalesh Dec 18, 2016 Member

FYI, Reflect.get is not different from regular computed access (unless you are using receiver parameter).
It also calls [[Get]] and ToPropertyKey. The only thing they are different is that Reflect methods throw on non-object target (however target here will always be an object). Same goes for Reflect.has above: in does the same thing. We can simplify checks and code by removing Reflect usages.

@lucasfcosta
lucasfcosta Dec 18, 2016 Member

I think that by using Reflect we get more semantic code, for me it just feels like a good practice and a more functional way to invoke the language's internal operations.

However, since it won't have any impact and it's just a matter of personal preference I'd be happy with either options.

@meeber
meeber Dec 18, 2016 Contributor

I don't have much of a preference either way. Anyone have a strong preference here?

@shvaikalesh
shvaikalesh Dec 18, 2016 edited Member

I have an idea on how to improve proxies performance by much, utilizing third parameter to Reflect.get, so I think we should leave it like this.

PS: much pleasure to review such careful commits 👍

+ // The `keep_ssfi` flag is set so that if this assertion ends up calling
+ // the overwritten assertion, then the overwritten assertion doesn't attempt
+ // to use itself as the starting point for removing implementation frames
+ // from the stack trace of a failed assertion.
flag(this, 'keep_ssfi', true);
@lucasfcosta
lucasfcosta Dec 18, 2016 Member

Let me see if I've got this right, I'm not sure if I fully understand what this does.

So, we use the SSFI flag to store the function which should indicate where the real stack trace will start in order to remove internal implementation details, right?

Whenever an assertion gets overwritten we must turn the keep_ssfi flag to true before calling the old assertion (_super) in order to avoid that assertion being used as the start of the stack trace. Right?

@meeber
meeber Dec 18, 2016 Contributor

Right, so the function stored in the ssfi flag serves as the second parameter for Error.captureStackTrace, meaning that the function itself, and all functions called after it (which are just the internals of Chai and plugins), will be removed from the stack trace if an error is thrown. In the case of property assertions, this function is actually the proxy getter, which is why this PR was needed. But in the case of method assertions, the proxy getter function completely returns before the method assertion's function is invoked, so the method assertion's function needs to be stored in the ssfi flag instead of the proxy getter.

But what about all functions that came before the function stored in the ssfi flag? Usually, the first function that comes before it is the function that the user passed as the second argument to a Mocha it invocation. This function contains the user's assertion that failed, so it's good that it's included in the stack trace. But there's a bunch of other functions still in the stack before this one: Mocha's internals. It turns out that these don't appear in the stack traces either because Mocha does its own manual filtering of the stack trace to get rid of their internals. It's important to remember this when troubleshooting an issue with Mocha.

(Note that because the assert interface acts as a wrapper around Chai assertions, there's currently an extra function call between the user's assertion and the one stored in the ssfi flag, so it shows in the stack trace. That's what #878 is about. It's fixable but will take some work on the assert interface.)

As for your final question, yes, the keep_ssfi flag is needed so that the overwriting function remains stored in the ssfi flag even after it proceeds to call the original function that it overwrote. The first function after the user's assertion is the one that needs to be in the ssfi flag in order for this to all work correctly.

@lucasfcosta
Member
lucasfcosta commented Dec 18, 2016 edited

This LGTM!

I wasn't really aware of how this whole Stack Trace manipulation worked, so I did a bit of research and I found it to be very interesting.

Just in case anyone wants to read more about it, take a look at the AssertionError constructor and the Error.captureStackTrace docs.

I made a few comments on the source just to make sure I got it right and added a little consideration to improve the readability of the code regarding the ssfi flag we're using, just to make it more clear to new contributors. Adding the suggested comment, however, is not really mandatory, but it would be good IMO.

Awesome job @meeber! 😄

@meeber
Contributor
meeber commented Dec 18, 2016

Pushed another commit:

  • Rename third parameter of Assertion constructor from stack to
    ssfi for consistency's sake.
  • Add documentation to Assertion constructor explaining what the object,
    message, and ssfi flags are for.
@lucasfcosta
Member

@meeber awesome job! Chai's code is becoming even more easy and pleasurable to read than it was before with all these useful and well written comments.

I'm in love with this codebase ❤️

@lucasfcosta
Member
lucasfcosta commented Jan 2, 2017 edited

Hi friends, sorry for pinging everyone right after holidays, but I've seen we have many open Pull Requests so I thought it would be a good idea for us to start reviewing and approving in order to avoid accumulating too much work.

Also, can anyone make sure LGTM is working? I remember talking about abandoning it in favor of github's review system. What do you think?

cc @keithamus @shvaikalesh @vieiralucas

@shvaikalesh
Member

Awesome changes. Makes code so much clearer. LGTM.

@keithamus
Member

@meeber think you could resolve the conflicts for this, so we can merge. All LGTM 😄

meeber added some commits Dec 10, 2016
@meeber meeber refactor(proxify): move feature detection to util
Currently, only one module needs to detect if Chai's proxy protection
is enabled. However, upcoming changes will involve performing this
detection in other modules as well. This commit moves the detection
logic to its own utility module for easy reuse.
5a1e676
@meeber meeber refactor(proxify): use Reflect for property access
The proper way to perform an operation's original behavior from
within a proxy trap is by using `Reflect`.
78f1808
@meeber meeber refactor(utils): improve function names
Many of the utility functions had slightly misleading names or no
names at all. This commit renames the functions with misleading names
and adds names to functions that were missing one.
96f958c
@meeber meeber test(includeStack): improve tests
Only a couple of types of assertions were being tested for correct
stack traces. This commit cleans up the existing tests and adds tests
for the missing assertion types.
8fa4f78
@meeber meeber fix: remove proxy frames from stack traces
Proxy-related implementation frames were showing up in the stack
traces for failed property assertions. This commit removes them by
setting the proxy getter (instead of the property getter) as the
starting point to remove all implementation frames.
b88e536
@meeber meeber test(includeStack): add `should` interface
Only the `expect` interface was being tested for correct stack traces.
This commit adds identical tests for the `should` interface.
bf71196
@meeber meeber refactor(ssfi): remove dead code and update docs
There was some dead code leftover from before `includeStack` was made
into a config value (as opposed to existing as a property on the
Assertion object). This commit removes that dead code, and adds inline
documentation for the remaining stack-related code.
82ca613
@meeber meeber refactor(Assertion): small edit and improve docs
- Rename third parameter of Assertion constructor from `stack` to
  `ssfi` for consistency's sake.
- Add documentation to Assertion constructor explaining what the `object`,
  `message`, and `ssfi` flags are for.
0c09836
@meeber
Contributor
meeber commented Jan 3, 2017

@keithamus Rebased and resolved conflicts!

@keithamus keithamus merged commit 877dde8 into chaijs:master Jan 3, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment