Skip to content

fix: stop sinon.restore() cascading into sub-sandboxes#2704

Open
charlieleith wants to merge 1 commit intosinonjs:mainfrom
charlieleith:fix-2701-isolated-sandboxes
Open

fix: stop sinon.restore() cascading into sub-sandboxes#2704
charlieleith wants to merge 1 commit intosinonjs:mainfrom
charlieleith:fix-2701-isolated-sandboxes

Conversation

@charlieleith
Copy link
Copy Markdown

fixes #2701

Summary

Sub-sandboxes returned by sinon.createSandbox() used to be isolated: only subSandbox.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

  • 21.0.x had `createSandbox: createSandbox` (no nesting), which is the behaviour this PR returns to.
  • `createStubInstance` still pushes its synthesized methods into the root collection — that's intentional because there's no per-stub-instance handle to restore them with, and was true pre-21.1 as well. Untouched here.

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sinon.restore() also restores sandboxes in 21.1.0

1 participant