Automattic / expect.js

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

Allow .and chaining for .ok and .throwError #64

Closed gabalafou closed 3 years ago

skeggse commented 10 years ago

LGTM, though would it not make more sense to test the function throwing in the opposite order?

expect(function () { throw 'foo'; }).to.be.a('function').and.to.throwError();

If the object is not a function, it would just fail when attempting to invoke it, no?

gabalafou commented 10 years ago

No, because that does not demonstrate that .throwError now allows chaining. You need to have .throwError().and

Both additions to the unit test code that I made were to demonstrate that chaining now works after invocations to .throwError and .ok

skeggse commented 10 years ago

Yeah, I got that, but I'm not sure what real use-case it has. It seems to me like you'd verify the function meets other expectations before you invoke it. Not saying this shouldn't be added, it looks like a nice addition.

gabalafou commented 10 years ago

I added chaining to .throwError because I had written a test that utilized such chaining for some real world code where I work. However, I don't remember what that test line looked like, nor if it was actually a good use case. :-)

However, I think API self-consistency is sufficient justification for a change like this; I don't think one needs to come up with a good use case for said consistency. It's the opposite—it is the inconsistencies within an API—that need justification through concrete examples.

And in expect.js, every other Assertion method is chainable because every other Assertion method returns this. If .throwError and .ok are going to break from this pattern, then there should be a really good reason for it, and it ought to be documented.

skeggse commented 10 years ago

However, I think API self-consistency is sufficient justification for a change like this; I don't think one needs to come up with a good use case for said consistency. It's the opposite—it is the inconsistencies within an API—that need justification through concrete examples.

Ha! Well said :smiley:

truongsinh commented 10 years ago

@gabalafou I agree that this should be documented. I think it is not about the API self-consistency, but good habit. The lib makes developer do the good way; .ok and .throwException should be terminal flag

gabalafou commented 10 years ago

Why should .ok, .throw, .empty, and .fail be terminal? Why is that good habit?