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
Adds guard for empty properties in deepEqual when a matcher is provided #1901
Adds guard for empty properties in deepEqual when a matcher is provided #1901
Conversation
In JavaScript every acts like the "for all" quantifier in mathematics. In particular, for an empty array, it returns true. (It is vacuously true that all elements of the empty set satisfy any given condition.) This is not the intended behaviour for deepEqual, we should treat empty properties as falsy. Resolves sinonjs#1900
Pull Request Test Coverage Report for Build 2667
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent pull request!
I just have one minor comment
test/issues/issues-test.js
Outdated
|
||
describe("#1900 - calledWith returns false positive", function () { | ||
it("should return false when call args don't match", function () { | ||
var stub = sinon.stub(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order to eliminate potential for regressions, it would be better to verify this using spy
, as a stub
is a spy
, but a spy
is not a stub
(the same is true for spy
and fake
).
Something like this:
var spy = sinon.spy();
var dateOne = new Date("2018-07-01");
var dateTwo = new Date("2018-07-31");
spy(dateOne);
var calledWith = spy.calledWith(dateTwo);
assert.same(calledWith, false);
Please ignore this blip. I'm just closing and reopening this issue to get all the tests to run again. Some may have been updated since the PR was created. |
This has been published to NPM as |
…ed (sinonjs#1901) In JavaScript every acts like the "for all" quantifier in mathematics. In particular, for an empty array, it returns true. (It is vacuously true that all elements of the empty set satisfy any given condition.) This is not the intended behaviour for deepEqual, we should treat empty properties as falsy. Resolves sinonjs#1900
Fix issue #1900 by guarding against empty properties in
deepEqual
when a matcher is provided.My big assumption here is that the behaviour of
Array.every
when passed an empty array is not expected. I've tried to keep the pattern of only exiting thedeepEqual
function early when we find equality. If we end up at the bottom of the function and we have a matcher (due to being called viacalledWith
) then we should not assume we are an object with properties. Guarding against this by checking the length ofObject.keys
fixes this bug and does not break any other tests so I assume it is the correct solution.Happy to hear alternative suggestions though as I am not too familiar with the code base.
How to verify - mandatory
npm install
npm test
Checklist for author
npm run lint
passes