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

Make object "has" matcher recursive #1422

Merged
merged 4 commits into from
May 25, 2017

Conversation

mareq
Copy link
Contributor

@mareq mareq commented May 25, 2017

Purpose (TL;DR)

Enable matching properties of embedded objects using sinon.match.has matcher.

Background (Problem in detail)

"test should assert fuzzy": function () {
    var book = {
        pages: 42,
        author: {
            name: "cj"
            surname: "no" 
        }
    };
    var spy = sinon.spy();
    spy(book);

    // currently, this will NOT match (entire object has to be passed as value)
    sinon.assert.calledWith(spy, sinon.match.has("author", { name: "cj" } ));

    // this pull request enable this to match
    sinon.assert.calledWith(spy, sinon.match.has("author.name", "cj" ));
}

Solution

createPropertyMatcher has been modified such that:

  • existence of "dot.separated.embedded.property" is checked using recursive helper function
  • deepEqual is run only on the value of the requested property, not on the whole top-level object

How to verify - mandatory

  1. Check out this branch (see github instructions below)
  2. npm install
  3. npm test (test case for the new functionality has been added)

@coveralls
Copy link

coveralls commented May 25, 2017

Coverage Status

Coverage decreased (-0.1%) to 94.755% when pulling d04f7af on mareq:mareq/recursive-object-has-matcher into 15871b2 on sinonjs:master.

@mantoni
Copy link
Member

mantoni commented May 25, 2017

I can see how this simplifies the notation when testing details in large objects. However, there is already a way to do the same thing, and I'm not sure if @sinonjs/sinon-core wants to introduce an alternative.

sinon.assert.calledWith(spy, sinon.match.has("author", sinon.match.has("name", "cj")));

Also, consider using calledWithMatch:

sinon.assert.calledWithMatch(spy, {
  author: sinon.match.has("name", "cj")
});

@coveralls
Copy link

coveralls commented May 25, 2017

Coverage Status

Coverage remained the same at 94.896% when pulling 1035e77 on mareq:mareq/recursive-object-has-matcher into 15871b2 on sinonjs:master.

@coveralls
Copy link

coveralls commented May 25, 2017

Coverage Status

Coverage remained the same at 94.896% when pulling 0d9c367 on mareq:mareq/recursive-object-has-matcher into 15871b2 on sinonjs:master.

@mareq
Copy link
Contributor Author

mareq commented May 25, 2017

@mantoni: Thanks for hint, I did not know about the functionality already existed (actually tried that composition way first, but because of my mistake it did not work - I like the composition more in fact).

One of the reasons (apart of me trying to match stuff on serialised string instead of real object) why I did not know the feature is already present is that I did not find unit test nor documentation for it, so I have added both. Also reverted my original changes.

@mantoni
Copy link
Member

mantoni commented May 25, 2017

@mareq Nice, thank you! You've added the documentation specifically for the v17 release. Can you add the documentation changes also to /docs/release-source/release so that it's part of the current docs and in every future release?

@mareq mareq force-pushed the mareq/recursive-object-has-matcher branch from 0d9c367 to a0115f8 Compare May 25, 2017 09:20
@coveralls
Copy link

coveralls commented May 25, 2017

Coverage Status

Coverage remained the same at 94.896% when pulling a0115f8 on mareq:mareq/recursive-object-has-matcher into 15871b2 on sinonjs:master.

@mareq
Copy link
Contributor Author

mareq commented May 25, 2017

@mantoni Done: amended my last commit to move it from v17 to /docs/release-source/release

@mareq mareq force-pushed the mareq/recursive-object-has-matcher branch from a0115f8 to 13b93e1 Compare May 25, 2017 09:24
@coveralls
Copy link

coveralls commented May 25, 2017

Coverage Status

Coverage remained the same at 94.896% when pulling 13b93e1 on mareq:mareq/recursive-object-has-matcher into 15871b2 on sinonjs:master.

@mantoni mantoni merged commit ebe37da into sinonjs:master May 25, 2017
@mantoni
Copy link
Member

mantoni commented May 25, 2017

Thank you!

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

Successfully merging this pull request may close these issues.

None yet

3 participants