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.

Sign up for GitHub

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

Jump to bottom

Fix stacktraces for overwritten properties and methods #661

Merged
merged 6 commits into from
Apr 4, 2016
Merged

Fix stacktraces for overwritten properties and methods #661

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
be8189c
Remove unnecessary config checks
Turbo87 Mar 31, 2016
cf378e4
test/configuration: Use describe('includeStack') block
Turbo87 Mar 31, 2016
cceadf6
overwriteProperty: Fix "ssfi" flag overwriting
Turbo87 Mar 31, 2016
1d8c3d9
overwriteMethod: Fix "ssfi" flag overwriting
Turbo87 Mar 31, 2016
c4f522f
test: Add "overwriteProperty" stack trace tests
Turbo87 Apr 3, 2016
58532d9
test: Add "overwriteMethod" stack trace tests
Turbo87 Apr 3, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 1 addition & 2 deletions lib/chai/utils/addChainableMethod.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@

var transferFlags = require('./transferFlags');
var flag = require('./flag');
var config = require('../config');

/*!
* Module variables
Expand Down Expand Up @@ -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

flag(this, 'ssfi', assert);
var result = chainableBehavior.method.apply(this, arguments);
return result === undefined ? this : result;
Expand Down
4 changes: 2 additions & 2 deletions lib/chai/utils/addMethod.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
*/

var chai = require('../../chai');
var config = require('../config');
var flag = require('./flag');
var transferFlags = require('./transferFlags');

Expand Down Expand Up @@ -37,8 +36,9 @@ var transferFlags = require('./transferFlags');

module.exports = function (ctx, name, method) {
ctx[name] = function () {
var keep_ssfi = flag(this, 'keep_ssfi');
var old_ssfi = flag(this, 'ssfi');
if (old_ssfi && config.includeStack === false)
if (!keep_ssfi && old_ssfi)
flag(this, 'ssfi', ctx[name]);

var result = method.apply(this, arguments);
Expand Down
4 changes: 2 additions & 2 deletions lib/chai/utils/addProperty.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
*/

var chai = require('../../chai');
var config = require('../config');
var flag = require('./flag');
var transferFlags = require('./transferFlags');

Expand Down Expand Up @@ -40,8 +39,9 @@ module.exports = function (ctx, name, getter) {

Object.defineProperty(ctx, name,
{ get: function addProperty() {
var keep_ssfi = flag(this, 'keep_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 also explain why we need the keep_ssfi flag. As far as I can see it is used to to override ssfi when we chain on properties - is this 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.

imagine expect(true).to.be.false with false overwritten:

  • expect sets ssfi
  • to overwrites ssfi
  • be overwrites ssfi
  • custom false overwrites ssfi
  • _super false overwrites ssfi

but actually that last part shouldn't happen because you don't want the _super in the stack trace. setting keep_ssfi during the _super prevents that from happening.

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we test if _super is really in the stack trace when we've got properties/methods overwritten?
What do you guys think about this?

Tests are okay now, but maybe we could have this too.

Copy link
Member

Choose a reason for hiding this comment

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

Yup, great idea 👍 💯

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lucasfcosta but sure I understand what you meant?

Copy link
Member

Choose a reason for hiding this comment

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

@Turbo87, sorry but I didn't understand what you just said :(

Tests for this should be similar to what we already have, we just need to overwrite the property and them check the stack trace to make sure it does not include that _super call. Basically you've got to ensure the second example you posted on this issue won't happen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

damn autocorrect... that was "not sure I understand what you meant" 😉

var old_ssfi = flag(this, 'ssfi');
if (old_ssfi && config.includeStack === false)
if (!keep_ssfi && old_ssfi)
flag(this, 'ssfi', addProperty);

var result = getter.call(this);
Expand Down
10 changes: 10 additions & 0 deletions lib/chai/utils/overwriteMethod.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
* MIT Licensed
*/

var flag = require('./flag');

/**
* ### overwriteMethod (ctx, name, fn)
*
Expand Down Expand Up @@ -46,7 +48,15 @@ module.exports = function (ctx, name, method) {
_super = _method;

ctx[name] = function () {
var keep_ssfi = flag(this, 'keep_ssfi');
var old_ssfi = flag(this, 'ssfi');
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

var result = method(_super).apply(this, arguments);
flag(this, 'keep_ssfi', false);

return result === undefined ? this : result;
}
};
12 changes: 11 additions & 1 deletion lib/chai/utils/overwriteProperty.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
* MIT Licensed
*/

var flag = require('./flag');

/**
* ### overwriteProperty (ctx, name, fn)
*
Expand Down Expand Up @@ -46,8 +48,16 @@ module.exports = function (ctx, name, getter) {
_super = _get.get

Object.defineProperty(ctx, name,
{ get: function () {
{ get: function overwriteProperty() {
var keep_ssfi = flag(this, 'keep_ssfi');
var old_ssfi = flag(this, 'ssfi');
if (!keep_ssfi && old_ssfi)
flag(this, 'ssfi', overwriteProperty);

flag(this, 'keep_ssfi', true);
var result = getter(_super).call(this);
flag(this, 'keep_ssfi', false);

return result === undefined ? this : result;
}
, configurable: true
Expand Down
114 changes: 58 additions & 56 deletions test/configuration.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,71 +25,73 @@ describe('configuration', function () {
chai.expect('foo').to.not.exist;
}

it('includeStack is true for method assertions', function () {
chai.config.includeStack = true;

try {
fooThrows();
assert.ok(false, 'should not get here because error thrown');
} catch (err) {
// not all browsers support err.stack
if ('undefined' !== typeof err.stack) {
assert.include(err.stack, 'assertEqual', 'should have internal stack trace in error message');
assert.include(err.stack, 'fooThrows', 'should have user stack trace in error message');
describe('includeStack', function() {
it('is true for method assertions', function () {
chai.config.includeStack = true;

try {
fooThrows();
assert.ok(false, 'should not get here because error thrown');
} catch (err) {
// not all browsers support err.stack
if ('undefined' !== typeof err.stack) {
assert.include(err.stack, 'assertEqual', 'should have internal stack trace in error message');
assert.include(err.stack, 'fooThrows', 'should have user stack trace in error message');
}
}
}

});
});

it('includeStack is false for method assertions', function () {
chai.config.includeStack = false;

try {
fooThrows();
assert.ok(false, 'should not get here because error thrown');
} catch (err) {
// IE 10 supports err.stack in Chrome format, but without
// `Error.captureStackTrace` support that allows tuning of the error
// message.
if ('undefined' !== typeof err.stack && 'undefined' !== typeof Error.captureStackTrace) {
assert.notInclude(err.stack, 'assertEqual', 'should not have internal stack trace in error message');
assert.include(err.stack, 'fooThrows', 'should have user stack trace in error message');
it('is false for method assertions', function () {
chai.config.includeStack = false;

try {
fooThrows();
assert.ok(false, 'should not get here because error thrown');
} catch (err) {
// IE 10 supports err.stack in Chrome format, but without
// `Error.captureStackTrace` support that allows tuning of the error
// message.
if ('undefined' !== typeof err.stack && 'undefined' !== typeof Error.captureStackTrace) {
assert.notInclude(err.stack, 'assertEqual', 'should not have internal stack trace in error message');
assert.include(err.stack, 'fooThrows', 'should have user stack trace in error message');
}
}
}
});
});

it('includeStack is true for property assertions', function () {
chai.config.includeStack = true;

try {
fooPropThrows();
assert.ok(false, 'should not get here because error thrown');
} catch (err) {
// not all browsers support err.stack
// Phantom does not include function names for getter exec
if ('undefined' !== typeof err.stack && 'undefined' !== typeof Error.captureStackTrace) {
assert.include(err.stack, 'addProperty', 'should have internal stack trace in error message');
assert.include(err.stack, 'fooPropThrows', 'should have user stack trace in error message');
it('is true for property assertions', function () {
chai.config.includeStack = true;

try {
fooPropThrows();
assert.ok(false, 'should not get here because error thrown');
} catch (err) {
// not all browsers support err.stack
// Phantom does not include function names for getter exec
if ('undefined' !== typeof err.stack && 'undefined' !== typeof Error.captureStackTrace) {
assert.include(err.stack, 'addProperty', 'should have internal stack trace in error message');
assert.include(err.stack, 'fooPropThrows', 'should have user stack trace in error message');
}
}
}

});
});

it('includeStack is false for property assertions', function () {
chai.config.includeStack = false;

try {
fooPropThrows();
assert.ok(false, 'should not get here because error thrown');
} catch (err) {
// IE 10 supports err.stack in Chrome format, but without
// `Error.captureStackTrace` support that allows tuning of the error
// message.
if ('undefined' !== typeof err.stack && 'undefined' !== typeof Error.captureStackTrace) {
assert.notInclude(err.stack, 'addProperty', 'should not have internal stack trace in error message');
assert.include(err.stack, 'fooPropThrows', 'should have user stack trace in error message');
it('is false for property assertions', function () {
chai.config.includeStack = false;

try {
fooPropThrows();
assert.ok(false, 'should not get here because error thrown');
} catch (err) {
// IE 10 supports err.stack in Chrome format, but without
// `Error.captureStackTrace` support that allows tuning of the error
// message.
if ('undefined' !== typeof err.stack && 'undefined' !== typeof Error.captureStackTrace) {
assert.notInclude(err.stack, 'addProperty', 'should not have internal stack trace in error message');
assert.include(err.stack, 'fooPropThrows', 'should have user stack trace in error message');
}
}
}
});
});

describe('truncateThreshold', function() {
Expand Down
108 changes: 108 additions & 0 deletions test/utilities.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,60 @@ describe('utilities', function () {
expect(expect('foo').result()).to.equal('result');
});

describe('overwriteMethod', function () {
before(function() {
chai.config.includeStack = false;

chai.use(function(_chai, _) {
_chai.Assertion.addMethod('four', function() {
this.assert(this._obj === 4, 'expected #{this} to be 4', 'expected #{this} to not be 4', 4);
});

_chai.Assertion.overwriteMethod('four', function(_super) {
return function() {
if (typeof this._obj === 'string') {
this.assert(this._obj === 'four', 'expected #{this} to be \'four\'', 'expected #{this} to not be \'four\'', 'four');
} else {
_super.call(this);
}
}
});
});
});

after(function() {
delete chai.Assertion.prototype.four;
});

it('calling _super has correct stack trace', function() {
try {
expect(5).to.be.four();
expect(false, 'should not get here because error thrown').to.be.ok;
} catch (err) {
// not all browsers support err.stack
// 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('overwriteMethod');
}
}
});

it('overwritten behavior has correct stack trace', function() {
try {
expect('five').to.be.four();
expect(false, 'should not get here because error thrown').to.be.ok;
} catch (err) {
// not all browsers support err.stack
// 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('overwriteMethod');
}
}
});
});

it('addProperty', function () {
chai.use(function (_chai, _) {
_chai.Assertion.addProperty('tea', function () {
Expand Down Expand Up @@ -417,6 +471,60 @@ describe('utilities', function () {
expect(expect('foo').result).to.equal('result');
});

describe('overwriteProperty', function () {
before(function() {
chai.config.includeStack = false;

chai.use(function(_chai, _) {
_chai.Assertion.addProperty('four', function() {
this.assert(this._obj === 4, 'expected #{this} to be 4', 'expected #{this} to not be 4', 4);
});

_chai.Assertion.overwriteProperty('four', function(_super) {
return function() {
if (typeof this._obj === 'string') {
this.assert(this._obj === 'four', 'expected #{this} to be \'four\'', 'expected #{this} to not be \'four\'', 'four');
} else {
_super.call(this);
}
}
});
});
});

after(function() {
delete chai.Assertion.prototype.four;
});

it('calling _super has correct stack trace', function() {
try {
expect(5).to.be.four;
expect(false, 'should not get here because error thrown').to.be.ok;
} catch (err) {
// not all browsers support err.stack
// 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');
}
}
});

it('overwritten behavior has correct stack trace', function() {
try {
expect('five').to.be.four;
expect(false, 'should not get here because error thrown').to.be.ok;
} catch (err) {
// not all browsers support err.stack
// 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');
}
}
});
});

it('getMessage', function () {
chai.use(function (_chai, _) {
expect(_.getMessage({}, [])).to.equal('');
Expand Down