Silence the spurious vi.mock hoisting warnings - #24621
Conversation
|
Warning This pull request does not link an OpenProject work package. Please add a link to the work package in the description, or reference it in the |
There was a problem hiding this comment.
Pull request overview
This PR reduces noisy Vitest warning output caused by Angular’s unit-test bundling (esbuild wrapping specs in __commonJS), by converting top-level vi.mock usage to vi.doMock in affected frontend specs and adjusting imports/initialization order where needed so mocks still apply.
Changes:
- Replace
vi.mockwithvi.doMockacross several sortable-lists-related specs to avoid spurious hoisting warnings. - Remove reliance on
vi.hoisted()in the engine specs by using plain module-scope constants. - Restructure the two engine specs to lazily import
createSortableRootafter mocks are registered.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/stimulus/controllers/dynamic/sortable-lists/scrollable.controller.spec.ts | Switch auto-scroll mock to vi.doMock to avoid hoisting warnings. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists/list.controller.spec.ts | Switch Pragmatic adapter mock to vi.doMock and document dynamic-import ordering. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists/item.controller.spec.ts | Switch multiple Pragmatic mocks to vi.doMock to avoid hoisting warnings. |
| frontend/src/stimulus/controllers/dynamic/sortable-lists.controller.spec.ts | Switch Pragmatic mocks to vi.doMock and update rationale comment. |
| frontend/src/common/drag-and-drop/sortable-lists-engine.spec.ts | Replace vi.hoisted() with const state, move createSortableRoot to lazy import after doMock. |
| frontend/src/common/drag-and-drop/sortable-lists-engine.preview.spec.ts | Same as above for preview-offset assertions: const state + lazy import after doMock. |
Suppressed comments (2)
frontend/src/common/drag-and-drop/sortable-lists-engine.spec.ts:65
typeof createSortableRootFnrelies on a type-only import alias, which does not exist at runtime and typically fails TypeScript type-checking. Use the localCreateSortableRoottype (or atypeof import()query) for the lazy binding’s type instead.
let createSortableRoot:typeof createSortableRootFn;
frontend/src/common/drag-and-drop/sortable-lists-engine.preview.spec.ts:54
- After switching away from the
import typealias, update the lazy binding’s type to use theCreateSortableRootalias so this remains valid under TypeScript type-checking.
let createSortableRoot:typeof createSortableRootFn;
vi.mock hoisting warnings
f3da126 to
63d626a
Compare
c24f965 to
76bb224
Compare
63d626a to
6e3c26e
Compare
e7a6126 to
f22e7c6
Compare
|
Warning Flaky specs
🤖 Ask Copilot to investigateCopy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer. |
f22e7c6 to
5a2889c
Compare
|
Warning Flaky specs
🤖 Ask Copilot to investigateCopy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer. |
5a2889c to
f1f3f93
Compare
f1f3f93 to
a3f233c
Compare
cf99663 to
5cbd3ae
Compare
|
Warning Flaky specs
🤖 Ask Copilot to investigateCopy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer. |
5cbd3ae to
9065e03
Compare
a3f233c to
9577b7a
Compare
9065e03 to
3e98867
Compare
9577b7a to
d5bd278
Compare
|
Warning Flaky specs
🤖 Ask Copilot to investigateCopy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer. |
3e98867 to
dbcbe0d
Compare
d5bd278 to
9f2a00b
Compare
|
Warning Flaky specs
🤖 Ask Copilot to investigateCopy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer. |
dbcbe0d to
41a3461
Compare
c3c7239 to
c694439
Compare
41a3461 to
9fcbd5f
Compare
|
Warning Flaky specs
🤖 Ask Copilot to investigateCopy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer. |
9fcbd5f to
7089eaa
Compare
c694439 to
d2402c9
Compare
|
Warning Flaky specs
🤖 Ask Copilot to investigateCopy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer. |
b6f2e48 to
02f5560
Compare
d2402c9 to
2eba20d
Compare
|
Warning Flaky specs
🤖 Ask Copilot to investigateCopy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer. |
294f54a to
2eba20d
Compare
2eba20d to
294f54a
Compare
|
Warning Flaky specs
🤖 Ask Copilot to investigateCopy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer. |
The Angular unit-test builder bundles each spec into a __commonJS wrapper, so Vitest's hoist validator walks that wrapper instead of our source and reports top-level vi.mock calls as nested. Only mock, unmock and hoisted are hoist-checked, so vi.doMock sidesteps it. These specs already import the mocked packages and the controller under test lazily in beforeAll, which is the ordering doMock needs. A note above each call records that, since doMock's position above the imports no longer carries the guarantee vi.mock's hoisting gave it. The root spec also claimed it read back the combine, prevent-unhandled and drag-preview options. It never did; they keep the mounted item controller's side effects inert.
vi.doMock only affects later imports, so the statically imported createSortableRoot would have pulled in the real auto-scroll module first and the mock would have stopped applying silently. A type import plus a lazy binding restores the ordering in both engine specs, and drops vi.hoisted with it. The preview spec also loses the file-split rationale that cited a shared module registry, which isolate:true removed. The split itself stands: the sibling renders previews for real.
294f54a to
90c3b50
Compare
|
Warning Flaky specs
🤖 Ask Copilot to investigateCopy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer. |
Ticket
No work package — frontend test-output hygiene, noticed while working on AGILE-361.
What are you trying to accomplish?
A full
npm run test -- --browsers chromiumrun passes, but buries that result in warning noise. This PR removes 15 of those warnings; the PR stacked on top removes ~43 more.They are false positives — every one of those calls is already at the top level of its spec file. The Angular unit-test builder esbuild-bundles each spec before Vitest sees it, wrapping the module body in a lazy
__commonJSinitialiser.@vitest/mockervalidates hoisting by walkingast.body; inside that wrapper the node is not a direct child ofProgram, so it survives the sweep and warns. No source-level placement can satisfy the check.What approach did you choose and why?
vi.mock→vi.doMock.@vitest/mockerhoist-checks onlymock,unmockandhoisted;doMockis excluded, so the wrapper stops mattering. And becausedoMockis not hoisted,vi.hoisted()is no longer needed — a plain module-scopeconstinitialises before the factory runs.Four of the six specs needed nothing but the keyword: they already hold
import typeat the top and pull both the mocked packages and the subject under test viaawait import(...)inbeforeAll, which is exactly the orderingdoMockrequires.The two engine specs did need restructuring. Both statically imported the value
createSortableRoot, which transitively imports the mocked@atlaskitmodule — underdoMockthat resolves before the mock registers, so the mock would silently stop applying. Both now use a type-only import plus a lazy binding. That failure mode is quiet, so both were falsification-tested: with thevi.doMockblock commented out,sortable-lists-engine.spec.tsfails 4liveRegistrations()assertions and…preview.spec.tsfails 2previewCallsassertions.Why
doMockis safe againvi.doMockwas abandoned in these specs because spec files poisoned each other's module registry — a consequence of the builder'sisolate: falsedefault, which 797894c ("Isolate vitest spec files") fixed by settingisolate: trueinvitest-base.config.ts. The reason it was unsafe no longer holds. Three specs also carried comments still describing the pre-isolate: trueworld; those are corrected here.Alternatives considered
Suppressing the warning does not work: it is emitted node-side during transform, before the
RUNbanner, so the existingonConsoleLoghook invitest-base.config.tsnever sees it. Silencing it would need a custom Vite plugin or a patchedconsole— and the warning is slated to become a hard error in a future Vitest, so suppression only defers the work.Worth reporting upstream separately:
@angular/build's unit-test builder wrapping each spec in__commonJSis what makes Vitest's hoist validator misfire, and that is fixable at source.Merge checklist