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

Array matchers #1208

Merged
merged 5 commits into from Dec 20, 2016

Conversation

Projects
None yet
4 participants
@lucasfcosta
Member

lucasfcosta commented Dec 19, 2016

Purpose (TL;DR)

This adds the following array matchers as indicated by @mroderick at #1113:

  • match.array.equals(): matches when the arrays are equal (unstrict)
  • match.array.startWith(): matches sequence against start of array
  • match.array.endsWith(): matches sequence against end of array
  • match.array.contains(): matches sequence anywhere in the array

The equals matcher has also been renamed to deepEquals, because as @fearphage suggested the deepEquals name is more explicit since we're not doing a strict comparison between array elements.

Solution

I added a new matcher for each one of the suggested items and tests for each one of them too.

Here go a few considerations about the implementation:

  • At the deepEquals matcher I've added a length check to avoid iterating through array items in case they don't have the same length. This makes it faster to spot differences in most cases and also allows me to iterate through every item in actual later to finish the check
  • At the endsWith matcher I've used an offset in order to know from where we should start comparing items in order to match the last items in the actual array. I've added a comment to make that clear but let me know if you think that comment is unnecessary
  • At the contains matcher I'm using a simple indexOf call to find items into the actual array. I thought about mapping the actual array's items using an object (O(n)) in order to traverse it only once since we would then be able to find each element by an O(1) factor. Since we would also have to traverse the whole expectation array I believe that would be O(n * m) where n is actual's length and m is expectation's length. Let me know if I said anything wrong or if I'm overthinking. I avoided that solution because I was not sure it would be too much over engineering.
  • When creating a message for each one of the new matchers I've just used <nameOfMatcher>([x,y,z]), please let me know if you don't like that output format.
  • I also invoked Array.prototype's methods by using call to avoid incompatibilities with fake arrays or overwritten methods

How to verify

  1. Check out this branch (see github instructions below)
  2. npm install
  3. npm run test will run the tests I added to test/match-test.js.

Please let me know if I missed anything or if you disagree with any of these changes and I'll be more than happy to fix this PR.

Thanks everyone for reading this. You rock, thanks for maintaining this awesome project ❤️

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Dec 19, 2016

Coverage Status

Coverage increased (+0.02%) to 96.335% when pulling 0bd2c71 on lucasfcosta:array-matchers into 7bb7613 on sinonjs:master.

coveralls commented Dec 19, 2016

Coverage Status

Coverage increased (+0.02%) to 96.335% when pulling 0bd2c71 on lucasfcosta:array-matchers into 7bb7613 on sinonjs:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Dec 19, 2016

Coverage Status

Coverage increased (+0.02%) to 96.335% when pulling 0bd2c71 on lucasfcosta:array-matchers into 7bb7613 on sinonjs:master.

coveralls commented Dec 19, 2016

Coverage Status

Coverage increased (+0.02%) to 96.335% when pulling 0bd2c71 on lucasfcosta:array-matchers into 7bb7613 on sinonjs:master.

@fatso83

This comment has been minimized.

Show comment
Hide comment
@fatso83

fatso83 Dec 19, 2016

Contributor

This is great. Just want to have it browsed by someone else than me :-)

Contributor

fatso83 commented Dec 19, 2016

This is great. Just want to have it browsed by someone else than me :-)

@fearphage

Excellent work. Thank you!

I have a few small comments and questions.

Show outdated Hide outdated test/match-test.js
it("matches arrays with the exact same elements", function () {
var deepEquals = sinonMatch.array.deepEquals([1, 2, 3]);
assert(deepEquals.test([1, 2, 3]));
assert(!deepEquals.test([1, 2]));

This comment has been minimized.

@fearphage

fearphage Dec 20, 2016

Member

The negation here is easy to overlook. Could you change these to assert.isFalse throughout?

@fearphage

fearphage Dec 20, 2016

Member

The negation here is easy to overlook. Could you change these to assert.isFalse throughout?

This comment has been minimized.

@lucasfcosta

lucasfcosta Dec 20, 2016

Member

Yes, of course, that would be much more clear and explicit indeed.
Thanks for the feedback!

@lucasfcosta

lucasfcosta Dec 20, 2016

Member

Yes, of course, that would be much more clear and explicit indeed.
Thanks for the feedback!

Show outdated Hide outdated lib/sinon/match.js
return match(function (actual) {
// Comparing lengths is the fastest way to spot a difference before iterating through every item
var sameLength = actual.length === expectation.length;
return sameLength && Array.prototype.every.call(actual, function (element, index) {

This comment has been minimized.

@fearphage

fearphage Dec 20, 2016

Member

Any reason you didn't use actual.every(....)? Same question for all the array methods here (indexOf, toString, etc').

@fearphage

fearphage Dec 20, 2016

Member

Any reason you didn't use actual.every(....)? Same question for all the array methods here (indexOf, toString, etc').

This comment has been minimized.

@lucasfcosta

lucasfcosta Dec 20, 2016

Member

Sometimes people end up creating iterables without those methods, so if we just used actual.every or other methods directly on actual they might fail due to the lack of them being implemented. However they would still work when called this way. An example for this would be arguments. In order to make it an array and call these methods on it we need to use Array.prototype.slice.call before, since it doesn't have the other methods implemented.

Also, I think this approach is more functional and explicit, but this second reason is just a matter of personal opinion.

@lucasfcosta

lucasfcosta Dec 20, 2016

Member

Sometimes people end up creating iterables without those methods, so if we just used actual.every or other methods directly on actual they might fail due to the lack of them being implemented. However they would still work when called this way. An example for this would be arguments. In order to make it an array and call these methods on it we need to use Array.prototype.slice.call before, since it doesn't have the other methods implemented.

Also, I think this approach is more functional and explicit, but this second reason is just a matter of personal opinion.

This comment has been minimized.

@lucasfcosta

lucasfcosta Dec 20, 2016

Member

However, I was just thinking if we should act upon those iterables as if they were arrays, after all this matcher has the word array in it.

Please let me know what you think, I can change this without problems if you want me to 😄

Also, thank you both for the feedback, it's always good to have such a careful review. I usually learn a lot with it.

@lucasfcosta

lucasfcosta Dec 20, 2016

Member

However, I was just thinking if we should act upon those iterables as if they were arrays, after all this matcher has the word array in it.

Please let me know what you think, I can change this without problems if you want me to 😄

Also, thank you both for the feedback, it's always good to have such a careful review. I usually learn a lot with it.

This comment has been minimized.

@fatso83

fatso83 Dec 20, 2016

Contributor

Well, you could still cache the references to the functions to make it a bit less verbose.

const proto = Array.prototype;
const every = proto.every;
@fatso83

fatso83 Dec 20, 2016

Contributor

Well, you could still cache the references to the functions to make it a bit less verbose.

const proto = Array.prototype;
const every = proto.every;

This comment has been minimized.

@lucasfcosta

lucasfcosta Dec 20, 2016

Member

@fatso you're right!
I've just seen you doing that on the behavior module for push and it looks much better.
I think I'll start doing this too the next time I have to call Array methods like this. Thanks for the tip, I'm gonna update this PR as soon as I get home 😄

@lucasfcosta

lucasfcosta Dec 20, 2016

Member

@fatso you're right!
I've just seen you doing that on the behavior module for push and it looks much better.
I think I'll start doing this too the next time I have to call Array methods like this. Thanks for the tip, I'm gonna update this PR as soon as I get home 😄

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Dec 20, 2016

Coverage Status

Coverage increased (+0.02%) to 96.359% when pulling 363e3e4 on lucasfcosta:array-matchers into a465244 on sinonjs:master.

coveralls commented Dec 20, 2016

Coverage Status

Coverage increased (+0.02%) to 96.359% when pulling 363e3e4 on lucasfcosta:array-matchers into a465244 on sinonjs:master.

@lucasfcosta

This comment has been minimized.

Show comment
Hide comment
@lucasfcosta

lucasfcosta Dec 20, 2016

Member

Hi friends!
Here are the changes you have requested.

Thanks for the review, your considerations were great, specially the idea of assigning Array.prototype's methods to other variables in order to make the code less verbose. I tried to follow the same pattern you used on the behavior.js file for doing that.

Let me know if I missed something and I'll happily fix it, thanks again for reviewing 😄

Member

lucasfcosta commented Dec 20, 2016

Hi friends!
Here are the changes you have requested.

Thanks for the review, your considerations were great, specially the idea of assigning Array.prototype's methods to other variables in order to make the code less verbose. I tried to follow the same pattern you used on the behavior.js file for doing that.

Let me know if I missed something and I'll happily fix it, thanks again for reviewing 😄

@fatso83 fatso83 merged commit 1924a89 into sinonjs:master Dec 20, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.02%) to 96.359%
Details
@fearphage

This comment has been minimized.

Show comment
Hide comment
@fearphage

fearphage Dec 20, 2016

Member

@lucasfcosta Thank you for the contribution.

Member

fearphage commented Dec 20, 2016

@lucasfcosta Thank you for the contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment