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 stacktraces for overwritten properties and methods #661

Merged
merged 6 commits into from
Apr 4, 2016

Conversation

Turbo87
Copy link
Contributor

@Turbo87 Turbo87 commented Apr 1, 2016

This PR resolves #659

Since testing overwritten properties/methods doesn't seems to be possible at the moment I tested this manually and it seems to work now. It would be good if this could be verified by someone else before merging though!

/cc @keithamus

@Turbo87
Copy link
Contributor Author

Turbo87 commented Apr 3, 2016

@keithamus any comments on this?

@@ -79,7 +78,7 @@ module.exports = function (ctx, name, method, chainingBehavior) {

var assert = function assert() {
var old_ssfi = flag(this, 'ssfi');
if (old_ssfi && config.includeStack === false)
if (old_ssfi)
Copy link
Member

Choose a reason for hiding this comment

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

Could you please explain why you've removed the config.includeStack part here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That flag is already checked in the assert() method and checking it here has no real benefit aside from making the logic more complicated

@keithamus
Copy link
Member

LGTM, I'd like @lucasfcosta to review this one also though.

@Turbo87
Copy link
Contributor Author

Turbo87 commented Apr 3, 2016

Sure, it needs a bit of brain twisting to understand what's going on 😉

if (!keep_ssfi && old_ssfi)
flag(this, 'ssfi', ctx[name]);

flag(this, 'keep_ssfi', true);
Copy link
Member

Choose a reason for hiding this comment

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

Hi @Turbo87, I just want to confirm I understood the whole thing here:

Whenever you overwrite a method you set keep_ssfi to true before calling _super so you won't get that call on the stack due to those checks you added to addMethod, addChainableMethod and addProperty, am I right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct

@lucasfcosta
Copy link
Member

Hello everyone, first of all, thanks for the PR @Turbo87, great job here 😄 !

I needed to pay a lot of attention to really understand how to handle the Stack Trace properly, but I think I've got it. This is looking good in general, anyway, I just added two notes to the code just to ensure we're going in the right direction here (and if I've understood everything correctly).

My two notes are here and here.

@Turbo87
Copy link
Contributor Author

Turbo87 commented Apr 4, 2016

@lucasfcosta I've added a few tests to verify that the stack traces work correctly when overwriting properties and methods

// Phantom does not include function names for getter exec
if ('undefined' !== typeof err.stack && 'undefined' !== typeof Error.captureStackTrace) {
expect(err.stack).to.include('utilities.js');
expect(err.stack).to.not.include('overwriteProperty');
Copy link
Member

Choose a reason for hiding this comment

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

I believe you should be checking if it does not include overwriteMethod instead of overwriteProperty here, shouldn't you?

Copy link
Member

Choose a reason for hiding this comment

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

hahaha, np, happens to everyone sometimes 😆

@lucasfcosta
Copy link
Member

Great job @Turbo87, I just think you accidentally looked for overwriteProperty on the stack-trace for overwriteMethod tests.

@Turbo87
Copy link
Contributor Author

Turbo87 commented Apr 4, 2016

@lucasfcosta fixed

@lucasfcosta
Copy link
Member

This LGTM!
Nice job!

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.

stack trace wrong for overridden properties
3 participants