stub.resolves(xxx) still applies after stub.callThrough()

[Sigill]

Describe the bug

I have a test where a stub is initially configured to stub.resolve(xxx).
Later I stub.callThrough() to restore the the initial method.
However, when calling the stubbed method, the original method is not called, resolve(xxx) still applies.

To Reproduce

it("#xxx - callThrough not clearing resolves()", async function () {
    const ns = {
        async foo() { return 'bar'; }
    };

    const stub = sinon.stub(ns, 'foo');
    stub.resolves('baz');

    assert.equals(await ns.foo(), 'baz');

    stub.callThrough();

    assert.equals(await ns.foo(), 'bar'); // fails, stub resolves with 'baz'.
});

Note that the same test without the async/await is pass.

Expected behavior

The original method should have been called.

Context (please complete the following information):

• Sinon version : 17.0.2 to 21.0.0
• Runtime: Node 20
• Output of npx envinfo --browsers --binaries:

  Binaries:
    Node: 20.17.0 - /home/cfx/.nvm/versions/node/v20.17.0/bin/node
    npm: 10.8.2 - /home/cfx/.nvm/versions/node/v20.17.0/bin/npm
    pnpm: 10.26.0 - /home/cfx/.nvm/versions/node/v20.17.0/bin/pnpm
  Browsers:
    Chrome: 127.0.6533.72

• Other relevant environmental info:
• Other libraries you are using:
• Example URL:

Additional context
The issue appears to have been introduced by #2593, the issue does not occur with Sinon 17.0.1.

Inspired by various cleanup logic from default-behaviors.js, I've been able to quick fix the issue with:

diff --git a/lib/sinon/default-behaviors.js b/lib/sinon/default-behaviors.js
index ba85007e..76d64556 100644
--- a/lib/sinon/default-behaviors.js
+++ b/lib/sinon/default-behaviors.js
@@ -240,6 +240,14 @@ const defaultBehaviors = {
     },
 
     callThrough: function callThrough(fake) {
+        fake.returnValue = undefined;
+        fake.resolve = false;
+        fake.resolveThis = false;
+        fake.reject = false;
+        fake.returnValueDefined = false;
+        fake.exception = undefined;
+        fake.exceptionCreator = undefined;
+        fake.fakeFn = undefined;
         fake.callsThrough = true;
     },
 

However I'm not sure to understand why the cleanup logic varies so much from case to case in this file and what really needs to be cleaned-up, so this might not be correct/sufficient.

Seeing the implementation of callThroughWithNew() in default-behaviors.js, I'm wondering if it has the same issue.

[fatso83]

Thanks for looking into this. The whole mutability aspect of this part of the API is why we are not spending a lot of time trying to fix it: it is just so complex to keep the mutation space in your head, as you have seen.

I would not mind having a PR for this with this fix, as long as other tests don't break. You could try the other fix for callThroughWithNew and see how it goes.

[fatso83]

Be aware of #2656 too. This is fragile territory ☠️

[Sigill]

Ok, I came up with this fix: #2670.

Regarding callThroughWithNew():
The fix could be nearly identical, but at the moment, I've made simpler.
The reason is because callThroughWithNew() is meant to be used with constructors, which means returnThis, resolves, resolvesArg, resolvesThis, rejects, returnsArg and callsFake don't apply.

Unless (I believe, still need to test that theory) we want to support stuff like:

function MyClass() {
    if (!(this instanceof MyClass)) {
        return new MyClass();
    }
    // rest of the initialization...
}

To allow an instance of MyClass to be instantiated with or without the new operator.
I'm not sure if this is something that people do (it's not possible with classes, only constructor functions), but at least Date does it.
It opens a whole can of worm, e.g. having a constructor which, when called without new, is async.
Applying the cleanup logic from callThrough() to callThroughWithNew() should work, I just want a second opinion before doing so.

[fatso83]

Sorry for keeping you hanging ... let's keep it simple :-)