[code-infra] Move the shared spy helpers off Sinon - #23466
Conversation
Fourth step away from Sinon, following mui#23443, mui#23460 and mui#23464. Those took the spies each test owns. This one takes the two shared helpers and every call site that reads what they return, which had to move together. `spyApi` in `test/utils/helperFn.ts` wraps a Grid API method, so it maps straight to `vi.fn`. `StoreSpy` is the first real spy-on-object in this series: `spy(store, method)` becomes `vi.spyOn(store, method)`, and the `restore()` in its cleanup becomes `mockRestore()`. The helper has always restored its own spy, so the shared teardown still needs no `vi.restoreAllMocks()`. `calledBefore` has no direct equivalent and becomes a comparison of `mock.invocationCallOrder`. Two files keep their Sinon import for reasons outside this step: `cellSelection.DataGridPremium` still uses `stub`, and `rowEditing.DataGridPro` still uses the Sinon fake timers. Sinon is now down to 29 files.
JCQuintas
added
test
type: enhancement
Deploy previewBundle size
Check out the code infra dashboard for more information about this PR. |
|
@claude review |
PR reviewNothing here is merge-blocking. This is a mechanical Sinon→Vitest migration of the two shared spy helpers ( Tests (1)1. 🟡 Double-negative assertion no longer matches the test nameLocation: it('should not call getDataAsExcel', async () => {
render(<TestCaseExcelExport />);
const getDataAsExcelSpy = spyApi(apiRef.current!, 'getDataAsExcel');
await act(() => apiRef.current?.exportDataAsExcel({ worker: () => workerMock as any }));
expect(getDataAsExcelSpy.mock.calls.length).not.to.equal(1);
});The old Failure scenario: A regression that makes the worker path call Fix: Simplifications (4)1. 🟡
|
- `should not call getDataAsExcel` asserted `not.to.equal(1)`, the literal translation of the old `calledOnce === false`, which passes on two calls as well. Pin it to zero. - Give `spyApi` a real return type and drop the `as any` on the spy. The wrapper it returns is `Mock` plus the `spying`/`target` fields the grid reads back, so consumers now get their `.mock.*` reads type-checked. Only the two writes onto the API keep an escape hatch: indexing with `keyof GridApiCommon` collapses to an intersection of every method signature. - Assert the call before destructuring `mock.lastCall`, so an un-called spy fails by name instead of surfacing later as a property TypeError. - Drop a duplicated assertion and a dead optional call on `mockRestore`. Kept `to.be.greaterThan(0)` over the suggested `to.not.equal(0)` for the call-count checks. These spies are read through `?.`, so a spy that was never created reads as `undefined`, and `expect(undefined).to.not.equal(0)` passes while `to.be.greaterThan(0)` fails.
|
Applied everything except one sub-suggestion, which I want to flag because it would weaken the assertion. In Simplifications 2 the suggestion was to prefer These spies come from expect(selectRecurringEventScopeSpy?.mock.calls.length).to.be.greaterThan(0);I checked both matchers against
Everything else is in:
|
|
@claude review |
PR reviewNothing merge-blocking. This is a mechanical Sinon→Vitest swap of the two shared test helpers plus every call site, and the accessor mappings check out: Three assertions are strengthened rather than mapped one-to-one, and are worth a reviewer's eye since they are the only non-mechanical parts of a 750-line diff: Tests (2)1. ℹ️
|
|
Took one of the three, and I checked the other two rather than reasoning about them. Tests 1, Sinon's Tests 2, the "vacuous" assertion: this one does not hold. The claim is that The assertion already fails loudly on a never-called mock, so there is nothing to strengthen and no reason to reach for a non-null assertion. Worth noting the general shape, since it came up in the previous review too: on an optional-chained read, Simplifications 1, (api as unknown as Record<string, unknown>)[methodKey] = spyFn;
|
LukasTy
left a comment
LukasTy
left a comment
There was a problem hiding this comment.
Claude Opus findings
-
mock.lastCall?.[0]makes the fourto.equal(undefined)assertions vacuous. On a never-called mock the optional chain yieldsundefined, so the assertion passes where the Sinon original threw. I probed it rather than reasoning about it:expect(neverCalled.mock.lastCall?.[0].foo).to.equal(undefined)PASSES, whilemock.lastCall![0].fooTHROWS. The sites areeditComponents.DataGridPro.test.tsx:258and:436(.debounceMs), andcellEditing.DataGridPro.test.tsx:247androwEditing.DataGridPro.test.tsx:289(.foo). All four are covered today by a neighbouring assertion that fails first, so there is no live gap. Butmock.lastCall![0]is the faithful translation, and this is the rule worth carrying into the remaining 29 files: on an optional-chained read,to.equal(<concrete value>)is safe andto.equal(undefined)is not. -
The
?? {}fallbacks are now dead. Withexpect(...mock.calls.length).to.equal(1)in front of each destructure,lastCallcannot beundefinedatexportExcel.DataGridPremium.test.tsx:777orEventDialog.test.tsx:3360. Both expressions are alreadyany, so nothing needs the fallback to type-check either. The call-count assertion fixed the diagnostic; the fallback can go with it.
2 participants
Fourth step away from Sinon, following #23443, #23460 and #23464.
The previous steps took the spies each test file owns. This one takes the two shared helpers, and every call site that reads what they return. Those had to move together: a helper cannot change its return type without every consumer changing its accessors in the same commit.
Sinon goes from 37 files to 29. It stays a dependency, and
test/setupVitest.tskeeps callingsinon.restore().The two helpers
spyApi(test/utils/helperFn.ts) wraps a Grid API method with a recording function and swaps it onto the api object. It never used the spy-on-object form, so it maps straight tovi.fn.StoreSpy(test/utils/scheduler/StoreSpy.tsx) is the first real spy-on-object in this series:Worth noting for the
vi.spyOnstep that follows: this helper has always restored its own spy in its effect cleanup, so it needs nothing from the shared teardown. Novi.restoreAllMocks()is required here.Their spies are typed accordingly,
MockforspyApiandMockInstanceforStoreSpy.calledBeforeThe one accessor with no equivalent. Sinon compares call objects; Vitest exposes a global ordering counter instead:
Types that needed a real signature
vi.fn(() => new Promise(() => {}))declares no parameters, somock.lastCalltypes as an empty tuple even though the Grid calls it with a row. TwoprocessRowUpdatemocks now declare the parameter their assertions read, the same fix #23464 needed for a getter mock.Still on Sinon here
One file keeps its Sinon import for a reason outside this step:
cellSelection.DataGridPremiumusesstubforrequestAnimationFrame. It now reads its helper-derived spies through the Vitest API while that waits for a later step.Status
typescriptandeslintare clean, and the browser suite is fully green: 761 files passed, 4 skipped, 0 failures.The jsdom suite shows the usual intermittent failures in
x-telemetry/src/postinstall/get-project-id.test.tson my machine. That file is untouched here, it passes when its package runs alone, andtest_unithas been green on it in CI, so it is local environment noise.