Open koresar opened 10 years ago
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. :)
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
?
Or
myObj.must.atLeast.have.keys(...);
myObj.must.atLeast.ownKeys(...);
What do you think?
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. ;)
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.
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.
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?
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!
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" notifications@github.com 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/moll/js-must/issues/6#issuecomment-31199842 .
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)
I have edited the previous comment about dozen times. :) Sorry.
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
});
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)
}
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.
Привет. :)
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. :)
@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.
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.
Assume we have object:
We need to ensure the object has
a b c d e f
. So I used:My first impression of the
ownKeys
andkeys
functions was that they should check given properties for existence. However they also checkfoo
andbar
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
andownKeys
which would use different comparator: