search

Automattic / expect.js

Minimalistic BDD-style assertions for Node.JS and the browser.
2.1k stars 208 forks source link

"Not have keys" assertion seems unintuitive #105

Open aseemk opened 10 years ago

aseemk commented 10 years ago

If I write:

expect(obj).to.not.have.keys('password', 'secret', 'token');

You'd think it would fail if the object had any of those keys. But it actually passes even if it has one or more of them... as long as it doesn't have all of them.

I get why this is: because expect(obj).to.have.keys(keys) asserts that all of the given keys are there, so negating that means negating "all" means "none or some".

But this bit me pretty hard just now, so I wonder if you'd be open to special-casing this negative assertion to be the more defensive and, arguably, intuitive and useful.

maximilianschmitt commented 10 years ago

I spent the last half hour searching for a bug in my code and tests because of this behavior. Your explanation makes sense on why the behavior is logical but unintuitive. Thanks for that!

mattlucock commented 10 years ago

+1

HarryPehkonen commented 10 years ago

I see @aseemk opened an issue with Chai.js with the same symptom:

https://github.com/chaijs/chai/issues/254

Should be careful with any changes in behaviour here. Especially if they will differ from other libraries.

aseemk commented 10 years ago

Yep, we use both Chai and expect.js in our app. (We'd like to consolidate on one, but several of the APIs are different, so we haven't prioritized updating all of our existing tests.)

dkillebrew commented 10 years ago

I ran into this recently. I would say the current implementation violates the principle of least surprise. It's clear that the current API is unclear and a source of bugs for some subset of users.

My suggestion is to add 2 new flags that are only relevant when checking keys, 'any' and 'all'. expect(obj).to.not.have.any.keys('password', 'secret', 'token'); or expect(obj).to.not.have.all.keys('password', 'secret', 'token');

The first is what some people (including my team and I) want. It is an error if any of the keys are present in obj.

The second is the current behavior. It is an error if all of the keys are present in obj.

With the new flags, the behavior of both is obvious (to me, at least).

To avoid breaking API changes, we can leave the current behavior alone, i.e. if you don't specify 'any' or 'all', the 'all' behavior is used. I think it would be wise to deprecate not using either of the flags, and require a flag at some future date.

aseemk commented 10 years ago

Love your suggestion, @dkillebrew. +1 from me.