refactor(test): share test setup across the suites - #1069
Draft
lukecotter wants to merge 10 commits into
Draft
lukecotter wants to merge 10 commits into
lukecotter wants to merge 10 commits into
Conversation
21 suites carried 35 copies of jest.mock('#vscode-elements/…', () => ({})).
jsdom's ElementInternals has no setFormValue, so a form-associated
element fails on its first update, and vscode-icon warns on every
connect about the missing codicon stylesheet. That is a library-wide
fact, not a per-suite decision, so it moves to moduleNameMapper.
This widens the rule from opt-in to blanket. 38 suites reach a
#vscode-elements import transitively and 21 already mocked, so 17 now
get the stub where they previously loaded the real module. None assert
vendor-element behaviour and the test count is unchanged.
TimelineKey and DockLayout stubbed a whole component apiece to dodge
the same jsdom problem. The mapper covers it, so both now exercise the
real OverflowList and DetailDock.
vscode-single-select is exempt: VsSelect extends VscodeSingleSelect and
spreads its static styles, so a stub cannot be extended. VsSelect says
so, and warns that mounting a <vs-select> under jsdom needs its own
setFormValue shim.
34 suites each wrote their own create, configure, append and await. They now call
mountElement, which owns that sequence and throws a named error when a tag is not a
rendered Lit element — an unregistered tag otherwise awaits an undefined
updateComplete and fails later on a null shadowRoot.
Three suites drop an `as unknown as { logStore }` cast: the cast was there only
because the mount was typed LitElement, not the component's own exported class.
Two suites settle one update more than before, because the helper always awaits the
first render where their old helper went straight to a longer settle:
LogInspector.test.ts and NamespaceTimeBar.test.ts. Both pass; extra settling cannot
cause a false failure, but it can mask a render-timing regression.
Left alone: skeletonStatus.test.ts mounts into a ContextProvider host rather than the
body, and AnalysisView.test.ts constructs the view without ever adding it to the DOM.
Four suites each wrote the same createEvent, differing only in which optional fields they exposed. One builder now serves all four. It carries the fields these passes actually read and no others, typed Partial<LogEvent> rather than cast blind, so a renamed or reshaped field on the real class fails the typecheck here. Fourteen fields the old builders set and nothing reads are gone. The per-suite timestamp counter and its eight beforeEach resets are gone too: no module these suites drive reads event.timestamp, so the resets did nothing. eventText defaulted exitStamp to 1000, the others to null. null is the class default and models a frame that never exited, so it wins, and the two cases that read a duration now say exitStamp where they assert on it. Left alone: ExecutionHighlights derives exitStamp from a running stamp and needs eventIndex, and the timeline builders are positional and shape their duration differently. Roughly nine local event builders remain across the repo.
The glob was unquoted, so the shell expanded it before prettier saw it. Without globstar that reaches 5 files; the repo holds 537 TypeScript files alone. Every nested path went unchecked. It stayed invisible because lint-staged writes prettier over staged files on the way into a commit, so the repo is formatted and the gate that was meant to prove it was reading almost nothing. Quoted, prettier expands the glob itself, as prettier-format already does.
pnpm puts its store in .pnpm-store here when the usual location is not writable, which leaves 947MB untracked and one git add -A away from the history.
A mounted component stayed connected once its test ended, and the listeners its connectedCallback registered stayed live with it. A suite that emitted on the eventBus after several mounts reached every one of them. Cleanup was per suite and ad hoc: sixteen hooks cleared the body, four of them doing nothing else, and the rest of the jsdom suites cleared nothing. setup.ts now does it once for all of them, guarded because the suites run under node unless a file asks for jsdom. replaceChildren rather than innerHTML, so disconnectedCallback runs and the subscriptions release.
core/utility/FrameBudget.ts exports waitForNextFrame, and six sites in three suites wrote it out again. The tests now yield through the same helper the code under test yields through. The setTimeout(0) waits stay as they are: waitForNextTask posts through a MessageChannel, which does not order the same way against a timer.
Three suites each wrapped a body in the same header, execution and code unit, then parsed it. The envelope, the settings headers and the two eventsById lookups now have one home. The bodies stay in the tests. A body is what its test is about, and the whole input should read where it is asserted on; only the constant wrapper moved. The settings header takes its level from LOG_LEVEL, the enum recordsVariables branches on, so the fixture and the code under test cannot drift apart. VariablesDetail held a third and fourth copy of the isParent lookup and a local indexOf taking a store where the sibling suite's took a log. One name, one signature now: call sites pass store.log.
Four test files each built their own ViewportState factory and two built
the same FlameChart internals stub. makeViewport and stubChartInternals
give both one home.
stubChartInternals starts every renderDirty flag false, unlike the real
init(), so a case proves the render it asked for. HoverRehit passes
{ culling: true } where it depends on one.
internalsOf names the cast past the public API once, in place of the
eight copies the two chart suites carried.
asContext replaces 67 hand-written casts of a MockContext to Context, and lastRegisteredCommand replaces three copies of the expression that digs the handler out of registerCommand's last call. Both were already written once, privately, in WhatsNewNotification's suite. Fifteen beforeEach blocks cleared the mocks again. setup.ts clears them in a global beforeEach, which runs first, so the second clear did nothing. Three of the fifteen had nothing else to do.
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
1 participant
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.
📝 PR Overview
Gives the test suites shared setup. Second of two reduction PRs, and stacked on
#1066 — its base is
refactor-drop-dead-code, so review that one first and this diffstays readable.
The suites are 43,933 lines against 77,031 of source. Most of the excess is the same
setup written again in each file: 28 local
mounthelpers, four near-identicalcreateEventbuilders, the sameEXECUTION_STARTEDfixture line pasted across 22 files.This PR gives each of those one home.
Nothing here changes shipped code. The one production-file edit in the first commit is a
comment.
Draft: in progress. More commits are coming on this branch; each lands with
pnpm lintandpnpm testgreen, and I check the passing-test count is unchanged so ashared helper cannot quietly stop a case running.
🛠️ Changes made
Landed so far:
#vscode-elementsthrough the jest config. 21 suites carried 35 copies ofjest.mock('#vscode-elements/…', () => ({})). jsdom'sElementInternalshas nosetFormValue, so a form-associated element fails on its first update, andvscode-iconwarns on every connect about the missing codicon stylesheet — alibrary-wide fact, not a per-suite decision.
TimelineKeyandDockLayouteachstubbed a whole component to dodge the same problem and now exercise the real
OverflowListandDetailDock.Still to come on this branch:
mount/settle/cleanupDom(28 local copies today).createEventclones, ~250 lines).__mocks__file.asContextcast (67 occurrences) andlastRegisteredCommand.it.eachcollapses, and one governor-tier policy currently asserted twice.🧩 Type of change (check all applicable)
📷 Screenshots / gifs / video [optional]
No UI change.
🔗 Related Issues
Stacked on #1066.
✅ Tests added?
This PR changes how tests are set up, not what they cover. The bar for every commit is
that the passing-test count does not move: 2,567 before and after.
Two behaviour widenings are worth a reviewer's attention rather than being buried:
#vscode-elementsrule went from opt-in to blanket. 38 suites reach such an importtransitively and 21 already mocked, so 17 now get the stub where they previously loaded
the real module. A control run with the mapper removed fails 18 suites and 260 tests,
which is what proves the stub is load-bearing and that no passing assertion was
weakened.
TimelineKeyandDockLayoutno longer stub outOverflowListandDetailDock, sothose two suites now cover more than they did.
vscode-single-selectis exempt from the mapper:VsSelectextendsVscodeSingleSelectand spreads its static styles, so a stub cannot be extended. That exemption currently
holds only because no suite mounts a
<vs-select>— the real element is form-associatedand would hit the missing
setFormValue.VsSelectcarries a comment saying so. Theclean fix is an
ElementInternalsno-op polyfill beside the existingResizeObserveronein
setup.ts; filed as a follow-up rather than smuggled in here.📚 Docs updated?
Test-only change, so no CHANGELOG entry.
Anything else we need to know? [optional]
Verified per commit with
pnpm lintandpnpm test, and by comparing the passing-testcount before and after.
Out of scope by agreement:
apex-log-parser, and the legacy timeline behindlana.timeline.legacy.