Phrasing of the 'keys' function is misleading

[koresar]

Assume we have object:

var obj = { a: 1, b: 2, c: 3, d: 4, e: 5, f: 6, foo: 27, bar: 28 }

We need to ensure the object has a b c d e f. So I used:

obj.must.have.keys(['a', 'b', 'c', 'd', 'e', 'f']); 

My first impression of the ownKeys and keys functions was that they should check given properties for existence. However they also check foo and bar properties must be absent.

The underlying reason is that these functions use exports.eql comparator, which ensures the key arrays are 100% equal.

I would recommend to rename keys -> onlyKeys, and ownKeys -> ownOnlyKeys. And create two more functions keys and ownKeys which would use different comparator:

exports.incl = function ...

[moll]

Hey,

First off, sorry that this caused confusion!

The reasoning behind it was to be as strict as possible by default on all matchers. False negatives in testing are less harming than a false positives. In this case it meant asserting that only the listed keys are found and none else.

The case of confirming a subset of keys could then be handled with the existing property matcher in the style of:

obj.must.have.property("a")
obj.must.have.property("b")
obj.must.have.ownProperty("c")

It is a tad longer, but I don't yet have input of how common testing for a subset of key names is to know if this is a problem. What do you think?

Oh, and last, to have less surprises when migrating away from Should.js and Chai.js. :)

[koresar]

Hello, and sorry for the late reply.

Your reason to name those functions the way they are name is clear. Let me explain my case.

The coding principle I use is to create object using factories, not constructors.
I use: routeFactory.create(). I do not use: new Route().
The library also gives me the ability to chain factories: stampit().state(initialVars).methods(bar).enclose(something). This is the line I want to test with must-js. I want to make sure the resulting object of my factory contains the initialVars properties.

What do you think of adding new must-js functions, for example: includesKeys, or subsetKeys, or subsetOwnKeys?

[koresar]

Or

myObj.must.atLeast.have.keys(...);
myObj.must.atLeast.ownKeys(...);

What do you think?

[moll]

Thanks for your thoughts! I'll sleep on this a little and get back to you.

The atLeast.keys or have.keys syntax (like Chai does it) though would be something I'd steer away from. They're not as straightforward as a single function would be. ;)

[koresar]

That confuses. First you say that "to have less surprises when migrating away from Should.js and Chai.js". I thought that is something important.
Now you say "syntax (like Chai does it) though would be something I'd steer away from".
You should take one of the two possible paths: either be better than Should/Chai at every point, or preserve the compatibility with their APIs.

So.

1. The have.keys(...) you have adopted is really misleading. You shouldn't have done it.
   OR
2. You should adopt atLeast.keys(...) too.

[moll]

Aah, my bad for not elaborating what I meant.

So, less surprises when migrating is something I value, but that goes for things that are good design choices. Making better design decisions trumps any API compatibility. That was one of the reasons I started Must.js — to simplify and not make it as complex as others are.

I now realize my example of the have.keys syntax was unclear. Sorry. Must.js does indeed have a have property but that's entirely syntatic sugar. It doesn't affect any matchers. The way, however, that Chai uses some properties (like include) is that they affect matchers that come after them. In the case of keys, obj.should.include.keys(..) changes keys to check only a subset. That was the interdependency that I was criticizing.

[koresar]

I see. You think the syntax of have.keys is a good design choice, whereas I think it's a bad one. Okay. You're the author, I won't argue on that any more.

I can clearly see from the code what the have prop is. Mate, I and probably many others really need this atLeast subset matcher (and some other matchers too, but I'll borrow your attention later). :)

Any plans on adding other matchers?

[moll]

Definitely plan to add others. :)

Just to double check, do I understand correctly that the decision you consider a bad one is that the keys matcher checks for every key and not merely a subset? Or did it have anything to do with the have "sugar" property, too?

Also, thanks for caring!

[koresar]

Yeah, the first one. I thought "keys" matcher is a subset matcher. (See
first message of this issue.) I realise that "have" is just sugar.

Thanks for listening!
On Dec 26, 2013 1:11 AM, "Andri Möll" [email protected] wrote:

Definitely plan to add others. :)

Just to double check, do I understand correctly that the decision you
consider a bad one is that the keys matcher checks for every key and not
merely a subset? Or did it have anything to do with the have "sugar"
property, too?

Also, thanks for caring!

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/6#issuecomment-31199842
.

[koresar]

Okay, last attempt. Let's enhance include function? Allow not only single parameter, but also multiple.
So that people could check several keys for existence in a single line like this:

obj.must.contain(['a', 'b', 'c', 'd', 'e', 'f']); 

Untested sample code of must-js include function:

exports.include = function(expected) {
  var found;
  if (Array.isArray(expected)) { // New code starts here
    found = true;
    for (var i = 0; i < expected.length; i++) {
      var expectedKey = expected[i];
      if (!this.actual[expectedKey]) { found = false; break }
    }
  } // New code ends here
  else if (Array.isArray(this.actual) || kindof(this.actual) == "string")
    found = ~this.actual.indexOf(expected)
  else
    for (var key in this.actual)
      if (this.actual[key] === expected) { found = true; break }

  insist.call(this, found, "include", expected)
}

However, now my implementation checks both value(s) (when used with a single argument), and keys (when array is passed in). This can be also a confusion.

Mate, I really do not understand why the original must-js assertion function include checks for value. This is really a confusing syntax: object.must.include(someValueHere). Why value? In order to be consistent you should have gone with the following syntax:

obj.must.have.values(valueHere)

so that it looks like existing:

obj.must.have.keys(keyHere)

[koresar]

I have edited the previous comment about dozen times. :) Sorry.

[enobrev]

I had been similarly confused while reading over the api for must.have.keys, since "must have" doesn't explicitly designate the idea that it "must ONLY have" those keys. I was thinking that since expect(x).to.have.property(k [, v]) allows one to set a match against a single property of an object while disregarding others, might it possibly make sense to have something like:

// Check that specific keys exist
expect(x).to.have.properties(['k1', 'k2']);

// Check that specific key/value pairs exist and match
expect(x).to.have.properties({
    k1: 'v1',
    k2: 2
});

[enobrev]

I tried to match your coding style for a temporary implementation. I would submit as a pull request, but don't currently have the time to write unit tests unfortunately (and I'm not sure you'd want to go this route), but here's what I'm using in the meantime.

/**
 * Assert that an object has proprties `properties`.
 * Optionally assert they *equals* (`===`) to `value`.
 *
 * @example
 * ({life: 42, love: 69, liberty: 500}).must.have.properties(['life', 'love'])
 * ({life: 42, love: 69, liberty: 500}).must.have.properties({life: 42, love: 69})
 *
 * @method properties
 * @param properties
 */
exports.properties = function(properties) {
  var ok = true;
  if (Array.isArray(properties)) {
    var missing = [];
    for (var i = 0; i < properties.length; i++) {
      if (properties[i] in Object(this.actual) === false)
        missing.push(properties[i])
    }
    ok = missing.length === 0
  }
  else {
    var missing = {};
    for (var key in properties)
      if (this.actual[key] !== properties[key]) {
        missing[key] = properties[key]
        ok = false;
      }
  }

  insist.call(this, ok, "have properties", missing)
}

[koresar]

It's very good the must-js did not inherit the property assertion problems of Chai and Should. However must-js inherited their somewhat misleading API (including the one discussed here).

Andri Möll, I love your must-js lib, though I do not share same feeling to its API.

Привет. :)

[moll]

A few days ago I also realized that using the negative form of keys right now is either useless or confusing:

obj.must.not.have.keys(["name", "age"])

That would pass if the object had more or less than only those two keys. Which is a test I can't see any use in. So I might be leaning towards turning keys to a subset matcher after all. I'll review this once I've refactored Must's own tests.

And thanks @koresar and @enobrev to your other suggestions with matchers, too! I'll get back to you later in more detail. :)

[moll]

@koresar:

This is really a confusing syntax: object.must.include(someValueHere). Why value?

To be honest, that was fairly arbitrary. I didn't have a good argument for either checking inclusion of keys nor values, precisely because include could be said to apply to both in this case.

But now that I think about it, perhaps it does make more sense to check for values. Objects are associative arrays after all and if you check for values in regular arrays, you might as well check for them in associative arrays.

Overloading include to check for multiple objects wouldn't work, however, because that would break searching for a whole array inside another. But I do get the desire of a matcher that would check for multiple objects at once! Haven't decided on a name. Technically it'd be a superset matcher, but I always have to think twice to parse the phrase A must be a superset of B. :-) Suggestions welcome as always.

[Rob-Morris]

I wholeheartedly concur that the obj.must.not.have.keys test is both close-to-useless and also confusing, as it currently works; a subset match for keys would be much more (occasionally) useful.