Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add possibility to use custom equality comparator in #61

Open
bennidi opened this issue Mar 8, 2017 · 10 comments
Open

Add possibility to use custom equality comparator in #61

bennidi opened this issue Mar 8, 2017 · 10 comments

Comments

@bennidi
Copy link

bennidi commented Mar 8, 2017

I have recently been using the .have.properties() assertion a lot and in all my use cases I needed a value based comparison (deep equals) instead of ===

I would suggest to add an optional parameter to the properties method that will take a custom comparison function. The default value could be an implementation that uses '==='.

This is how it would look in coffeescript (sorry, for being too lazy to translate into ES)

isEqual = (a,b) -> a is b

(props, equals= isEqual) ->
  obj =@actual
  ok = @actual isnt null

  if ok then for key,value of props
      ok = key of obj and equals obj[key], props[key]
      if not ok then break
  @assert(ok, "have properties", {expected: props, diffable: true})
@moll
Copy link
Owner

moll commented Mar 8, 2017

Hey! Thanks for sharing your thoughts!

I've had that need occasionally myself, too, and have had an idea to add a Must.prototype.props that internally uses the same equality algorithm that Must.prototype.eql does (also covered in #43). Would that cover your case?

@bennidi
Copy link
Author

bennidi commented Mar 8, 2017

I believe that it would cover my case. However, I like the approach of customization more than introduction of different flavours of equal, eql, properties, props etc.
Taking inspiration from libraries like lodash or react I think that the pattern of allowing customization of internal parts of an algorithm covers more cases and leaves more control with the user. Providing sensible defaults also allows to maintain backward compatibility.

@moll
Copy link
Owner

moll commented Mar 8, 2017

That's true, but in this case wouldn't you say making it customizeable would make it more convoluted for the reader? The Must.prototype.properties matcher is, after all, equivalent to the 3 lines below, and that way any custom equivalence requirement could be written explicitly:

obj.a.must.equal(1)
obj.b.must.equal(2)
obj.c.must.equal(3)

While we're talking about equality, which I'd say is so often done so wrong in JavaScript codebases, have you seen my Egal.js (https://github.com/moll/js-egal/)? ^_^

@bennidi
Copy link
Author

bennidi commented Mar 8, 2017

In my scenario I am testing REST APIs, so I am comparing JSON structures. When I POST an entity to an endpoint and it returns me the stored representation of it, then I want the result to have the same structure as the original. That would be very simple to do if properties() used deep value equality.

I don't object introducing .props() method as it solves me usecase. It doesn't satisfy me own idea about API design though :)

@moll
Copy link
Owner

moll commented Mar 8, 2017

I'll do props for sure, but coming back to testing your responses, have you thought of just doing body.must.eql({a: 1, b: 2})? Where do the other properties come from that you don't want to check?

My controller tests are usually like this (ignore the generator stuff):

var model = yield db.create({accountId: this.account.id, title: "Work"})
var res = yield request("/activities/" + model.id)
res.statusCode.must.equal(200)
res.body.must.eql(serialize(model))

The serialize function I export from the controller and test that separately as a unit test. That way I can without worry use that to check controller responses.

@bennidi
Copy link
Author

bennidi commented Mar 8, 2017

That approach only works if the controller does not add or filter properties. The assumption that the database representation is structurally identical to the REST resource representation does not hold in some (many?) cases.

@moll
Copy link
Owner

moll commented Mar 8, 2017

Yep, my controller responses differ from the database or model instances too — that's why I mentioned theserialize function. If, however, the controller adds properties above and beyond the functional serialize, I usually do:

res.body.must.eql({__proto__: serialize(model), addedAttribute: 123})

I wouldn't do prototypical extension that way in the browser, but with finite runtime environments (like Node.js on V8), __proto__ is a great "standardized" feature.

@bennidi
Copy link
Author

bennidi commented Mar 8, 2017

Hmmm. I like it. Do you have some gists or other guides where you show a bit more about how you use must.js in production. I bet you have more "tricks".

@dzek69
Copy link
Contributor

dzek69 commented Apr 8, 2017

If I can add my thoughts - I'm against adding possibility to globally overrite the matcher.

Methods returning different values for same arguments but different environment configuration (that's what overriding matcher would be) is very confusing and misleading. Have you even wrote something in PHP near 5.0 - 5.4 versions? Hell. I know, they had (probably still have) A LOT of things that depended on server configuration that could break the code and you only want to add one, but still - it's smelly for me.

@dietergeerts
Copy link

Will this be added in the near feature? I am needing this.
For the moment, I use https://www.npmjs.com/package/is-subset with to.be.true()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants