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

Immutable actuals #16

Closed
wants to merge 3 commits into from
Closed

Immutable actuals #16

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
bb16a99
Test that Immutable comparison is used when "expected" value is an Im…
matthewwithanm Jun 10, 2015
9cc8f0f
Use Immutable.is when either value is an Immutable collection
matthewwithanm Jun 10, 2015
d1d4f87
:lipstick:
matthewwithanm Jul 6, 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
4 changes: 2 additions & 2 deletions chai-immutable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ module.exports = function (chai, utils) {
return function (collection) {
var obj = this._obj;

if (obj && obj instanceof Collection) {
if (obj && obj instanceof Collection || collection instanceof Collection) {
Copy link
Owner

Choose a reason for hiding this comment

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

@matthewwithanm, I'm not sure I see why we need this.

Currently:

expect([]).to.not.equal(List.of(1, 2, 3)); // Passes
expect([]).to.equal(List.of(1, 2, 3)); // Fails with "AssertionError: expected [] to equal List [ 1, 2, 3 ]"

which seems like what we should expect: if obj is not a Collection, the equality assertion is deferred to the original assertion.

Is there something I am I missing 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.

Hm, yeah it doesn't seem like we do…sorry, I can't remember anymore! I can remove it and see if we stumble on why again. Sound good?

this.assert(
Immutable.is(obj, collection),
'expected #{this} to equal #{exp}',
Expand Down Expand Up @@ -437,7 +437,7 @@ module.exports = function (chai, utils) {
*/

assert.equal = function (actual, expected) {
if (actual instanceof Collection) {
if (actual instanceof Collection || expected instanceof Collection) {
return new Assertion(actual).equal(expected);
}
else return assert.equal;
Expand Down
14 changes: 14 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,13 @@ describe('chai-immutable', function () {
});

describe('equal method', function () {
it(
'fails when only the "expected" value is an Immutable collection',
function () {
assert.throws(expect([]).to.equal.bind(null, List()));
Copy link
Owner

Choose a reason for hiding this comment

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

Kudos on providing failing tests beforehand :-)

A few things:

  • Why the bind method here?
  • When testing that something throws an exception, would you mind using this form instead?
  • This test passes with and without your fix, are you sure it really tests what you wanted to test?

That being said, the entire test suite checks that the expected behaviors but never that the proper exceptions are thrown, which is quite pointless in the end... I should work on that, thanks for reminding me :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

bind is just used to create a partially applied version of equal (instead of wrapping the call manually with a function declaration), but I can update it to declare a new function if you want.

Are you sure this test is passing without my fix? It isn't for me. Can you double check that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, sorry you're right. I was seeing the other failure. I just added this one because it seemed like there should be a test for both interfaces.

}
);

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 +304,13 @@ describe('chai-immutable', function () {

describe('TDD interface', function () {
describe('equal assertion', function () {
it(
'fails when only the "expected" value is an Immutable collection',
function () {
assert.throws(assert.equal.bind(null, [], List()));
}
);

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