Make sandbox concurrency safe #2575
Conversation
There was a problem hiding this comment.
Thanks for having a go at this, @remorses! I tried taking a proper look, and I think I got there in the end, even though it was late.
There are two main issues I need covered:
- this needs to work for fakes, stubs and mocks as well
- Optional context seems like a bad idea
Since stubs are spies that should be a wrap. I guess mocks are covered by being proxies ... ? But AFAIK fakes do not share much code with these, so that needs to be addressed (I noted that in #2472 (comment)).
I am pretty sure you could template the test case so that you can make the same test for both spies and fakes, btw, just parameterising it by passing in some factory method.
Wrt the context, the fact that it has been made optional concerns me. Why is it optional? Is it just to avoid having to change testcases? A good IDE should be able to fill in default values for you when adding a new parameter. If it's just for convenience, I would rather just fix it. All spies are created from a sandbox, even the ones on the default Sinon instance, so there should be no need for the global module, AFAIK.
| describe("concurrency safe", function () { | ||
| it("spy", async function () { | ||
| class F { | ||
| 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) { | ||
| for (const f of this[s]) { | ||
| await f(); | ||
| } | ||
| } |
There was a problem hiding this comment.
This test was a bit hard to wrap my head around, at least at midnight ... 😄 Could you clarify what you are really doing? I mean, I assume you are somehow testing quazi-concurrent runs, interlacing runs from different sandboxes, but I still find it a bit hard to reason around the order of the calls.
Is there some way you could re-order or refactor this in some way that made the intent as expressed through the code clearer?
To me, it is not at all clear how the sandbox runs are being interlaced and in what order functions are being called. Would it not be much clearer if doing something a la the following pseudo-code?
const sandboxASpy1 = sandboxA.spy();
const sandboxASpy2 = sandboxA.spy();
const sandboxBSpy1 = sandboxB.spy();
const sandboxBSpy2 = sandboxB.spy();
sandboxASpy1();
sandboxBSpy1();
sandboxBSpy2();
sandboxASpy2();
assert(sandboxASpy1.calledImmediatelyBefore(sandboxASpy2));
assert(sandboxBSpy1.calledImmediatelyBefore(sandboxBSpy2));There is no need for awaits AFAIK. JS is single threaded no matter what, and the asynchronicity just muddles up what is going on IMHO.
| object, | ||
| property, | ||
| types, | ||
| sandbox, |
There was a problem hiding this comment.
If you are passing in the sandbox as the context, then what is the need for the global context object?
| @@ -253,7 +254,7 @@ function createProxy(func, originalFunc) { | |||
| return proxy; | |||
| } | |||
|
|
|||
| function wrapFunction(func, originalFunc) { | |||
| function wrapFunction(func, originalFunc, ctx = globalContext) { | |||
There was a problem hiding this comment.
Why are all of these context objects optional, with a default global? Even the default Sinon instance is a sandbox in itself.
| if (ctx.callId == null) { | ||
| ctx.callId = 0; | ||
| } |
There was a problem hiding this comment.
When does this ever happen? I cannot see any tests exercising this.
I see the fallback global module, where it is already initialised, and I see the sandbox, which is also initialised.
| "use strict"; | ||
|
|
||
| module.exports = { | ||
| callId: 0, | ||
| }; |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #2575 +/- ##
==========================================
- Coverage 96.02% 95.98% -0.04%
==========================================
Files 40 41 +1
Lines 1912 1918 +6
==========================================
+ Hits 1836 1841 +5
- Misses 76 77 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
This currently fails on your branch: cat > test/concurrency-test.js << EOF
const assert = require("node:assert");
const sinon = require("../lib/sinon");
const sbA = sinon.createSandbox();
const sbB = sinon.createSandbox();
const F = sbA.fake();
const f1 = sbB.fake();
const f2 = sbB.fake();
f1();
F();
f2();
assert(f1.calledImmediatelyBefore(f2));
EOF
node test/concurrency-test.js |
|
@remorses Do you need some guidance in how to progress? AFAIK the old-skool API is covered (haven't checked the mocks API, but I think that should be covered by the proxy stuff). |
|
Ping? |
|
This does not seem like it is going places. Closing due to unresponsiveness. |
Make sandbox concurrency safe
Fix #2472, sandbox not working when running tests concurrently because of global callId variable
Solution
Store callId in a context passed with an optional argument, which is the sandbox object when using sandbox
How to verify - mandatory
Added a test with the #2472 reproduction
npm installChecklist for author
npm run lintpasses