Sinon fails in comparing call order when running tests in parallel

[Ethan-Arrowood]

Describe the bug

spy1.calledImmediatelyBefore(spy2) fails when used in a node-tap test suite that is being executed in parallel. In serial mode it works without issue. calledBefore continues to work as normal.

I've even tried using sandboxes for each test but it did not fix the issue.

To Reproduce

Repo: https://github.com/Ethan-Arrowood/sinon-spy-parallel-bug-reproduction

git clone https://github.com/Ethan-Arrowood/sinon-spy-parallel-bug-reproduction
cd sinon-spy-parallel-bug-reproduction
pnpm install
pnpm run test

To see tests passing comment out line 27 in test/index.ts:

+ // tap.jobs = 2;
- tap.jobs = 2;

Expected behavior

I'd expect my spy assertions to pass in both serial and parallel mode

Context (please complete the following information):

• Library version: 14.0.0
• Environment: Node.js 18
• Example URL:
• Other libraries you are using: node-tap, typescript

[Ethan-Arrowood]

Here is the test file you can use to reproduce:

import tap from 'tap'
import sinon from 'sinon'

class F {
    first: any[]
    second: any[]
    third: any[]
    constructor () {
        this.first = []
        this.second = []
        this.third = []
    }
    async run () {
        await this.execute('first');
        await Promise.resolve();
        await this.execute('second');
        await Promise.resolve();
        await this.execute('third');
    }
    async execute(s: 'first' | 'second' | 'third') {
        for (const f of this[s]) {
            await f();
        }
    }
}

tap.jobs = 2;

tap.test('test 1', async t => {
    const sinonSandbox = sinon.createSandbox();

    const f = new F();

    const a = sinonSandbox.spy();
    const b = sinonSandbox.spy();
    const c = sinonSandbox.spy();

    f.first.push(a);
    f.second.push(b);
    f.third.push(c);

    await f.run();

    t.ok(a.calledBefore(b));
    t.ok(b.calledBefore(c));

    t.ok(a.calledImmediatelyBefore(b));
    t.ok(b.calledImmediatelyBefore(c));

    t.end();
})

tap.test('test 2', async t => {
    const sinonSandbox = sinon.createSandbox();

    const f = new F();

    const a = sinonSandbox.spy();
    const b = sinonSandbox.spy();
    const c = sinonSandbox.spy();

    f.first.push(a);
    f.second.push(b);
    f.third.push(c);

    await f.run();

    t.ok(a.calledBefore(b));
    t.ok(b.calledBefore(c));

    t.ok(a.calledImmediatelyBefore(b));
    t.ok(b.calledImmediatelyBefore(c));

    t.end();
})

[Ethan-Arrowood]

I did some digging into the source code. I believe the issue is that the counter used for callId here:

sinon/lib/sinon/proxy-invoke.js

Line 12 in 238f14b

var callId = 0;

is shared between sandboxes.

[mroderick]

That's an interesting one. Thank you for reporting it.
I will take a closer look at this one soon 🤞

[fatso83]

Just as a side note, the quickest path to getting a fix is just making a patch yourself. You already did most of the hard investigative work, so all that's missing is a repro testcase and a PR with the patch😘

[Ethan-Arrowood]

Yeah this was just for a side project so I didn't have the time to dive into a solution. It wasn't immediately obvious how to make callId unique per sandbox. When I have time I'll circle back unless someone else solves it first 😄

[fatso83]

Verified (had to remove some TS to make it work): https://runkit.com/fatso83/sinon-2472-tap-parallel-issue

EDIT: lol, did not see you actually had posted a link to a repo 🙈

[fatso83]

Creating a per-sandbox context object works. This makes your test pass. Not complete, but could be expanded for the other users of createProxy (fake, stub, mock). And tests need updating too:

diff --git a/lib/sinon/proxy-invoke.js b/lib/sinon/proxy-invoke.js
index 4c9c29f0..54934af9 100644
--- a/lib/sinon/proxy-invoke.js
+++ b/lib/sinon/proxy-invoke.js
@@ -9,11 +9,9 @@ const concat = arrayProto.concat;
 const ErrorConstructor = Error.prototype.constructor;
 const bind = Function.prototype.bind;
 
-let callId = 0;
-
-module.exports = function invoke(func, thisValue, args) {
+module.exports = function invoke(ctx, func, thisValue, args) {
     const matchings = this.matchingFakes(args);
-    const currentCallId = callId++;
+    const currentCallId = ctx.callId++;
     let exception, returnValue;
 
     proxyCallUtil.incrementCallCount(this);
diff --git a/lib/sinon/proxy.js b/lib/sinon/proxy.js
index 70a84fd2..2368821f 100644
--- a/lib/sinon/proxy.js
+++ b/lib/sinon/proxy.js
@@ -93,6 +93,7 @@ const proxyApi = {
     },
 
     calledImmediatelyBefore: function calledImmediatelyBefore(proxy) {
+        debugger;
         if (!this.called || !proxy.called) {
             return false;
         }
@@ -240,8 +241,14 @@ delegateToCalls(proxyApi, "alwaysReturned", false, "returned");
 delegateToCalls(proxyApi, "calledWithNew", true);
 delegateToCalls(proxyApi, "alwaysCalledWithNew", false, "calledWithNew");
 
-function createProxy(func, originalFunc) {
-    const proxy = wrapFunction(func, originalFunc);
+/**
+ *
+ * @param ctx
+ * @param func
+ * @param originalFunc
+ */
+function createProxy(ctx, func, originalFunc) {
+    const proxy = wrapFunction(ctx, func, originalFunc);
 
     // Inherit function properties:
     extend(proxy, func);
@@ -253,7 +260,13 @@ function createProxy(func, originalFunc) {
     return proxy;
 }
 
-function wrapFunction(func, originalFunc) {
+/**
+ *
+ * @param ctx sandbox context
+ * @param func
+ * @param originalFunc
+ */
+function wrapFunction(ctx, func, originalFunc) {
     const arity = originalFunc.length;
     let p;
     // Do not change this to use an eval. Projects that depend on sinon block the use of eval.
@@ -262,72 +275,72 @@ function wrapFunction(func, originalFunc) {
         /*eslint-disable no-unused-vars, max-len*/
         case 0:
             p = function proxy() {
-                return p.invoke(func, this, slice(arguments));
+                return p.invoke(ctx, func, this, slice(arguments));
             };
             break;
         case 1:
             p = function proxy(a) {
-                return p.invoke(func, this, slice(arguments));
+                return p.invoke(ctx, func, this, slice(arguments));
             };
             break;
         case 2:
             p = function proxy(a, b) {
-                return p.invoke(func, this, slice(arguments));
+                return p.invoke(ctx, func, this, slice(arguments));
             };
             break;
         case 3:
             p = function proxy(a, b, c) {
-                return p.invoke(func, this, slice(arguments));
+                return p.invoke(ctx, func, this, slice(arguments));
             };
             break;
         case 4:
             p = function proxy(a, b, c, d) {
-                return p.invoke(func, this, slice(arguments));
+                return p.invoke(ctx, func, this, slice(arguments));
             };
             break;
         case 5:
             p = function proxy(a, b, c, d, e) {
-                return p.invoke(func, this, slice(arguments));
+                return p.invoke(ctx, func, this, slice(arguments));
             };
             break;
         case 6:
             p = function proxy(a, b, c, d, e, f) {
-                return p.invoke(func, this, slice(arguments));
+                return p.invoke(ctx, func, this, slice(arguments));
             };
             break;
         case 7:
             p = function proxy(a, b, c, d, e, f, g) {
-                return p.invoke(func, this, slice(arguments));
+                return p.invoke(ctx, func, this, slice(arguments));
             };
             break;
         case 8:
             p = function proxy(a, b, c, d, e, f, g, h) {
-                return p.invoke(func, this, slice(arguments));
+                return p.invoke(ctx, func, this, slice(arguments));
             };
             break;
         case 9:
             p = function proxy(a, b, c, d, e, f, g, h, i) {
-                return p.invoke(func, this, slice(arguments));
+                return p.invoke(ctx, func, this, slice(arguments));
             };
             break;
         case 10:
             p = function proxy(a, b, c, d, e, f, g, h, i, j) {
-                return p.invoke(func, this, slice(arguments));
+                return p.invoke(ctx, func, this, slice(arguments));
             };
             break;
         case 11:
             p = function proxy(a, b, c, d, e, f, g, h, i, j, k) {
-                return p.invoke(func, this, slice(arguments));
+                return p.invoke(ctx, func, this, slice(arguments));
             };
             break;
         case 12:
             p = function proxy(a, b, c, d, e, f, g, h, i, j, k, l) {
-                return p.invoke(func, this, slice(arguments));
+                return p.invoke(ctx, func, this, slice(arguments));
             };
             break;
         default:
             p = function proxy() {
-                return p.invoke(func, this, slice(arguments));
+                return p.invoke(ctx, func, this, slice(arguments));
             };
             break;
         /*eslint-enable*/
diff --git a/lib/sinon/sandbox.js b/lib/sinon/sandbox.js
index 47b75eb0..07e874f8 100644
--- a/lib/sinon/sandbox.js
+++ b/lib/sinon/sandbox.js
@@ -37,6 +37,7 @@ function applyOnEach(fakes, method) {
 
 function Sandbox() {
     const sandbox = this;
+    sandbox.callId = 0
     let fakeRestorers = [];
     let promiseLib;
 
@@ -381,7 +382,7 @@ function Sandbox() {
     }
 
     sandbox.spy = function spy() {
-        const createdSpy = sinonSpy.apply(sinonSpy, arguments);
+        const createdSpy = sinonSpy.apply(sinonSpy, [sandbox, ...arguments]);
         return commonPostInitSetup(arguments, createdSpy);
     };
 
diff --git a/lib/sinon/spy.js b/lib/sinon/spy.js
index 761a557d..82204871 100644
--- a/lib/sinon/spy.js
+++ b/lib/sinon/spy.js
@@ -21,6 +21,12 @@ const filter = Array.prototype.filter;
 
 let uuid = 0;
 
+/**
+ *
+ * @param fake
+ * @param args
+ * @param strict
+ */
 function matches(fake, args, strict) {
     const margs = fake.matchingArguments;
     if (
@@ -130,7 +136,12 @@ delegateToCalls(
     }
 );
 
-function createSpy(func) {
+/**
+ *
+ * @param context
+ * @param func
+ */
+function createSpy(context, func) {
     let name;
     let funk = func;
 
@@ -142,7 +153,7 @@ function createSpy(func) {
         name = functionName(funk);
     }
 
-    const proxy = createProxy(funk, funk);
+    const proxy = createProxy(context, funk, funk);
 
     // Inherit spy API:
     extend.nonEnum(proxy, spyApi);
@@ -155,13 +166,20 @@ function createSpy(func) {
     return proxy;
 }
 
-function spy(object, property, types) {
+/**
+ *
+ * @param context
+ * @param object
+ * @param property
+ * @param types
+ */
+function spy(context, object, property, types) {
     if (isEsModule(object)) {
         throw new TypeError("ES Modules cannot be spied");
     }
 
     if (!property && typeof object === "function") {
-        return createSpy(object);
+        return createSpy(context, object);
     }
 
     if (!property && typeof object === "object") {
@@ -169,20 +187,24 @@ function spy(object, property, types) {
     }
 
     if (!object && !property) {
-        return createSpy(function () {
+        return createSpy(context, function () {
             return;
         });
     }
 
     if (!types) {
-        return wrapMethod(object, property, createSpy(object[property]));
+        return wrapMethod(
+            object,
+            property,
+            createSpy(context, object[property])
+        );
     }
 
     const descriptor = {};
     const methodDesc = getPropertyDescriptor(object, property);
 
     forEach(types, function (type) {
-        descriptor[type] = createSpy(methodDesc[type]);
+        descriptor[type] = createSpy(context, methodDesc[type]);
     });
 
     return wrapMethod(object, property, descriptor);

[fatso83]

The difficult is done, so if anyone wants this fixed, provide a PR with the remaining bits :) Passing on the sandbox was just a shortcut. One could also pass on a variable const context = { callCount : 0} or something. Just a POC.

[stale]

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[Ethan-Arrowood]

Didn't realize @fatso83 posted a nearly complete solution. I still don't have the time to make this contribution, but I will share it on twitter. Maybe someone else will be interested in it.

[remorses]

I am working on a PR