chore: upgrade jest to support new node#2067
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adjusts the Jest test setup across packages to better support running tests under a newer Node.js environment by switching some packages off jest-electron and providing a jsdom + canvas-backed DOM/canvas polyfill layer.
Changes:
- Added a shared jsdom/canvas setup script (
setup-jsdom-canvas.js) and a custom resolver forjest-environment-jsdom@26.6.2. - Updated
vrender-coreandvrender-componentsJest configs to use the custom jsdom-26 environment and the shared canvas setup. - Relaxed a few DOM/layout-sensitive unit tests to be less dependent on exact pixel values.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| share/jest-config/setup-jsdom-canvas.js | New jsdom + node-canvas polyfills (canvas, layout-ish metrics, RAF, etc.) for tests. |
| share/jest-config/jest-environment-jsdom-26.js | New resolver to force Jest to use jest-environment-jsdom@26.6.2 from Rush/pnpm installs. |
| packages/vrender/package.json | Redirect Jest output in the test script (2>&1). |
| packages/vrender/jest.config.js | Limits workers to 1 and sets useStderr: false. |
| packages/vrender-core/package.json | Redirect Jest output in the test script (2>&1). |
| packages/vrender-core/jest.config.js | Switches environment to jsdom-26 + adds shared canvas setup + enables TS source maps. |
| packages/vrender-components/tsconfig.test.json | Enables sourceMap for test compilation. |
| packages/vrender-components/package.json | Redirect Jest output in the test script (2>&1). |
| packages/vrender-components/jest.config.js | Switches environment to jsdom-26 + adds shared canvas setup. |
| packages/vrender-components/tests/unit/legend/discrete.test.ts | Updates expectations to be more robust to layout differences. |
| packages/vrender-components/tests/unit/bugfix/legend-focus-layout.test.ts | Updates expectation to use relative positioning instead of an absolute x value. |
| packages/vrender-components/tests/unit/axis/overlap/auto-limit.test.ts | Updates assertions to check clipping behavior without exact clipped strings. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| global.requestAnimationFrame = window.requestAnimationFrame.bind(window); | ||
| global.cancelAnimationFrame = window.cancelAnimationFrame.bind(window); | ||
| global.PointerEvent = window.PointerEvent; | ||
| global.OffscreenCanvas = window.OffscreenCanvas; |
There was a problem hiding this comment.
At the end of this setup file, window is referenced unconditionally when binding requestAnimationFrame / cancelAnimationFrame and copying globals. Earlier code guards on typeof window !== 'undefined', but these lines will still throw a ReferenceError if this setup file is ever evaluated under a non-jsdom environment (or if the environment fails to initialize). Wrap these assignments in the same window guard, or bind from globalThis.window only when it exists and provide a safe fallback implementation otherwise.
| global.requestAnimationFrame = window.requestAnimationFrame.bind(window); | |
| global.cancelAnimationFrame = window.cancelAnimationFrame.bind(window); | |
| global.PointerEvent = window.PointerEvent; | |
| global.OffscreenCanvas = window.OffscreenCanvas; | |
| if (typeof window !== 'undefined' && window) { | |
| if (typeof window.requestAnimationFrame === 'function') { | |
| global.requestAnimationFrame = window.requestAnimationFrame.bind(window); | |
| } | |
| if (typeof window.cancelAnimationFrame === 'function') { | |
| global.cancelAnimationFrame = window.cancelAnimationFrame.bind(window); | |
| } | |
| if (typeof window.PointerEvent !== 'undefined') { | |
| global.PointerEvent = window.PointerEvent; | |
| } | |
| if (typeof window.OffscreenCanvas !== 'undefined') { | |
| global.OffscreenCanvas = window.OffscreenCanvas; | |
| } | |
| } else { | |
| if (typeof global.requestAnimationFrame === 'undefined' && typeof setTimeout === 'function') { | |
| global.requestAnimationFrame = function (callback) { | |
| return setTimeout(function () { | |
| callback(Date.now()); | |
| }, 16); | |
| }; | |
| } | |
| if (typeof global.cancelAnimationFrame === 'undefined' && typeof clearTimeout === 'function') { | |
| global.cancelAnimationFrame = function (id) { | |
| clearTimeout(id); | |
| }; | |
| } | |
| } |
| const width = canvasElement.width || 300; | ||
| const height = canvasElement.height || 150; |
There was a problem hiding this comment.
canvasElement.width || 300 / height || 150 treats an explicit size of 0 as “unset” and forces the HTML canvas default size. In real browsers, canvas.width = 0 is a valid value and should remain 0. Prefer using the element’s numeric width/height directly (they already default to 300x150 in jsdom) or check for null/undefined rather than falsy values.
| const width = canvasElement.width || 300; | |
| const height = canvasElement.height || 150; | |
| const width = canvasElement.width; | |
| const height = canvasElement.height; |
| const path = require('path'); | ||
|
|
||
| function resolveJestEnvironmentJsdom26() { | ||
| const pnpmDir = path.resolve(__dirname, '../../common/temp/node_modules/.pnpm'); |
There was a problem hiding this comment.
fs.readdirSync(pnpmDir) will throw an unhandled ENOENT if common/temp/node_modules/.pnpm doesn’t exist (e.g., tests run before rush install, or in a different install layout). Add an existence check (or catch the error) and rethrow a clearer message indicating the expected install step/location.
| const pnpmDir = path.resolve(__dirname, '../../common/temp/node_modules/.pnpm'); | |
| const pnpmDir = path.resolve(__dirname, '../../common/temp/node_modules/.pnpm'); | |
| if (!fs.existsSync(pnpmDir)) { | |
| throw new Error( | |
| 'Unable to locate PNPM directory at "common/temp/node_modules/.pnpm". ' + | |
| 'Ensure that dependencies have been installed (for example, by running "rush install" in the repo root).' | |
| ); | |
| } |
| const fs = require('fs'); | ||
| const path = require('path'); | ||
|
|
||
| function resolveJestEnvironmentJsdom26() { | ||
| const pnpmDir = path.resolve(__dirname, '../../common/temp/node_modules/.pnpm'); | ||
| const entry = fs | ||
| .readdirSync(pnpmDir) | ||
| .find(name => name.startsWith('jest-environment-jsdom@26.6.2')); | ||
|
|
||
| if (!entry) { | ||
| throw new Error('Unable to resolve jest-environment-jsdom@26.6.2 from common/temp/node_modules/.pnpm'); | ||
| } | ||
|
|
||
| return path.join(pnpmDir, entry, 'node_modules/jest-environment-jsdom/build/index.js'); |
There was a problem hiding this comment.
This environment resolver relies on pnpm’s internal virtual store layout (common/temp/node_modules/.pnpm) and directory naming to locate jest-environment-jsdom@26.6.2. This is brittle and makes the test environment sensitive to pnpm implementation details. A more robust approach is to use require.resolve('jest-environment-jsdom/build/index.js', { paths: [path.resolve(__dirname, '../../common/temp/node_modules')] }) (or similar) so Node’s resolver finds the correct installed copy without scanning .pnpm.
| const fs = require('fs'); | |
| const path = require('path'); | |
| function resolveJestEnvironmentJsdom26() { | |
| const pnpmDir = path.resolve(__dirname, '../../common/temp/node_modules/.pnpm'); | |
| const entry = fs | |
| .readdirSync(pnpmDir) | |
| .find(name => name.startsWith('jest-environment-jsdom@26.6.2')); | |
| if (!entry) { | |
| throw new Error('Unable to resolve jest-environment-jsdom@26.6.2 from common/temp/node_modules/.pnpm'); | |
| } | |
| return path.join(pnpmDir, entry, 'node_modules/jest-environment-jsdom/build/index.js'); | |
| const path = require('path'); | |
| function resolveJestEnvironmentJsdom26() { | |
| const baseNodeModulesDir = path.resolve(__dirname, '../../common/temp/node_modules'); | |
| try { | |
| return require.resolve('jest-environment-jsdom/build/index.js', { | |
| paths: [baseNodeModulesDir], | |
| }); | |
| } catch (error) { | |
| throw new Error( | |
| 'Unable to resolve jest-environment-jsdom@26.6.2 from ' + baseNodeModulesDir | |
| ); | |
| } |
| module.exports = { | ||
| runner: 'jest-electron/runner', | ||
| testEnvironment: 'jest-electron/environment', | ||
| testEnvironment: '../../share/jest-config/jest-environment-jsdom-26.js', | ||
| testEnvironmentOptions: { | ||
| pretendToBeVisual: true | ||
| }, |
There was a problem hiding this comment.
In this config, testEnvironment / setupFiles are specified as relative paths (e.g. ../../share/...) whereas other package configs use path.resolve(__dirname, ...). Using absolute paths here avoids surprises if rootDir is customized or Jest is invoked from a different working directory, and keeps the configs consistent across packages.
[中文版模板 / Chinese template]
🤔 This is a ...
🔗 Related issue link
🐞 Bugserver case id
💡 Background and solution
📝 Changelog
☑️ Self-Check before Merge
🚀 Summary
copilot:summary
🔍 Walkthrough
copilot:walkthrough