Skip to content

Commit cc748d3

Browse files
committed
feat(FR-2746): re-enable 5 tests skipped during Vitest migration (#7071)
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).
1 parent dd140f8 commit cc748d3

5 files changed

Lines changed: 99 additions & 49 deletions

File tree

packages/backend.ai-ui/setupTests.ts

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,58 @@
1+
// Auto-complete `@rc-component/motion` (used by antd v6 Modal, Drawer,
2+
// Button-loading-icon) animations under jsdom. rc-motion attaches a
3+
// `transitionend` listener on each element that has a `-leave-active`,
4+
// `-enter-active`, or `-appear-active` class, and waits for the browser to
5+
// dispatch that event. jsdom does not run real CSS animations, so the
6+
// event never fires; antd components then stay stuck in the active phase
7+
// past `waitFor` timeouts.
8+
//
9+
// Jest's older jsdom didn't expose the vendor-prefixed style props that
10+
// rc-motion probes for support detection, so its `supportTransition` flag
11+
// resolved to false and the active-phase classes were applied
12+
// synchronously — masking this issue. jsdom 29 (used by Vitest) exposes
13+
// those props, so `supportTransition` is true and we have to manually fire
14+
// the end event.
15+
//
16+
// Approach: a MutationObserver watches the whole document for class
17+
// additions matching `*-(leave|enter|appear)-active`, and queues a
18+
// microtask to dispatch a synthetic `transitionend` on the element. This
19+
// is exactly what a real browser would do once the CSS transition
20+
// completes — rc-motion's listener fires its `onInternalMotionEnd`
21+
// callback, the active class gets removed, and the test sees the expected
22+
// post-transition DOM.
23+
if (typeof MutationObserver !== 'undefined' && typeof document !== 'undefined') {
24+
const ACTIVE_CLASS_RE = /(?:-(?:leave|enter|appear))-active(?:\s|$)/;
25+
const fireTransitionEnd = (el: Element) => {
26+
if (!(el instanceof HTMLElement)) return;
27+
const evt = new Event('transitionend', { bubbles: true });
28+
// rc-motion checks `event.target !== element` and `!event.deadline`
29+
// so the dispatch must come FROM the element itself.
30+
el.dispatchEvent(evt);
31+
};
32+
const checkAndFire = (el: Element) => {
33+
if (ACTIVE_CLASS_RE.test(el.className ?? '')) {
34+
queueMicrotask(() => fireTransitionEnd(el));
35+
}
36+
};
37+
const observer = new MutationObserver((records) => {
38+
for (const r of records) {
39+
if (r.type === 'attributes' && r.attributeName === 'class') {
40+
checkAndFire(r.target as Element);
41+
} else if (r.type === 'childList') {
42+
r.addedNodes.forEach((n) => {
43+
if (n instanceof Element) checkAndFire(n);
44+
});
45+
}
46+
}
47+
});
48+
observer.observe(document.documentElement, {
49+
attributes: true,
50+
attributeFilter: ['class'],
51+
subtree: true,
52+
childList: true,
53+
});
54+
}
55+
156
// jest-dom adds custom jest matchers for asserting on DOM nodes.
257
// allows you to do things like:
358
// expect(element).toHaveTextContent(/react/i)

packages/backend.ai-ui/src/components/BAIButton.test.tsx

Lines changed: 2 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -86,12 +86,7 @@ describe('BAIButton', () => {
8686
});
8787
});
8888

89-
// TODO(FR-2609): re-enable when rc-motion works under Vitest's jsdom 29.
90-
// rc-motion's `supportTransition` probes vendor-prefixed style props on a
91-
// div; jsdom 29 exposes them, so it waits for a `transitionend` that never
92-
// fires, leaving `.ant-btn-loading-icon` in `-leave-active` class forever.
93-
// Jest's older jsdom did not expose them, so motion completed synchronously.
94-
it.skip('should clear loading state after action completes', async () => {
89+
it('should clear loading state after action completes', async () => {
9590
const action = jest.fn().mockResolvedValue(undefined);
9691
const user = userEvent.setup();
9792
render(<BAIButton action={action}>Complete Action</BAIButton>);
@@ -132,9 +127,7 @@ describe('BAIButton', () => {
132127
});
133128
});
134129

135-
// TODO(FR-2609): same rc-motion / jsdom 29 incompat as above. Re-enable
136-
// once rc-motion clears `-leave-active` class without a real `transitionend`.
137-
it.skip('should handle async action with successful resolution', async () => {
130+
it('should handle async action with successful resolution', async () => {
138131
const action = jest.fn().mockResolvedValue('success');
139132
const user = userEvent.setup();
140133
render(<BAIButton action={action}>Async Success</BAIButton>);

packages/backend.ai-ui/src/components/BAIUnmountAfterClose.test.tsx

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -120,12 +120,7 @@ describe('BAIUnmountAfterClose', () => {
120120
expect(originalAfterClose).not.toHaveBeenCalled();
121121
});
122122

123-
// TODO(FR-2609): re-enable once rc-motion + jsdom 29 play nicely.
124-
// jsdom 29 exposes vendor-prefixed transition props, so rc-motion waits
125-
// for a `transitionend` event that never fires, leaving the Modal
126-
// mounted past the waitFor timeout. Jest's older jsdom completed the
127-
// motion synchronously.
128-
it.skip('should keep modal mounted when initially open and then closed, and unmount after animation', async () => {
123+
it('should keep modal mounted when initially open and then closed, and unmount after animation', async () => {
129124
const { rerender: _rerender } = render(
130125
<BAIUnmountAfterClose>
131126
<Modal open={true} title="Test Modal">
@@ -354,10 +349,7 @@ describe('BAIUnmountAfterClose', () => {
354349
);
355350
});
356351

357-
// TODO(FR-2609): same rc-motion / jsdom 29 incompat as the earlier
358-
// `should keep modal mounted…` test. Re-enable when the `transitionend`
359-
// fallback is reliable under vitest.
360-
it.skip('should maintain modal state during open->close transition', async () => {
352+
it('should maintain modal state during open->close transition', async () => {
361353
const TestComponent = () => {
362354
const [isOpen, setIsOpen] = React.useState(true);
363355
const [inputValue, setInputValue] = React.useState('test');

scripts/i18n-merge-driver.test.ts

Lines changed: 29 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,15 @@
1-
import { readFileSync } from "fs";
1+
import { mkdtempSync, rmSync, writeFileSync } from "fs";
2+
import { tmpdir } from "os";
3+
import { join } from "path";
24

3-
// Mock fs functions. Must be `vi.mock` (not `jest.mock`) — only the literal
4-
// `vi.mock` identifier is hoisted by Vitest above the `import` statements.
5-
vi.mock("fs", () => ({
6-
readFileSync: vi.fn(),
7-
}));
8-
9-
// Mock process.exit to avoid test termination
10-
const mockExit = jest.fn();
5+
// Mock process.exit to avoid test termination if the driver triggers it.
6+
const mockExit = vi.fn();
117
process.exit = mockExit as any;
128

13-
// Import functions from the actual implementation
9+
// Import functions from the actual CJS driver. Using `require()` (not
10+
// `vi.mock`) — the driver itself does `require("fs")` internally, and that
11+
// CJS require slips past Vitest's module mock transform. Tests below that
12+
// need to exercise file IO use real temp files instead of mocks.
1413
const {
1514
readJSON,
1615
deepEqual,
@@ -19,36 +18,36 @@ const {
1918
pathKey,
2019
} = require("./i18n-merge-driver");
2120

22-
const mockReadFileSync = readFileSync as jest.MockedFunction<
23-
typeof readFileSync
24-
>;
25-
2621
describe("i18n-merge-driver utility functions", () => {
2722
beforeEach(() => {
28-
jest.clearAllMocks();
23+
vi.clearAllMocks();
2924
});
3025

3126
describe("readJSON", () => {
32-
// TODO(FR-2609): vitest's `vi.mock('fs', ...)` does not propagate into
33-
// the CJS `require("fs")` at the top of `i18n-merge-driver.js`. Jest's
34-
// `require` patching applied globally; vitest only intercepts imports on
35-
// its own transform pipeline, and the JS file's inline `require()` slips
36-
// through. Re-enable after migrating the driver to ESM or switching to
37-
// `vi.mock` on the driver module directly.
38-
it.skip("should parse JSON from file correctly", () => {
39-
const mockData = '{"test": "value"}';
40-
mockReadFileSync.mockReturnValue(mockData);
41-
42-
const result = readJSON("test.json");
43-
44-
expect(mockReadFileSync).toHaveBeenCalledWith("test.json", "utf8");
27+
let tmpDir: string;
28+
29+
beforeEach(() => {
30+
tmpDir = mkdtempSync(join(tmpdir(), "i18n-merge-driver-test-"));
31+
});
32+
33+
afterEach(() => {
34+
rmSync(tmpDir, { recursive: true, force: true });
35+
});
36+
37+
it("should parse JSON from file correctly", () => {
38+
const file = join(tmpDir, "valid.json");
39+
writeFileSync(file, '{"test": "value"}', "utf8");
40+
41+
const result = readJSON(file);
42+
4543
expect(result).toEqual({ test: "value" });
4644
});
4745

4846
it("should throw error for invalid JSON", () => {
49-
mockReadFileSync.mockReturnValue("{invalid json}");
47+
const file = join(tmpDir, "invalid.json");
48+
writeFileSync(file, "{invalid json}", "utf8");
5049

51-
expect(() => readJSON("invalid.json")).toThrow();
50+
expect(() => readJSON(file)).toThrow();
5251
});
5352
});
5453

vitest.config.ts

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,13 @@ export default defineConfig({
3232
"packages/**",
3333
],
3434

35+
// V8 coverage instrumentation slows down tests significantly on CI's
36+
// smaller boxes. `gen-theme-schema.test.ts > generates a valid JSON
37+
// schema file` exercises the antd type tree several times per test
38+
// and routinely takes 5–10s under coverage. Bump the per-test timeout
39+
// so CI matches a normal local run without coverage.
40+
testTimeout: 30000,
41+
3542
// Coverage settings — see comment in `react/vitest.config.ts`. Same
3643
// reporters and provider so the `davelosert/vitest-coverage-report-action`
3744
// PR comment shape is consistent across the three workspaces.
@@ -44,6 +51,10 @@ export default defineConfig({
4451
"{src,scripts}/**/*.{test,spec}.ts",
4552
"src/wsproxy/**",
4653
"src/lib/backend.ai-client-node.*",
54+
// Build tooling — instrumenting these slows tests without giving
55+
// useful coverage signal. They are exercised end-to-end by the
56+
// tests but the coverage % of build scripts is not actionable.
57+
"scripts/**/*.cjs",
4758
],
4859
},
4960
},

0 commit comments

Comments
 (0)