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

#include(object) #43

Open
kaievns opened this issue Apr 28, 2016 · 8 comments
Open

#include(object) #43

kaievns opened this issue Apr 28, 2016 · 8 comments

Comments

@kaievns
Copy link

kaievns commented Apr 28, 2016

Hey @moll, I've got a feature request, if you'd be so kind

Wouldn't it be brilliant if the #include method would match objects agains subset objects? Like for example this:

it('throws 404 error', function* () {
  const response = yield request('/non-existing');
  response.must.include({status: 404, body: 'Not found'});
});

Matching the nested subsets might be a bit tricky, but worth the effort i think.

Thanks!

@moll
Copy link
Owner

moll commented Apr 28, 2016

Hey! Thanks for taking the time to think along! Definitely, something akin to that would be useful.

Going further, I think there are a few related possible matchers:

  1. Matching multiple properties together (what you proposed). What now is:

    res.statusCode.must.equal(400)
    res.body.must.equal("Not Found")

    ..could be combined. There's Must.prototype.property for checking a single property. What about using the plural of that for checking more than one strictly (===)?

    res.must.have.properties({statusCode: 404, body: 'Not found'});
  2. A version of the above for checking as Must.prototype.eql does:

    ({name: "Amy", parent: {name: "John"}}).must.have.$some_matcher({parent: {name: "John"}})
  3. A version of include that searches for values in arrays and objects by comparing them as `Must.prototype.eql does:

    [{name: "John"}, {name: "Mike"}].must.$some_matcher({name: "John"})
  4. A version of include that searches for value in arrays and objects by a subset of their values, like point 1 above.

And a few permutations of the above with shallow and deep options. Adding them all would probably get too complicated for anyone to remember. Just listing them out for completeness.

Coming back to your idea, would starting with properties for shallow strict checking fulfill what you had in mind? And possibly one (still shallow) version of it for matching with eql-style for plain and value objects?

@kaievns
Copy link
Author

kaievns commented Apr 28, 2016

hey, thanks for a quick response!

so, the option number 1. is how i do it at the moment, but it is a bit of a pain, plus it is not going to work with promises (.must.resolve.to.have.property('status', 404); ....?)

but, I like the idea of #properties, that would do the trick i think.

although, #includes is the first thing i'd go for tbh. it works with strings and arrays, so i'd expect it to work with objects as well. but that's just me. as i said, #properties will be fine if you feel strongly about it

@moll
Copy link
Owner

moll commented Apr 29, 2016

Morning!

Actually I think you could use the former with promises.

// When using Mocha to return a promise:
return Promise.all([
  obj.must.then.have.property("foo", 1),
  obj.must.then.have.property("bar", 2),
])

// When using Co to drive the generators:
yield [
  obj.must.then.have.property("foo", 1),
  obj.must.then.have.property("bar", 2),
]

Using a single combined matcher will be shorter though. :)

The existing include matcher works with (plain or not) objects, but it just checks if they have a any key with the given value. I try to avoid overloading based on argument types in Must.js to reduce the chance of JavaScript's peculiarities causing bugs. If there were an overload, someone will end up calling obj.must.include(new Date(2000, 5)) and with Dates being empty objects, cause a false positive. That's why I'd add a new matcher for [shallowly] asserting on multiple properties at once.

@kaievns
Copy link
Author

kaievns commented Apr 29, 2016

makes sense. and i definitely appreciate the thoughtfulness you bring into designing the API!

as for the promises I guess i meant a case that look somewhat like this one:

it('returns the good data back', () => {
  return request('/some/url').must.resolve.to.include({
    status: 200,
    body:  {some: 'data'}
  });
});

I'm not sure if there is a way i can have two assertions in a situation like this one. I normally just use generators or async/await and match both properties separately. But, unfortunately, not everyone is keen on using generators for an async control flow; sometimes you have to fall back to promises. And then I'd have to have an extra step, like .then(res => ({status: res.status, body: res.body}) and match it cleanly. Which, in my view obscures the intent a bit. .resolve.to.include({this and that}) would communicate the requirements more clearly I think

@moll
Copy link
Owner

moll commented May 22, 2016

Hey again!

I added Must.prototype.properties and Must.prototype.ownProperties (https://github.com/moll/js-must/blob/master/CHANGELOG.md). While they don't yet cover the nested object assertion example you have above ({body: {some: data}}), they should be a start. :)

Would you mind giving it a quick sanity check before I cut a stable release? You can install from here with npm install moll/js-must. Thanks!

@kaievns
Copy link
Author

kaievns commented May 23, 2016

it looks pretty good to me, but i think matching nested params needs to be there. otherwise it will just explode unexpectedly on users

@moll
Copy link
Owner

moll commented May 23, 2016

Absolutely, a "nesting" matcher will be useful, too. While I don't find the short-is-more-lax pattern (inspired by == vs ===) particularly intuitive, I'm thinking of naming the eql-based matcher props to be consistent for now. Until a better naming scheme comes along at least. What do you think?

@kaievns
Copy link
Author

kaievns commented May 23, 2016

come to think about it, this is a really good idea. then it makes perfect sense

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

2 participants