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

Support Immutable with chai's property assertions #43

Merged
merged 1 commit into from Apr 20, 2016

Conversation

4 participants
@scottnonnenberg
Contributor

scottnonnenberg commented Mar 18, 2016

Property and deep property assertions, because I really like that syntax when I use chai with plain objects. :0)

var obj = Immutable.fromJS({ x: 1 });
expect(obj).to.have.property('x', 1);

var obj = Immutable.fromJS({ x: 1, y: { x: 2, y: 3 } });
expect(obj).to.have.deep.property(['y', 'x'], 2);

Based on original property() here: https://github.com/chaijs/chai/blob/8e8a728921085351ecd050cd606b2ef6a1fb1338/lib/chai/core/assertions.js#L881

Like other methods in chai-immutable, falls back to original chai methods if the target object is not isIterable().

100% code coverage for newly-added code. Also, I noticed that changing chai.config.truncateThreshold can make display of Immutable objects a lot nicer.

@scottnonnenberg

This comment has been minimized.

Contributor

scottnonnenberg commented Mar 18, 2016

Fixes #9, #41, #14.

@coveralls

This comment has been minimized.

coveralls commented Mar 18, 2016

Coverage Status

Coverage increased (+0.3%) to 98.315% when pulling 56eaf4a on scottnonnenberg:property into 655d187 on astorije:master.

@astorije

This comment has been minimized.

Owner

astorije commented Mar 19, 2016

Waouh @scottnonnenberg, thanks a lot for this, it looks like tremendous work! As a matter of fact, this is something I wanted to tackle fairly soon as I saw different requests about it coming up.
Give me a few days to review thoroughly and get back to you, and feel free to ping here if you think I forgot.

Hey what the hell @coveralls, you commenting here is supposed to be disabled!

@scottnonnenberg

This comment has been minimized.

Contributor

scottnonnenberg commented Apr 2, 2016

Ping! :0)

@@ -540,5 +540,97 @@
assert.sizeOf = function (collection, expected) {
new Assertion(collection).size(expected);
};
/**
* ### .property(path, value)

This comment has been minimized.

@astorije

astorije Apr 11, 2016

Owner

For alphabetical sorting consistency, would you mind moving this addition between keys and size, in the BDD reference section of the code please?

* @api public
*/
function assertProperty(_super) {
return function (path, val, msg) {

This comment has been minimized.

@astorije

astorije Apr 11, 2016

Owner

Note that providing a custom message is not available in any other assertion of this plugin. I would prefer we don't include it in this PR for this assertion only.
If you really need custom messages, you can open an issue to solve that globally and we'll start with this assertion.

Does that make sense?

var value;
if (!Immutable.Iterable.isIterable(obj)) {
return _super.apply(this, arguments);

This comment has been minimized.

@astorije

astorije Apr 11, 2016

Owner

Could you follow the same pattern as other assertions, that is, having this check as high in the function as possible and returning _super.apply(this, arguments); in an else statement at the very end?
Probably not the nicest way to do so, but at least consistent with the rest of the file.

@astorije

This comment has been minimized.

Owner

astorije commented Apr 11, 2016

Hey @scottnonnenberg, sorry for being silent, busy and all.
I started briefly looking into your work, it's awesome, gonna be a great addition to the plugin!

The biggest thing I need to address with you is the format of the deep properties.
Chai "use[s] dot- and bracket-notation for deep references into objects and arrays", while you went for the same argument as passed to Iterable#getIn(). Since this plugin aims at providing transparent Chai assertions for Immutable, I mention using same notation as Chai's in #14. My questions are as follows:

  • Why did you choose one over the other?
  • Is there an easy equivalence between the getIn notation and the dot/bracket-notation, specifically for the indices? I don't see the use of indices in your tests.

Depending on these questions, I think we could either switch to the dot/bracket-notation or, if the getIn notation makes sense (it's more of a use case question than a technical one), if we can convert one to the other when receiving it as an input.

@scottnonnenberg

This comment has been minimized.

Contributor

scottnonnenberg commented Apr 11, 2016

Quick split() call and we support both! :0)

@astorije

This comment has been minimized.

Owner

astorije commented Apr 13, 2016

Great work, @scottnonnenberg! Could you squash your commits into one commit before I can merge, please?

Support Immutable with chai's property assertions
Reorder methods for proper alphabetical sorting
Remove msg parameter
Match isIterable check with other methods in file
property() - Add an array index deep property test
property: Support string deep property syntax
Fix alphabetical in tests
More test reordering
@scottnonnenberg

This comment has been minimized.

Contributor

scottnonnenberg commented Apr 13, 2016

Squash complete!

@astorije

This comment has been minimized.

Owner

astorije commented Apr 20, 2016

Awesome, 👍 and merging!

@astorije astorije merged commit 12536fd into astorije:master Apr 20, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@JalilArfaoui

This comment has been minimized.

JalilArfaoui commented May 26, 2016

Hello both, Fantastic work !
Could you release a new version ?
I'm using latest npm (1.5.4) which does not support this ...

@astorije

This comment has been minimized.

Owner

astorije commented May 26, 2016

@JalilArfaoui Will do ASAP, I need to update the doc first. Stay tuned 😄

@astorije

This comment has been minimized.

Owner

astorije commented Jun 12, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment