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

Remove implementation frames from stack trace #922

Merged
merged 5 commits into from Apr 4, 2017

Conversation

Projects
None yet
4 participants
@meeber
Contributor

meeber commented Jan 29, 2017

Note to self: Rebase after #899 is merged and update to turn Error into an AssertionError that properly retains ssfi. A follow up PR will properly retain the user's custom message here and elsewhere.

This PR is split into five commits:

  1. Expand the role of the keep_ssfi flag so that it's easier to tell Chai not to overwrite an existing ssfi when it shouldn't, such as when an assertion is being called from within another assertion, or when an assertion is wrapped in a function such as every assertion on the assert interface.

  2. Fix all issues on expect/should interfaces that were causing implementation frames to leak through. Also improve the internal Chai error test helper to validate stack traces.

  3. Fix all issues on assert interface that were causing implementation frames to leak through. Further improve the internal Chai error test helper to validate stack traces.

  4. Rename keep_ssfi flag to lockSsfi.

  5. Clean up excessive test comments.

Closes #878
Closes #904

@meeber

This comment has been minimized.

Show comment
Hide comment
@meeber
Contributor

meeber commented Feb 12, 2017

@lucasfcosta

Awesome job! Very clean implementation.

I also liked the tests, I think they cover all the edge cases and general functioning of this change.

I think, however, that some minor details in docstrings and error messages should be changed.

I left a few other comments throughout the code with suggestions and my thoughts on this matter.

I also tried to find any nested assertion invocations that did not set the keep_ssfi flag but I couldn't find any.

Thanks for your work @meeber!

Show outdated Hide outdated lib/chai/assertion.js
* @api private
*/
function Assertion (obj, msg, ssfi) {
function Assertion (obj, msg, ssfi, keep_ssfi) {

This comment has been minimized.

@lucasfcosta

lucasfcosta Feb 12, 2017

Member

Maybe freezeSsfi would look better. It's also camel-case instead of snake-case.

I know all the flags are called keep_ssfi, but maybe we could rename them to freeze_ssfi.

Anyway, this is just a suggestion. Please let me know what you think about that.

@lucasfcosta

lucasfcosta Feb 12, 2017

Member

Maybe freezeSsfi would look better. It's also camel-case instead of snake-case.

I know all the flags are called keep_ssfi, but maybe we could rename them to freeze_ssfi.

Anyway, this is just a suggestion. Please let me know what you think about that.

// Setting the `ssfi` flag to `chainableMethodWrapper` causes this
// function to be the starting point for removing implementation
// frames from the stack trace of a failed assertion.
//

This comment has been minimized.

@lucasfcosta

lucasfcosta Feb 12, 2017

Member

Let me see if I get this:

If we're dealing with a "single-level" assertion it is okay to remove these frames from the stack since they're irrelevant due to the fact that they're just calling the assertion's inner routine, but if we're dealing with "nested" assertions (assertions that call other assertions) we must not remove this because we need to have the frames from before calling this function (which is the topmost assertion's code).

Right?

@lucasfcosta

lucasfcosta Feb 12, 2017

Member

Let me see if I get this:

If we're dealing with a "single-level" assertion it is okay to remove these frames from the stack since they're irrelevant due to the fact that they're just calling the assertion's inner routine, but if we're dealing with "nested" assertions (assertions that call other assertions) we must not remove this because we need to have the frames from before calling this function (which is the topmost assertion's code).

Right?

This comment has been minimized.

@meeber

meeber Feb 12, 2017

Contributor

The ssfi flag should always contain a reference to the lowest internal Chai frame in the stack, because that frame and all frames above it will be filtered out. In other words, the very first Chai internal function that gets hit in an assertion should set the ssfi flag, and any functions after that shouldn't mess with the ssfi flag.

For example:

expect({foo: 1}).to.include({foo: 1});
  • When expect({foo: 1}) gets called, ssfi is undefined.
  • When .to gets called, ssfi gets set to either the proxyGetter or propertyGetter function for .to, depending on whether on not proxies are supported and enabled. Note that .to doesn't call any more internal Chai functions, so we're done with this step. But if it did happen to call more internal Chai functions, then it'd freeze it's ssfi before calling them since we want to keep ssfi pointing at the first internal Chai function that gets hit when .to is invoked.
  • When .include gets called, ssfi gets set to the chainableMethodWrapper function for .include. Now, .include calls the .property assertion internally, so we freeze ssfi before calling .property since we want to keep ssfi pointing at the first internal Chai function that gets hit when .include is invoked.
@meeber

meeber Feb 12, 2017

Contributor

The ssfi flag should always contain a reference to the lowest internal Chai frame in the stack, because that frame and all frames above it will be filtered out. In other words, the very first Chai internal function that gets hit in an assertion should set the ssfi flag, and any functions after that shouldn't mess with the ssfi flag.

For example:

expect({foo: 1}).to.include({foo: 1});
  • When expect({foo: 1}) gets called, ssfi is undefined.
  • When .to gets called, ssfi gets set to either the proxyGetter or propertyGetter function for .to, depending on whether on not proxies are supported and enabled. Note that .to doesn't call any more internal Chai functions, so we're done with this step. But if it did happen to call more internal Chai functions, then it'd freeze it's ssfi before calling them since we want to keep ssfi pointing at the first internal Chai function that gets hit when .to is invoked.
  • When .include gets called, ssfi gets set to the chainableMethodWrapper function for .include. Now, .include calls the .property assertion internally, so we freeze ssfi before calling .property since we want to keep ssfi pointing at the first internal Chai function that gets hit when .include is invoked.
Show outdated Hide outdated lib/chai/utils/expectTypes.js
@@ -13,6 +13,8 @@
*
* @param {Mixed} obj constructed Assertion
* @param {Array} type A list of allowed types for this assertion
* @param {fn} ssfi starting point for removing implementation frames from stack

This comment has been minimized.

@lucasfcosta

lucasfcosta Feb 12, 2017

Member

Isn't it @param {Function} instead of @param {fn}?

@lucasfcosta

lucasfcosta Feb 12, 2017

Member

Isn't it @param {Function} instead of @param {fn}?

Show outdated Hide outdated test/globalErr.js
throw Error();
});
} catch (e) {
throw Error("This should never happen");

This comment has been minimized.

@lucasfcosta

lucasfcosta Feb 12, 2017

Member

Perhaps these error messages here could be a little more descriptive to ease bug detection/interpretation.

@lucasfcosta

lucasfcosta Feb 12, 2017

Member

Perhaps these error messages here could be a little more descriptive to ease bug detection/interpretation.

Show outdated Hide outdated test/globalErr.js
throw Error();
}, undefined, true);
} catch (e) {
throw Error("This should never happen");

This comment has been minimized.

@lucasfcosta

lucasfcosta Feb 12, 2017

Member

The same goes for the Errors below.

@lucasfcosta

lucasfcosta Feb 12, 2017

Member

The same goes for the Errors below.

@meeber

This comment has been minimized.

Show comment
Hide comment
@meeber

meeber Feb 12, 2017

Contributor

Pushed a new version. Changes:

  • Added new commit that renames keep_ssfi flag to lockSsfi. (Avoided "freeze" to avoid confusion with JavaScript's concept of a frozen object.)
  • Modified existing commits to make the globalErr helper function throw a more descriptive error message when implementation frames are detected in the stack trace of a failed assertion.
  • Modified existing commits to add comments that make it more clear what exactly tests of the globalErr helper function are doing.
Contributor

meeber commented Feb 12, 2017

Pushed a new version. Changes:

  • Added new commit that renames keep_ssfi flag to lockSsfi. (Avoided "freeze" to avoid confusion with JavaScript's concept of a frozen object.)
  • Modified existing commits to make the globalErr helper function throw a more descriptive error message when implementation frames are detected in the stack trace of a failed assertion.
  • Modified existing commits to add comments that make it more clear what exactly tests of the globalErr helper function are doing.
@lucasfcosta

Awesome job @meeber!

The variable name change was much better than what I suggested, lock is really the right word for this. Very well thought.

Also, by reading the code I think that we have too many comments explaining simple tests, especially because some of them are almost identical versions of each other. IMO this adds unnecessary noise to the code and makes it more laborious to maintain (as I explained in another comment).

However, I won't reject these changes because of that, but please, if you agree with these opinions, let us know.

Show outdated Hide outdated test/globalErr.js
// is being asserted on. Instead, if a test is failing, then Mocha will pick
// up the error that's thrown by `err`.
describe('falsey', function () {
// The inner `err` executes `myGetter` and catches the thrown error. But

This comment has been minimized.

@lucasfcosta

lucasfcosta Feb 14, 2017

Member

I don't think we need these comments.

They are really well written, but I think it is very easy to understand what is happening just by reading the code. If we have too many comments on the code it just makes it more difficult to maintain because we gotta make sure we update all the related comments, otherwise they will tell a different story than the code does.

IMO the outer code is okay, but this one (inner one) seems too much.

Anyway, I won't reject this PR because of this, please let me know what you think about it. 😄

@lucasfcosta

lucasfcosta Feb 14, 2017

Member

I don't think we need these comments.

They are really well written, but I think it is very easy to understand what is happening just by reading the code. If we have too many comments on the code it just makes it more difficult to maintain because we gotta make sure we update all the related comments, otherwise they will tell a different story than the code does.

IMO the outer code is okay, but this one (inner one) seems too much.

Anyway, I won't reject this PR because of this, please let me know what you think about it. 😄

Show outdated Hide outdated test/globalErr.js
}, undefined, true);
});
// `err` executes `myWrapper` and catches the thrown error. Since

This comment has been minimized.

@lucasfcosta

lucasfcosta Feb 14, 2017

Member

These comments are almost all the same, I think it adds a lot of noise to the code, especially because one of them would already be enough, but even more because now that we've got more descriptive error messages and awesome test case descriptions that already do the job of conveying the message we want it to.

@lucasfcosta

lucasfcosta Feb 14, 2017

Member

These comments are almost all the same, I think it adds a lot of noise to the code, especially because one of them would already be enough, but even more because now that we've got more descriptive error messages and awesome test case descriptions that already do the job of conveying the message we want it to.

@meeber

This comment has been minimized.

Show comment
Hide comment
@meeber

meeber Feb 14, 2017

Contributor

Pushed new version with excessive comments removed. (Changed it in a new commit instead of rebase in case we change mind in future).

Contributor

meeber commented Feb 14, 2017

Pushed new version with excessive comments removed. (Changed it in a new commit instead of rebase in case we change mind in future).

@lucasfcosta

Looks much cleaner now!
Excellent work 😄

LGTM, let's wait for another review to merge this.

@snackycracky

This comment has been minimized.

Show comment
Hide comment
@keithamus

This comment has been minimized.

Show comment
Hide comment
@keithamus

keithamus Mar 23, 2017

Member

How are we with this @meeber? Given:

Note to self: Rebase after #899 is merged and update to turn Error into an AssertionError that properly retains ssfi. A follow up PR will properly retain the user's custom message here and elsewhere.

Member

keithamus commented Mar 23, 2017

How are we with this @meeber? Given:

Note to self: Rebase after #899 is merged and update to turn Error into an AssertionError that properly retains ssfi. A follow up PR will properly retain the user's custom message here and elsewhere.

@meeber

This comment has been minimized.

Show comment
Hide comment
@meeber

meeber Mar 23, 2017

Contributor

@keithamus I made that note to myself yesterday :D

I'll update this PR after work!

Contributor

meeber commented Mar 23, 2017

@keithamus I made that note to myself yesterday :D

I'll update this PR after work!

meeber added some commits Jan 28, 2017

feat: improve `keep_ssfi` flag
- Allows the `keep_ssfi` flag to be set when creating new Assertion
  objects by providing its value as the fourth argument.

- Updates the `transferFlags` util to only transfer `keep_ssfi` when the
  `includeAll` argument is set to `true`.

- Updates assertion creation and proxify utils to only set `ssfi` if
  `keep_ssfi` isn't set.

- Updates inline docs to reflect expanded usage of `keep_ssfi`.
fix: remove frames from bdd interface stack
- Make it so that only `AssertionError` is thrown from inside of an
  assertion

- Always pass the current `ssfi` to a `new AssertionError` inside of an
  assertion

- Always pass the current `ssfi` to a new `Assertion` inside of an
  assertion, and set the `keep_ssfi` flag

- Improve the `globalErr` test helper function to also validate that
  the thrown error's stack trace doesn't contain implementation frames
fix: remove frames from assert interface stack
- Make it so that only `AssertionError` is thrown from inside of an
  assert interface wrapper function

- Always set `ssfi` to the current function when creating a new
  `AssertionError` inside of an assert interface wrapper function

- Always set `ssfi` to the current function when creating a new
  `Assertion` inside of an assert interface wrapper function,
  and set the `keep_ssfi` flag

- Improve the `globalErr` test helper function to also validate that the
  thrown error's stack trace doesn't contain frames from the assert
  interface
@meeber

This comment has been minimized.

Show comment
Hide comment
@meeber

meeber Mar 23, 2017

Contributor

@keithamus @shvaikalesh @vieiralucas @lucasfcosta Rebased since #899 was merged. As expected, some instanceof tests started failing due to implementation frames in stack trace. Updated as planned. Tests now pass.

I think this PR is ready to go. After this is merged, there will be a follow-up PR related to messages.

Contributor

meeber commented Mar 23, 2017

@keithamus @shvaikalesh @vieiralucas @lucasfcosta Rebased since #899 was merged. As expected, some instanceof tests started failing due to implementation frames in stack trace. Updated as planned. Tests now pass.

I think this PR is ready to go. After this is merged, there will be a follow-up PR related to messages.

@lucasfcosta

This comment has been minimized.

Show comment
Hide comment
@lucasfcosta

lucasfcosta Mar 27, 2017

Member

LGTM!
Good job 😄

Member

lucasfcosta commented Mar 27, 2017

LGTM!
Good job 😄

@meeber

This comment has been minimized.

Show comment
Hide comment
@meeber

meeber Apr 4, 2017

Contributor

@keithamus @shvaikalesh @vieiralucas Need a second review on this one!

Contributor

meeber commented Apr 4, 2017

@keithamus @shvaikalesh @vieiralucas Need a second review on this one!

@keithamus

This comment has been minimized.

Show comment
Hide comment
@keithamus

keithamus Apr 4, 2017

Member

LGTM @meeber! Great work, it's not always glamorous to do this kind of work, but it's definitely needed. That's one of the many reasons you're such a valuable contributor 😘 👍 💯

Member

keithamus commented Apr 4, 2017

LGTM @meeber! Great work, it's not always glamorous to do this kind of work, but it's definitely needed. That's one of the many reasons you're such a valuable contributor 😘 👍 💯

@keithamus keithamus merged commit e5ac3b0 into chaijs:master Apr 4, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@meeber meeber deleted the meeber:remove-chai-frames branch Aug 6, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment