Fix #1372: make sandbox.resetHistory also reset spies #1424

mroderick · 2017-05-25T20:41:30Z

This PR fixes #1372, by improving the implementation of sandbox.resetHistory() method

To verify

Check out this branch
npm install
npm test

fearphage · 2017-05-26T00:31:10Z

lib/sinon/collection.js

+                return;
+            }
+
+            if (typeof fake.reset === "function" && /^spy\#/.test(fake.id)) {


What else has no resetHistory but does have a reset method besides a spy?

None that I know of. Do you think I should just test for a reset method, and call it?

I believe so. I believe I've already seen code like that in another pr (if resetHistory else-if reset).

Fixed. I've pushed a new commit that just tests for the presence of reset before calling it

fearphage · 2017-05-26T00:31:24Z

lib/sinon/collection.js

+        getFakes(this).forEach(function (fake) {
+            if (typeof fake.resetHistory === "function") {
+                fake.resetHistory();
+                return;


Use else-if instead.

No thanks, there is no else if

I definitely agree with point number 2. I'm even working on a PR related to that. But point number one is a little misguided.

Nothing is special about else-if in javascript. The logic is the same in every language that I can think of. You can always rewrite an else-if as a 3 statements instead of 2. It's a core tenant of Logic.

Splashing random returns around seems less explicit and it's obscuring the intent. Is the goal of this code to return undefined?

The most important difference to me is consistency. We use else-if throughout the code. In my opinion, it reads better and is easier to reason about.

Another problem I foresee is future work. So let's say there was something I wanted to do no matter which fork of the if branch the code went down.. I'd have to "fix" your code first before I could accomplish that. In an if-else case, the change would be purely additive.

How would you feel adding a (pointless) return statement to the other if for consistency's sake?

OK, I'll take the bait.

If follow your original suggestion, while writing JavaScript without omitting syntax, we'll end up with this:

getFakes(this).forEach(function (fake) { if (typeof fake.resetHistory === "function") { fake.resetHistory(); } else { if (typeof fake.reset === "function") { fake.reset(); } } });

In my opinion, that requires a lot more effort to understand.

Using return as a control flow statement helps me keep indentation low, which I think we all agree makes code easier to read.

Regarding consistency: I don't think we're that consistent in this codebase. We've been using return as a control flow statement across this codebase for awhile, try this search:

$ ag --ignore=docs/ -Q "return;"

I am all for consistency across a codebase ... until we find a better way of doing things, then we should slowly refactor to adopt the better practice. I believe that using return to reduce indentation is a good practice.

How would you feel adding a (pointless) return statement to the other if for consistency's sake?

ESLint understands how to keep consistent return, which is configured in eslint-config-sinon.

I think it is known to any (non-novice) reader, that the default return value of a function undefined.

</bike-shed>

In my opinion, that requires a lot more effort to understand.

I would get the above code fixed in code review. It should be an else-if. I agree that the way it is written is more complex than an else-if although logically congruent.

Using return as a control flow statement helps me keep indentation low, which I think we all agree makes code easier to read.

I believe that using return to reduce indentation is a good practice.

We agree there, but an else-if is the same amount of indentation as a plain if:

if (foo) { // one indentation return; } if (bar) { // one indentation } // VS if (foo) { // one indentation } else if { // one indentation }

You're not saving anything whatsoever in this case. You're actually typing a few extra characters if you're concerned about keystroke counts at all.

try this search

We use empty returns all the time. That is not the/a problem. You're not getting the full story though.

ag --ignore=docs/ -Q "return;" -A 5 -B 3

I only found one instance of if-return-if usage. PS. I had been putting off installing/using ag until just now.

RE: consistency

Apparently every fake has resetHistory or reset so we should remove the second if and make it an if-else like in spy:

if (this.fakes) { this.fakes.forEach(function (fake) { if (fake.resetHistory) { fake.resetHistory(); } else { fake.reset(); } }); }

Then this whole conversation goes away. 🙂

Or just fake fake.reset() directly

fearphage · 2017-05-26T00:31:51Z

test/issues/issues-test.js

+            spy();
+            assert.equals(spy.callCount, 2);
+
+            this.sandbox.resetHistory();


I'm assuming there are already related tests for stubs. It wouldn't hurt to have a test with both since you're potentially affecting both.

I have expanded the test for collection.resetHistory to have a fake that looks like a spy in it

coveralls · 2017-05-27T10:46:39Z

Coverage increased (+0.01%) to 94.968% when pulling 0500231 on mroderick:fix-1372-sandbox-reset-history-on-spies into 350ba0a on sinonjs:master.

coveralls · 2017-05-29T15:57:33Z

Coverage increased (+0.007%) to 94.963% when pulling d52c92d on mroderick:fix-1372-sandbox-reset-history-on-spies into e0c75bd on sinonjs:master.

fearphage · 2017-05-29T16:01:55Z

lib/sinon/collection.js

+        getFakes(this).forEach(function (fake) {
+            var method = fake.resetHistory || fake.reset;
+
+            if (method) {


Is this check necessary? What is it catching?

Situations where neither method exists

Which is a bit synthetic right now .... I could also just call fake.reset, since that is what stub.resetHistory does ... but that just seems wrong

If you're going for the check, shouldn't it throw if there is no way to reset the thing this is called on?

If you're going for the check, shouldn't it throw if there is no way to reset the thing this is called on?

I didn't want to create something that was vastly different from what each does in the same file.

https://github.com/sinonjs/sinon/blob/master/lib/sinon/collection.js#L19-L28

Ah yes, it filtered non-function values before iterating. 👍

Are they fakes that have neither resetHistory nor reset? It seems like we could simplify this a bit and be in the same place:

(fake.resetHistory || fake.reset).call(fake);

If you stub a complete object the fake has no resetHistory() nor reset():

function Test() {} Test.prototype.foo = function () {} Test.prototype.bar = function () {} let sandbox = Sinon.sandbox.create(); sandbox.stub(Test.prototype); sandbox.resetHistory();

It might be useful to add a note about why resetHistory is calling reset for future contributors.

What do you think about renaming/aliasing spy.reset to spy.resetHistory and deprecating reset? Not for this ticket, but just putting the idea out there.

Flarna · 2017-05-30T11:54:00Z

I tested the change in my local environment and it solves the issue #1372 reported by me 👍

mroderick · 2017-06-01T09:34:54Z

What do you think about renaming/aliasing spy.reset to spy.resetHistory and deprecating reset? Not for this ticket, but just putting the idea out there.

I think that would be a good improvement. Will you add a ticket for it and describe how it would work? Perhaps we can even attract a new contributor with it ;)

mantoni · 2017-06-10T09:31:34Z

I don't think there is anything else to add here. Like @mroderick suggested, we could think about refactoring the whole thing.

mroderick · 2017-06-10T09:41:51Z

I have published this change to npm as v2.3.4

…#1424)

fearphage requested changes May 26, 2017

View reviewed changes

mroderick force-pushed the fix-1372-sandbox-reset-history-on-spies branch 3 times, most recently from e55de8d to 0500231 Compare May 27, 2017 10:43

mroderick added the semver:patch changes will cause a new patch version label May 28, 2017

mroderick force-pushed the fix-1372-sandbox-reset-history-on-spies branch 2 times, most recently from 3c860e9 to d52c92d Compare May 29, 2017 15:56

Fix sinonjs#1372: make sandbox.resetHistory also reset spies

d52c92d

fearphage reviewed May 29, 2017

View reviewed changes

fearphage approved these changes May 29, 2017

View reviewed changes

fearphage mentioned this pull request Jun 2, 2017

Deprecate spy.reset() in favor of the more explicit spy.resetHistory() #1446

Closed

mantoni merged commit 330101a into sinonjs:master Jun 10, 2017

mroderick deleted the fix-1372-sandbox-reset-history-on-spies branch June 10, 2017 09:39

renovate bot mentioned this pull request Jul 8, 2017

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

Merged

franck-romano pushed a commit to franck-romano/sinon that referenced this pull request Oct 1, 2019

Fix sinonjs#1372: make sandbox.resetHistory also reset spies (sinonjs…

ef35ee0

…#1424)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #1372: make sandbox.resetHistory also reset spies #1424

Fix #1372: make sandbox.resetHistory also reset spies #1424

mroderick commented May 25, 2017

fearphage May 26, 2017

mroderick May 26, 2017

fearphage May 26, 2017

mroderick May 27, 2017

fearphage May 26, 2017

mroderick May 27, 2017

fearphage May 27, 2017

mroderick May 28, 2017

fearphage May 28, 2017

mroderick May 29, 2017

fearphage May 26, 2017

mroderick May 27, 2017

coveralls commented May 27, 2017

coveralls commented May 29, 2017 •

edited

fearphage May 29, 2017

mroderick May 29, 2017

mroderick May 29, 2017

mantoni May 29, 2017

mroderick May 29, 2017

mantoni May 29, 2017

fearphage May 29, 2017

Flarna May 30, 2017

fearphage May 31, 2017

Flarna commented May 30, 2017

mroderick commented Jun 1, 2017

mantoni commented Jun 10, 2017

mroderick commented Jun 10, 2017

Fix #1372: make sandbox.resetHistory also reset spies #1424

Fix #1372: make sandbox.resetHistory also reset spies #1424

Conversation

mroderick commented May 25, 2017

To verify

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

Choose a reason for hiding this comment

RE: consistency

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented May 27, 2017

coveralls commented May 29, 2017 • edited

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

Choose a reason for hiding this comment

Flarna commented May 30, 2017

mroderick commented Jun 1, 2017

mantoni commented Jun 10, 2017

mroderick commented Jun 10, 2017

coveralls commented May 29, 2017 •

edited