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

meeber · 2016-12-11T22:28:43Z

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.

lucasfcosta · 2016-12-18T16:51:00Z

lib/chai/utils/addChainableMethod.js

@@ -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) {


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.

lucasfcosta · 2016-12-18T16:53:52Z

lib/chai/utils/addChainableMethod.js

+          // 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);


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).

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.

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.

shvaikalesh · 2016-12-18T16:59:32Z

lib/chai/utils/proxify.js

@@ -64,7 +64,7 @@ module.exports = function proxify (obj, nonChainableMethodName) {
        }
      }

-      return target[property];
+      return Reflect.get(target, property);


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.

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.

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

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 👍

lucasfcosta · 2016-12-18T17:55:38Z

lib/chai/utils/overwriteMethod.js

+    // 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);


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?

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 · 2016-12-18T18:04:50Z

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 · 2016-12-18T23:58:01Z

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 · 2016-12-19T10:10:08Z

@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 · 2017-01-02T22:38:53Z

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 · 2017-01-02T22:43:22Z

Awesome changes. Makes code so much clearer. LGTM.

keithamus · 2017-01-03T10:06:33Z

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

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.

The proper way to perform an operation's original behavior from within a proxy trap is by using `Reflect`.

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.

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.

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.

Only the `expect` interface was being tested for correct stack traces. This commit adds identical tests for the `should` interface.

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.

- 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.

meeber · 2017-01-03T21:28:45Z

@keithamus Rebased and resolved conflicts!

meeber changed the title ~~Fix stack~~ Remove proxy frames from stack traces and improve docs/tests Dec 11, 2016

lucasfcosta reviewed Dec 18, 2016

View reviewed changes

shvaikalesh reviewed Dec 18, 2016

View reviewed changes

lucasfcosta reviewed Dec 18, 2016

View reviewed changes

meeber mentioned this pull request Dec 19, 2016

Plugin issues with Chai v4 #890

Closed

meeber added 8 commits January 3, 2017 16:21

refactor(proxify): use Reflect for property access

78f1808

The proper way to perform an operation's original behavior from within a proxy trap is by using `Reflect`.

refactor(utils): improve function names

96f958c

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.

test(includeStack): improve tests

8fa4f78

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.

test(includeStack): add should interface

bf71196

Only the `expect` interface was being tested for correct stack traces. This commit adds identical tests for the `should` interface.

refactor(Assertion): small edit and improve docs

0c09836

- 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.

meeber force-pushed the fix-stack branch from 0fcb9b9 to 0c09836 Compare January 3, 2017 21:24

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

lucasfcosta mentioned this pull request May 10, 2017

4.x.x Complete Migration Guide #781

Closed

meeber deleted the fix-stack branch August 6, 2017 13:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

meeber commented Dec 11, 2016

lucasfcosta Dec 18, 2016

lucasfcosta Dec 18, 2016

meeber Dec 18, 2016

lucasfcosta Dec 18, 2016

shvaikalesh Dec 18, 2016

lucasfcosta Dec 18, 2016

meeber Dec 18, 2016

shvaikalesh Dec 18, 2016 •

edited

lucasfcosta Dec 18, 2016

meeber Dec 18, 2016

lucasfcosta commented Dec 18, 2016 •

edited

meeber commented Dec 18, 2016

lucasfcosta commented Dec 19, 2016

lucasfcosta commented Jan 2, 2017 •

edited

shvaikalesh commented Jan 2, 2017

keithamus commented Jan 3, 2017

meeber commented Jan 3, 2017

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

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

Conversation

meeber commented Dec 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shvaikalesh Dec 18, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucasfcosta commented Dec 18, 2016 • edited

meeber commented Dec 18, 2016

lucasfcosta commented Dec 19, 2016

lucasfcosta commented Jan 2, 2017 • edited

shvaikalesh commented Jan 2, 2017

keithamus commented Jan 3, 2017

meeber commented Jan 3, 2017

shvaikalesh Dec 18, 2016 •

edited

lucasfcosta commented Dec 18, 2016 •

edited

lucasfcosta commented Jan 2, 2017 •

edited