Silence the spurious `vi.mock` hoisting warnings by myabc · Pull Request #24621 · opf/openproject

myabc · 2026-08-05T18:49:39Z

Ticket

No work package — frontend test-output hygiene, noticed while working on #AGILE-361.

Stacked on #24525 (implementation/AGILE-361-batch-selection), with #24622 on top of this one.

What are you trying to accomplish?

A full npm run test -- --browsers chromium run passes, but buries that result in warning noise. This PR removes 15 of those warnings; the PR stacked on top removes ~43 more.

The warnings look like this, once per mock:

Warning: A vi.mock("@atlaskit/pragmatic-drag-and-drop/element/adapter") call in
"…/sortable-lists.controller.spec.ts" is not at the top level of the module.
…This will become an error in a future version.

They are false positives. Every one of those calls is already at the top level of its spec file.

The cause is the Angular unit-test builder. It esbuild-bundles each spec before Vitest sees it, wrapping the module body in a lazy CommonJS initialiser:

import { vi } from "vitest";
var require_sortable_lists_engine_spec = __commonJS({
  "src/common/drag-and-drop/sortable-lists-engine.spec.ts"(exports) {
    init_native_drag_simulation();
    init_sortable_lists_engine();
    var { autoScrollRegistrations } = vi.hoisted(() => ({ … }));
    vi.mock("@atlaskit/pragmatic-drag-and-drop-auto-scroll/element", () => ({ … }));

@vitest/mocker validates hoisting by walking ast.body and deleting any hoisted node it finds there. Inside that wrapper the node is not a direct child of Program, so it survives the sweep and warns. No source-level placement can satisfy the check — the calls are already where the warning asks them to be.

What approach did you choose and why?

vi.mock → vi.doMock. @vitest/mocker hoist-checks only mock, unmock and hoisted; doMock is excluded, so the wrapper stops mattering. And because doMock is not hoisted, vi.hoisted() is no longer needed either — a plain module-scope const initialises before the factory runs.

Four of the six specs needed nothing but the keyword: they already hold import type at the top and pull both the mocked packages and the subject under test via await import(...) in beforeAll, which is exactly the ordering doMock requires.

The two engine specs did need restructuring. Both statically imported the value createSortableRoot, which transitively imports the mocked @atlaskit module — under doMock that 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, which is why both engine specs were falsification-tested rather than merely re-run: with the vi.doMock block commented out, sortable-lists-engine.spec.ts produces 4 failures on its liveRegistrations() assertions and …preview.spec.ts produces 2 on its previewCalls assertions. A green run there would have meant the conversion was wrong.

Why `doMock` is safe again

vi.doMock was abandoned in these specs because spec files poisoned each other's module registry. That was a consequence of the builder's isolate: false default, which 797894c ("Isolate vitest spec files") fixed by setting isolate: true in vitest-base.config.ts. The reason it was unsafe no longer holds, and the commit body says so, so this doesn't get reverted blind later.

Three specs also carried comments still describing the pre-isolate: true world; those are corrected here rather than left to contradict the code.

Alternatives considered

Suppressing the warning was the obvious cheap option, and it does not work: the warning is emitted node-side during transform, before the RUN banner, so the existing onConsoleLog hook in vitest-base.config.ts never sees it. Silencing it would have meant a custom Vite plugin or patching console — and the warning is slated to become a hard error in a future Vitest, so suppression would only defer the work.

Worth reporting upstream separately: @angular/build's unit-test builder wrapping each spec in __commonJS is what makes Vitest's hoist validator misfire, and that is fixable at source.

Merge checklist

Added/updated tests — test-only change; all 167 files / 1548 tests still pass
Added/updated documentation in Lookbook (patterns, previews, etc) — n/a
Tested major browsers (Chrome, Firefox, Edge, ...) — verified on chromium only; see below

Verification

Full suite run three times on chromium, because doMock has a flake history here and one green run would not have shown an intermittent registry problem.

167 files / 1548 tests passing on every run
Warning: A vi. count 0 / 0 / 0
warning output byte-identical between runs

Remaining output is untouched and out of scope for this PR: the stimulus-use useDispatch deprecation (10), NG0912 (1), the Lit dev-mode banner (2), and Error updating activities list (2) — the last of which is a spec deliberately exercising an error path and should stay.

github-actions · 2026-08-05T18:50:03Z

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
title in square brackets, e.g. [SLUG-123] My title here.

Copilot

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.mock with vi.doMock across 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 createSortableRoot after 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 createSortableRootFn relies on a type-only import alias, which does not exist at runtime and typically fails TypeScript type-checking. Use the local CreateSortableRoot type (or a typeof 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 type alias, update the lazy binding’s type to use the CreateSortableRoot alias so this remains valid under TypeScript type-checking.

let createSortableRoot:typeof createSortableRootFn;

github-actions · 2026-08-06T09:15:50Z

Warning

Flaky specs

rspec ./modules/backlogs/spec/features/inbox_column_spec.rb[1:7:1]
rspec ./spec/features/activities/work_package/activities_spec.rb[1:5:2:1]

🤖 Ask Copilot to investigate

Copy 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.

@copilot The following spec(s) are flaky in CI (first seen on PR #24621, linked for reference only):

- `rspec ./modules/backlogs/spec/features/inbox_column_spec.rb[1:7:1]`
- `rspec ./spec/features/activities/work_package/activities_spec.rb[1:5:2:1]`

Treat this as a standalone task, unrelated to PR #24621. Create a new branch from origin/dev and open a new pull request targeting dev — do not stack it on PR #24621 or reuse that branch.

Follow the playbook in docs/development/testing/handling-flaky-tests/README.md to find the root cause and fix the underlying race — do not skip, delete, or weaken the spec to make it pass; disabling is a last resort per the playbook, and only with a bug ticket. Verify the fix by running the spec(s) repeatedly (e.g. `script/bulk_run_rspec --run-count 10`).

If you cannot reproduce the flake or are not confident in a fix after reasonable investigation, do not fabricate a change or skip the spec to force CI green. Instead, leave the pull request in draft and document what you tried, the suspected cause, and any leads in its description, then assign @myabc to take over.

Once the fix is verified, title the PR after the spec(s) it fixes, and use the PR description to explain the root cause, how the change resolves it, and the before/after results. Label the PR `flaky-spec`, assign @myabc, and request a review from @myabc.
On every commit, set @myabc as the sole co-author with a `Co-authored-by:` trailer (use their GitHub no-reply email so it links to their account), so it is traceable who dispatched the fix.

github-actions · 2026-08-06T09:46:25Z

Warning

Flaky specs

rspec ./spec/features/activities/work_package/activities_spec.rb[1:5:2:1]
rspec ./spec/features/projects/lists/filters_spec.rb[1:6:1]

🤖 Ask Copilot to investigate

Copy 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.

@copilot The following spec(s) are flaky in CI (first seen on PR #24621, linked for reference only):

- `rspec ./spec/features/activities/work_package/activities_spec.rb[1:5:2:1]`
- `rspec ./spec/features/projects/lists/filters_spec.rb[1:6:1]`

Treat this as a standalone task, unrelated to PR #24621. Create a new branch from origin/dev and open a new pull request targeting dev — do not stack it on PR #24621 or reuse that branch.

Follow the playbook in docs/development/testing/handling-flaky-tests/README.md to find the root cause and fix the underlying race — do not skip, delete, or weaken the spec to make it pass; disabling is a last resort per the playbook, and only with a bug ticket. Verify the fix by running the spec(s) repeatedly (e.g. `script/bulk_run_rspec --run-count 10`).

If you cannot reproduce the flake or are not confident in a fix after reasonable investigation, do not fabricate a change or skip the spec to force CI green. Instead, leave the pull request in draft and document what you tried, the suspected cause, and any leads in its description, then assign @myabc to take over.

Once the fix is verified, title the PR after the spec(s) it fixes, and use the PR description to explain the root cause, how the change resolves it, and the before/after results. Label the PR `flaky-spec`, assign @myabc, and request a review from @myabc.
On every commit, set @myabc as the sole co-author with a `Co-authored-by:` trailer (use their GitHub no-reply email so it links to their account), so it is traceable who dispatched the fix.

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.

github-actions · 2026-08-06T12:21:21Z

Warning

Flaky specs

rspec ./modules/team_planner/spec/features/team_planner_overview_spec.rb[1:4:4:1]

🤖 Ask Copilot to investigate

Copy 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.

@copilot The following spec(s) are flaky in CI (first seen on PR #24621, linked for reference only):

- `rspec ./modules/team_planner/spec/features/team_planner_overview_spec.rb[1:4:4:1]`

Treat this as a standalone task, unrelated to PR #24621. Create a new branch from origin/dev and open a new pull request targeting dev — do not stack it on PR #24621 or reuse that branch.

Follow the playbook in docs/development/testing/handling-flaky-tests/README.md to find the root cause and fix the underlying race — do not skip, delete, or weaken the spec to make it pass; disabling is a last resort per the playbook, and only with a bug ticket. Verify the fix by running the spec(s) repeatedly (e.g. `script/bulk_run_rspec --run-count 10`).

If you cannot reproduce the flake or are not confident in a fix after reasonable investigation, do not fabricate a change or skip the spec to force CI green. Instead, leave the pull request in draft and document what you tried, the suspected cause, and any leads in its description, then assign @myabc to take over.

Once the fix is verified, title the PR after the spec(s) it fixes, and use the PR description to explain the root cause, how the change resolves it, and the before/after results. Label the PR `flaky-spec`, assign @myabc, and request a review from @myabc.
On every commit, set @myabc as the sole co-author with a `Co-authored-by:` trailer (use their GitHub no-reply email so it links to their account), so it is traceable who dispatched the fix.

Copilot AI lite review requested due to automatic review settings August 5, 2026 18:49

myabc mentioned this pull request Aug 5, 2026

Model a scroll container in the drag-and-drop spec fixtures #24622

Open

3 tasks

Copilot started reviewing on behalf of myabc August 5, 2026 18:50 View session

Copilot AI reviewed Aug 5, 2026

View reviewed changes

Comment thread frontend/src/common/drag-and-drop/sortable-lists-engine.spec.ts

Comment thread frontend/src/common/drag-and-drop/sortable-lists-engine.preview.spec.ts

myabc added needs review javascript Pull requests that update Javascript code labels Aug 5, 2026

myabc changed the title ~~Silence the spurious vi.mock hoisting warnings~~ Silence the spurious vi.mock hoisting warnings Aug 5, 2026

myabc force-pushed the fix/vitest-mock-hoisting-warnings branch from c24f965 to 76bb224 Compare August 5, 2026 23:26

myabc force-pushed the fix/vitest-mock-hoisting-warnings branch 2 times, most recently from e7a6126 to f22e7c6 Compare August 6, 2026 08:56

myabc force-pushed the fix/vitest-mock-hoisting-warnings branch from f22e7c6 to 5a2889c Compare August 6, 2026 09:27

myabc force-pushed the fix/vitest-mock-hoisting-warnings branch from 5a2889c to f1f3f93 Compare August 6, 2026 10:20

myabc added this to the 17.8.x milestone Aug 6, 2026

myabc added 2 commits August 6, 2026 13:58

myabc force-pushed the fix/vitest-mock-hoisting-warnings branch from f1f3f93 to a3f233c Compare August 6, 2026 12:00

Navigation Menu

Uh oh!

Silence the spurious `vi.mock` hoisting warnings - #24621

Silence the spurious `vi.mock` hoisting warnings#24621
myabc wants to merge 2 commits into
implementation/AGILE-361-batch-selectionfrom
fix/vitest-mock-hoisting-warnings

myabc commented Aug 5, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Aug 5, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Aug 6, 2026

Uh oh!

github-actions Bot commented Aug 6, 2026

Uh oh!

github-actions Bot commented Aug 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

myabc commented Aug 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Ticket

What are you trying to accomplish?

What approach did you choose and why?

Why doMock is safe again

Alternatives considered

Merge checklist

Verification

Uh oh!

github-actions Bot commented Aug 5, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Aug 6, 2026

Uh oh!

github-actions Bot commented Aug 6, 2026

Uh oh!

github-actions Bot commented Aug 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

myabc commented Aug 5, 2026 •

edited

Loading

Why `doMock` is safe again