Skip to content

feat(FR-2746): re-enable 5 tests skipped during Vitest migration#7071

Merged
graphite-app[bot] merged 1 commit intomainfrom
04-27-feat_fr-2746_re-enable_5_tests_skipped_during_vitest_migration
Apr 30, 2026
Merged

feat(FR-2746): re-enable 5 tests skipped during Vitest migration#7071
graphite-app[bot] merged 1 commit intomainfrom
04-27-feat_fr-2746_re-enable_5_tests_skipped_during_vitest_migration

Conversation

@nowgnuesLee
Copy link
Copy Markdown
Contributor

@nowgnuesLee nowgnuesLee commented Apr 27, 2026

Resolves #7070(FR-2746)

Summary

During FR-2609 + the BUI/root follow-up, 5 tests were skipped with TODO(FR-2609) markers because of two distinct compatibility issues. Both are now resolved.

Group A — @rc-component/motion + jsdom 29 (4 tests)

  • packages/backend.ai-ui/src/components/BAIButton.test.tsx (2 tests asserting the loading icon clears after async action completion)
  • packages/backend.ai-ui/src/components/BAIUnmountAfterClose.test.tsx (2 tests asserting the Modal unmounts after the close animation finishes)

Root cause: antd v6 uses @rc-component/motion (CSSMotion) for Button loading-icon and Modal close transitions. The package probes vendor-prefixed style props on a div at module init to compute a supportTransition flag. Jest's older jsdom did not expose those props → flag resolved to false → motion completed synchronously. jsdom 29 (Vitest) exposes them → flag is true → motion waits for a transitionend event that jsdom never fires.

Fix: a MutationObserver in setupTests.ts watches the document for class additions matching *-(leave|enter|appear)-active, then queues a microtask to dispatch a synthetic transitionend on the element. This emulates what a real browser does when the CSS transition completes — rc-motion's listener fires, the active class is removed, and the assertion sees the expected post-transition DOM.

This was chosen after vi.mock('@rc-component/motion/es/util/motion', ...) failed to intercept the package-internal relative import (./util/motion). The MutationObserver works regardless of how supportTransition resolves and is the most robust fix.

Group B — CJS require('fs') not interceptable by vi.mock (1 test)

  • scripts/i18n-merge-driver.test.ts (should parse JSON from file correctly)

Root cause: the driver is a CommonJS .js file invoked by git via node scripts/i18n-merge-driver.js %O %A %B (per .git/config). It must remain CJS. Its inline require('fs') slips past Vitest's module-mock transform, so vi.mock('fs', ...) did not propagate.

Fix: rewrite the test to use a real temp file via fs.mkdtempSync, torn down in afterEach. This bypasses the CJS-mock incompat by not mocking, while still validating the observable behavior — readJSON parses JSON correctly and throws on invalid input.

Bonus catch: the existing should throw error for invalid JSON test was actually testing missing-file ENOENT (the mock had been silently failing in that test too). It now correctly tests invalid JSON parsing as labelled.

CI tweak — root testTimeout + cjs coverage exclude

The first CI run on this PR exposed two coverage-related issues that didn't surface locally:

  • scripts/gen-theme-schema.test.ts > generates a valid JSON schema file — exercises antd's type tree several times. Locally completes in ~2s; under V8 coverage instrumentation on CI's smaller runners it routinely exceeds the default 5 s per-test timeout.
  • After that test failed, the davelosert action couldn't post a coverage comment because the coverage-summary.json was generated late / partially.

Two small adjustments to vitest.config.ts (root) make CI match local behaviour:

  • testTimeout: 30000 — generous per-test budget that matches what coverage adds in CI.
  • coverage.exclude: 'scripts/**/*.cjs' — build tooling (the gen-theme-schema generator, the i18n merge driver) is exercised by the tests but its coverage % is not actionable; excluding it removes the heaviest source of v8 instrumentation overhead in this suite.

These are bundled here because the failing CI run on this PR was the first surfaceing of the issue; both adjustments are tightly scoped to root coverage runs.

Verification

before:
  react/   856 pass | 0  skip
  BUI      315 pass | 5  skip
  root      90 pass | 1  skip
  total   1261 pass | 6  skip

after:
  react/   856 pass | 0  skip
  BUI      319 pass | 1  skip   ← 4 newly passing (Group A)
  root      91 pass | 0  skip   ← 1 newly passing (Group B)
  total   1266 pass | 1  skip

The remaining 1 skip in BUI is BAIDomainSelect.test.tsx describe.skip from FR-1731 (pre-existing, unrelated to this migration).

Local pnpm exec vitest run --coverage now produces coverage/coverage-summary.json for the root suite without timing out.

Out of scope

  • Fixing the FR-1731 BAIDomainSelect skip (separate concern).
  • A global motion-disable opt-out beyond the helper — the MutationObserver is sufficient and minimally invasive.

Stack

Top of the Vite migration stack (now 13 PRs). Builds on PR #7065 (feat(FR-2744) Vitest coverage reporting).

Copy link
Copy Markdown
Contributor Author

nowgnuesLee commented Apr 27, 2026


How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • flow:merge-queue - adds this PR to the back of the merge queue
  • flow:hotfix - for urgent changes, fast-track this PR to the front of the merge queue

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has required the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 27, 2026

Coverage Report for backend-ai-ui-coverage (./packages/backend.ai-ui)

Status Category Percentage Covered / Total
🔵 Lines 7.91% 322 / 4066
🔵 Statements 7.2% 368 / 5106
🔵 Functions 8.72% 88 / 1009
🔵 Branches 6.39% 321 / 5020
File CoverageNo changed files found.
Generated in workflow #111 for commit cc748d3 by the Vitest Coverage Report Action

@nowgnuesLee nowgnuesLee force-pushed the 04-27-feat_fr-2746_re-enable_5_tests_skipped_during_vitest_migration branch from 18b4caa to 388ded7 Compare April 27, 2026 08:23
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 27, 2026

Coverage Report for root-coverage

Status Category Percentage Covered / Total
🔵 Lines 3.76% 85 / 2258
🔵 Statements 3.97% 92 / 2312
🔵 Functions 3.85% 14 / 363
🔵 Branches 5.11% 68 / 1329
File CoverageNo changed files found.
Generated in workflow #111 for commit cc748d3 by the Vitest Coverage Report Action

@nowgnuesLee nowgnuesLee force-pushed the 04-27-feat_fr-2744_restore_vitest_coverage_reporting_in_ci branch from 4ee4407 to ce03855 Compare April 27, 2026 13:57
@nowgnuesLee nowgnuesLee force-pushed the 04-27-feat_fr-2746_re-enable_5_tests_skipped_during_vitest_migration branch from 388ded7 to 06b0e97 Compare April 27, 2026 13:57
@nowgnuesLee nowgnuesLee marked this pull request as ready for review April 28, 2026 04:25
Copilot AI review requested due to automatic review settings April 28, 2026 04:25
@nowgnuesLee nowgnuesLee force-pushed the 04-27-feat_fr-2744_restore_vitest_coverage_reporting_in_ci branch from ce03855 to bbf0e01 Compare April 28, 2026 05:02
@nowgnuesLee nowgnuesLee force-pushed the 04-27-feat_fr-2746_re-enable_5_tests_skipped_during_vitest_migration branch from 06b0e97 to a00cbcb Compare April 28, 2026 05:02
@nowgnuesLee nowgnuesLee force-pushed the 04-27-feat_fr-2744_restore_vitest_coverage_reporting_in_ci branch from bbf0e01 to 7892855 Compare April 28, 2026 05:15
@nowgnuesLee nowgnuesLee force-pushed the 04-27-feat_fr-2746_re-enable_5_tests_skipped_during_vitest_migration branch from a00cbcb to 4c7f5bb Compare April 28, 2026 05:15
@nowgnuesLee nowgnuesLee force-pushed the 04-27-feat_fr-2744_restore_vitest_coverage_reporting_in_ci branch from 7892855 to be9870b Compare April 28, 2026 05:19
@nowgnuesLee nowgnuesLee force-pushed the 04-27-feat_fr-2746_re-enable_5_tests_skipped_during_vitest_migration branch 2 times, most recently from a579c9b to 866800d Compare April 28, 2026 05:25
@nowgnuesLee nowgnuesLee force-pushed the 04-27-feat_fr-2744_restore_vitest_coverage_reporting_in_ci branch from be9870b to 8979590 Compare April 28, 2026 05:25
@nowgnuesLee nowgnuesLee force-pushed the 04-27-feat_fr-2746_re-enable_5_tests_skipped_during_vitest_migration branch from 866800d to cfea1f5 Compare April 28, 2026 05:28
@yomybaby yomybaby force-pushed the 04-27-feat_fr-2746_re-enable_5_tests_skipped_during_vitest_migration branch from cfea1f5 to 1034319 Compare April 30, 2026 12:08
@yomybaby yomybaby force-pushed the 04-27-feat_fr-2744_restore_vitest_coverage_reporting_in_ci branch from 1719229 to 27c1b3a Compare April 30, 2026 12:08
@graphite-app
Copy link
Copy Markdown

graphite-app Bot commented Apr 30, 2026

Merge activity

Resolves #7070(FR-2746)

## Summary

During FR-2609 + the BUI/root follow-up, 5 tests were skipped with `TODO(FR-2609)` markers because of two distinct compatibility issues. Both are now resolved.

### Group A — `@rc-component/motion` + jsdom 29 (4 tests)

- `packages/backend.ai-ui/src/components/BAIButton.test.tsx` (2 tests asserting the loading icon clears after async action completion)
- `packages/backend.ai-ui/src/components/BAIUnmountAfterClose.test.tsx` (2 tests asserting the Modal unmounts after the close animation finishes)

**Root cause**: antd v6 uses `@rc-component/motion` (CSSMotion) for Button loading-icon and Modal close transitions. The package probes vendor-prefixed style props on a `div` at module init to compute a `supportTransition` flag. Jest's older jsdom did not expose those props → flag resolved to `false` → motion completed synchronously. jsdom 29 (Vitest) exposes them → flag is `true` → motion waits for a `transitionend` event that jsdom never fires.

**Fix**: a MutationObserver in `setupTests.ts` watches the document for class additions matching `*-(leave|enter|appear)-active`, then queues a microtask to dispatch a synthetic `transitionend` on the element. This emulates what a real browser does when the CSS transition completes — rc-motion's listener fires, the active class is removed, and the assertion sees the expected post-transition DOM.

This was chosen after `vi.mock('@rc-component/motion/es/util/motion', ...)` failed to intercept the package-internal relative import (`./util/motion`). The MutationObserver works regardless of how `supportTransition` resolves and is the most robust fix.

### Group B — CJS `require('fs')` not interceptable by `vi.mock` (1 test)

- `scripts/i18n-merge-driver.test.ts` (`should parse JSON from file correctly`)

**Root cause**: the driver is a CommonJS `.js` file invoked by git via `node scripts/i18n-merge-driver.js %O %A %B` (per `.git/config`). It must remain CJS. Its inline `require('fs')` slips past Vitest's module-mock transform, so `vi.mock('fs', ...)` did not propagate.

**Fix**: rewrite the test to use a real temp file via `fs.mkdtempSync`, torn down in `afterEach`. This bypasses the CJS-mock incompat by not mocking, while still validating the observable behavior — `readJSON` parses JSON correctly and throws on invalid input.

**Bonus catch**: the existing `should throw error for invalid JSON` test was actually testing missing-file `ENOENT` (the mock had been silently failing in that test too). It now correctly tests invalid JSON parsing as labelled.

### CI tweak — root testTimeout + cjs coverage exclude

The first CI run on this PR exposed two coverage-related issues that didn't surface locally:

- `scripts/gen-theme-schema.test.ts > generates a valid JSON schema file` — exercises antd's type tree several times. Locally completes in ~2s; under V8 coverage instrumentation on CI's smaller runners it routinely exceeds the default 5 s per-test timeout.
- After that test failed, the davelosert action couldn't post a coverage comment because the `coverage-summary.json` was generated late / partially.

Two small adjustments to `vitest.config.ts` (root) make CI match local behaviour:
- `testTimeout: 30000` — generous per-test budget that matches what coverage adds in CI.
- `coverage.exclude: 'scripts/**/*.cjs'` — build tooling (the gen-theme-schema generator, the i18n merge driver) is exercised by the tests but its coverage % is not actionable; excluding it removes the heaviest source of v8 instrumentation overhead in this suite.

These are bundled here because the failing CI run on this PR was the first surfaceing of the issue; both adjustments are tightly scoped to root coverage runs.

## Verification

```
before:
  react/   856 pass | 0  skip
  BUI      315 pass | 5  skip
  root      90 pass | 1  skip
  total   1261 pass | 6  skip

after:
  react/   856 pass | 0  skip
  BUI      319 pass | 1  skip   ← 4 newly passing (Group A)
  root      91 pass | 0  skip   ← 1 newly passing (Group B)
  total   1266 pass | 1  skip
```

The remaining 1 skip in BUI is `BAIDomainSelect.test.tsx` `describe.skip` from FR-1731 (pre-existing, unrelated to this migration).

Local `pnpm exec vitest run --coverage` now produces `coverage/coverage-summary.json` for the root suite without timing out.

## Out of scope

- Fixing the FR-1731 `BAIDomainSelect` skip (separate concern).
- A global motion-disable opt-out beyond the helper — the MutationObserver is sufficient and minimally invasive.

## Stack

Top of the Vite migration stack (now 13 PRs). Builds on PR #7065 (`feat(FR-2744)` Vitest coverage reporting).
@graphite-app graphite-app Bot force-pushed the 04-27-feat_fr-2744_restore_vitest_coverage_reporting_in_ci branch from 27c1b3a to dd140f8 Compare April 30, 2026 12:19
@graphite-app graphite-app Bot force-pushed the 04-27-feat_fr-2746_re-enable_5_tests_skipped_during_vitest_migration branch from 1034319 to cc748d3 Compare April 30, 2026 12:19
graphite-app Bot pushed a commit that referenced this pull request Apr 30, 2026
… TS globals (#7083)

Resolves #7081(FR-2748)

## Summary

Finishes the Jest → Vitest migration started in FR-2609. The earlier PRs left two compat shims as migration aids (`vitest.jest-compat.ts` setting `globalThis.jest = vi`, and `jest-globals-shim.ts` aliasing `@jest/globals`). Useful at the time but they obscured what was actually a vitest test, and left a TypeScript gap exposed by the audit (`vi` not typed; project globals from the deleted `react/src/react-app-env.d.ts` lost). This PR completes the rename so the shims become unnecessary.

### Mechanical jest.* → vi.* migration (15 test files)

- Single-line: `jest.fn`, `jest.spyOn`, `jest.mocked`, `jest.clearAllMocks`, `jest.resetAllMocks`, `jest.restoreAllMocks`, `jest.useFakeTimers`, `jest.useRealTimers`, `jest.runAllTimers`, `jest.advanceTimersByTime`, `jest.runOnlyPendingTimers`.
- Multi-line patterns (`const x = jest\n.fn()`) handled via `perl -0777`.
- Type positions: `jest.MockedFunction<T>` / `jest.MockedClass<T>` / `jest.SpyInstance` / `jest.SpiedFunction` rewritten to use `vi.mocked(...)` (value) or `MockInstance` / `ReturnType<typeof vi.spyOn>` (type) imported from `'vitest'`.
- `matchMedia.mock.js` (used by both react/ and BUI) also moved off `jest.fn()`.

### Drop the runtime compat shims

- Removed `__test__/vitest.jest-compat.ts` from root, react/, and BUI.
- Removed `packages/backend.ai-ui/__test__/jest-globals-shim.ts`.
- Dropped the `@jest/globals` alias from BUI's `vitest.config.ts`.
- Dropped the second `setupFiles` entry in all three vitest configs.

### Restore the project's ambient TypeScript types

FR-2611 deleted `react/src/react-app-env.d.ts` (CRA-shaped name) along with CRA itself, but the file held real custom types: `DeepPartial`, `SelectivePartial`, `OptionalFieldsOnly`, `NonNullableItem`, `NonNullableNodeOnEdges`, `BackendAIOptions`, `BackendAIClient`, `globalThis.{isElectron, packageVersion, electronInitialHref, …}`, `Window.switchLanguage`, and the `backend.ai-client-esm` module shim. Those all return, now living in `react/src/ambient.d.ts` with the CRA reference removed.

### Wire vitest globals into TypeScript

Used `/// <reference types="vitest/globals" />` in ambient `.d.ts` files instead of touching the `tsconfig.json` `"types"` arrays:
- `react/src/ambient.d.ts`
- `packages/backend.ai-ui/svg.d.ts`

**Why not `"types": ["vitest/globals"]` in tsconfig**: that would exclude `@types/jest` from auto-loading, which then breaks `@testing-library/jest-dom`'s default matcher augmentation (`expect(el).toBeInTheDocument()` etc). The reference directive pulls in vitest globals additively without changing the tsconfig `"types"` contract.

### Keep one `globalThis.jest = vi` line in setupTests

`@testing-library/dom`'s `waitFor` checks `globalThis.jest` to detect Jest-style fake timers; if present, it switches to its timer-aware polling loop. Without that detection, tests that combine `vi.useFakeTimers()` with `await waitFor(…)` hang — the polling `setTimeout` never fires under faked timers.

The line is **no longer a compat shim for project test code** (all test code uses `vi.*` now); it's purely a `@testing-library/dom` integration hook. Comment in both `react/src/setupTests.ts` and `packages/backend.ai-ui/setupTests.ts` explains the new purpose.

## Verification

- `bash scripts/verify.sh` → ALL PASS (lint + format + TypeScript).
- `pnpm --prefix ./react run vitest` → **856 / 856** pass.
- `pnpm --prefix ./packages/backend.ai-ui run vitest` → **319 / 320** (1 pre-existing FR-1731 skip).
- `pnpm exec vitest run` (root) → **91 / 91**.
- `pnpm run build` (root) and `make dep` (Electron) succeed; built `index.html` references only `manifest.json` (no `manifest.webmanifest`); Electron build emits `es6://assets/index-*` URLs.

## Out of scope

- Removing the `globalThis.jest = vi` line entirely (would require reconfiguring `@testing-library/dom` to use vitest's fake-timer integration manually — larger change).
- Switching to `@testing-library/jest-dom/vitest` entry (kept the default entry to avoid the BAIFlex snapshot client conflict observed during exploration).
- The pre-existing `BAIDomainSelect.test.tsx` `describe.skip` (FR-1731 issue, unrelated to this migration).

## Stack

Top of the Vite migration stack (now 14 PRs). Builds on PR #7071 (`feat(FR-2746)` re-enable skipped tests).
Base automatically changed from 04-27-feat_fr-2744_restore_vitest_coverage_reporting_in_ci to main April 30, 2026 12:35
@graphite-app graphite-app Bot merged commit cc748d3 into main Apr 30, 2026
8 checks passed
@graphite-app graphite-app Bot deleted the 04-27-feat_fr-2746_re-enable_5_tests_skipped_during_vitest_migration branch April 30, 2026 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L 100~500 LoC

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Re-enable 5 tests skipped during Vitest migration

1 participant