Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.Sign up
Concerns about `not.property` and `not.throws` #892
I'm having second thoughts about some of the recent changes made to
Consider these examples:
expect(myCat).to.not.have.property("numFleas", 7); expect(groomCat).to.not.throw(TypeError, "Invalid brush");
There are two ways to interpret these assertions:
On the surface, the new way seems more intuitive. But in actuality it encourages writing bad tests in the exact same manner that adding an
Interpreting the assertions the new way means that
When is it ever a good idea to test all of these different possibilities with a single assertion? Of course, you could always revert to the old way of interpretation by protecting your assertions like so:
expect(myCat).to.have.property("numFleas"); // Protection expect(myCat).to.not.have.property("numFleas", 7); expect(groomCat).to.throw(); // Protection expect(groomCat).to.throw(TypeError); // Protection expect(groomCat).to.not.throw(TypeError, "Invalid brush");
But it seems to me that the above protections should be there by default. I haven't been able to think of a single example when those protections should be removed. After all, without those protections in place, the new way of interpretation is just stringing together multiple assertions via "or".
I'm currently in favor of reverting the behavior of
Edit: Actually I think it was only
Hi @meeber, thanks for the detailed explanation, you always have interesting thoughts.
That's something we should definitely talk about.
When reading the first examples you've written the new way of interpretation is the first thing that comes to my mind. So, even though that encourages writing "bad" tests, I think that's the most obvious thing and I think that's our users would expect it to do.
We may be entering the realm of semantically obvious assertions versus educating users about it again. IMO our job is to write a library which does what people expect it to do when using certain methods, we're just giving them the best tool we can but it's their job to use it correctly.
When paying close attention to what happens whenever people use
IMO we should keep the new behavior since its the most obvious thing and since the root cause of this problem is not really the
Please let me know if you disagree or you have any further interesting thoughts
As an aside, it's worth noting that the assertions under discussion are very strange to begin with:
expect(myObj).to.not.have.property('myProp'); // Normal expect(myObj).to.not.have.property('myProp', unexpectedVal); // Strange; use expect(myObj).to.have.property('myProp', expectedVal) instead expect(myFn).to.not.throw(); // Normal expect(myFn).to.not.throw(unexpectedErrType); // Strange; use expect(myFn).to.throw(expectedErrType) instead expect(myFn).to.not.throw(unexpectedErrType, unexpectedMsg); // Strange; use expect(myFn).to.throw(expectedErrType, expectedMsg) instead expect(myFn).to.not.throw(unexpectedMsg); // Strange; use expect(myFn).to.throw(expectedMsg) instead
In almost every case, it's better to use the non-negated version of the assertion.
Anyway, there's actually an issue here that's completely separate and unrelated to the concept you described above regarding a
The separate issue only involves assertions such as
expect(groomCat).to.throwAnError() .and.expectTheErrorToBeATypeError() .and.expectTheErrorToHaveThisMessage("Invalid brush");
Unfortunately, we run into problems when applying
expect(groomCat).to.notThrowAnError() .or.expect(groomCat).to.throwAnErrorButNotATypeError() .or.expect(groomCat).to.throwATypeErrorButNotWithThisMessage("Invalid brush");
This is a horrible assertion, but it's what Chai's new interpretation does by default when
Anyway, it's much safer to instead interpret
expect(groomCat).to.throwAnError() .and.expectTheErrorToBeATypeError() .and.expectTheErrorToNotHaveThisMessage("Invalid brush");
That way, only one thing is being asserted, instead of three things combined with
I'm not so sure that this interpretation defies user expectations either. Partially because
I feel like someone writing:
Also keep in mind that we're already doing something like this with other assertions. For example:
Your argument regarding coding the
Actually the user is not saying that he wants a
Perhaps we could analyze the possibility of removing the message check from the
I'm still struggling to think of a single instance in which someone would want to use
Maybe the thing to do here is disallow
I browsed through all of our assertions, and here are the ones that suffer from this problem when paired with
expect(obj).not.include(obj); // Only a problem when more than one property pair in 2nd obj expect(obj).not.property(name, val); expect(obj).not.ownPropertyDescriptor(name, descriptor); expect(obj).not.keys(keys); // Only a problem when more than one key and `any` flag isn't set expect(fn).not.throw(errType); expect(fn).not.throw(errMsg); expect(fn).not.throw(errType, errMsg); expect(array).not.members(array); // Only a problem when more than one item in 2nd array
In each case above, multiple assertions are being performed internally, and each assertion is connected via
The questions are:
It's worth noting that in general there's very few instances in which it makes sense to use
As you've said yourself, it's better to assert on what is expected rather than asserting on what is not expected. Whenever using
Just by using
Again, here I feel like we're in a case of what we think is correct versus what is semantically more obvious. Whenever confronted with these two options I would choose the second one. I'd rather allow people to do things that are not the best solution but work as they expect it to than reducing their options.
However, I'm feeling more and more inclined to start adding warnings to our assertions in order to start educating people about it. Maybe we could write a few detailed
@lucasfcosta Good points. If Chai was a young project, a strong argument could be made about reversing the decision to build