Fix constructor spy in unit test with Sinon 4.1.3

[danxuliu]

TL;DR;

When spying on a constructor using Sinon the objects created by the constructor are got using spy.getCall(XXX).returnValue.

All the long and boring details

As explained in the MDN Web Docs, new Foo(...) is executed in three steps:

1. First a new object that inherits from Foo.prototype is created.
2. Then the constructor function Foo is called with this bound to that new object.
3. Finally the object returned by the constructor, or the object created in the first step if none is returned, becomes the result of the whole new expression.

When a constructor (or any other function) is spied using Sinon the original constructor is replaced by a proxy. When the proxy is called it will record the arguments it was called with, call the original constructor, and finally record the value returned or the exceptions thrown by the original constructor.

Before Sinon 4.1.3 when the proxy was called using the new operator the proxy called the original constructor directly, passing it the thisValue, which was the object created in step 1 above. If the original constructor function did not return an object (and note that this is the most common case, as the value returned by a constructor is usually undefined; as explained above new is the one that returns the created object when the constructor returns no value) then returnValue was set to thisValue; this is the value returned by the proxy to new, and thus is the value returned by the whole new expression as explained in the step 3. Therefore, before Sinon 4.1.3, spy.getCall(XXX).thisValue and spy.getCall(XXX).returnValue had (if the constructor returned no value, like in the failing test) the same value.

This has changed in Sinon 4.1.3, as now when the proxy is called using the new operator the proxy calls the original constructor using new too. When new is called inside the proxy the three steps outlined above happen again: a new object is created, the original constructor is called with this bound to that object, and the object is returned as the result of the whole new (bind.apply(this.func || func, [thisValue].concat(args)))() expression; as returnValue is an object it is not replaced by thisValue and thus returnValue is the value returned by the proxy to the new operator that called it.

Hey, wait a minute, when the original constructor is called inside the proxy it is bound to thisValue, so when new calls the original constructor in the step 2 it will use thisValue instead of the newly created object as this, right? Nope, as explained again in the MDN Web Docs when a function is bound this is ignored if the function is called by new.

Therefore, in Sinon 4.1.3, thisValue contains the object created by new proxy, but returnValue contains the real object created by new originalConstructor inside the proxy and also returned by the outermost new expression. That is, returnValue is the one you want to use.

Seriously, are you still reading this? Well, thanks :-) It is great that someone else besides me read it; it seems that I did not waste my time after all :-P

[codecov]

Codecov Report

Merging #7433 into master will increase coverage by 0.51%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master    #7433      +/-   ##
============================================
+ Coverage     50.42%   50.94%   +0.51%     
  Complexity    24717    24717              
============================================
  Files          1522     1586      +64     
  Lines         86432    94163    +7731     
  Branches          0     1365    +1365     
============================================
+ Hits          43587    47971    +4384     
- Misses        42845    46192    +3347

Impacted Files	Coverage Δ	Complexity Δ
lib/private/Security/CertificateManager.php	91.08% <0%> (-1%)	39% <0%> (ø)
apps/files_trashbin/lib/Trashbin.php	72.28% <0%> (-0.25%)	136% <0%> (ø)
core/js/systemtags/systemtagsmappingcollection.js	68.42% <0%> (ø)	0% <0%> (?)
core/js/shareitemmodel.js	87.22% <0%> (ø)	0% <0%> (?)
core/js/sharedialoglinkshareview.js	59.21% <0%> (ø)	0% <0%> (?)
core/js/sharedialogshareelistview.js	44.91% <0%> (ø)	0% <0%> (?)
apps/comments/js/commentstabview.js	84.16% <0%> (ø)	0% <0%> (?)
settings/js/users/deleteHandler.js	96.05% <0%> (ø)	0% <0%> (?)
apps/files_versions/js/versionstabview.js	86.84% <0%> (ø)	0% <0%> (?)
core/js/eventsource.js	7.69% <0%> (ø)	0% <0%> (?)
... and 59 more

[MorrisJobke]

Seriously, are you still reading this? Well, thanks :-) It is great that someone else besides me read it; it seems that I did not waste my time after all :-P

😜

Anyways thanks a lot 👍

[ProLoser]

I'm confused. Is the new behavior better or worse?

[danxuliu]

@ProLoser

I'm confused. Is the new behavior better or worse?

I would say just different. According to your pull request calling new on the spied constructor is needed to instantiate ES6 objects, but that prevents spying the this value used in the constructor (if the constructor returned a different value than this); whether that is better or worse depends on your needs ;-)

[MorrisJobke]

stable12 backport: #7451