Add object includes a string key and exclusion test for objects #86

calebmer · 2016-04-09T15:58:51Z

First of all, thank you so much for writing this package. It makes writing tests so nice 😊. In addition this codebase is some of the best code I have ever read.

I just ran into two things which I expected to exist but didn't, this PR adds these things. Specifically this PR adds one feature and fixes one bug.

The feature this PR adds is a simple property string test for toInclude. Now, the user can ignore the value of an object and just test if a property is included in an object like so: expect({ a: 1 }).toInclude('a').

This feature also fixes a bug where objects were not enabled to be used by toExclude making it asymmetrical to toInclude which tests objects.

ljharb · 2016-04-09T16:01:13Z

README.md

@@ -258,10 +258,25 @@ expect([ 1, 2, 3 ]).toExclude(4)
 Asserts the given `object` contains all keys and values in `value`, recursively. The `comparator` function, if given, should compare two objects and either `return false` or `throw` if they are not equal. It defaults to `assert.deepEqual`.

 ```js
+expect({ a: 1, b: 2 }).toInclude('a')


i'm not really a fan of overloading APIs to take multiple types. I'd rather see a separate .toIncludeKeys that takes an array of strings, for example.

ljharb · 2016-04-09T16:03:08Z

I do like the toExclude change - if you could separate out the "string key" stuff in a separate PR, I could see being able to quickly update toExclude (without the string key checking, so that toInclude/toExclude are always parallel).

ljharb · 2016-04-09T16:04:12Z

modules/Expectation.js

@@ -334,8 +334,8 @@ class Expectation {

  toExclude(value, compareValues, message) {
    assert(
-      isArray(this.actual) || typeof this.actual === 'string',
-      'The "actual" argument in expect(actual).toExclude() must be an array or a string'
+      isArray(this.actual) || isObject(this.actual) || typeof this.actual === 'string',


i wonder if there's a way we could make toExclude call into toInclude, or have them share a core function, rather than duplicating the checks?

👍 Check my latest commit and tell me what you think.

This reverts commit b8fed4b.

ljharb · 2016-04-09T16:31:05Z

modules/Expectation.js

@@ -293,77 +293,47 @@ class Expectation {
    return this
  }

-  toInclude(value, compareValues, message) {
+  toInclude(value, compareValues, message, _exclude = false) {


Definitely not a fan of the _exclude param - this adds an obscure boolean param to the public API. Could we refactor both toInclude and toExclude to call into a private (closed-over) function instead?

Is a precedence for this already included for the Expectation class? My next instinct would to be to create a TestUtils function.

That seems reasonable.

Ok, ammended the last commit with the core implementation in TestUtils.

calebmer · 2016-04-09T17:25:17Z

@ljharb I used the same TestUtils approach to abstracting similar functionalities between toInclude and toExclude in #87. If you make a comment that is applicable to both PRs (#86 and #87) I will change it in both.

ljharb · 2016-04-09T19:49:29Z

modules/TestUtils.js

+ */
+export const containsHelper = (actual, value, compareValues, exclude, funcName, message) => {
+  if (compareValues == null)
+    compareValues = isEqual


could this be done with default values in the signature instead?

ljharb · 2016-04-09T19:50:21Z

In general I think this looks great overall - I like the reduced code duplication, and the new docs/tests are solid.

calebmer · 2016-04-16T19:38:17Z

@mjackson what's the status on this PR and #87?

mjackson · 2016-04-18T15:57:06Z

modules/Expectation.js

-        value
-      )
-    }
+    containsHelper(


I know this probably sounds silly, but I think I'd prefer to leave the branching in this method rather than tuck it away under containsHelper. It personally makes it easier for me to read the code and figure out e.g. what kinds of actual values toInclude accepts.

Thanks, doesn't sound silly. Would you prefer duplicating the logic or adding a private boolean switch as an undocumented parameter to switch the behavior like I had before?

My vote is to duplicate the logic rather than adding public API.

I'd like to see it with the logic duplicated for now. We can take another pass at it later if it becomes ridiculously long.

Sounds great 👍

mjackson · 2016-04-18T16:05:07Z

Hi @calebmer – thanks for your work here! I'm sorry to be slow on the response. Just got back from 2 weeks away. I like the docs updates, additional tests, and support for objects in toExclude, but I'd prefer to leave the code branching inline in the toInclude/toExclude methods because it's a little easier for me to read that way. Would you mind making those changes? I'll get this merged ASAP. Thanks!

mjackson · 2016-04-18T19:57:20Z

modules/Expectation.js

    }

+    assert(


👏 Well done. This is some de-duplication I can live with!

calebmer added 2 commits April 9, 2016 11:53

Add object contains property string

b8fed4b

Fix toExclude to test objects

f7c2ee5

ljharb reviewed Apr 9, 2016
View reviewed changes

Revert "Add object contains property string"

6ec27a7

This reverts commit b8fed4b.

calebmer force-pushed the feat/object-include-exclude-keys branch from 45a4514 to 096af85 Compare April 9, 2016 16:26

ljharb reviewed Apr 9, 2016
View reviewed changes

calebmer force-pushed the feat/object-include-exclude-keys branch 2 times, most recently from afce62d to 57f2343 Compare April 9, 2016 17:05

Dry up toInclude/toExclude

c103bcd

calebmer force-pushed the feat/object-include-exclude-keys branch from 57f2343 to c103bcd Compare April 9, 2016 17:09

ljharb reviewed Apr 9, 2016
View reviewed changes

ljharb added the enhancement label Apr 9, 2016

ljharb assigned mjackson Apr 9, 2016

wuct mentioned this pull request Apr 14, 2016

Add toIncludeKeys and toExcludeKeys #87

Merged

mjackson reviewed Apr 18, 2016
View reviewed changes

Improve code readability

6a2aa24

mjackson reviewed Apr 18, 2016
View reviewed changes

modules/Expectation.js

}

assert(

Copy link

Owner

mjackson Apr 18, 2016

Choose a reason for hiding this comment

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

👏 Well done. This is some de-duplication I can live with!

mjackson merged commit a78e970 into mjackson:master Apr 18, 2016

calebmer deleted the feat/object-include-exclude-keys branch April 18, 2016 21:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add object includes a string key and exclusion test for objects #86

Add object includes a string key and exclusion test for objects #86

calebmer commented Apr 9, 2016

ljharb Apr 9, 2016

ljharb commented Apr 9, 2016

ljharb Apr 9, 2016

calebmer Apr 9, 2016

ljharb Apr 9, 2016

calebmer Apr 9, 2016

ljharb Apr 9, 2016

calebmer Apr 9, 2016

calebmer commented Apr 9, 2016

ljharb Apr 9, 2016

ljharb commented Apr 9, 2016

calebmer commented Apr 16, 2016

mjackson Apr 18, 2016

calebmer Apr 18, 2016

ljharb Apr 18, 2016

mjackson Apr 18, 2016

calebmer Apr 18, 2016

mjackson commented Apr 18, 2016 •

edited

Loading

mjackson Apr 18, 2016

Add object includes a string key and exclusion test for objects #86

Add object includes a string key and exclusion test for objects #86

Conversation

calebmer commented Apr 9, 2016

Choose a reason for hiding this comment

ljharb commented Apr 9, 2016

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

Choose a reason for hiding this comment

calebmer commented Apr 9, 2016

Choose a reason for hiding this comment

ljharb commented Apr 9, 2016

calebmer commented Apr 16, 2016

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

mjackson commented Apr 18, 2016 • edited Loading

Choose a reason for hiding this comment

mjackson commented Apr 18, 2016 •

edited

Loading