Deep flag support for keys assertion #668

lucasfcosta · 2016-04-06T21:23:35Z

This adds support for the deep flag on the keys assertion (for Maps/Sets) as I've talked to @keithamus at #633.

This PR should include:

Deep flag support for the keys assertion (Maps/Sets)
Updated doc comments with examples and a general explanation about the deep flag on this assertion
Tests on the should interface
Tests on the expect interface
Bindings for these on the assert interface
Tests on the assert interface

Currently the only thing missing are the bindings for these assertions on the assert interface, I didn't do it yet because I'm not sure if adding lots of bindings for this to the assert interface would be cool. If we added bindings for every possible combination of the keys assertion flags we would have:

hasAnyKeys
hasAllKeys
containsAllKeys
doesNotHaveAnyKeys
doesNotHaveAllKeys
hasAnyDeepKeys
hasAllDeepKeys
containsAllDeepKeys
doesNotHaveAnyDeepKeys
doesNotHaveAllDeepKeys

IMO we should have all of them on the assert interface because I think it should have the same features as the other interfaces do, but I just wanted to make sure you guys agree with me before adding all these bindings.

Important Detail:

Currently we need to fix #662 in order to enable all tests, so I think we may keep this open but on hold until we've got inspect to work properly for maps and sets so we can fully test this.
What's your opinion on this?

Thanks in advance, please feel free to point any possible mistakes/improvements 😄

lucasfcosta · 2016-09-14T20:27:10Z

Hello everyone, I'll finish the items left on this one since we're getting close to releasing 4.x.x.
But before getting this merged I'd like to have #662 fixed just so we can have complete tests for it.
What do you think?

keithamus · 2016-09-14T20:31:23Z

@lucasfcosta my work with refactoring loupe is quite a ways off. We can probably apple a simple fix for #662 though, to get it working with Maps and Sets for now.

lucasfcosta · 2016-09-14T20:33:25Z

@keithamus seems like a good idea. Let's solve this one that way then so we can get this merged.

lucasfcosta · 2016-09-15T17:49:16Z

This one is done!
Feel free to review and merge this if you think it's good to go.
Thanks everyone! 😄

vieiralucas · 2016-09-15T18:11:30Z

test/assert.js

@@ -778,7 +794,7 @@ describe('assert', function () {
      var errMap = new Map();

      errMap.set({1: 20}, 'number');
-
+   


I see trailing spaces 😱

My vim does only removes it when there's an .editorconfig file in the root folder of where the file is 😢

I've just fixed that, thanks for noticing 😄

haha

I add a .editorconfig file on my home with some rules that I always want.
I think it goes looking upwards until it finds one

Yes it does, great idea 😄

vieiralucas · 2016-09-15T18:31:56Z

Awesome work!
Specially with the docs and tests 🎉 🎉 🎉

What could we do to remember enabling these commented tests when #662 gets solved?
Maybe solving that issue should require enabling this tests?

lucasfcosta · 2016-09-15T18:56:03Z

@vieiralucas Yes, I think that would be ideal.
I'll leave a comment there so we won't forget it, thanks for noticing 😄

meeber

@lucasfcosta Excellent work sir, this will be a fine addition to Chai. I have a few small comments sprinkled throughout (it only looks like a lot of comments cause I repeated them wherever applicable).

meeber · 2016-09-19T23:02:11Z

lib/chai/core/assertions.js

      , str
      , ok = true
      , mixedArgsMsg = 'when testing keys against an object or an array you must give a single Array|Object|String argument or multiple String arguments';

    if (_.type(obj) === 'map' || _.type(obj) === 'set') {
+      str = isDeep ? 'deeply ' : '';


I realize messages aren't being tested yet due to an issue with inspect and sets/maps, but just noting that I believe str will get overwritten here.

You are right!
I'm going to use a different variable to store that String, thanks 😄

meeber · 2016-09-19T23:16:10Z

lib/chai/interface/assert.js

+  assert.hasAnyDeepKeys = function (obj, keys, msg) {
+     if (keys === undefined) {
+      new Assertion(obj, msg).to.have.any.deep.keys();
+    }


What if the undefined check was at the top of the actual assertKeys function instead?

meeber · 2016-09-19T23:16:59Z

lib/chai/interface/assert.js

+   *
+   * @name doesNotHaveAllKeys
+   * @param {Mixed} object
+   * @param {...String|Array|Object} keys


It doesn't look to me like the ...String style of the keys argument is supported here.

meeber · 2016-09-19T23:20:48Z

lib/chai/interface/assert.js

+   *
+   * @name hasAllDeepKeys
+   * @param {Mixed} object
+   * @param {...String|Array|Object} keys


It doesn't look to me like the ...String style of the keys argument is supported here.

meeber · 2016-09-19T23:22:29Z

lib/chai/interface/assert.js

+ /**
+   * ### .containsAllDeepKeys(object, [keys], [message])
+   *
+   * Asserts that `object` contains all of the `keys` provided.


Even though it's shown in the example, probably worth mentioning explicitly that the object can have more keys than those provided, just to contrast with hasAll

meeber · 2016-09-19T23:30:04Z

test/assert.js

@@ -877,6 +903,22 @@ describe('assert', function () {
      errSet.add({1: 20});
      errSet.add('number');

+      // Tests for the deep variations of the keys assertion


I'm thinking a test should be added to the non-deep variety of assertion above demonstrating that it uses strict equality instead of deep. For example:

assert.doesNotHaveAnyKeys(testSet, {thisIs: 'anExampleObject'});

meeber · 2016-09-19T23:31:42Z

test/expect.js

@@ -1205,6 +1205,27 @@ describe('expect', function () {
      expect(testMap).to.not.have.any.keys([20, 1, {13: 37}]);
      expect(testMap).to.not.have.all.keys([aKey, {'iDoNot': 'exist'}]);

+      // Using the same assertions as above but with `.deep` flag instead of using referential equality


Same thing as mentioned in the assert interface tests about adding a non-deep test to demonstrate strict equality.

meeber · 2016-09-19T23:31:50Z

test/expect.js

@@ -1285,6 +1316,26 @@ describe('expect', function () {
      expect(testSet).to.not.have.any.keys([20, 1, {13: 37}]);
      expect(testSet).to.not.have.all.keys([aKey, {'iDoNot': 'exist'}]);

+      // Using the same assertions as above but with `.deep` flag instead of using referential equality


Same thing as mentioned in the assert interface tests about adding a non-deep test to demonstrate strict equality.

meeber · 2016-09-19T23:31:55Z

test/should.js

@@ -1051,6 +1051,23 @@ describe('should', function() {
      testMap.should.not.have.any.keys([20, 1, {13: 37}]);
      testMap.should.not.have.all.keys([aKey, {'iDoNot': 'exist'}]);

+      // Using the same assertions as above but with `.deep` flag instead of using referential equality


Same thing as mentioned in the assert interface tests about adding a non-deep test to demonstrate strict equality.

meeber · 2016-09-19T23:31:59Z

test/should.js

@@ -1131,6 +1158,26 @@ describe('should', function() {
      testSet.should.not.have.any.keys([20, 1, {13: 37}]);
      testSet.should.not.have.all.keys([aKey, {'iDoNot': 'exist'}]);

+      // Using the same assertions as above but with `.deep` flag instead of using referential equality


Same thing as mentioned in the assert interface tests about adding a non-deep test to demonstrate strict equality.

lucasfcosta · 2016-09-22T22:35:01Z

Here it is!
So, with the removal of the if conditions from the assert interface this becomes a breaking change.

Please let me know if I missed something.

I'd also like to add a note here for my future self: it would be nice to refactor these methods in order for them to support what I've said in this comment and also throw an error indicating keys are required when no keys are passed for the other combination of keys assertions other than the ones I mentioned there.

meeber · 2016-09-25T23:28:46Z

@lucasfcosta I like the extra tests you added since the last commit on the assert interface that demonstrate that strict equality is used by default, but I think similar tests should be added on the expect and should interfaces, just for consistency.

lucasfcosta · 2016-09-26T12:01:08Z

@meeber, seems adequate!
Good idea 😄
I'm gonna do it later today, thanks for noticing.

lucasfcosta · 2016-09-27T21:01:33Z

@meeber here it is!
I think we've got everything covered by now.
I added tests to see if every combination of have, contain, all and any is using strict equality.

meeber · 2016-09-27T21:59:40Z

@lucasfcosta Well done sir! LGTM! The only way this could be better is if we add another 5 or 6 interfaces so you can add all of the same tests on those interfaces too! Wouldn't that be fun?! :D

Pinging @keithamus and @vieiralucas!

vieiralucas · 2016-09-28T22:12:07Z

As I have said previously, this is awesome work.

LGTM

lucasfcosta mentioned this pull request Apr 11, 2016

Fix bug when asserting some valid ES6 keys #676

Merged

keithamus modified the milestone: 4.0 Sep 6, 2016

keithamus assigned lucasfcosta Sep 14, 2016

lucasfcosta force-pushed the deep-flag-for-keys branch 3 times, most recently from a72431a to a911ec0 Compare September 15, 2016 17:48

lucasfcosta changed the title ~~[WIP] Deep flag support for keys assertion~~ Deep flag support for keys assertion Sep 15, 2016

lucasfcosta force-pushed the deep-flag-for-keys branch from a911ec0 to 2f4a242 Compare September 15, 2016 17:57

vieiralucas reviewed Sep 15, 2016

View reviewed changes

lucasfcosta force-pushed the deep-flag-for-keys branch from 2f4a242 to e8bde1c Compare September 15, 2016 18:13

lucasfcosta mentioned this pull request Sep 15, 2016

Inspect doesn't work properly for Maps and Sets - It displays an empty object #662

Closed

meeber reviewed Sep 19, 2016

View reviewed changes

lucasfcosta force-pushed the deep-flag-for-keys branch from e8bde1c to a0a5fc1 Compare September 22, 2016 22:31

lucasfcosta force-pushed the deep-flag-for-keys branch from a0a5fc1 to 816b5a3 Compare September 22, 2016 22:38

Deep flag support for keys assertion

9498811

lucasfcosta force-pushed the deep-flag-for-keys branch from 816b5a3 to 9498811 Compare September 27, 2016 21:00

vieiralucas approved these changes Sep 28, 2016

View reviewed changes

vieiralucas merged commit 29e7d87 into chaijs:master Sep 28, 2016

lucasfcosta mentioned this pull request Oct 2, 2016

4.x.x Complete Migration Guide #781

Closed

astorije mentioned this pull request Dec 18, 2016

Add support for deep flag for the keys assertion astorije/chai-immutable#73

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deep flag support for keys assertion #668

Deep flag support for keys assertion #668

lucasfcosta commented Apr 6, 2016 •

edited

lucasfcosta commented Sep 14, 2016

keithamus commented Sep 14, 2016

lucasfcosta commented Sep 14, 2016 •

edited

lucasfcosta commented Sep 15, 2016

vieiralucas Sep 15, 2016

lucasfcosta Sep 15, 2016

lucasfcosta Sep 15, 2016

vieiralucas Sep 15, 2016 •

edited

lucasfcosta Sep 15, 2016 •

edited

vieiralucas commented Sep 15, 2016

lucasfcosta commented Sep 15, 2016

meeber left a comment

meeber Sep 19, 2016

lucasfcosta Sep 21, 2016

meeber Sep 19, 2016

meeber Sep 19, 2016 •

edited

meeber Sep 19, 2016

meeber Sep 19, 2016

meeber Sep 19, 2016 •

edited

meeber Sep 19, 2016

meeber Sep 19, 2016

meeber Sep 19, 2016

meeber Sep 19, 2016

lucasfcosta commented Sep 22, 2016 •

edited

meeber commented Sep 25, 2016

lucasfcosta commented Sep 26, 2016

lucasfcosta commented Sep 27, 2016

meeber commented Sep 27, 2016 •

edited

vieiralucas commented Sep 28, 2016

		@@ -778,7 +794,7 @@ describe('assert', function () {
		var errMap = new Map();

		errMap.set({1: 20}, 'number');

Deep flag support for keys assertion #668

Deep flag support for keys assertion #668

Conversation

lucasfcosta commented Apr 6, 2016 • edited

Important Detail:

lucasfcosta commented Sep 14, 2016

keithamus commented Sep 14, 2016

lucasfcosta commented Sep 14, 2016 • edited

lucasfcosta commented Sep 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vieiralucas Sep 15, 2016 • edited

Choose a reason for hiding this comment

lucasfcosta Sep 15, 2016 • edited

Choose a reason for hiding this comment

vieiralucas commented Sep 15, 2016

lucasfcosta commented Sep 15, 2016

meeber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meeber Sep 19, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meeber Sep 19, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucasfcosta commented Sep 22, 2016 • edited

meeber commented Sep 25, 2016

lucasfcosta commented Sep 26, 2016

lucasfcosta commented Sep 27, 2016

meeber commented Sep 27, 2016 • edited

vieiralucas commented Sep 28, 2016

lucasfcosta commented Apr 6, 2016 •

edited

lucasfcosta commented Sep 14, 2016 •

edited

vieiralucas Sep 15, 2016 •

edited

lucasfcosta Sep 15, 2016 •

edited

meeber Sep 19, 2016 •

edited

meeber Sep 19, 2016 •

edited

lucasfcosta commented Sep 22, 2016 •

edited

meeber commented Sep 27, 2016 •

edited