[JCQuintas]

Fifth step away from Sinon, following #23443, #23460, #23464 and #23466. Sinon goes from 29 files to 22.

Bare stub()

A standalone stub is a vi.fn(), with its configuration methods renamed:

stub()            -> vi.fn()
.returns(value)   -> .mockReturnValue(value)
.resolves(value)  -> .mockResolvedValue(value)
.rejects(error)   -> .mockRejectedValue(error)
.reset()          -> .mockReset()
.restore()        -> .mockRestore()

stub(obj, 'method') is not a plain vi.spyOn

This is the part worth reviewing. Sinon replaces the method with a no-op that returns undefined; vi.spyOn wraps the method but keeps calling the original. So the replacement has to be spelled out:

-stub(console, 'error');
+vi.spyOn(console, 'error').mockImplementation(() => {});

That one is load-bearing: it backs the swallow warnings suite in layout.DataGrid.test.tsx, and a bare vi.spyOn there would stop swallowing and let the suite fail on the warnings it exists to absorb. The same applies to the performance.now, navigator.clipboard.writeText and requestAnimationFrame stubs, which all carried a callsFake and now carry a mockImplementation.

Restoring

Four of the five spy-on-object sites already restored explicitly, in an afterEach or a try/finally, so they only needed the method renamed.

AdapterDayjs.test.tsx was the one relying on the shared sinon.restore(), and said so in a comment. It now restores through onTestFinished. So this step still needs no vi.restoreAllMocks() in the shared teardown.

Files that come along

Four files were held back from the earlier steps only by their stub usage, so their remaining spy() calls and accessors migrate here too, leaving them fully Sinon-free: aiAssistant, layout, rows and clipboard.

Status

typescript and eslint are clean, and the changed test files pass in the browser.

Two things this step caught that are worth knowing for the remaining migrations:

• A blanket spy( to vi.fn( rewrite silently produces vi.fn(obj, 'method') where the source was a spy-on-object. TypeScript catches it on arity, but only because vi.fn takes at most one argument. There are no remaining occurrences anywhere in the repo.
• Renaming stub() to vi.fn() does not update the accessors on that variable, and .args on a Mock is any, so both tsc and eslint stay green while the test is broken. Only running the suite surfaced it.

[code-infra-dashboard]

Deploy preview

https://deploy-preview-23472--material-ui-x.netlify.app/

Bundle size

Bundle	Parsed size	Gzip size
@mui/x-data-grid	0B(0.00%)	0B(0.00%)
@mui/x-data-grid-pro	0B(0.00%)	0B(0.00%)
@mui/x-data-grid-premium	0B(0.00%)	0B(0.00%)
@mui/x-charts	0B(0.00%)	0B(0.00%)
@mui/x-charts-pro	0B(0.00%)	0B(0.00%)
@mui/x-charts-premium	0B(0.00%)	0B(0.00%)
@mui/x-date-pickers	0B(0.00%)	0B(0.00%)
@mui/x-date-pickers-pro	0B(0.00%)	0B(0.00%)
@mui/x-tree-view	0B(0.00%)	0B(0.00%)
@mui/x-tree-view-pro	0B(0.00%)	0B(0.00%)
@mui/x-scheduler	0B(0.00%)	0B(0.00%)
@mui/x-scheduler-premium	0B(0.00%)	0B(0.00%)
@mui/x-chat	0B(0.00%)	0B(0.00%)
@mui/x-license	0B(0.00%)	0B(0.00%)

Details of bundle changes

---

Check out the code infra dashboard for more information about this PR.

[JCQuintas]

@claude review

[github-actions]

PR review

This is a faithful, mechanical Sinon→Vitest migration of seven test files, and the part the description flags as load-bearing is handled correctly: every stub(obj, 'method') site became vi.spyOn(...).mockImplementation(...) / .mockReturnValue(...) rather than a bare vi.spyOn, so console.error, performance.now, requestAnimationFrame and dayjs.tz.guess still stop calling through, while the five spy(navigator.clipboard, 'writeText') sites correctly keep call-through. reset()→mockReset() and resetHistory()→mockClear() are also mapped the right way round, and all six spy-on-object sites restore locally, so the removal of the Sinon default-sandbox net doesn't leak here. Nothing is merge-blocking; the findings below are a latent shadowing hazard and three robustness/heads-up notes for the rest of the series.

Tests (3)

1. 🟡 promptSpy's seeded resolved value is dead, so the data-sampling tests run onPrompt as undefined

Location: packages/x-data-grid-premium/src/tests/aiAssistant.DataGridPremium.test.tsx:43

const promptSpy = vi.fn().mockResolvedValue({});
// ...
beforeEach(() => {
  promptSpy.mockReset();
});

mockReset() clears the implementation, and since {} was installed via .mockResolvedValue() rather than passed to vi.fn(impl), the seed on line 43 is wiped before the first test ever runs. promptSpy therefore returns undefined in the two data sampling tests, await onPrompt(...) yields undefined, and applyPromptResult(undefined) throws on result.filters.map inside the try at useGridAiAssistant.tsx:462, silently pushing the prompt down the catch branch. The tests still pass because they only assert on the arguments promptSpy received.

This is not a regression — stub().resolves({}) + .reset() behaved identically — but the migration is the natural moment to stop pretending the seed does something.

Failure scenario: A reader adds an assertion that the prompt renders in the success variant after data sampling and it fails for a reason unrelated to sampling, because onPrompt resolved to undefined and the grid took the error path.

Fix: Either drop the .mockResolvedValue({}) on line 43 as dead, or move a real default into the hook so the happy path is actually exercised:

const promptSpy = vi.fn();

beforeEach(() => {
  promptSpy.mockReset();
  promptSpy.mockResolvedValue({ select: -1, filters: [], aggregation: {}, sorting: [], grouping: [] });
});

Worth carrying into the remaining 22 files: vi.fn(impl).mockReset() is not equivalent to Sinon's stub().reset() — Vitest restores impl, Sinon drops the configured behaviour. The description's reset() -> mockReset() row holds only while the mock is created bare, as it is here.

2. ℹ️ Optional chaining on lastCall still throws one property later

Location: packages/x-data-grid-premium/src/tests/clipboard.DataGridPremium.test.tsx:997

expect(onClipboardPasteEnd.mock.lastCall?.[0].newRows.get(0)).to.equal(replacement);

The ?. guards the array index but not .newRows, so a spy that was never called yields TypeError: Cannot read properties of undefined (reading 'newRows') instead of an assertion failure. The other mock.lastCall?.[0] conversions in this PR end the chain at the index and are fine; this is the only one that keeps dereferencing.

Failure scenario: A future change stops emitting clipboardPasteEnd and the test reports an opaque TypeError in the middle of the assertion rather than "expected undefined to equal { … }".

Fix: Assert the call first, then index without the guard — expect(onClipboardPasteEnd).toHaveBeenCalled(); followed by onClipboardPasteEnd.mock.lastCall![0].

3. ℹ️ These files lose the Sinon sandbox's restore-of-last-resort

Location: test/setupVitest.ts:44

afterEach(() => {
  // Restore Sinon default sandbox to avoid memory leak
  sinon.restore();
  config.disabled = false;
});

stub(console, 'error') and stub(navigator.clipboard, 'writeText') were registered in the default sandbox, so this global afterEach restored them even if a local hook was skipped. After this PR the six vi.spyOn sites depend entirely on their own afterEach / try…finally / onTestFinished — all of which are present and correct, so there is nothing broken today. The description's claim that no shared vi.restoreAllMocks() is needed is accurate for this step.

Failure scenario: A later migration step converts a stub(obj, 'method') and forgets the local restore; console.error or navigator.clipboard.writeText stays mocked for the rest of the file and a downstream test fails somewhere unrelated, with no hint of the cause.

Fix: Consider adding vi.restoreAllMocks() next to sinon.restore() in this hook as the migration continues, so the safety net is restored once rather than audited per file.

Simplifications (1)

1. 🟡 Local requestAnimationFrame shadows the global inside the Auto-scroll suite

Location: packages/x-data-grid-premium/src/tests/cellSelection.DataGridPremium.test.tsx:609

describe.skipIf(isJSDOM)('Auto-scroll', () => {
  let requestAnimationFrame: MockInstance;

  beforeEach(() => {
    requestAnimationFrame = vi.spyOn(window, 'requestAnimationFrame').mockImplementation(() => 0);
  });

Inside lines 608–717 the identifier requestAnimationFrame now resolves to the MockInstance, not the DOM API. No current test in that block calls it unqualified (the two bare requestAnimationFrame(...) calls at lines 1187–1188 are in the Fill handle suite, outside this scope), so nothing is broken — but the name is a trap, and it's inconsistent with the writeTextStub, performanceNowStub and consoleError naming used for the equivalent sites elsewhere in this same PR.

Failure scenario: Someone adds await new Promise((resolve) => requestAnimationFrame(resolve)) to an auto-scroll test; it calls the mock object instead of the browser API and fails with requestAnimationFrame is not a function, or hits a TDZ error if referenced before beforeEach assigns it.

Fix: Rename the variable — let rafStub: MockInstance; — matching the *Stub convention the rest of the diff uses.

Verdict

Approve after nits — the semantically risky conversions (stub(obj, 'method') → spyOn + mockImplementation, reset vs resetHistory, and per-site restoration) are all correct; the remaining items are a shadowed identifier and three robustness notes for the rest of the migration.

---

🤖 Review generated with Claude Code · Opus 5 (High) · medium review depth · 36 turns · 6m27s · $2.17 · run

[JCQuintas]

Took two of the four, and checked the two behavioural claims rather than reasoning about them.

Tests 1, the dead promptSpy seed: confirmed, dropped. The asymmetry you describe is real:

PROBE bare before reset: {"seeded":true}
PROBE bare after reset: undefined
PROBE vi.fn(impl) after reset: {"fromImpl":true}

mockReset restores the implementation passed to vi.fn(impl) but drops one configured afterwards, so the mockResolvedValue({}) on line 43 was wiped before the first test. I dropped it rather than seeding a real default, since the seed was equally dead under Sinon and installing one now would change which branch these tests exercise, which is beyond a migration. Worth someone deciding separately whether the data-sampling tests should be on the success path.

Good catch on the general rule, and it is not what the description's mapping table implies. reset() -> mockReset() only holds for a bare mock.

Simplifications 1, the shadowed requestAnimationFrame: renamed to rafStub. Agreed on both counts, the shadowing and the inconsistency with writeTextStub / performanceNowStub / consoleError in the same diff.

Tests 2, the optional chain: this one does not hold. The claim is that onClipboardPasteEnd.mock.lastCall?.[0].newRows.get(0) throws Cannot read properties of undefined (reading 'newRows') when the spy was never called. Optional chaining short-circuits the entire remaining chain, not just the indexing step:

PROBE lastCall?.[0].newRows.get(0) threw: false | result: undefined

So it evaluates to undefined and the assertion fails normally. It would only throw if lastCall were defined and lastCall[0] were not, which cannot happen for a recorded call. Left as is.

Tests 3, vi.restoreAllMocks() in the shared teardown: agreed in principle, deferring. Every step in this series has turned out to restore locally, so it has not been needed yet. It also is not free: restoreAllMocks clears module-scope vi.fn() mocks that back a vi.mock() factory, which is exactly the shape x-telemetry/src/postinstall/get-project-id.test.ts uses. Adding it as a blanket net would need that checked first. I would rather add it in the step that converts the remaining spy(obj, 'method') sites, where it is actually load-bearing, than now.