Maps matchers #1215

lucasfcosta · 2016-12-27T20:25:27Z

Purpose (TL;DR) - mandatory

As discussed on #1210, this adds the following matchers for the Map type:

sinon.match.map(map) - Requires an object to be a Map
sinon.match.map.deepEquals(map) - Requires a Map to be deep equal another one
sinon.match.map.contains(map) - Requires a Map to contain each one of the items the given map has.

This also adds tests for each matcher and updates the docs.

Background (Problem in detail)

Due to the fact that the order of elements should not mean anything in a Map (and we can't really have any item's index), I didn't include the startsWith and endsWith matchers for Maps.

Since the old matchers should work for Array objects only, the new ones for Map and Set objects will be strict as well.

This PR adds matchers for Maps only, I'll be working on the Sets matchers as soon as this gets merged. I'm submitting this one first because I want to make sure we all agree on the spec before going further.

How to verify - mandatory

Check out this branch (see github instructions below)
npm install
npm run test - This will run the newly created tests

Let me know if I missed something or if there's anything to improve and I'll happily update this PR. It's always great to hear your feedback.

Thanks for reading this 😄

coveralls · 2016-12-27T20:27:06Z

Coverage decreased (-0.8%) to 95.543% when pulling eed4caf on lucasfcosta:maps-matchers into 02bbc83 on sinonjs:master.

fatso83 · 2016-12-27T23:58:48Z

Damn, you're productive around XMas! I am busy having holidays so will need to check back.

mroderick · 2016-12-28T09:31:45Z

AFAICT, Map wasn't implemented in IE until IE11.

Will the saucelabs tests error out in IE10 when merged into master?

I would suggest using https://github.com/medikoo/es6-map in the test file, to ensure that the tests can run, even when Map is not implemented in the JavaScript engine.

mroderick

Thank you for the PR @lucasfcosta.

I only have a few comments.

mroderick · 2016-12-28T08:56:34Z

lib/sinon/match.js

-var arrayToString = Array.prototype.toString;
+
+// This is an `every` implementation that works for all iterables
+var every = function (obj, fn) {


Would you mind extracting this to it's own file in lib/sinon/util/core/, and sprinkle a few tests?

mroderick · 2016-12-28T08:56:48Z

lib/sinon/match.js

+    return pass;
+};
+
+var iterableToString = function (obj) {


Would you mind extracting this to it's own file in lib/sinon/util/core/, and sprinkle a few tests?

lucasfcosta · 2016-12-28T20:53:06Z

Thanks for the feedback @mroderick, that's a great idea indeed! It will be a lot better to test these functions individually just to make sure they're working whenever we need to make changes on methods that use them. It makes the whole code a lot safer to change.

Regarding the IE10 tests, they (hopefully) won't fail on saucelabs because I've added guards to ensure the Map matchers will only run when Map is implemented. We also have these same checks on Chai's tests and everything runs fine on saucelabs. IMO adding that module in order to be able to run Map related tests on older browsers wouldn't add any value to the testing process since we will be relying on a third-party package in order to test something that doesn't even exist on that platform.

Let me know if I misunderstood what you've said or if you disagree. I can do whatever you think it's better. 😄

@fatso83 hahahaha I'm a bit lonely this during this year's holidays so I'm having a lot of time to work on Open Source and study things. Don't worry about reviewing this fast, I'm already very grateful for you all to be reading these PRs and giving me feedback.

Make sure you spend quality time with your friends and loved ones on these holidays 😄

coveralls · 2016-12-28T22:16:32Z

Coverage decreased (-0.8%) to 95.506% when pulling c2a8bd7 on lucasfcosta:maps-matchers into 3ce4717 on sinonjs:master.

coveralls · 2016-12-28T22:26:17Z

Coverage decreased (-0.8%) to 95.506% when pulling 6e3a677 on lucasfcosta:maps-matchers into 3ce4717 on sinonjs:master.

lucasfcosta · 2016-12-28T22:28:19Z

I have just move those methods to the core and added tests for them.
That also helped me spot some improvement possibilities such as stopping the forEach loop by using a throw if any of the elements is false. This is the only way to stop that loop since forEach is the only method available for Map, Set and Array objects.
Let me know if I missed anything!

EDIT: I have just noticed the coverage decreased because the Maps tests did not run because the environment which submitted its coverage reports didn't support it. Maybe it would be a good idea to add the module @mroderick suggested, to make sure the coverage reports will still be uniform for all platforms. What do you think?

fatso83 · 2016-12-29T11:05:28Z

Adding code that patches the run system in Sinon is not a good idea. It will lead to false negatives: running tests using Sinon will then give the impression that code works on platforms that does not support those features. But as soon as one removes Sinon the code starts breaking. That reminds me that I made a mistake in accepting the resolves and rejects functionality, since that included a polyfill.

I don't mind adding a polyfill in the test code, mind you.

mroderick · 2016-12-29T12:40:27Z

lib/sinon/util/core/iterable-to-string.js

+"use strict";
+var typeOf = require("../../typeOf");
+
+module.exports = function (obj) {


To aid in debugging, especially in less capable JavaScript engines, it is a good practice to name essential functions. I would suggest naming this one iterableToString.

I totally agree, I always name my functions whether they get assigned to a variable or are used only once, I just thought you prefer these functions to not have names due to what I've seen in other files.

Thanks for the feedback!

mroderick · 2016-12-29T12:44:32Z

lib/sinon/util/core/iterable-to-string.js

+        return typeof item === "string" ? "'" + item + "'" : String(item);
+    }
+
+    if (typeOf(obj) === "map") {


In my eyes iterableToString is a higher abstraction function that covers two input scenarios, Map or generic input.

I would suggest de-structuring the iterableToString function to call lower abstraction functions, perhaps something like this.

"use strict"; var typeOf = require("../../typeOf"); function stringify(item) { return typeof item === "string" ? "'" + item + "'" : String(item); } function mapToString(map) { var representation = ""; map.forEach(function (value, key) { representation += "[" + stringify(key) + "," + stringify(value) + "],"; }); representation = representation.slice(0, -1); return representation; } function genericIterableToString(obj) { var representation = ""; obj.forEach(function (value) { representation += stringify(value) + ","; }); representation = representation.slice(0, -1); return representation; } module.exports = function iterableToString(obj) { if (typeOf(obj) === "map") { return mapToString(obj); } return genericIterableToString(obj); };

Excellent idea, it really looks better.
Thanks for the help!

mroderick · 2016-12-29T12:46:45Z

test/util/core/iterable-to-string-test.js

+    });
+
+    it("returns an String representation of Map objects", function () {
+        if (typeof Map === "function") {


Wouldn't it be better to wrap the it statement with the conditional, like you did in match-test.js?

Yes it would be better since then this test would not count as passed, it would just be ignored.
Well noticed, thanks!

mroderick · 2016-12-29T12:47:36Z

test/util/core/iterable-to-string-test.js

+    });
+
+    it("returns an String representation of Set objects", function () {
+        if (typeof Set === "function") {


Same question as above regarding wrapping the itstatement with the conditional

lucasfcosta · 2016-12-29T14:39:09Z

Thanks everyone for your feedback, I've just learned a lot by reading your considerations! 😄
I'll make the suggested changes as soon as I get home.

The ramifications of letting the Promise polyfill enter the main Sinon code escaped me at the time I reviewed sinonjs#1211, but it struck me when reviewing sinonjs#1215 that had similar issues. In short: having the polyfill bundled with Sinon will lead to false negatives: running tests using Sinon will then give the impression that code works on platforms that does not support the features that were polyfilled. This reverts the introduction of `native-promise-only` in the main code and moves it to its proper place: the test code.

The ramifications of letting the Promise polyfill enter the main Sinon code escaped me at the time I reviewed #1211, but it struck me when reviewing #1215 that had similar issues. In short: having the polyfill bundled with Sinon will lead to false negatives: running tests using Sinon will then give the impression that code works on platforms that does not support the features that were polyfilled. This reverts the introduction of `native-promise-only` in the main code and moves it to its proper place: the test code.

coveralls · 2016-12-31T10:43:36Z

Coverage decreased (-0.9%) to 95.467% when pulling cc92daa on lucasfcosta:maps-matchers into b762c5e on sinonjs:master.

fatso83

All looks great, but just to improve on the general standard would you mind naming the anonymous functions?

fatso83 · 2016-12-31T10:45:59Z

lib/sinon/util/core/every.js

+"use strict";
+
+// This is an `every` implementation that works for all iterables
+module.exports = function (obj, fn) {


Name anonymous function to aid debugging (ref previous review).

Yup, I'm doing this right now, I'm a bit of an anxious guy so I have this bad habit of pushing commits as soon as I do any rebase.

fatso83 · 2016-12-31T10:48:11Z

lib/sinon/match.js

+
+match.map = match.typeOf("map");
+
+match.map.deepEquals = function (expectation) {


Name anonymous function to aid debugging (ref previous review).

fatso83 · 2016-12-31T10:48:30Z

lib/sinon/util/core/iterable-to-string.js

+"use strict";
+var typeOf = require("../../typeOf");
+
+module.exports = function iterableToString(obj) {


Name anonymous function to aid debugging (ref previous review).

coveralls · 2016-12-31T11:08:01Z

Coverage decreased (-0.9%) to 95.467% when pulling c0f9856 on lucasfcosta:maps-matchers into b762c5e on sinonjs:master.

coveralls · 2016-12-31T11:08:01Z

Coverage decreased (-0.9%) to 95.467% when pulling c0f9856 on lucasfcosta:maps-matchers into b762c5e on sinonjs:master.

lucasfcosta · 2016-12-31T11:13:26Z

I've just finished this, here are all the changes 😄

I definitely gotta stop doing multiple pushes to my fork while working, I always forget about this coveralls bot 😅

I've named all the anonymous functions added in this PR and I've also improved iterableToString according to @mroderick suggestions.

As for the Set and Map polyfills, I think they should not be added to this PR because since we're using the typeOf utility function and it uses Object.prototype.toString in order to get the type of an object, we would need a polyfill there too, because the Map and Set polyfills also polyfill the Symbol.toStringTag symbol and therefore it does not get used by older platforms unless we also try to use it globally in order for toString to be aware of it.

Anyway, would you like to add it? What do you think its better?

Also, thanks for the feedback, it was really valuable.

fatso83 · 2016-12-31T11:17:40Z

@lucasfcosta, sorry for jumping the gun. I just got the email about it being updated, so jumped on it. Please do add the polyfills if you can find some decent ones, but add it to the test suites, not the main library. See 942fc53 for why and how.

lucasfcosta · 2016-12-31T11:45:34Z

@fatso83 Don't worry, actually it's me that should not do a push everytime I rebase something.

I've been trying to use some symbol Polyfills but none of them seem to reimplement Object.prototype.toString in order for it to use Symbol.toStringTag so all tests that use typeOf end up returning object.
Any ideas on how to workaround that?

fatso83 · 2016-12-31T12:39:30Z

@lucasfcosta Well, we could fix the polyfills, submit a PR, and use a github branch reference while we might for it to be merged upstream? Might be a bit of a detour of course ...

On the other hand, if I understand you correctly, the polyfill for Symbol at polyfill.io seems to implement toString(). Is that what you need?

lucasfcosta · 2016-12-31T13:26:38Z

@fatso83 Yeah, I thought about that, but it would definitely be a detour, however, if that's the only way to fix this, we've gotta do it.

I tried a couple more possibilities including the service you listed and none of them seemed to work.

Apparently polyfill.io is geared towards browsers and requires user-agent strings in order to target specific browser builds. Also, whenever using it with some old UA strings or nothing it just failed because of some YAML Parser module.

I also tried to use core-js and the only way to override Object.prototype.toString would be to use a global polyfill or call Map.prototype.toString instead of Object.prototype.toString, which is something we can't do too because of how typeOf is expected to work. (Reference)

All the other individual polyfills I've tried didn't address this too.

I'm running out of ideas, I guess I'll just have to implement it somewhere.

fatso83 · 2016-12-31T15:44:59Z

You know what, let's just leave it for now. I will merge this, as the tests have been implemented and are being run and that is the main thing. Upping the coverage report is not "up there" on the priority list IMHO. Any objections? Otherwise I'll merge this ASAP as every other item has been checked off the review list.

The changes have been implemented.

lucasfcosta · 2016-12-31T15:57:48Z

@fatso83 I'll work on another PR for Sets soon and I'll try to tackle this problem again in it.

If I can't find a way to fix it in that PR I'll just reimplement Object.prototype.toString in order for it to read the fake Symbols these polyfills use.

For now, I'd just like to thank everyone for the feedback and for your work, it helped a lot. 😄

fatso83 · 2016-12-31T16:32:57Z

Huh, it seems to fail on IE11

  1) sinonMatch .map map.deepEquals has a .deepEquals matcher:
     AssertionError: [assert.equals] deepEquals(Map[1,2,3]) expected to be equal to deepEquals(Map[['one',1],['two',2],['three',3]])
      at referee.fail (node_modules/referee/lib/referee.js:197)
      at ctx.fail (node_modules/referee/lib/referee.js:90)
      at assertion (node_modules/referee/lib/referee.js:98)
      at referee[type][name] (node_modules/referee/lib/referee.js:118)
      at Anonymous function (test/match-test.js:732)
  2) sinonMatch .map map.deepEquals matches maps with the exact same elements:
     AssertionError: [assert] Expected false to be truthy
      at referee.fail (node_modules/referee/lib/referee.js:197)
      at assert (node_modules/referee/lib/referee.js:164)
      at Anonymous function (test/match-test.js:751)
  3) sinonMatch .map map.contains has a .contains matcher:
     AssertionError: [assert.equals] contains(Map[1,2,3]) expected to be equal to contains(Map[['one',1],['two',2],['three',3]])
      at referee.fail (node_modules/referee/lib/referee.js:197)
      at ctx.fail (node_modules/referee/lib/referee.js:90)
      at assertion (node_modules/referee/lib/referee.js:98)
      at referee[type][name] (node_modules/referee/lib/referee.js:118)
      at Anonymous function (test/match-test.js:795)
  4) sinonMatch .map map.contains matches maps containing the given elements:
     AssertionError: [assert] Expected false to be truthy
      at referee.fail (node_modules/referee/lib/referee.js:197)
      at assert (node_modules/referee/lib/referee.js:164)
      at Anonymous function (test/match-test.js:817)
  5) util/core/iterable-to-string returns an String representation of Map objects:
     AssertionError: [assert.equals] 1,'one',true,undefined,null expected to be equal to [1,1],['one','one'],[true,true],[undefined,undefined],[null,null]
      at referee.fail (node_modules/referee/lib/referee.js:197)
      at ctx.fail (node_modules/referee/lib/referee.js:90)
      at assertion (node_modules/referee/lib/referee.js:98)
      at referee[type][name] (node_modules/referee/lib/referee.js:118)
      at Anonymous function (test/util/core/iterable-to-string-test.js:29)
  6) util/core/iterable-to-string returns an String representation of Set objects:
     AssertionError: [assert.equals]  expected to be equal to 1,'one',true,undefined,null
      at referee.fail (node_modules/referee/lib/referee.js:197)
      at ctx.fail (node_modules/referee/lib/referee.js:90)
      at assertion (node_modules/referee/lib/referee.js:98)
      at referee[type][name] (node_modules/referee/lib/referee.js:118)
      at Anonymous function (test/util/core/iterable-to-string-test.js:38)

lucasfcosta · 2016-12-31T18:08:55Z

It seems like these tests are failing because typeOf can't detect the type Map properly so it always ends up not entering the right if statements.

In Chai we've got this awesome module for type-detection and it works pretty well. Maybe we could start using it instead of this typeOf implementation. What do you think? For now I'll try to see what can be done regarding what have already got here.

I have substituted typeOf for this code:

var type = require('type-detect');

module.exports = function typeOf(value) {
    return type(value).toLowerCase();
};

And every test is still passing. However, I need to set up karma in order to test properly in IE11.

Edit:

My speculation was wrong. Apparently this is a problem regarding how the Map and Set objects were created. I'm still investigating it.

lucasfcosta · 2016-12-31T18:55:37Z

I managed to discover the reason of this error:

  6) util/core/iterable-to-string returns an String representation of Set objects:
     AssertionError: [assert.equals]  expected to be equal to 1,'one',true,undefined,null
      at referee.fail (node_modules/referee/lib/referee.js:197)
      at ctx.fail (node_modules/referee/lib/referee.js:90)
      at assertion (node_modules/referee/lib/referee.js:98)
      at referee[type][name] (node_modules/referee/lib/referee.js:118)
      at Anonymous function (test/util/core/iterable-to-string-test.js:38)

This happens because the Set constructor in IE11 doesn't use the array passed to it to define its elements, so we've gotta do this instead:

        it("returns an String representation of Set objects", function () {
            var set = new Set();
            set.add(1);
            set.add("one");
            set.add(true);
            set.add(undefined);
            set.add(null);

            var expected = "1,'one',true,undefined,null";

            assert.equals(iterableToString(set), expected);
        });

However, when running the other tests in that stack trace on my IE11 VM they didn't fail.

31 12 2016 16:48:02.334:INFO [karma]: Karma v1.3.0 server started at http://localhost:9876/
31 12 2016 16:48:02.335:INFO [launcher]: Launching browser IE11 - Win7 with concurrency 3
31 12 2016 16:48:02.364:INFO [launcher]: Starting browser IE11 - Win7
31 12 2016 16:48:02.592:INFO [launcher.ievms]: Opening VM IE11 - Win7
31 12 2016 16:48:06.371:INFO [IE 11.0.0 (Windows 7 0.0.0)]: Connected on socket /#u43qju6LZsUJ7dbxAAAA with id 81320953
IE 11.0.0 (Windows 7 0.0.0) LOG:

IE 11.0.0 (Windows 7 0.0.0): Executed 9 of 1577 SUCCESS (0.035 secs / 0.008 secs)
31 12 2016 16:48:07.152:INFO [launcher.ievms]: Force stopping VM IE11 - Win7
31 12 2016 16:48:07.152:INFO [launcher.ievms]: Closing VM
31 12 2016 16:48:07.155:INFO [launcher.ievms]: Force stopping VM IE11 - Win7
31 12 2016 16:48:07.155:INFO [launcher.ievms]: Closing VM

fatso83 · 2017-01-02T11:57:45Z

Hmm weird. I still think your type library is interesting. We are not doing this because we like maintaining code, so if it is better and well maintained; all the better. Maybe a later PR ;)

mroderick previously requested changes Dec 28, 2016

View reviewed changes

lucasfcosta force-pushed the maps-matchers branch from eed4caf to c2a8bd7 Compare December 28, 2016 22:14

lucasfcosta force-pushed the maps-matchers branch 3 times, most recently from ea30483 to 6e3a677 Compare December 28, 2016 22:25

mroderick reviewed Dec 29, 2016

View reviewed changes

fatso83 mentioned this pull request Dec 31, 2016

Remove polyfill for Promise #1224

Merged

lucasfcosta force-pushed the maps-matchers branch from 6e3a677 to cc92daa Compare December 31, 2016 10:42

fatso83 reviewed Dec 31, 2016

View reviewed changes

lucasfcosta added 6 commits December 31, 2016 09:06

Generic every implementation for all iterables

a18d3be

Use generic toString method for iterables on matchers

946a99c

Matcher for Map type

0bfc586

Deep equals matcher for Map

53826dd

Contains matcher for maps

f9a09bb

Add docs for new map matchers

c0f9856

lucasfcosta force-pushed the maps-matchers branch from cc92daa to c0f9856 Compare December 31, 2016 11:06

fatso83 merged commit 436c97f into sinonjs:master Dec 31, 2016

This was referenced Jan 2, 2017

Fix set iterableToString test on IE11 #1230

Merged

Sets matchers #1232

Merged

lucasfcosta mentioned this pull request Feb 21, 2017

Refactoring: use ES5.1 features #1271

Merged


		match.map = match.typeOf("map");

		match.map.deepEquals = function (expectation) {

Maps matchers #1215

Maps matchers #1215

Conversation

lucasfcosta commented Dec 27, 2016

Purpose (TL;DR) - mandatory

Background (Problem in detail)

How to verify - mandatory

coveralls commented Dec 27, 2016 • edited

fatso83 commented Dec 27, 2016

mroderick commented Dec 28, 2016

mroderick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucasfcosta commented Dec 28, 2016 • edited

coveralls commented Dec 28, 2016 • edited

coveralls commented Dec 28, 2016 • edited

lucasfcosta commented Dec 28, 2016 • edited

fatso83 commented Dec 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mroderick Dec 29, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mroderick Dec 29, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucasfcosta commented Dec 29, 2016

coveralls commented Dec 31, 2016 • edited

fatso83 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 31, 2016

coveralls commented Dec 31, 2016 • edited

lucasfcosta commented Dec 31, 2016

fatso83 commented Dec 31, 2016

lucasfcosta commented Dec 31, 2016 • edited

fatso83 commented Dec 31, 2016

lucasfcosta commented Dec 31, 2016

fatso83 commented Dec 31, 2016

lucasfcosta commented Dec 31, 2016

fatso83 commented Dec 31, 2016 • edited

lucasfcosta commented Dec 31, 2016 • edited

lucasfcosta commented Dec 31, 2016 • edited

fatso83 commented Jan 2, 2017

coveralls commented Dec 27, 2016 •

edited

lucasfcosta commented Dec 28, 2016 •

edited

coveralls commented Dec 28, 2016 •

edited

coveralls commented Dec 28, 2016 •

edited

lucasfcosta commented Dec 28, 2016 •

edited

mroderick Dec 29, 2016 •

edited

mroderick Dec 29, 2016 •

edited

coveralls commented Dec 31, 2016 •

edited

coveralls commented Dec 31, 2016 •

edited

lucasfcosta commented Dec 31, 2016 •

edited

fatso83 commented Dec 31, 2016 •

edited

lucasfcosta commented Dec 31, 2016 •

edited

lucasfcosta commented Dec 31, 2016 •

edited