sinonjs / sinon

Test spies, stubs and mocks for JavaScript.
https://sinonjs.org/
Other
9.6k stars 769 forks source link

Make sandbox concurrency safe #2575

Closed remorses closed 4 months ago

remorses commented 6 months ago

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

  1. Check out this branch
  2. npm install
  3. npm test

Checklist for author

codecov[bot] commented 6 months ago

Codecov Report

Attention: 11 lines in your changes are missing coverage. Please review.

Comparison is base (adcf936) 96.02% compared to head (fabf95c) 95.98%.

Files Patch % Lines
lib/sinon/proxy.js 37.50% 10 Missing :warning:
lib/sinon/proxy-invoke.js 80.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ 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 ``` | [Flag](https://app.codecov.io/gh/sinonjs/sinon/pull/2575/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sinonjs) | Coverage Δ | | |---|---|---| | [unit](https://app.codecov.io/gh/sinonjs/sinon/pull/2575/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sinonjs) | `95.98% <63.33%> (-0.04%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sinonjs#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

fatso83 commented 6 months ago

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
fatso83 commented 6 months ago

@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).

fatso83 commented 5 months ago

Ping?

fatso83 commented 4 months ago

This does not seem like it is going places. Closing due to unresponsiveness.