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

Fix broken assert.equal assertion when not applied on a Collection #18

Merged
merged 3 commits into from
Jul 9, 2015
Merged

Fix broken assert.equal assertion when not applied on a Collection #18

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a4d510d
Test that Immutable comparison is used when "expected" value is an Im…
matthewwithanm Jun 10, 2015
6972e4e
Fall back to original `equal` method
matthewwithanm Jun 10, 2015
d1d4c5a
Match established test styles
matthewwithanm Jul 9, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion chai-immutable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -416,6 +416,7 @@ module.exports = function (chai, utils) {
*/

var assert = chai.assert;
var originalEqual = assert.equal;

/**
* ### .equal(actual, expected)
Expand All @@ -437,10 +438,15 @@ module.exports = function (chai, utils) {
*/

assert.equal = function (actual, expected) {
/*
* It seems like we shouldn't actually need this check, however,
* `assert.equal` actually behaves differently than its BDD counterpart!
* Namely, the BDD version is strict while the "assert" one isn't.
*/
Copy link
Owner

Choose a reason for hiding this comment

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

Good point to add that comment, I was actually just thinking "wait, why do we just and only call equal in the equal assertion?!".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah /:

Not too happy that chai does that but oh well. Incidentally, I wasn't sure what comment style to use here…this was the only thing that passed the linter!

Copy link
Owner

Choose a reason for hiding this comment

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

Mmh, not cool re: linter. I'll check, edit the rules and replace with //s instead, makes more sense here. Sorry about that.

if (actual instanceof Collection) {
return new Assertion(actual).equal(expected);
}
else return assert.equal;
else return originalEqual(actual, expected);
Copy link
Owner

Choose a reason for hiding this comment

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

You nailed it, actually! The issue isn't so much that I didn't call the original assertion, but that I was returning a function, which never throws anything. And of course, not storing the original assertion somewhere triggers an infinite loop.
Good job!

That's one more proof we need more tests...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(:

};

/**
Expand Down
16 changes: 16 additions & 0 deletions test/test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,14 @@ describe('chai-immutable', function () {
});

describe('equal method', function () {
it(
'fails when only the "expected" value is an Immutable collection',
Copy link
Owner

Choose a reason for hiding this comment

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

I'd prefer to keep should fail for consistency.

function () {
var fn = function () { expect([]).to.equal(List()); };
expect(fn).to.throw(Error);
}
);

it('should be true when compared structure is equal', function () {
expect(list3).to.equal(List.of(1, 2, 3));
});
Expand Down Expand Up @@ -297,6 +305,14 @@ describe('chai-immutable', function () {

describe('TDD interface', function () {
describe('equal assertion', function () {
it(
'fails when only the "expected" value is an Immutable collection',
Copy link
Owner

Choose a reason for hiding this comment

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

Same as https://github.com/astorije/chai-immutable/pull/18/files#r34209790.

function () {
var fn = function () { assert.equal([], List()); };
expect(fn).to.throw(Error);
Copy link
Owner

Choose a reason for hiding this comment

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

Argh, sorry, I should have been more explicit on my original comment. Would you mind using assert.throw here?

My rationale behind that is to keep the APIs "self contained". It may help future contributors who only deal with one API only, they don't have to use both APIs at the same time. Makes sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Done.

}
);

it('should be true when compared structure is equal', function () {
assert.equal(list3, List.of(1, 2, 3));
});
Expand Down