search

moll / js-must

An assertion library for JavaScript and Node.js with a friendly BDD syntax (awesome.must.be.true()). It ships with many expressive matchers and is test runner and framework agnostic. Follows RFC 2119 with its use of MUST. Good stuff and well tested.
Other
336 stars 35 forks source link

Add assertions/matchers for sinon #25

Closed estilles closed 9 years ago

estilles commented 9 years ago

One of the reasons I had preferred chai to must in the past was it's ecosystem of community provided plugins. sinon-chai is one that I use frequently. I hope we can agree on a way to achieve #24, so that we can start providing equivalent plugins for must.

moll commented 9 years ago

Hey again!

Adding support for Sinon to Must.js itself is probably outside its scope, but please do let me know how I can help make it easy for you to create a plugin for it. :-)

Let's carry the plugin conversation in an older issue, https://github.com/moll/js-must/issues/12. I'll close this one for now as it isn't actionable within Must.

Thanks again!

estilles commented 9 years ago

@moll, I know 0.13.0 is not yet ready, but would you mind taking a look at my plugin implementation for Sinon matchers?

Also, I know that with the exception of .not, your not very keep with passthrough properties setting flags not the Must object. However, I have a series of matchers that all contain "always". I would like to implement .always as a passthrough property, but it would require setting a flag similar to .not.

Current Matcher Would become ...
alwaysCalledOn() always.calledOn()
alwaysCalledWith() always.calledWith()
alwaysCalledWithExactly() always.calledWithExactly()
alwaysCalledWithNew() always.calledWithNew()
alwaysReturned() always.returned()

Would you be okay with that? This would considerably simplify my plugin.

moll commented 9 years ago

Hey!

Yeah, need a little bit of tweaking of eql to finish v0.13. After that I'll have to go through https://github.com/moll/js-must/issues/21 and https://github.com/moll/js-must/issues/6 and decide how to handle those for v1. Must.js been sitting on the 0 version for way too long. :-)

So, back to your Sinon matchers. Well, I can't stop you from adding always, but if I were you, I'm not sure I'd do so. ;-)

First, it doesn't really make it more readable — it contains the same amount of characters and the separation between a dotted always and the rest of the camel cased matcher feels arbitrary. Was italways.calledWithNew or always.called.withNew, is a question one might ask.

Second, always, as a modifier property doesn't apply equally to all matchers, only a subset — the Sinon matchers. I find that inconsistency inelegant. Reminds me of Chai and its "deep" et alii modifiers, which I couldn't make a simple mental model out of.

[{a: 1}, {b: 2}].must.always.have.property("a")

The above looks as if it works, but really doesn't.

Which reminds me, you'll be coming up against the same always-never problem as described in https://github.com/moll/js-must/issues/6. That is, the not modifier inverts the whole assertion without paying attention to what the sentence may imply. That is, while always sounds and behaves like all, never (being an alias to not) behaves like !all, but sounds like !some. In other words, spy.must.never.have.been.calledWith(1) will succeed if a single call invoked it with something other than 1.

You'll probably end up having to make never into a modifier too as it won't work as an alias of not. But then there's not much to gain by having dots between never and calledWith. They might as well be one matcher with less magic.

moll commented 9 years ago

I've been wanting to add all, any (a.k.a every, some) to Must.js once I figure out how to make it compose with other transforms like with then and reject. Those would, like the promise support, apply to all matchers. Would this pattern be of use to you? Though, I'm not sure how much of an improvement a dedicated specific collection transform would be compared to accessing them directly on a spy.

spy.must.have.alwaysBeenCalledWith(model)
spy.must.have.had.thisValues.equal(model)
spy.thisValues.must.all.equal(model)

thisValues — Just picked a random name for a transform that picks the thisValues from a spy to assert on. It does sure read a little weird. :-)

estilles commented 9 years ago

TL;DR ... you have been warned. ;-)

First, I'd like to say thanks for the work on 0.13 it's really helped the progress towards plugins.

Now, I think I must respectfully disagree with some of your comments above, unless of course I'm completely misinterpreting the intention behind having community developed plugins.

IMHO:

  1. Plugins should be able to add matchers, modifier properties, and passthrough chainable properties that don't already exist in Must.js (e.g. those for Sinon)
  2. The intent of matchers, modifier properties, and passthrough chainable properties added by plugins is to add domain specific language that will be used to describe assertions.
  3. Modifier properties and passthrough chainable properties added by plugins need NOT be universally applicable since they may be domain specific (and by definition not applicable to other domains).
  4. Plugins should be able to override/enhance Must.js core matchers and properties if it so chooses, so that they serve a particular domain. However, this behavior must be intentional, hence there should be some way to detect "unintentional" collisions.
  5. Plugins that implement domain specific matchers/modifiers and/or override Must.js core matchers/modifiers MUST be properly documented to avoid confusion among its consumers.

With regards to some of your comments:

always, as a modifier property doesn't apply equally to all matchers, only a subset — the Sinon matchers.

Yes, that is exactly the point. The plugin is meant to be used specifically with Sinon and its modifier properties are not intended to be used outside the Sinon domain.

it doesn't really make it more readable

I guess the examples I provided aren't representative of my intended syntax. The idea was for:

spy.must.have.been.alwaysCalledWith(context)

to read as:

spy.must.have.always.been.calledWith(context)

... so the assertion of more English-like.

spy.must.never.have.been.calledWith(1) will succeed if a single call invoked it with something other than 1.

Yes, that is exactly what it does and what it means. It should succeed no matter how many times spy was invoked, so long as none of those calls where with 1. So spy.must.never.have.been.calledWith(1) is supposed to be equivalent to spy.must.not.have.been.calledWith(1). In the Sinon domain never is equivalent to not. Again, the expectation should not be that modifier properties in the Sinon domain have the same meaning in other domains.

This won't necessarily be true for all plugins. There may be plugins which add matchers and properties that are universally applicable, but I don't think that should be the rule because that would considerable limit the fluidity of the assertion syntax a plugin could create for a particular domain.

All this being said, once the community starts creating the likelihood of collisions between plugins will be inevitable. While all plugins MUST be compatible with Must.js, they may not necessarily be compatible with all other plugins.

As it stands right now, since the current architecture requires plugins to inject themselves into Must.js, it would be the plugin's responsibility to detect/handle any such collisions. That, IMHO, creates a significant amount of unnecessary boilerplate/overhead for the plugin. In addition to the fact that plugin developers may not (either by choice or ignorance) properly handle collision detection.

Personally, I would have preferred if Must.js provided a unified interface for plugins. An interface that will:

  1. allow plugins to add multiple matchers/properties via configuration object
  2. allow plugins to add individual matchers/properties via method calls
  3. detect/handle unintentional collisions (the interface should inform consumers which plugins have conflicting implementations, so they may inform plugin developers who may not be aware of the conflict)
  4. allow for "intentional" overriding of matchers/properties

I believe such an interface would not only simplify by also promote plugin development. A single/unified interface would be much easier to debug as well. Imagine 10 different plugins with 10 different mechanisms to inject themselves into Must.js.

The addition of all, any, etc. to the Must.js core is an excellent idea. Not sure how much value it could have for must-sinon, but there are other plugins I already have in the pipeline in which they will be most definitely useful.