Fix #1274 callCount property is incorrect #1313

kbtknr · 2017-03-02T05:17:07Z

Purpose (TL;DR)

Fix issue #1274, Rewrite matchingFake in spy.js

Background (Problem in detail)

spy.withArgs function makes fake object and store this.fakes, depending on the combination of arguments.
And spy.invoke function makes use of this.fakes.

matchingFake function called byspy.invoke, returns the only one fake if arguments are matched.
And spy.invoke update the fake properties like callCount.
In this time, if some fakes in this.fakes are matched arguments, It make this issue happen.

For example

spy.withArgs(1, 1);
spy.withArgs(1); // spy.fakes = [ fake(1, 1), fake(1) ]
spy.invoke(1, 1); // invoke increment callCount either fake(1) or fake(1, 1), but not both.

Solution

Modify matchingFake to matchingFakes, and return array of all fakes matched.
invoke function updates properties of this and all matched fakes.
spy.withArgs and stub are affected by the above fix.
spy.withArgs Fixed to fit conventional working.

Additional

I investigate around the matchingFake function like stub.
And found case3 of #1274.
Conventionally, matchingFake return last fake (pop()).
I Fixed to fit conventional working, but case3 is not solved.
So, I commit 4e68e4d to make use of the most matched argument.

How to verify

Check out this branch (see github instructions below)
npm install
npm test # include test for this issue

coveralls · 2017-03-02T05:18:26Z

Coverage increased (+0.02%) to 95.402% when pulling 4e68e4d on takasmiley:issues/#1274 into adaf1ad on sinonjs:master.

mroderick

What a great PR! Thank you!

After reviewing your work, I realised that there is conflicting wording between the documentation for withArgs for spies and withArgs for stubs

Creates a spy that only records calls when the received arguments match those passed to withArgs.

vs.

Stubs the method only for the provided arguments

I think that the spy docs suggests that withArgs will loosely match on arguments, while the stub documentation suggests that it only matches for exact arguments (current behaviour).

The documentation should be corrected to read something like "Creates a spy that only records calls when the received arguments exactly match those passed to withArgs"

Once the documentation has been corrected, then I think that we should consider adding a new method withArgsMatch, that uses sinon.match() to match arguments more widely.

Perhaps we make a decision on those questions first, before you put in more effort on this PR.

What do you think?

mroderick · 2017-03-02T06:28:17Z

lib/sinon/spy.js

@@ -308,6 +314,16 @@ var spyApi = {
        return fake;
    },

+    matchingFakes: function (args, strict) {
+        if (!this.fakes) {
+            return undefined;


This could return an empty array, which would make it easier to write elegant consumers of the returned value

mroderick · 2017-03-02T06:31:16Z

lib/sinon/spy.js


        incrementCallCount.call(this);
        push.call(this.thisValues, thisValue);
        push.call(this.args, args);
-        push.call(this.callIds, callId++);
+        push.call(this.callIds, currentCallId);
+        if (matchings) {


If matchingFakes always returns arrays, then you can simplify this code (Sinon.JS codebase uses ES5.1, so the array extras are allowed).

matchings.forEach(function (matching) { incrementCallCount.call(matchings); push.call(matchings.thisValues, thisValue); push.call(matchings.args, args); push.call(matchings.callIds, currentCallId); });

mroderick · 2017-03-02T06:33:25Z

lib/sinon/spy.js

@@ -202,6 +195,13 @@ var spyApi = {

        push.call(this.exceptions, exception);
        push.call(this.returnValues, returnValue);
+        if (matchings) {


As above, assume arrays from matchingFakes and prefer ES5.1 array extras over for loops.

mroderick · 2017-03-02T06:34:01Z

lib/sinon/spy.js

@@ -210,6 +210,11 @@ var spyApi = {
            throw err;
        } catch (e) {/* empty */}
        push.call(this.errorsWithCallStack, err);
+        if (matchings) {


As above, assume arrays from matchingFakes and prefer ES5.1 array extras over for loops.

mroderick · 2017-03-02T06:35:15Z

lib/sinon/spy.js

@@ -271,10 +276,11 @@ var spyApi = {
        var args = slice.call(arguments);

        if (this.fakes) {
-            var match = matchingFake(this.fakes, args, true);
+            var matchings = this.matchingFakes(args, true);
+            var matching = matchings && matchings.pop();


If matchingFakes always returns arrays, you don't need to test for the result first, and can just use matchings.pop() directly.

mroderick · 2017-03-02T06:36:16Z

lib/sinon/stub.js

+            var args = slice.call(arguments);
+            var matchings = functionStub.matchingFakes(args);
+
+            var fnStub = (matchings && matchings.pop()) || functionStub;


This would also benefit if matchingFakes always returns an Array

mroderick · 2017-03-02T06:45:45Z

lib/sinon/stub.js

@@ -70,7 +70,15 @@ var proto = {
            var args = slice.call(arguments);
            var matchings = functionStub.matchingFakes(args);

-            var fnStub = (matchings && matchings.pop()) || functionStub;
+            var fnStub;
+            if (matchings) {


matchingFakes should always return arrays, so this test is not necessary

kbtknr · 2017-03-02T11:04:56Z

Hi @mroderick,
Thank you very much for reviewing my work.
I post my opinion as below.

If withArgs function can understand whether each argument is matcher or not completely,
I think that withArgsMatch may not be necessary.
Because withArgs function can understand whether loosely match or exactly match.
And, existing test code is affected and it seems to don't working using withArgs(include sinon-matcher).

Currently, spy.withArgs and stub.withArgs are same function.
I think that it should be not withArgs and withArgsMatch but spy.withArgs and stub.withArgs.
It seems to behavior of spy.withArgs and stub.withArgs is different.

The biggest problem I think is, how priority fakes, and the best matched fake from some fakes, and return it.
In spy.invoke, it should update all matched fakes(callCount ...) that is returning matchingFakes function.
But, spy.withArgs, stub.withArgs, stub.invoke must select one or no fake from returning matchingFakes function.

For Example,

this.fakes = [
  /* 0 */ fake(1, 1),
  /* 1 */ fake(sinon.match.any, 1),
  /* 2 */ fake(sinon.match.number, 1)
]

In spy or stub object is above status, What returning fake is by following action?

spy.withArgs(1, 1) is called, Every fake is matched, How priority? I expect 0.
spy.withArgs(1, 2) is called, 1and2 is matched, How priority? I expect 2 rather than 1,
stub.withArgs(1, 1) is called, Every fake is matched, How priority? I expect 0.
stub.withArgs(2, 1) is called, 1and2 is matched, What fake does select?, I think no fake selected, and create a new fake(2, 1).
If it return this.fakes[2], and its behavior updated, stub.invoke(3, 1) is affected and unexpected.
stub.invoke(1, 1) is called, Every fake is matched, How priority? I expect 0.
stub.invoke(2, 1) is called, 1and2 is matched, What fake does select?, I expect 2.
This is different way from stub.withArgs(2, 1).

It seems to be good that matchingFakes function always return array of fake matched.
Four functions which make use of matchingFakes, do each behavior, each selecting mechanism.

In addition, like above question 2, if sinon.match.number prior to sinon.match.any, priority must be defined in simon-matcher.

first of all, I should separate spy.withArgs and stub.withArgs.

coveralls · 2017-03-03T02:43:54Z

Coverage increased (+0.02%) to 95.398% when pulling e11b3ac on takasmiley:issues/#1274 into adaf1ad on sinonjs:master.

kbtknr · 2017-03-03T08:24:57Z

I'm sorry, perhaps, I might have misunderstood.
After I had send PR, I found three methods, spy.calledWith, spy.calledWithExactly, spy.calledWithMatch.
And, spy object has some series of methods like ***With, ***WithExactly, ***WithMatch.
And, I see calledWith, calledWithMatch, calledWithExactly in call.js.

I see it is different from withArgs and new withArgsMatch.
Adding a new method withArgsMatch looks good to me.

fatso83 · 2017-03-13T18:18:57Z

Hmm ... this seems to have stalled. I am not totally sure I understand where this is going. Are you waiting for some feedback, @takasmiley? Wondering about the next step to take to get this merged.

kbtknr · 2017-03-14T06:53:43Z

I use sinonjs in usage like case 1, and I found this problem and fixed it.
The already pushed code solves the problem of case 1 to case 3.
Please code review.

In case4 and case5 of #1274, I think that the essence of the problem is different from case1.
I would like to discuss with another issue or PR.

fearphage

Just a few missing and misplaced tests mostly.

fearphage · 2017-05-19T02:11:21Z

test/issues/issues-test.js

@@ -200,4 +200,61 @@ describe("issues", function () {
            assert(firstFake !== secondFake);
        });
    });
+
+    describe("#1274 - spy.withArgs(args...).callCount is incorrect", function () {


Although you're fixing a bug, I believe these tests should go in their respective test files (spy, stub, etc.)

fearphage · 2017-05-19T02:12:15Z

test/issues/issues-test.js

+            assert.equals(spy.withArgs(1, 1).callCount, 1);
+            assert.equals(spy.withArgs(1, 2).callCount, 1);
+        });
+        it("case2: should count accurately", function () {


Are case 1 and 2 identical? Please make the descriptions more meaningful.

fearphage · 2017-05-19T02:37:13Z

lib/sinon/spy.js

@@ -308,6 +307,12 @@ var spyApi = {
        return fake;
    },

+    matchingFakes: function (args, strict) {


Since this is publicly accessible now, it should be tested thoroughly.

fatso83 · 2017-06-22T14:49:24Z

@takasmiley As @fearphage noted, this is good to merge, except for some very minor fixes to the tests concerning some naming and location. We would love to have your work integrated into master, so could you please update the code with the last fixes?

coveralls · 2017-06-23T06:10:49Z

Coverage increased (+0.02%) to 94.994% when pulling 820b107 on takasmiley:issues/#1274 into 917366d on sinonjs:master.

fatso83 · 2017-06-23T06:14:08Z

Thanks for such a quick turnaround! This seems to address Phreds issues. @fearphage?

fearphage

@takasmiley Thank you!

@mroderick You're up.

fatso83 · 2017-06-27T10:37:42Z

@mroderick do you feel there is something that needs to be addressed before merging this? I never use the matchers, so I don't feel at home in the discussion on the API behavior - even after reading through it all.

mroderick · 2017-06-27T17:59:43Z

This looks good! Thank you!

mroderick · 2017-06-27T18:00:41Z

@takasmiley would you mind rebasing the branch to get rid of the merge commit at the end?

1. Update matchingFake function in spy.js Rename to matchingFakes, and return not last one fake (pop()) but all fakes matched. 2. Move matchingFakes function into spyApi Because of calling by stub.js 3. Modify spyApi.invoke function Apply processing at function invoked to all matching fakes. 4. Modify part of calling matchingFake() in spyApi.withArgs Fit logic, affected by 1. 5. Update proto.create.functionStub in stub.js Try to get the function stub using spyApi.matchingFakes() and pop(), affected by 1.

Change last pop to the most matched arguments at stub.

coveralls · 2017-06-28T01:46:16Z

Coverage increased (+0.02%) to 94.994% when pulling 1661a0f on takasmiley:issues/#1274 into d8f005b on sinonjs:master.

fatso83 · 2017-06-28T08:54:52Z

Thank you for putting up with us and our prolonged process, @takasmiley 😊

mroderick · 2017-06-28T18:09:57Z

This has been released as [email protected] 🎆

fatso83 · 2017-06-30T10:59:58Z

@takasmiley, it looks like this might have introduced a regression. Do you have any chance to look into the findings in #1476? We are not totally sure what is happening, but maybe you do .

mroderick requested changes Mar 2, 2017

View reviewed changes

fearphage requested changes May 19, 2017

View reviewed changes

fearphage added the semver:patch changes will cause a new patch version label May 21, 2017

fearphage approved these changes Jun 23, 2017

View reviewed changes

mroderick approved these changes Jun 27, 2017

View reviewed changes

takasmiley added 8 commits June 28, 2017 10:39

Add test for issue sinonjs#1274

62ee2e0

Add test for sinonjs#1274 case3

76ac773

Fix stub.withArgs(args...) is incorrect

f012fff

Change last pop to the most matched arguments at stub.

Update matchingFakes, always returns array

e72fee4

Replace for loop with Array.prototype.forEach

833ff2c

Move sinonjs#1274 test code to respective test files

cf3a34e

Add test code of spy.matchingFakes

1661a0f

kbtknr force-pushed the issues/#1274 branch from 820b107 to 1661a0f Compare June 28, 2017 01:43

fatso83 merged commit d22eda3 into sinonjs:master Jun 28, 2017

renovate bot mentioned this pull request Jul 8, 2017

chore(deps): Update dependency sinon to version ^2.0.0 factset/labeledpipe#15

Merged

mroderick mentioned this pull request Aug 9, 2017

Cannot stub Array.prototype.filter #1521

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #1274 callCount property is incorrect #1313

Fix #1274 callCount property is incorrect #1313

kbtknr commented Mar 2, 2017

coveralls commented Mar 2, 2017

mroderick left a comment

mroderick Mar 2, 2017

mroderick Mar 2, 2017

mroderick Mar 2, 2017

mroderick Mar 2, 2017

mroderick Mar 2, 2017

mroderick Mar 2, 2017

mroderick Mar 2, 2017

kbtknr commented Mar 2, 2017

coveralls commented Mar 3, 2017 •

edited

Loading

kbtknr commented Mar 3, 2017

fatso83 commented Mar 13, 2017

kbtknr commented Mar 14, 2017

fearphage left a comment

fearphage May 19, 2017

fearphage May 19, 2017

fearphage May 19, 2017

fatso83 commented Jun 22, 2017

coveralls commented Jun 23, 2017 •

edited

Loading

fatso83 commented Jun 23, 2017

fearphage left a comment •

edited

Loading

fatso83 commented Jun 27, 2017

mroderick commented Jun 27, 2017

mroderick commented Jun 27, 2017

coveralls commented Jun 28, 2017 •

edited

Loading

fatso83 commented Jun 28, 2017

mroderick commented Jun 28, 2017

fatso83 commented Jun 30, 2017

Fix #1274 callCount property is incorrect #1313

Fix #1274 callCount property is incorrect #1313

Conversation

kbtknr commented Mar 2, 2017

Purpose (TL;DR)

Background (Problem in detail)

Solution

Additional

How to verify

coveralls commented Mar 2, 2017

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

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

kbtknr commented Mar 2, 2017

coveralls commented Mar 3, 2017 • edited Loading

kbtknr commented Mar 3, 2017

fatso83 commented Mar 13, 2017

kbtknr commented Mar 14, 2017

fearphage 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

fatso83 commented Jun 22, 2017

coveralls commented Jun 23, 2017 • edited Loading

fatso83 commented Jun 23, 2017

fearphage left a comment • edited Loading

Choose a reason for hiding this comment

fatso83 commented Jun 27, 2017

mroderick commented Jun 27, 2017

mroderick commented Jun 27, 2017

coveralls commented Jun 28, 2017 • edited Loading

fatso83 commented Jun 28, 2017

mroderick commented Jun 28, 2017

fatso83 commented Jun 30, 2017

coveralls commented Mar 3, 2017 •

edited

Loading

coveralls commented Jun 23, 2017 •

edited

Loading

fearphage left a comment •

edited

Loading

coveralls commented Jun 28, 2017 •

edited

Loading