`expect(inList).to.be.oneOf` assertion #534

Merged
merged 3 commits into from Oct 16, 2015

Projects

None yet

2 participants

@Droogans
Contributor

I see now why you wanted to name it oneOf, as opposed to in. The rules for matching against arbitrarily deep objects using _.isEqual() were not very clear. This PR looks to add just enough functionality to support most typical use cases.

This will reject anything in the expect part of the test that is an array or an object. It will only match vai list.indexOf(inList) > -1 allow for strict reference comparisons for all objects that are passed in. For deeply nested objects, only first level members are considered for matches.

Droogans added some commits Oct 13, 2015
@Droogans Droogans docs(contributing): Highlight test steps db21e48
@Droogans Droogans feat(matchers): Add value-in-list assertion
Adds a way to test if a value appears in a flat list. Compliments
`include.members` to reverse the order of the input. Now the list
appears in the assertion, as opposed to the `expect`.

Closes #532
179d470
@Droogans
Contributor

I'll push up a commit with a version bump once this gets slated for a merge into master (or you can just do it for me).

@keithamus keithamus and 1 other commented on an outdated diff Oct 13, 2015
lib/chai/core/assertions.js
+ *
+ * expect('a').to.be.oneOf(['a', 'b', 'c']);
+ * expect(9).to.not.be.oneOf(['z']);
+ *
+ * @name oneOf
+ * @param {Array<*>} list
+ * @param {String} message _optional_
+ * @api public
+ */
+
+ function oneOf (list, msg) {
+ if (msg) flag(this, 'message', msg);
+ var expected = flag(this, 'object');
+ new Assertion(list).to.be.an('array');
+ new Assertion(expected).to.not.be.an('array');
+ new Assertion(expected).to.not.be.an('object');
@keithamus
keithamus Oct 13, 2015 Member

I wonder if this requirement really matters? Would it be so wrong to have oneOf work with object or arrays? Referential equality would be fine for an initial implementation - we can add .deep.oneOf() later if we need to.

@Droogans
Droogans Oct 13, 2015 Contributor

I could probably get that in there. Just so you know, it'll act like Python's in keyword. Meaning:

>>> [3] in [1, 2, [3]]
True
>>> [4] in [1, 2, [3, [4]]]
False
>>> [3, [4]] in [1, 2, [3, [4]]]
True

In other words, unless I'm matching something from the first level all the way to the end of that first level object-type, it'll fail. That's the simplest way I can think to handle object-types inside of a list without a deep flag.

@keithamus
keithamus Oct 13, 2015 Member

Without the restrictions, and without deep, I'd expect the following:

expect(3).to.be.oneOf([1,2,3]) // pass
var three = [3];
expect(three).to.be.oneOf([1,2,three]) // pass
expect([3]).to.be.oneOf([1,2,[3]]) // fails because [3] !== [3] (two different array literals)

The above is fine and completely expected and acceptable piece of JavaScript. We can add .deep later to do more interesting things, but there's not reason to stop people doing the above.

@Droogans
Droogans Oct 13, 2015 Contributor

Ok I'll do that. I was using _.isEqual (which is like == for objects in lodash), but now I will use strict referencing via ===.

@keithamus
Member

@Droogans don't worry about version bumping or anything - that's handled as part of the release process.

@Droogans
Contributor

@keithamus I'm not sure you caught this but I added object support to the PR.

@keithamus
Member

Sorry @Droogans, I did not. Github (annoyingly) only notifies when someone comments in a PR, not when they push up new changes. For future, I'd recommend any changes to a PR are followed up with a comment to bump to the relevant project, until Github gets their act together 😉

@keithamus keithamus and 1 other commented on an outdated diff Oct 15, 2015
lib/chai/core/assertions.js
+ function oneOf (list, msg) {
+ if (msg) flag(this, 'message', msg);
+ var expected = flag(this, 'object');
+ new Assertion(list).to.be.an('array');
+ var assertion = function () {
+ var matched = false;
+ // without a `deep` flag set, only examine top-level members. Do not recurse.
+ list.forEach(function (topLevelMember) {
+ // must be an exact reference to match. There are no comparisons for content!
+ if (topLevelMember === expected) {
+ matched = true;
+ return false;
+ }
+ });
+ return matched;
+ };
@keithamus
keithamus Oct 15, 2015 Member

We didn't have this function in the previous commits did we? I'm not sure it is needed. indexOf should work just as well here, no?

(from http://www.ecma-international.org/ecma-262/6.0/index.html#sec-array.prototype.indexof)

indexOf compares searchElement to the elements of the array, in ascending order, using the Strict Equality Comparison algorithm (7.2.13), and if found at one or more indices, returns the smallest such index; otherwise, −1 is returned.

I think having list.indexOf(expected) !== -1 would work just as well. What do you think?

@Droogans
Droogans Oct 15, 2015 Contributor

@keithamus I put it back...I was halfway done with _.isEquals-style object comparison (content matches, not references). I forgot that I could use the simpler indexOf once you asked for reference-based matching.

I force pushed it up to keep the history tidy.

@keithamus keithamus commented on the diff Oct 15, 2015
test/assert.js
+ assert.oneOf(1, { a: 1 });
+ }, 'expected { a: 1 } to be an array');
+
+ err(function() {
+ assert.oneOf(9, [1, 2, 3], 'Message');
+ }, 'Message: expected 9 to be one of [ 1, 2, 3 ]');
+
+ err(function() {
+ assert.oneOf([3], [1, 2, [3]]);
+ }, 'expected [ 3 ] to be one of [ 1, 2, [ 3 ] ]');
+
+ err(function() {
+ assert.oneOf({ four: 4 }, [1, 2, { four: 4 }]);
+ }, 'expected { four: 4 } to be one of [ 1, 2, { four: 4 } ]');
+
+ });
@keithamus
keithamus Oct 15, 2015 Member

This is a really nice comprehensive set of tests. Thanks for this.

@keithamus
Member

Wonderful work @Droogans! Also thank-you for keeping a really nice commit history - I was considering adding some contributing guidelines to enforce Angular style commit messages, and you may have just made a convincing argument with your lovely tidy commits.

@keithamus keithamus merged commit 6472cb9 into chaijs:master Oct 16, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@marekventur marekventur referenced this pull request in Gamevy/pixi-packer Dec 14, 2015
Merged

Update "chai" to version ^3.4.1 #15

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