test(1882): add test case #1905

mgred · 2018-09-16T21:32:01Z

Purpose (TL;DR) - mandatory

See: #1882, sinonjs/samsam#37

Background (Problem in detail) - optional

add test case provided in sinon.stub().withArgs() matches too often #1882
change some eslint config to match test:
** class definitions are not enabled in ecmaVersion 5,
** addint Proxy as global

Solution - optional

This test is passing for me locally

How to verify - mandatory

Check out this branch
npm install
npm run test-node

Checklist for author

npm run lint passes
References to standard library functions are cached.

* add test case provided in sinonjs#1882 * change some eslint config to match test: ** class definitions are not enabled in ecmaVersion 5, ** addint Proxy as global See: sinonjs#1882, sinonjs/samsam#37

coveralls · 2018-09-16T21:36:07Z

Pull Request Test Coverage Report for Build 2676

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 95.356%

Totals
Change from base Build 2673:	0.0%
Covered Lines:	2043
Relevant Lines:	2097

💛 - Coveralls

mroderick

Thank you fro your pull request.
I have a few comments related to using ES5.1 instead of ES6.

mroderick · 2018-09-17T07:34:15Z

.eslintrc.yml


 plugins:
  - ie11
  - local-rules

+parserOptions:


Sinon is an ES5.1 project, as described in CONTRIBUTING.md.

If we allow ES6 syntax, we drop support for IE11 and older Safari. I don't think we're quite there yet.

Ok, Change this to fit the example. I will revert

mroderick · 2018-09-17T07:49:47Z

test/issues/issues-test.js

+
+    describe("#1882", function () {
+        it("should use constructor name when checking deepEquality", function () {
+            class ClassWithoutProps {}


class is an ES6 convenience over using prototype. This means that you can achieve the same using prototype.

function ClassWithoutProps(){} ClassWithoutProps.prototype.constructor = ClassWithoutProps;

Ahhh, yeah let me change this...

mroderick · 2018-09-17T07:53:22Z

test/issues/issues-test.js

+        it("should use constructor name when checking deepEquality", function () {
+            class ClassWithoutProps {}
+            const arg1 = new ClassWithoutProps(); //arg1.constructor.name === ClassWithoutProps
+            const arg2 = new Proxy({}, {}); //arg2.constructor.name === Object


Can this test be done without using Proxy, which is from ES6 and is not supported in IE11?

Perhaps create AnotherClassWithoutProps?

That's true I will rewrite with to your suggestion, which makes the most sense I think

mgred · 2018-09-17T10:35:14Z

Thanks for all your suggestions, and sorry for ignoring the ES5 restriction for IE11. I keep learning.

* Rewert config to support IE11 again * Adopt changes to test to be ES5

mroderick · 2018-09-21T08:03:03Z

Thank you 👍

fatso83 · 2018-09-21T09:17:14Z

WebWorker test crashed on Node 10 for some unknown reason after merge to master.

fatso83 · 2018-09-24T08:13:06Z

This test started failing on IE 11 in our Sauce Labs suite last Friday after merge. It was "hidden" from view due to the webworker test also failing on Circle CI@Node 10 (for some unknown reason I am trying to look into).

See the logs (overview):

    "result": "....\n\n  1572 passing (9s)\n  8 pending\n  1 failing\n\n  1) issues\n       #1882\n         should use constructor name when checking deepEquality:\n     AssertionError: [assert.same] 5 expected to be the same object as undefined\n     at referee.fail (Unknown script code:6419:9)\n     at ctx.fail (Unknown script code:6280:17)\n     at assertion (Unknown script code:6288:13)\n     at referee[type][name] (Unknown script code:6315:9)\n     at Anonymous function (Unknown script code:24983:13)\n     at callFn (Unknown script [truncated]",

fatso83 · 2018-09-24T13:54:25Z

I found more information on this and detailed this out in this comment sinonjs/samsam#41 (comment). It seems like samsam already failed 4 tests when running in IE11 (not known to us) before merging sinonjs/samsam#38. That didn't matter much, as it seems we did not rely on those parts of samsam.

But the change to constructor equality broke 3 additional tests on IE 11, and this did break things in Sinon 😿 Would you like to have a go at trying to get them passing, @mgred? I can supply you with Sauce Labs credentials (mail in profile).

fatso83 · 2018-09-24T15:28:17Z

I used mochify --consolify index.html lib/*test.js to produce a html file with the tests and set up a web server to serve it. Then used Sauce Labs "Live Testing" feature and the sc binary to live test in the IE11 debugger.

Turns out no stack or message props are created for errors when they are not thrown in IE, meaning two errors are otherwise identical if not thrown.

@mroderick Suggestions for fix? They are deeply equal, but maybe it's ok to assume that all errors are thrown? Then I could simply replace new Error() with createError() which does a try/catch/return.

mgred · 2018-09-24T15:57:27Z

@fatso83 puh that escalated quickly, Sorry guys. Sure, I will have a look and see what I can find out.

fatso83 · 2018-09-24T20:20:12Z

@mgred you absolutely don't have to, and it's our own fault that we didn't run these tests on our supported browsers to begin with. It's hard to know you are breaking compatibility with some semi-ancient browser when no-one tells you :-) As already stated, 4 tests were already failing on IE11 prior to this, so we should try to tackle them before you look at the new ones, in case they're related. I'll file an issue.

mroderick · 2018-09-24T20:54:49Z

@mroderick Suggestions for fix? They are deeply equal, but maybe it's ok to assume that all errors are thrown? Then I could simply replace new Error() with createError() which does a try/catch/return.

Where does createError() come from? I can't seem to find it in sinon source.

fatso83 · 2018-09-25T07:46:34Z

@mroderick I made it up. I guess "with createError() which does a try/catch/return." should be "with a sort of createError() function that could do a try/catch/return". And zooming between various related repos is not what I do best, apparently, as I failed to mention that all of the code I am talking about belongs (/would belong) in the samsam repo.

fatso83 · 2018-09-25T08:10:29Z

ps. suggest we continue any discussion in sinonjs/samsam#42

mgred · 2018-09-25T13:44:24Z

@fatso83 let me see what I can find out

* test(1882): add test case * add test case provided in sinonjs#1882 * change some eslint config to match test: ** class definitions are not enabled in ecmaVersion 5, ** addint Proxy as global See: sinonjs#1882, sinonjs/samsam#37 * fix(1882): changes according to review * Rewert config to support IE11 again * Adopt changes to test to be ES5

test(1882): add test case

4c74c6d

* add test case provided in sinonjs#1882 * change some eslint config to match test: ** class definitions are not enabled in ecmaVersion 5, ** addint Proxy as global See: sinonjs#1882, sinonjs/samsam#37

mgred mentioned this pull request Sep 16, 2018

sinon.stub().withArgs() matches too often #1882

Closed

mroderick requested changes Sep 17, 2018

View reviewed changes

fix(1882): changes according to review

00215ca

* Rewert config to support IE11 again * Adopt changes to test to be ES5

mroderick approved these changes Sep 21, 2018

View reviewed changes

mroderick merged commit f6ee6ac into sinonjs:master Sep 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(1882): add test case #1905

test(1882): add test case #1905

mgred commented Sep 16, 2018

coveralls commented Sep 16, 2018 •

edited

mroderick left a comment

mroderick Sep 17, 2018

mgred Sep 17, 2018

mroderick Sep 17, 2018

mgred Sep 17, 2018

mroderick Sep 17, 2018

mgred Sep 17, 2018

mgred commented Sep 17, 2018

mroderick commented Sep 21, 2018

fatso83 commented Sep 21, 2018

fatso83 commented Sep 24, 2018

fatso83 commented Sep 24, 2018

fatso83 commented Sep 24, 2018

mgred commented Sep 24, 2018

fatso83 commented Sep 24, 2018

mroderick commented Sep 24, 2018

fatso83 commented Sep 25, 2018 •

edited

fatso83 commented Sep 25, 2018

mgred commented Sep 25, 2018

test(1882): add test case #1905

test(1882): add test case #1905

Conversation

mgred commented Sep 16, 2018

Purpose (TL;DR) - mandatory

Background (Problem in detail) - optional

Solution - optional

How to verify - mandatory

Checklist for author

coveralls commented Sep 16, 2018 • edited

Pull Request Test Coverage Report for Build 2676

💛 - Coveralls

mroderick left a comment

Choose a reason for hiding this comment

mroderick Sep 17, 2018

Choose a reason for hiding this comment

mgred Sep 17, 2018

Choose a reason for hiding this comment

mroderick Sep 17, 2018

Choose a reason for hiding this comment

mgred Sep 17, 2018

Choose a reason for hiding this comment

mroderick Sep 17, 2018

Choose a reason for hiding this comment

mgred Sep 17, 2018

Choose a reason for hiding this comment

mgred commented Sep 17, 2018

mroderick commented Sep 21, 2018

fatso83 commented Sep 21, 2018

fatso83 commented Sep 24, 2018

fatso83 commented Sep 24, 2018

fatso83 commented Sep 24, 2018

mgred commented Sep 24, 2018

fatso83 commented Sep 24, 2018

mroderick commented Sep 24, 2018

fatso83 commented Sep 25, 2018 • edited

fatso83 commented Sep 25, 2018

mgred commented Sep 25, 2018

coveralls commented Sep 16, 2018 •

edited

fatso83 commented Sep 25, 2018 •

edited