pawarprasad123 commented on code in PR #733:
URL: https://github.com/apache/atlas/pull/733#discussion_r3825044025
##########
dashboard/src/setupTests.simple.ts:
##########
@@ -19,6 +19,11 @@
import '@testing-library/jest-dom';
+// Polyfills for Node 12 / Jest jsdom compatibility
Review Comment:
Tests (npm run test)
84 failed, 109 passed — all failures: TextEncoder is not defined
##########
dashboard/src/setupTests.simple.ts:
##########
@@ -19,6 +19,11 @@
import '@testing-library/jest-dom';
+// Polyfills for Node 12 / Jest jsdom compatibility
+import { TextEncoder, TextDecoder } from 'util';
Review Comment:
Critical issue (blocker)
Polyfill runs too late — tests still crash
Polyfills were added in setupTests.simple.ts, which is wired via
setupFilesAfterEnv in jest.config.js. That runs after test modules load.
jest.mock(..., () => jest.requireActual('react-router-dom')) in many tests
loads react-router before the polyfill runs.
Evidence:
```
ReferenceError: TextEncoder is not defined
at node_modules/react-router/dist/development/index.js:350
at jest.requireActual('react-router-dom')
```
Fix verified locally: Moving polyfills to setupFiles (runs before imports)
makes router-integration.test.tsx pass (10/10).
Suggested fix:
```
// jest.config.js
setupFiles: ['<rootDir>/src/setupTests.polyfills.ts'],
setupFilesAfterEnv: ['<rootDir>/src/setupTests.simple.ts'],
```
```
// setupTests.polyfills.ts (or top of a dedicated early setup file)
import { TextEncoder, TextDecoder } from 'util';
(global as any).TextEncoder = TextEncoder;
(global as any).TextDecoder = TextDecoder;
```
line 22: Misleading comment
React Router v7 requires Node >= 20 (per lockfile engines). Update the
comment to reflect jsdom/Jest, not Node 12.
PR description accuracy
The description says all 189 suites / 4,790 tests pass. Locally on current
master + PR deps: 193 suites, 84 fail. Please re-run npm run test after the
polyfill fix and update counts/screenshots.
No engines field — package.json
v7 requires Node >= 20. Consider documenting minimum Node in package.json
engines or project docs so CI/dev envs stay compatible.
export {} placement — setupTests.simple.ts ~line 27
Polyfills are above export {}; mocks are below. Works, but splitting
polyfills into a dedicated setupFiles module would be cleaner and avoid
ordering confusion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
