fix: stop sinon.restore() cascading into sub-sandboxes#2704
Open
charlieleith wants to merge 1 commit intosinonjs:mainfrom
Open
fix: stop sinon.restore() cascading into sub-sandboxes#2704charlieleith wants to merge 1 commit intosinonjs:mainfrom
charlieleith wants to merge 1 commit intosinonjs:mainfrom
Conversation
The ESM port of `createApi` (sinonjs#2683, shipped in 21.1.0) replaced createSandbox: createSandbox with a wrapper that pushes every newly-created sandbox into the root sandbox's fake collection: createSandbox: function createSandbox(config) { const s = createConfiguredSandbox(config); sandbox.getFakes().push(s); return s; } `Sandbox#restore` then walks that collection and calls `.restore()` on each entry. Because a sub-sandbox is itself an entry, every top-level `sinon.restore()` cascades into every sub-sandbox and undoes its stubs/timers/etc. — defeating the whole point of having an isolated sub-sandbox. The same cascade hits `resetHistory` and `verifyAndRestore`. This is the regression reported in sinonjs#2701. Restore the pre-21.1 behaviour: hand the root API a direct reference to `createConfiguredSandbox`. Sub-sandboxes are now isolated; only `subSandbox.restore()` (or `verifyAndRestore`) clears their fakes. Also flip the four sandbox tests that were locking in the buggy cascade: they now assert the parent's restore/resetHistory leaves the child untouched, with an explicit child-side cleanup at the end. Closes sinonjs#2701
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fixes #2701
Summary
Sub-sandboxes returned by
sinon.createSandbox()used to be isolated: onlysubSandbox.restore()cleaned them up. The ESM port of `createApi` (#2683 / 21.1.0) changed that without anyone noticing — it now pushes every newly-created sandbox into the root sandbox's fake collection:```js
createSandbox: function createSandbox(config) {
const s = createConfiguredSandbox(config);
sandbox.getFakes().push(s); // <- new in 21.1.0
return s;
},
```
`Sandbox#restore()` then iterates that collection and calls `.restore()` on every entry, so any top-level `sinon.restore()` cascades into every sub-sandbox and undoes its stubs/timers/etc. Same goes for `resetHistory()` and `verifyAndRestore()` (and fake timers — `Date` etc. installed via `subSandbox.useFakeTimers()` get torn down too).
The repro from #2701 fails on `main`:
```js
const sandbox = Sinon.createSandbox();
const foor = { bar: () => "baz" };
sandbox.stub(foor, "bar").returns("qux");
Sinon.restore(); // <-- 21.1.0 wipes `sandbox`'s stub here
foor.bar(); // expected: "qux" actual: "baz"
```
Fix
Drop the wrapper and expose `createConfiguredSandbox` directly, matching pre-21.1 behaviour. Sub-sandboxes go back to being isolated.
Tests
`test/src/create-sinon-api-test.js` had a test that locked in the buggy cascade (`tracks created sandboxes in the root sandbox collection`). Inverted it, plus added a positive regression test that mirrors the issue's repro.
`test/src/sandbox-test.js` had four "nested sandboxes" tests doing the same — they each asserted that parent `restore`/`resetHistory`/`verifyAndRestore` reaches into the child. Inverted those four to assert the parent does not touch the child, with an explicit `child.restore()` at the end so each test still cleans up after itself.
`mocha 'test/src/*-test.js'` → 1426 passing.
`mocha test/issues/issues-test.js` → 41 passing.
(I couldn't run the full `npm run test` script end-to-end because the browser/distribution build runs OOM in this environment; the affected logic is purely in the Node-side `createApi` so the targeted runs above cover it.)
Notes