Fix: Tests not getting executed by Jest#3209
Fix: Tests not getting executed by Jest#3209SantamRC wants to merge 4 commits intoRocketChat:masterfrom
Conversation
WalkthroughBroadened Jest test discovery and test transforms; added transform ignore exceptions; refactored and extended tests for downloads, outlookCalendar, and spellChecking; introduced Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
SantamRC
changed the title
- Fix mockHandle hoisting issue in downloads/integration.spec.ts - Add missing electron mocks in spellChecking/main.spec.ts - Mock uuid to avoid ES module parsing in outlookCalendar tests - Add transformIgnorePatterns for uuid and ews-javascript-api - Add missing isDestroyed() and setSaveDialogOptions() to test mocks Fixes 3 test suites (69 tests total)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/downloads/integration.spec.ts (2)
120-121:onCompletedCallback as anysilently breaks theDownloadFiletype contract.The electron-dl
onCompletedcallback receives a fullDownloadFileobject (filename,path,fileSize,mimeType,url). Casting toanyand passing only{ filename: 'test-file.pdf' }means any production code inonCompletedthat readsfile.path(e.g., thedownloadStore.setcall insetupElectronDlWithTracking) receivesundefinedsilently. At minimum, include the fields actually exercised by the implementation.🛠 Suggested improvement
- const onCompletedCallback = electronDlCall[0].onCompleted! as any; + const onCompletedCallback = electronDlCall[0].onCompleted!;- onCompletedCallback!({ filename: 'test-file.pdf' }); + onCompletedCallback({ filename: 'test-file.pdf', path: '/downloads/test-file.pdf', fileSize: 2048, mimeType: 'application/pdf', url: 'https://example.com/file.pdf' });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/downloads/integration.spec.ts` around lines 120 - 121, The test is casting onCompletedCallback to any and passing a partial object which breaks the DownloadFile contract; update the test so electronDlCall[0].onCompleted is used with a properly shaped DownloadFile (at least include filename and path plus any fields used by setupElectronDlWithTracking such as fileSize, mimeType, url) instead of casting to any or passing only { filename: 'test-file.pdf' }, and remove the unsafe cast so the callback receives a full DownloadFile object that matches the production code expectations.
16-16: Consider renaming tointegration.main.spec.tsto match the main-process test naming convention.Per the project guidelines,
*.main.spec.tsis the convention for main-process tests. This file testssetupDownloadsandhandleWillDownloadEvent— both main-process entry points. The current.spec.tsname routes it to the renderer runner under the new pattern. Everything works because all Electron APIs are mocked, but the name misrepresents the test's process target and may confuse future contributors. Based on coding guidelines: "Use*.spec.tsfile naming for renderer process tests and*.main.spec.tsfor main process tests."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/downloads/integration.spec.ts` at line 16, This test file is a main-process test (it exercises setupDownloads and handleWillDownloadEvent) but is named with a renderer-style suffix; rename the file from integration.spec.ts to integration.main.spec.ts, update any test-runner or CI references that explicitly list this filename, and ensure any relative imports (e.g., setupElectronDlWithTracking, setupDownloads, handleWillDownloadEvent) still resolve after the rename so the test is discovered by the main-process test runner.src/outlookCalendar/getOutlookEvents.spec.ts (2)
189-193:toContainassertion is weaker than necessary — use an exact URL if the expected output is deterministic.
expect(result).toContain('/ews/exchange.asmx')passes for any URL containing that path, including malformed ones. If the fallback forinvalid://domain.comproduces a predictable URL (e.g.,https://domain.com/ews/exchange.asmx), assert it exactly withtoBeto make the test self-documenting and catch regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/outlookCalendar/getOutlookEvents.spec.ts` around lines 189 - 193, Update the test for sanitizeExchangeUrl to use a strict equality assertion instead of toContain: call sanitizeExchangeUrl('invalid://domain.com') and assert the exact expected fallback URL (e.g., expect(result).toBe('https://domain.com/ews/exchange.asmx')) so the spec for the sanitizeExchangeUrl fallback behavior is deterministic and will catch regressions; modify the test case named "handles invalid protocol via fallback" to compare the full URL string returned by sanitizeExchangeUrl rather than using toContain.
3-4: Bothv1andv4return the same UUID — consider returning distinct values per call.Returning an identical constant for every call means tests cannot detect bugs where the production code relies on uniqueness across calls (e.g., two concurrent event IDs colliding). Since the mock exists solely to avoid ES module parsing, making
v4return incrementing or distinct values is a cheap safety net.💡 Optional improvement
jest.mock('uuid', () => ({ - v1: jest.fn(() => '00000000-0000-0000-0000-000000000000'), - v4: jest.fn(() => '00000000-0000-0000-0000-000000000000'), + v1: jest.fn(() => '00000000-0000-0000-0000-000000000001'), + v4: jest.fn(() => '00000000-0000-0000-0000-000000000002'), }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/outlookCalendar/getOutlookEvents.spec.ts` around lines 3 - 4, The test mock returns the same UUID for both v1 and v4 which can hide bugs that rely on unique IDs; update the mock implementations for v1 and/or v4 so they produce distinct values per call (e.g., make v4 return different UUIDs each invocation or use a counter/mockImplementationOnce sequence) and keep the references to the mock functions (v1, v4) so tests still avoid ES module parsing while asserting uniqueness when needed.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
jest.config.jssrc/downloads/integration.spec.tssrc/outlookCalendar/getOutlookEvents.spec.tssrc/spellChecking/main.spec.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript strict mode enabled in TypeScript configuration
Use React functional components with hooks instead of class components
Follow FSA (Flux Standard Action) pattern for Redux actions
Use camelCase for file names and PascalCase for component file names
All code must pass ESLint and TypeScript checks
Write self-documenting code with clear naming; avoid unnecessary comments except for complex business logic or non-obvious decisions
Use Fuselage components from@rocket.chat/fuselagefor all UI work and only create custom components when Fuselage doesn't provide what's needed
CheckTheme.d.tsfor valid color tokens when using Fuselage components
Use defensive coding with optional chaining and fallbacks for Linux-only APIs (process.getuid(), process.getgid(), process.geteuid(), process.getegid()) to ensure cross-platform compatibility across Windows, macOS, and Linux
Files:
src/outlookCalendar/getOutlookEvents.spec.tssrc/spellChecking/main.spec.tssrc/downloads/integration.spec.ts
**/*.{spec,main.spec}.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{spec,main.spec}.ts: Use*.spec.tsfile naming for renderer process tests and*.main.spec.tsfor main process tests
Only mock platform-specific APIs in tests when defensive coding patterns cannot be used; use Object.defineProperty to mock process.platform and environment variables
Files:
src/outlookCalendar/getOutlookEvents.spec.tssrc/spellChecking/main.spec.tssrc/downloads/integration.spec.ts
🧠 Learnings (11)
📓 Common learnings
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-04T19:29:54.650Z
Learning: Use patch-package mechanism in `patches/` directory for `kayahr/jest-electron-runner` patches
📚 Learning: 2026-02-04T19:29:54.650Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-04T19:29:54.650Z
Learning: Applies to **/*.{spec,main.spec}.ts : Only mock platform-specific APIs in tests when defensive coding patterns cannot be used; use Object.defineProperty to mock process.platform and environment variables
Applied to files:
src/outlookCalendar/getOutlookEvents.spec.tsjest.config.jssrc/spellChecking/main.spec.tssrc/downloads/integration.spec.ts
📚 Learning: 2026-02-04T19:30:03.354Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: src/outlookCalendar/AGENTS.md:0-0
Timestamp: 2026-02-04T19:30:03.354Z
Learning: Applies to src/outlookCalendar/**/*@(calendar|outlook)*.{ts,tsx} : Use `createClassifiedError()` from `errorClassification.ts` for user-facing errors to provide proper error categorization (network, auth, exchange, unknown), user-friendly messages with troubleshooting steps, and structured error context
Applied to files:
src/outlookCalendar/getOutlookEvents.spec.ts
📚 Learning: 2026-02-04T19:30:03.354Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: src/outlookCalendar/AGENTS.md:0-0
Timestamp: 2026-02-04T19:30:03.354Z
Learning: Applies to src/outlookCalendar/**/*@(calendar|outlook)*.{ts,tsx} : Use `outlookError()` for errors that should always be logged regardless of verbose mode settings, since error visibility is critical for debugging
Applied to files:
src/outlookCalendar/getOutlookEvents.spec.ts
📚 Learning: 2026-02-04T19:30:03.354Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: src/outlookCalendar/AGENTS.md:0-0
Timestamp: 2026-02-04T19:30:03.354Z
Learning: Applies to src/outlookCalendar/**/preload.ts : In `preload.ts`, keep logging minimal as it runs in the renderer process and cannot access the centralized `global.isVerboseOutlookLoggingEnabled` variable, causing all logs to always appear
Applied to files:
src/outlookCalendar/getOutlookEvents.spec.ts
📚 Learning: 2026-02-04T19:30:03.354Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: src/outlookCalendar/AGENTS.md:0-0
Timestamp: 2026-02-04T19:30:03.354Z
Learning: Applies to src/outlookCalendar/**/*@(calendar|outlook)*.{ts,tsx} : Use `outlookLog()`, `outlookWarn()`, and `outlookDebug()` only when verbose logging is expected to be enabled - they respect the verbose logging toggle from Settings > Developer > Verbose Outlook Logging
Applied to files:
src/outlookCalendar/getOutlookEvents.spec.ts
📚 Learning: 2026-02-04T19:30:03.354Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: src/outlookCalendar/AGENTS.md:0-0
Timestamp: 2026-02-04T19:30:03.354Z
Learning: Applies to src/outlookCalendar/**/*@(calendar|outlook)*.{ts,tsx} : Always use the centralized logger from `logger.ts` (import `outlookLog`, `outlookError`, `outlookWarn`, or `outlookDebug`) instead of using `console.log` directly for Outlook Calendar module logging
Applied to files:
src/outlookCalendar/getOutlookEvents.spec.ts
📚 Learning: 2026-02-04T19:29:54.650Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-04T19:29:54.650Z
Learning: Applies to **/*.{spec,main.spec}.ts : Use `*.spec.ts` file naming for renderer process tests and `*.main.spec.ts` for main process tests
Applied to files:
jest.config.jssrc/spellChecking/main.spec.tssrc/downloads/integration.spec.ts
📚 Learning: 2026-02-04T19:29:54.650Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-04T19:29:54.650Z
Learning: Use patch-package mechanism in `patches/` directory for `kayahr/jest-electron-runner` patches
Applied to files:
jest.config.js
📚 Learning: 2026-02-04T19:29:54.650Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-04T19:29:54.650Z
Learning: Applies to **/*.{ts,tsx} : Use TypeScript strict mode enabled in TypeScript configuration
Applied to files:
jest.config.js
📚 Learning: 2026-02-04T19:29:54.650Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-04T19:29:54.650Z
Learning: Applies to **/*.{ts,tsx} : All code must pass ESLint and TypeScript checks
Applied to files:
jest.config.js
🧬 Code graph analysis (2)
src/outlookCalendar/getOutlookEvents.spec.ts (1)
src/outlookCalendar/getOutlookEvents.ts (1)
sanitizeExchangeUrl(120-221)
src/downloads/integration.spec.ts (4)
src/downloads/main/setup.ts (1)
setupElectronDlWithTracking(46-116)src/downloads/main.ts (1)
setupDownloads(103-242)src/store/index.ts (1)
dispatch(38-41)src/downloads/actions.ts (1)
DOWNLOADS_CLEARED(5-5)
🪛 ESLint
jest.config.js
[error] 14-16: Replace ⏎········'node_modules/(?!(uuid|ews-javascript-api)/)',⏎······ with 'node_modules/(?!(uuid|ews-javascript-api)/)'
(prettier/prettier)
[error] 29-31: Replace ⏎········'node_modules/(?!(uuid|ews-javascript-api)/)',⏎······ with 'node_modules/(?!(uuid|ews-javascript-api)/)'
(prettier/prettier)
src/downloads/integration.spec.ts
[error] 110-110: Delete ······
(prettier/prettier)
[error] 119-119: Delete ······
(prettier/prettier)
[error] 260-260: Delete ······
(prettier/prettier)
[error] 346-346: Delete ······
(prettier/prettier)
[error] 369-369: Delete ······
(prettier/prettier)
[error] 398-398: Delete ······
(prettier/prettier)
[error] 490-490: Delete ······
(prettier/prettier)
🪛 GitHub Check: check (ubuntu-latest)
jest.config.js
[failure] 14-14:
Replace ⏎········'node_modules/(?!(uuid|ews-javascript-api)/)',⏎······ with 'node_modules/(?!(uuid|ews-javascript-api)/)'
[failure] 29-29:
Replace ⏎········'node_modules/(?!(uuid|ews-javascript-api)/)',⏎······ with 'node_modules/(?!(uuid|ews-javascript-api)/)'
src/downloads/integration.spec.ts
[failure] 119-119:
Delete ······
[failure] 110-110:
Delete ······
[failure] 260-260:
Delete ······
[failure] 346-346:
Delete ······
[failure] 369-369:
Delete ······
[failure] 398-398:
Delete ······
[failure] 490-490:
Delete ······
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: check (windows-latest)
🔇 Additional comments (2)
src/spellChecking/main.spec.ts (1)
1-20: LGTM — mock placement and structure are correct.The
jest.mock('electron', ...)block is correctly placed before any imports so Jest's hoisting mechanism runs it first. All APIs exercised bysetupSpellChecking(ipcMain,app.whenReady,session.defaultSession,webContents.getAllWebContents) are stubbed.jest.config.js (1)
10-10: No concerns identified with the!(main)*pattern.The testMatch patterns correctly route tests—there are no
src/main*directories (other thansrc/mainitself) that could be accidentally routed to the renderer runner. Additionally, the config includes explicit naming patterns (renderer.(spec|test)andmain.(spec|test)), providing defense-in-depth alignment with the test naming conventions.Likely an incorrect or invalid review comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@jest.config.js`:
- Around line 14-16: The Prettier violation is caused by multi-line array
formatting for transformIgnorePatterns in jest.config.js; change each
transformIgnorePatterns declaration (the ones containing
'node_modules/(?!(uuid|ews-javascript-api)/)') to a single-line array form so
Prettier passes — update both occurrences of transformIgnorePatterns (the block
near the top and the identical block later) to be single-line arrays.
In `@src/downloads/integration.spec.ts`:
- Around line 272-278: The test currently skips a missing clearAllHandler and
later fails cryptically; add an upfront assertion that clearAllHandler is
defined before invoking it so test failures show the missing handler
immediately. Specifically, before awaiting clearAllHandler(), assert the handler
exists (e.g., expect(clearAllHandler).toBeDefined() / toBeTruthy()) referencing
the clearAllHandler symbol, then call await clearAllHandler() and keep the
existing expect(dispatch).toHaveBeenCalledWith({ type: DOWNLOADS_CLEARED }) to
verify behavior.
- Around line 109-121: The failing Prettier checks are caused by stray blank
lines immediately after opening braces in this test block; open the
setupDownloads/electronDl mock extraction section (functions/variables:
setupDownloads, electronDl, electronDlMock, electronDlCall, onStartedCallback,
onCompletedCallback) and remove any blank line(s) directly after opening braces
(and apply the same fix to the other flagged blocks noted in the comment), so
each block starts immediately with code instead of an empty line to satisfy
Prettier/ESLint.
---
Nitpick comments:
In `@src/downloads/integration.spec.ts`:
- Around line 120-121: The test is casting onCompletedCallback to any and
passing a partial object which breaks the DownloadFile contract; update the test
so electronDlCall[0].onCompleted is used with a properly shaped DownloadFile (at
least include filename and path plus any fields used by
setupElectronDlWithTracking such as fileSize, mimeType, url) instead of casting
to any or passing only { filename: 'test-file.pdf' }, and remove the unsafe cast
so the callback receives a full DownloadFile object that matches the production
code expectations.
- Line 16: This test file is a main-process test (it exercises setupDownloads
and handleWillDownloadEvent) but is named with a renderer-style suffix; rename
the file from integration.spec.ts to integration.main.spec.ts, update any
test-runner or CI references that explicitly list this filename, and ensure any
relative imports (e.g., setupElectronDlWithTracking, setupDownloads,
handleWillDownloadEvent) still resolve after the rename so the test is
discovered by the main-process test runner.
In `@src/outlookCalendar/getOutlookEvents.spec.ts`:
- Around line 189-193: Update the test for sanitizeExchangeUrl to use a strict
equality assertion instead of toContain: call
sanitizeExchangeUrl('invalid://domain.com') and assert the exact expected
fallback URL (e.g., expect(result).toBe('https://domain.com/ews/exchange.asmx'))
so the spec for the sanitizeExchangeUrl fallback behavior is deterministic and
will catch regressions; modify the test case named "handles invalid protocol via
fallback" to compare the full URL string returned by sanitizeExchangeUrl rather
than using toContain.
- Around line 3-4: The test mock returns the same UUID for both v1 and v4 which
can hide bugs that rely on unique IDs; update the mock implementations for v1
and/or v4 so they produce distinct values per call (e.g., make v4 return
different UUIDs each invocation or use a counter/mockImplementationOnce
sequence) and keep the references to the mock functions (v1, v4) so tests still
avoid ES module parsing while asserting uniqueness when needed.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/downloads/integration.spec.ts (1)
335-356: Repeated electronDl callback extraction could be a shared helper.The pattern of calling
setupElectronDlWithTracking(), extractingelectronDlMock.mock.calls[0], guarding with a throw, and pulling outonStarted/onCompletedis duplicated across four test blocks (lines 106‑121, 335‑347, 358‑370, 392‑399). A small helper would reduce noise and keep callback extraction consistent.Example helper
function getElectronDlCallbacks() { const electronDlMock = electronDl as jest.MockedFunction<typeof electronDl>; setupElectronDlWithTracking(); const call = electronDlMock.mock.calls[0]; if (!call?.[0]) { throw new Error('electronDl was not called'); } return { onStarted: call[0].onStarted!, onCompleted: call[0].onCompleted! as any, }; }Also applies to: 358-383, 386-399
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/downloads/integration.spec.ts` around lines 335 - 356, Extract the duplicated logic that calls setupElectronDlWithTracking(), accesses electronDl.mock.calls[0], guards the presence of call[0], and returns the onStarted/onCompleted callbacks into a shared helper (e.g., getElectronDlCallbacks) and use it in the four tests currently duplicating the pattern; locate usages around setupElectronDlWithTracking, electronDl, and webContents in the spec and replace the repeated block with a single call to the helper that returns { onStarted, onCompleted } (throwing if the mock call or callbacks are missing) so tests use the helper instead of repeating extraction logic.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
jest.config.jssrc/downloads/integration.spec.tssrc/outlookCalendar/getOutlookEvents.spec.tssrc/spellChecking/main.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/outlookCalendar/getOutlookEvents.spec.ts
- jest.config.js
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript strict mode enabled in TypeScript configuration
Use React functional components with hooks instead of class components
Follow FSA (Flux Standard Action) pattern for Redux actions
Use camelCase for file names and PascalCase for component file names
All code must pass ESLint and TypeScript checks
Write self-documenting code with clear naming; avoid unnecessary comments except for complex business logic or non-obvious decisions
Use Fuselage components from@rocket.chat/fuselagefor all UI work and only create custom components when Fuselage doesn't provide what's needed
CheckTheme.d.tsfor valid color tokens when using Fuselage components
Use defensive coding with optional chaining and fallbacks for Linux-only APIs (process.getuid(), process.getgid(), process.geteuid(), process.getegid()) to ensure cross-platform compatibility across Windows, macOS, and Linux
Files:
src/spellChecking/main.spec.tssrc/downloads/integration.spec.ts
**/*.{spec,main.spec}.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{spec,main.spec}.ts: Use*.spec.tsfile naming for renderer process tests and*.main.spec.tsfor main process tests
Only mock platform-specific APIs in tests when defensive coding patterns cannot be used; use Object.defineProperty to mock process.platform and environment variables
Files:
src/spellChecking/main.spec.tssrc/downloads/integration.spec.ts
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-04T19:29:54.650Z
Learning: Use patch-package mechanism in `patches/` directory for `kayahr/jest-electron-runner` patches
📚 Learning: 2026-02-04T19:29:54.650Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-04T19:29:54.650Z
Learning: Applies to **/*.{spec,main.spec}.ts : Only mock platform-specific APIs in tests when defensive coding patterns cannot be used; use Object.defineProperty to mock process.platform and environment variables
Applied to files:
src/spellChecking/main.spec.tssrc/downloads/integration.spec.ts
📚 Learning: 2026-02-04T19:29:54.650Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-04T19:29:54.650Z
Learning: Applies to **/*.{spec,main.spec}.ts : Use `*.spec.ts` file naming for renderer process tests and `*.main.spec.ts` for main process tests
Applied to files:
src/spellChecking/main.spec.tssrc/downloads/integration.spec.ts
📚 Learning: 2026-02-04T19:29:54.650Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-04T19:29:54.650Z
Learning: Applies to **/*.{ts,tsx} : All code must pass ESLint and TypeScript checks
Applied to files:
src/downloads/integration.spec.ts
🧬 Code graph analysis (1)
src/downloads/integration.spec.ts (2)
src/store/index.ts (1)
dispatch(38-41)src/downloads/actions.ts (1)
DOWNLOADS_CLEARED(5-5)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: check (ubuntu-latest)
- GitHub Check: check (macos-latest)
- GitHub Check: check (windows-latest)
🔇 Additional comments (4)
src/downloads/integration.spec.ts (4)
1-16: LGTM on imports and new mock wiring.The addition of
handlefrom../ipc/mainandsetupElectronDlWithTrackingfrom./main/setupaligns with the refactored test strategy of extracting callbacks from the electronDl mock and resolving IPC handlers via thehandlemock. The mock declarations below correctly cover both.
74-103: Mock helpers are well-structured.
createMockDownloadItemnow includessetSaveDialogOptionsandcreateMockWebContentsincludesisDestroyed— both additions align with the updated production code's contract. The spread-override pattern is clean and reusable.
106-204: End-to-end lifecycle test is thorough.The test validates the full download lifecycle through electron-dl callbacks (onStarted → progress update → done → onCompleted), verifying each
dispatchcall along the way. The guard at line 116–118 properly fails fast if electronDl wasn't invoked.
289-307: Good edge-case coverage for missing server.Tests that downloads still work when no server matches the webContents ID, falling back to
'unknown'/'Unknown Server'.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/spellChecking/main.spec.ts`:
- Around line 1-24: The top-of-file comment is misleading because it says the
electron mock is placed before imports when the jest.mock(...) call actually
appears after imports; update the comment to accurately state that jest.mock is
hoisted by Jest (or simply remove the comment). Locate the jest.mock block that
mocks electron (referenced by ipcMain, app.whenReady, session.defaultSession
methods) and either delete the existing comment line "// Mock electron's ipcMain
and session before any imports" or replace it with a concise note such as "//
Mock electron via jest.mock (Jest hoists mocks above imports)" so readers
understand the hoisting behavior.
---
Duplicate comments:
In `@src/downloads/integration.spec.ts`:
- Around line 272-275: The test currently skips the useful failure when
clearAllHandler is undefined; update the test to explicitly assert or fail if
clearAllHandler is not found before the call (e.g., assert clearAllHandler is
defined or throw a descriptive Error mentioning "clearAllHandler not found") and
only then invoke clearAllHandler(); keep references to the existing symbol names
clearAllHandler and the subsequent expect for DOWNLOADS_CLEARED so the failure
clearly reports the missing handler rather than just the final dispatch
assertion.
- Around line 242-249: Tests currently use optional chaining when invoking IPC
handler variables (e.g., showInFolderHandler?.(...), copyLinkHandler?.(...)),
which masks missing registrations; assert each handler is present before calling
it so failures are explicit: add an assertion like
expect(showInFolderHandler).toBeDefined() (and same for copyLinkHandler and the
other handler variables referenced in this spec) immediately before invoking the
handler, then call the handler without optional chaining
(showInFolderHandler(...)); apply this pattern to every handler invocation in
the file (including the calls referenced in the review) so a missing handler
fails the test with a clear message.
---
Nitpick comments:
In `@src/downloads/integration.spec.ts`:
- Around line 335-356: Extract the duplicated logic that calls
setupElectronDlWithTracking(), accesses electronDl.mock.calls[0], guards the
presence of call[0], and returns the onStarted/onCompleted callbacks into a
shared helper (e.g., getElectronDlCallbacks) and use it in the four tests
currently duplicating the pattern; locate usages around
setupElectronDlWithTracking, electronDl, and webContents in the spec and replace
the repeated block with a single call to the helper that returns { onStarted,
onCompleted } (throwing if the mock call or callbacks are missing) so tests use
the helper instead of repeating extraction logic.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/downloads/integration.spec.ts (1)
1-16:⚠️ Potential issue | 🟡 MinorRename to
integration.main.spec.tsper naming convention.The file exclusively tests main-process code (
handleWillDownloadEvent,setupDownloads,setupElectronDlWithTracking, IPC main handlers,electron-dl). Using the.spec.tssuffix signals a renderer-process test, which is misleading and conflicts with the file naming convention.♻️ Rename
-// src/downloads/integration.spec.ts +// src/downloads/integration.main.spec.tsIf the
jest.config.jstestMatchupdate in this PR only catches*.spec.tsin module folders (and not*.main.spec.ts), that pattern will also need to be extended to cover the renamed file.As per coding guidelines: "Use
*.spec.tsfile naming for renderer process tests and*.main.spec.tsfor main process tests."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/downloads/integration.spec.ts` around lines 1 - 16, This file contains only main-process tests (handleWillDownloadEvent, setupDownloads, setupElectronDlWithTracking, IPC main handlers and electron-dl), so rename the test file from integration.spec.ts to integration.main.spec.ts to follow the main/renderer naming convention; after renaming, run tests and, if your jest.config.js testMatch currently only matches *.spec.ts in module folders, update testMatch to include *.main.spec.ts so the renamed file is discovered.
🧹 Nitpick comments (2)
src/downloads/integration.spec.ts (2)
231-234: Consider a typed helper for repeated IPC handler lookup.The cast +
mock.calls.findpattern appears three times. A small helper would remove the duplication and centralize the type cast:♻️ Optional refactor
+ const getIpcHandler = (channel: string) => + (handle as jest.MockedFunction<typeof handle>).mock.calls.find( + ([ch]) => String(ch) === channel + )?.[1];Then each lookup becomes
const showInFolderHandler = getIpcHandler('downloads/show-in-folder');etc.Also applies to: 263-268, 322-329
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/downloads/integration.spec.ts` around lines 231 - 234, The test repeats the cast + mock.calls.find pattern to extract IPC handlers from the mocked handle function; create a small typed helper (e.g., getIpcHandler) that takes the channel string and returns the handler typed as the same type used in the cast (jest.MockedFunction<typeof handle>) so callers like showInFolderHandler can be retrieved via getIpcHandler('downloads/show-in-folder'); update all occurrences (the blocks using handle as jest.MockedFunction<typeof handle> and mock.calls.find) to use this helper to remove duplication and centralize the type cast.
68-72:createMainReduxStore()inbeforeEachis a no-op.Since
createMainReduxStoreis fully mocked asjest.fn()(line 53), calling it inbeforeEachdoesn't initialize any store.dispatchandselectare independently mocked and don't depend on this call. Remove it unlesshandleWillDownloadEventorsetupDownloadsexplicitly guards against an uninitialized store and that sentinel path needs exercising.♻️ Proposed fix
beforeEach(() => { jest.clearAllMocks(); - createMainReduxStore(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/downloads/integration.spec.ts` around lines 68 - 72, The call to createMainReduxStore() in beforeEach is a no-op because createMainReduxStore is fully mocked; remove that call from beforeEach to avoid confusing dead setup, unless you need to exercise the uninitialized-store sentinel in handleWillDownloadEvent or setupDownloads—in that case, restore a real store by replacing the mock with the actual implementation (or a minimal real store) before the test(s) that need it so dispatch and select behave correctly; reference createMainReduxStore, handleWillDownloadEvent, setupDownloads, dispatch and select when making the change.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/downloads/integration.spec.tssrc/spellChecking/main.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/spellChecking/main.spec.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript strict mode enabled in TypeScript configuration
Use React functional components with hooks instead of class components
Follow FSA (Flux Standard Action) pattern for Redux actions
Use camelCase for file names and PascalCase for component file names
All code must pass ESLint and TypeScript checks
Write self-documenting code with clear naming; avoid unnecessary comments except for complex business logic or non-obvious decisions
Use Fuselage components from@rocket.chat/fuselagefor all UI work and only create custom components when Fuselage doesn't provide what's needed
CheckTheme.d.tsfor valid color tokens when using Fuselage components
Use defensive coding with optional chaining and fallbacks for Linux-only APIs (process.getuid(), process.getgid(), process.geteuid(), process.getegid()) to ensure cross-platform compatibility across Windows, macOS, and Linux
Files:
src/downloads/integration.spec.ts
**/*.{spec,main.spec}.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{spec,main.spec}.ts: Use*.spec.tsfile naming for renderer process tests and*.main.spec.tsfor main process tests
Only mock platform-specific APIs in tests when defensive coding patterns cannot be used; use Object.defineProperty to mock process.platform and environment variables
Files:
src/downloads/integration.spec.ts
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-04T19:29:54.650Z
Learning: Use patch-package mechanism in `patches/` directory for `kayahr/jest-electron-runner` patches
📚 Learning: 2026-02-04T19:29:54.650Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-04T19:29:54.650Z
Learning: Applies to **/*.{spec,main.spec}.ts : Only mock platform-specific APIs in tests when defensive coding patterns cannot be used; use Object.defineProperty to mock process.platform and environment variables
Applied to files:
src/downloads/integration.spec.ts
📚 Learning: 2026-02-04T19:29:54.650Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-04T19:29:54.650Z
Learning: Applies to **/*.{spec,main.spec}.ts : Use `*.spec.ts` file naming for renderer process tests and `*.main.spec.ts` for main process tests
Applied to files:
src/downloads/integration.spec.ts
📚 Learning: 2026-02-04T19:29:54.650Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.Electron PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-04T19:29:54.650Z
Learning: Applies to **/*.{ts,tsx} : All code must pass ESLint and TypeScript checks
Applied to files:
src/downloads/integration.spec.ts
🧬 Code graph analysis (1)
src/downloads/integration.spec.ts (4)
src/downloads/main/setup.ts (1)
setupElectronDlWithTracking(46-116)src/downloads/main.ts (1)
setupDownloads(103-242)src/store/index.ts (1)
dispatch(38-41)src/downloads/actions.ts (1)
DOWNLOADS_CLEARED(5-5)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: check (ubuntu-latest)
- GitHub Check: check (windows-latest)
- GitHub Check: check (macos-latest)
🔇 Additional comments (4)
src/downloads/integration.spec.ts (4)
74-103: LGTM — mock factories correctly extended.Both
setSaveDialogOptionsandisDestroyedadditions map exactly to the new paths insetupElectronDlWithTracking(item.setSaveDialogOptions(...)and thewc.isDestroyed()guard loop insrc/downloads/main/setup.ts).
105-204: LGTM — lifecycle test is well-structured.The
onStartedCallback!(mockItem)→updatedListener!()→doneListener!()progression correctly exercisesDOWNLOAD_CREATED→DOWNLOAD_UPDATED→ completion. The assertions are accurate.One implicit assumption worth noting:
dispatchis asserted synchronously afteronStartedCallback!(mockItem)(line 167), which works becausehandleWillDownloadEventis invoked fire-and-forget (.catch(...)) insideonStarted, but dispatches synchronously in its own preamble before any internalawait. If that ever changes (e.g., anawaitis added beforedispatchinhandleWillDownloadEvent), this assertion would silently start passing vacuously or become non-deterministic. Consider adding a comment to that effect for future maintainers.
345-393: LGTM — edge case coverage is solid.No-webContents and destroyed-webContents paths are cleanly tested. Using a plain
() => true(rather thanjest.fn()) forisDestroyedon the destroyed mock is fine since the test doesn't assert it was called.
474-513: LGTM — cleanup path correctly exercisesgetState→completedtransition.The
mockItem.getState.mockReturnValue('completed')setup beforedoneListener!()(lines 499–502) ensures the done handler sees the completed state, which is the path that triggers cleanup logic. Good addition.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/downloads/integration.spec.ts`:
- Around line 1-16: This file contains only main-process tests
(handleWillDownloadEvent, setupDownloads, setupElectronDlWithTracking, IPC main
handlers and electron-dl), so rename the test file from integration.spec.ts to
integration.main.spec.ts to follow the main/renderer naming convention; after
renaming, run tests and, if your jest.config.js testMatch currently only matches
*.spec.ts in module folders, update testMatch to include *.main.spec.ts so the
renamed file is discovered.
---
Nitpick comments:
In `@src/downloads/integration.spec.ts`:
- Around line 231-234: The test repeats the cast + mock.calls.find pattern to
extract IPC handlers from the mocked handle function; create a small typed
helper (e.g., getIpcHandler) that takes the channel string and returns the
handler typed as the same type used in the cast (jest.MockedFunction<typeof
handle>) so callers like showInFolderHandler can be retrieved via
getIpcHandler('downloads/show-in-folder'); update all occurrences (the blocks
using handle as jest.MockedFunction<typeof handle> and mock.calls.find) to use
this helper to remove duplication and centralize the type cast.
- Around line 68-72: The call to createMainReduxStore() in beforeEach is a no-op
because createMainReduxStore is fully mocked; remove that call from beforeEach
to avoid confusing dead setup, unless you need to exercise the
uninitialized-store sentinel in handleWillDownloadEvent or setupDownloads—in
that case, restore a real store by replacing the mock with the actual
implementation (or a minimal real store) before the test(s) that need it so
dispatch and select behave correctly; reference createMainReduxStore,
handleWillDownloadEvent, setupDownloads, dispatch and select when making the
change.
1 participant
4 test files were not being executed by Jest because they were located directly in module folders rather than subdirectories.
The Jest config patterns only matched tests in subdirectories or files with special names (
main.spec.ts,renderer.spec.ts).Files not running:
src/outlookCalendar/getOutlookEvents.spec.tssrc/downloads/actions.spec.tssrc/downloads/notifications.spec.tssrc/downloads/integration.spec.tsFew tests were failing after including them,
Solution
Updated
testMatchpatterns injest.config.jsto include test files directly in module folders:Closes #3208
Summary by CodeRabbit