test: add coverage specs across preload and modules#3393
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds and updates Jest coverage across app persistence, IPC and deep-link flows, downloads, i18n and logging, communication features, servers preload bridges, and UI/system utilities. It also exports three dialog reducers and extends Jest test discovery. ChangesApp state, reducers, and dev helpers
Store and IPC helpers
Deep links, navigation, and downloads
i18n, logging, and utility timing
Communication feature tests
Servers preload bridge and common utility tests
System certificates, UI, and miscellaneous utilities
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/spellChecking/main.spec.ts (1)
1-46: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winTest doesn't exercise the language-filtering logic it claims to cover.
listenanddispatchare both no-op mocks (Lines 1-5), so callingdispatch(...)in "handles invalid languages" (Lines 36-45) never triggers the callback registered bysetupSpellCheckingvialisten. The test has no assertions and doesn't verifysession.defaultSession.setSpellCheckerLanguageswas called with the filtered list, so it provides no real coverage of the invalid-language filtering path described insrc/spellChecking/main.ts.✅ Proposed fix to actually exercise the listener callback
it('handles invalid languages', async () => { await setupSpellChecking(); - dispatch({ - type: SPELL_CHECKING_LANGUAGE_TOGGLED, - payload: { - name: 'wtf', - enabled: true, - }, - }); + const { listen } = require('../store'); + const languageToggledCall = (listen as jest.Mock).mock.calls.find( + ([type]) => type === SPELL_CHECKING_LANGUAGE_TOGGLED + ); + const [, callback] = languageToggledCall; + + callback({ + type: SPELL_CHECKING_LANGUAGE_TOGGLED, + payload: { name: 'wtf', enabled: true }, + }); + + const { session } = require('electron'); + expect( + session.defaultSession.setSpellCheckerLanguages + ).toHaveBeenCalledWith([]); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/spellChecking/main.spec.ts` around lines 1 - 46, The “handles invalid languages” test is not exercising the listener path because `dispatch` and `listen` are mocked as no-ops, so `setupSpellChecking` never processes the toggle event. Update the spec to capture and invoke the callback registered through `listen` (or otherwise use a real/mock store flow) and assert that `session.defaultSession.setSpellCheckerLanguages` receives only the filtered valid languages. Use `setupSpellChecking`, `listen`, `dispatch`, and `setSpellCheckerLanguages` to locate the affected test and verify the invalid-language case with an explicit expectation.
🧹 Nitpick comments (15)
src/downloads/integration.spec.ts (2)
291-326: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
jest.spyOnforDate.nowmocking.Manually saving/restoring
Date.now(lines 291, 294, 325) bypasses Jest's mock lifecycle; if an assertion throws before line 325, the originalDate.nowis never restored and leaks into subsequent tests.♻️ Suggested refactor
- const fixedNow = Date.now; - const downloadId = 1783398156938; - const mockEvent = createMockEvent(); - Date.now = jest.fn(() => downloadId); + const downloadId = 1783398156938; + const mockEvent = createMockEvent(); + const dateNowSpy = jest.spyOn(Date, 'now').mockReturnValue(downloadId);and at the end:
- Date.now = fixedNow; + dateNowSpy.mockRestore();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/downloads/integration.spec.ts` around lines 291 - 326, The download integration test is manually reassigning Date.now and restoring it later, which can leak state if the test fails early. Update the test in handleWillDownloadEvent to use jest.spyOn(Date, 'now') with a mock return value for the downloadId, and restore the spy in the proper Jest cleanup path so the mocked Date.now is always reset even on assertion failures.
318-320: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMisleading
isPausedstub in resume test.Setting
mockItem.isPaused.mockReturnValue(true)before invokingresumeHandlerdoesn't reflect the actual guard: persrc/downloads/main.ts,downloads/resumeonly checksitem.canResume(), notisPaused(). This stub gives a false impression that "paused state" is being verified, when onlycanResume(set at line 303) drives the assertion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/downloads/integration.spec.ts` around lines 318 - 320, The resume integration test is stubbing mockItem.isPaused even though downloads/resume in main.ts only gates on item.canResume(), so the test is misleading. Update the test around resumeHandler and mockItem to rely on canResume as the sole resumeability signal, and remove the unnecessary isPaused stub so the assertion matches the actual behavior being exercised.src/jitsi/__tests__/main.spec.ts (1)
21-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnnecessary
require()given no module reset is used here.Unlike
ipc.spec.ts/preload.spec.ts, this file doesn't calljest.resetModules(), so there's no need to defer loading viarequire()— jest hoistsjest.mock()calls automatically, so a static ESimportwould work identically and be more idiomatic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/jitsi/__tests__/main.spec.ts` around lines 21 - 22, The test file is using deferred loading with require() even though it never calls jest.resetModules(), so the imports can be made static. Update main.spec.ts to use normal ES import statements for isJitsiServerAllowed from main and JITSI_SERVER_CAPTURE_SCREEN_PERMISSION_UPDATED from actions, since jest.mock() is already hoisted and will behave the same. Keep the rest of the test setup unchanged.src/notifications/__tests__/renderer.spec.ts (1)
68-81: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueModule-level
iconCachepersists across tests without reset.
iconCacheinsrc/notifications/renderer.tsis a module-scopedMapthat isn't cleared between tests in this file. The current tests avoid collisions by using distinct URLs per test, but this makes the suite fragile to future additions that reuse a URL.Consider using
jest.resetModules()+ re-require-ing the module per test (as done insrc/jitsi/__tests__/ipc.spec.ts) to guarantee isolation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/notifications/__tests__/renderer.spec.ts` around lines 68 - 81, The shared module-level iconCache in renderer testing is leaking state between cases, making repeated URL tests fragile. Update src/notifications/__tests__/renderer.spec.ts to load src/notifications/renderer.ts in an isolated module context per test, using jest.resetModules() and re-requiring the module (as in ipc.spec.ts), so initNotificationsRenderer and the mockHandle handler each see a fresh cache and tests don’t depend on unique URLs.src/outlookCalendar/__tests__/logger.spec.ts (2)
42-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest doesn't verify the flags are actually set from selector values.
This test only asserts
select/watchcall counts; it never checks thatglobal.isVerboseOutlookLoggingEnabled/global.isDetailedEventsLoggingEnabledend up matching the mockedtrue/falsereturn values, despite the test name claiming to verify flags are "set ... from store selectors."♻️ Proposed strengthening
it('sets log flags during setup from store selectors', () => { (select as jest.Mock).mockReturnValueOnce(true).mockReturnValueOnce(false); setupOutlookLogger(); expect(select).toHaveBeenCalledTimes(2); expect(watch).toHaveBeenCalledTimes(2); + expect(global.isVerboseOutlookLoggingEnabled).toBe(true); + expect(global.isDetailedEventsLoggingEnabled).toBe(false); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/outlookCalendar/__tests__/logger.spec.ts` around lines 42 - 49, The setupOutlookLogger test only checks select and watch call counts, but does not verify that the mocked selector values are applied to the global flags. Update the test to assert that setupOutlookLogger sets global.isVerboseOutlookLoggingEnabled and global.isDetailedEventsLoggingEnabled to match the true/false values returned by the mocked select calls, while keeping the existing setupOutlookLogger, select, and watch expectations.
61-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWeak assertions don't verify each logging function individually.
toHaveBeenCalled()on shared spies doesn't confirmoutlookLog,outlookInfo,outlookDebug, andoutlookEventDetaileach actually produce output — a bug in any single function would go undetected as long as at least one of the others firesconsole.info.♻️ Proposed strengthening
- outlookLog('verbose message'); - outlookInfo('verbose info'); - outlookDebug('verbose debug'); - outlookEventDetail('detail'); - outlookWarn('warn msg'); - expect(consoleInfoSpy).toHaveBeenCalled(); - expect(consoleWarnSpy).toHaveBeenCalled(); + outlookLog('verbose message'); + expect(consoleInfoSpy).toHaveBeenCalledTimes(1); + outlookInfo('verbose info'); + expect(consoleInfoSpy).toHaveBeenCalledTimes(2); + outlookDebug('verbose debug'); + expect(consoleInfoSpy).toHaveBeenCalledTimes(3); + outlookEventDetail('detail'); + expect(consoleInfoSpy).toHaveBeenCalledTimes(4); + outlookWarn('warn msg'); + expect(consoleWarnSpy).toHaveBeenCalledTimes(1);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/outlookCalendar/__tests__/logger.spec.ts` around lines 61 - 67, The logger spec uses shared spies with broad toHaveBeenCalled assertions, so it does not verify each of the logging helpers individually. Update the assertions around outlookLog, outlookInfo, outlookDebug, and outlookEventDetail in logger.spec.ts so each function is checked against the appropriate console spy with its expected call, rather than relying on a single shared invocation; keep outlookWarn validated separately as well.src/logging/fallback.spec.ts (1)
4-52: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winLeaked
mockImplementationacross tests due toclearAllMocks().
jest.clearAllMocks()clears call history but not mock implementations. The throwing implementation set in the "swallows any internal fallback logging errors" test (line 47-49) persists into later tests since it's never restored. Current assertions still pass only because Jest records call args before invoking the implementation andfallbackLogswallows the error internally — this is fragile and could produce confusing cross-test failures if that internal behavior changes.♻️ Proposed fix to isolate the throwing implementation
it('swallows any internal fallback logging errors', () => { writeSpy.mockImplementation(() => { throw new Error('stderr failed'); }); expect(() => fallbackLog('error', 'boom')).not.toThrow(); + writeSpy.mockRestore(); + jest.spyOn(process.stderr, 'write'); });Alternatively, replace
jest.clearAllMocks()inbeforeEachwithjest.restoreAllMocks()/re-spy per test to guarantee a clean default implementation each run.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/logging/fallback.spec.ts` around lines 4 - 52, The fallbackLog spec is leaking the throwing stderr mock implementation between tests because beforeEach only calls jest.clearAllMocks(), which does not restore spy implementations. Update the test setup around writeSpy/fallbackLog so each test starts from a clean spy state by restoring all mocks or recreating the process.stderr.write spy per test, ensuring the "swallows any internal fallback logging errors" override does not affect later assertions.src/servers/preload/__tests__/sidebar.spec.ts (1)
215-256: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid
String['match']in type position; usetypeof String.prototype.matchinstead.Static analysis flags
Parameters<String['match']>[0]for usingStringas a type. Since the intent is to derive the parameter type ofString.prototype.match,typeof String.prototype.matchachieves the same without referencing the capitalizedStringinterface directly.🔧 Proposed fix
jest.spyOn(String.prototype, 'match').mockImplementation(function ( this: string, - matcher: Parameters<String['match']>[0] + matcher: Parameters<typeof String.prototype.match>[0] ) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/servers/preload/__tests__/sidebar.spec.ts` around lines 215 - 256, The test setup in the String.prototype.match spy is using a capitalized String type in a type position, which triggers static analysis. Update the matcher parameter type in the mocked match implementation to derive from typeof String.prototype.match instead of Parameters<String['match']>, keeping the rest of the fallback parsing test in sidebar.spec.ts unchanged.Source: Linters/SAST tools
src/servers/preload/__tests__/window.spec.ts (2)
143-145: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueWeak assertion for theme appearance test.
This test only asserts the call doesn't throw; it doesn't verify any dispatched action or state change, so it wouldn't catch a functional regression in
setUserThemeAppearance.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/servers/preload/__tests__/window.spec.ts` around lines 143 - 145, The `setUserThemeAppearance` test only checks that the call does not throw, so it misses regressions in actual behavior. Strengthen the spec in `window.spec.ts` by asserting the observable outcome of `setUserThemeAppearance` for the no-op API shape, such as the dispatched action or resulting state change, using the existing test setup around `setUserThemeAppearance` and the theme appearance mocks.
51-141: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSplit the duplicated preload coverage out of
window.spec.tsThetitle,uniqueID,badge,documentViewer,gitCommitHash,reloadServer, ande2ePdfPreviewSizeLimitcases are already covered by dedicated specs, so keeping them here adds maintenance overhead without extra coverage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/servers/preload/__tests__/window.spec.ts` around lines 51 - 141, The added preload tests in window.spec.ts duplicate coverage already present in the dedicated specs for reloadServer, setBadge, setGitCommitHash, setWorkspaceUID, setTitle, openDocumentViewer, supportedDocumentViewerFormats, and getE2ePdfPreviewSizeLimit. Remove these overlapping cases from window.spec.ts and keep only the unique assertions there so coverage stays centralized and maintenance is reduced.src/servers/preload/__tests__/utils.spec.ts (1)
23-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUntyped
require-based mocks diverge from the typed pattern used elsewhere.Sibling specs (
title.spec.ts,version.spec.ts,userLoggedIn.spec.ts) import with ESimportand cast mocks viajest.MockedFunction. This file usesrequire()and untyped destructuring, losing type-checking on mock call signatures.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/servers/preload/__tests__/utils.spec.ts` around lines 23 - 34, The test setup in utils.spec.ts is using untyped require-based imports for Electron and local modules, which breaks the typed mocking pattern used in sibling specs. Update the top-level imports in this spec to use ES import statements, then cast the mocked functions with jest.MockedFunction where they are asserted or configured so the call signatures stay type-safe. Keep the changes aligned with the existing mock usage around clipboard, ipcRenderer, dispatch, getServerUrl, clearUserRoles, updateUserRoles, writeTextToClipboard, setVersion, setUserLoggedIn, openInBrowser, WEBVIEW_SERVER_VERSION_UPDATED, and WEBVIEW_USER_LOGGED_IN.src/deepLinks/main.spec.ts (1)
742-1144: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract a helper to save/restore
process.argvsafely.Nearly every test in this suite mutates
process.argv, callsawait processDeepLinksInArgs(), then restoresprocess.argvmanually (e.g. lines 756-765, 778-787, 805-814, 837-846, 879-888, 927-936, 1029-1038, 1078-1087, 1128-1137). IfprocessDeepLinksInArgs()throws or an assertion between the mutation and restoration fails,process.argvleaks into subsequent tests, causing unrelated cascading failures.Consider a small helper to guarantee restoration:
♻️ Proposed helper
+const withArgv = async (argv: string[], fn: () => Promise<void>) => { + const savedArgv = process.argv; + process.argv = argv; + try { + await fn(); + } finally { + process.argv = savedArgv; + } +};Usage:
- const savedArgv = process.argv; - process.argv = [ - 'electron', - '.', - 'rocketchat://auth?host=https://chat.example.com&token=abc&userId=123', - ]; - - await processDeepLinksInArgs(); - - process.argv = savedArgv; - - expect(resolveServerUrlMock).toHaveBeenCalledWith(...); + await withArgv( + [ + 'electron', + '.', + 'rocketchat://auth?host=https://chat.example.com&token=abc&userId=123', + ], + processDeepLinksInArgs + ); + + expect(resolveServerUrlMock).toHaveBeenCalledWith(...);Aside from this, the new rocketchat:// suite (auth/room/invite/conference paths, server-addition approve/decline, invalid-URL warning) gives solid coverage and correctly matches
processDeepLinksInArgs/askForServerAddition/warnAboutInvalidServerUrlcontracts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/deepLinks/main.spec.ts` around lines 742 - 1144, The deep link spec mutates process.argv in many tests and restores it manually, which can leak state if processDeepLinksInArgs throws or an assertion fails. Add a small helper around the shared process.argv save/mutate/restore flow and use it in the rocketchat:// tests so restoration is guaranteed; update the repeated test blocks that call processDeepLinksInArgs, especially the ones covering auth, room, invite, conference, and invalid-URL warning paths. Keep the helper localized to this suite so the existing processDeepLinksInArgs, askForServerAddition, and warnAboutInvalidServerUrl coverage stays unchanged.src/ui/main/__tests__/mediaPermissions.spec.ts (1)
6-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestore
process.platformto the original value
resetPlatformshould restore the platform captured at module load instead of forcing'darwin'. As written, this leaves the Jest worker mutated for later spec files in the same process.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/main/__tests__/mediaPermissions.spec.ts` around lines 6 - 15, The platform reset helper is hardcoding process.platform to darwin instead of restoring the original value captured at module load, which can leak state into later Jest specs. Update resetPlatform in mediaPermissions.spec.ts to restore the saved original platform value using the same setProcessPlatform helper, and keep the module-level capture as the source of truth.src/ipc/main.spec.ts (1)
22-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove dead conditional branch; assert exact
webContents.sendargs.Per the
invokeimplementation,webContents.send(channel, id, ...args)is always called withchannel, id, a, b— 4 arguments. ThesentArgs.length === 3branch (Lines 38-40) is unreachable dead code, and theelsethrow is also unreachable. This conditional weakens the test's precision instead of asserting the actual known contract.♻️ Proposed fix to assert the exact call signature
const promise = invoke(webContents, 'channels/ping' as any, 'a', 'b'); const sentArgs = webContents.send.mock.calls[0]; - expect(sentArgs[0]).toContain('channels/ping'); - - if (sentArgs.length === 4) { - expect(sentArgs[1]).toMatch(/^[0-9a-f]+$/); - expect(sentArgs[2]).toBe('a'); - expect(sentArgs[3]).toBe('b'); - } else if (sentArgs.length === 3) { - expect(sentArgs[1]).toBe('a'); - expect(sentArgs[2]).toBe('b'); - } else { - throw new Error('Unexpected send args'); - } + expect(sentArgs[0]).toBe('channels/ping'); + expect(sentArgs[1]).toMatch(/^[0-9a-f]+$/); + expect(sentArgs[2]).toBe('a'); + expect(sentArgs[3]).toBe('b'); await expect(promise).resolves.toBe('ok');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ipc/main.spec.ts` around lines 22 - 47, The test for invoke in ipcMain.spec.ts contains an unreachable conditional around webContents.send that weakens the assertion. Update the spec for invoke and webContents.send to assert the exact call signature always used by the implementation: channel, correlation id, and the two payload args, and remove the 3-arg branch and the fallback throw so the test verifies the known contract precisely.src/ui/reducers/__tests__/stateGroups.spec.ts (1)
553-630: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate coverage with
featureFlags.spec.ts.
isMinimizeOnCloseEnabled,e2ePdfPreviewSizeLimit, andhasHideOnTrayNotificationShownare independently re-tested here and also insrc/ui/reducers/__tests__/featureFlags.spec.ts(lines 448-554), with overlapping scenarios (defaults,APP_SETTINGS_LOADEDoverrides, action updates). Consider consolidating each reducer's tests into a single file to avoid duplicate maintenance.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/reducers/__tests__/stateGroups.spec.ts` around lines 553 - 630, The tests for isMinimizeOnCloseEnabled, e2ePdfPreviewSizeLimit, and hasHideOnTrayNotificationShown are duplicated across this spec and featureFlags.spec.ts; consolidate the overlapping coverage into a single test file and remove the redundant cases here or in featureFlags.spec.ts. Keep the shared reducer behavior checks (defaults, APP_SETTINGS_LOADED handling, and action updates) under one canonical spec for each helper to avoid duplicate maintenance.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/__tests__/PersistableValues.spec.ts`:
- Around line 215-240: The test variables in PersistableValues.spec.ts violate
the camelCase lint rule because their names include underscore-separated numeric
suffixes. Rename `mas38_4` and the `from...` migration variables in the affected
migration tests to proper camelCase forms, and update each corresponding
reference in the same test block so CI passes without naming-lint errors.
- Line 2: The import order in PersistableValues.spec.ts is failing lint because
the `../../constants` import must come before the `../PersistableValues` import.
Reorder the imports at the top of the test file so the shared constants are
imported first, then keep the `PersistableValues` import after it; use the
existing import statements to locate the block quickly.
- Around line 17-37: The test helper currently hardcodes process.platform to
darwin in clearProcessPlatform, which can leak into later specs when
PersistableValues.spec.ts runs in the same Jest worker. Update the helper to
save the original process.platform descriptor/value before mocking it, and
restore that original state in afterEach instead of forcing darwin. Keep
setProcessMas unchanged, and make sure the restore logic is tied to the existing
clearProcessPlatform helper used by the describe block.
In `@src/app/main/dev.spec.ts`:
- Around line 23-24: The chokidar mocks used by the dev spec are currently
declared with names that are hoisted into the `jest.mock('chokidar', ...)`
factory before initialization, which can trigger TDZ issues when `./dev` imports
chokidar. Rename the `watchMock` and `watchOnMock` variables in
`src/app/main/dev.spec.ts` to `mockWatch` and `mockWatchOn`, or move their
definitions inside the mock factory, and update any references in the
`jest.mock` setup accordingly.
In `@src/navigation/main.spec.ts`:
- Around line 424-455: The test for the certificate-error flow is asserting the
wrong value for the second argument passed to askForCertificateTrustMock. In the
branch handled by certificate-error listener, the code formats the message
through t('error.differentCertificate', { detail }) before prompting, so update
the expectation in main.spec.ts to match the formatted detail string instead of
undefined, using the certificateErrorListener and askForCertificateTrustMock
symbols to locate the assertion.
In `@src/servers/preload/__tests__/internalVideoChatWindow.spec.ts`:
- Around line 29-52: The test cleanup in internalVideoChatWindow.spec.ts leaves
a mocked process.mas behind when there is no original descriptor. Update the
afterEach path around processMasDescriptor and setProcessMas so that the
synthetic process.mas property is removed entirely when it was created, instead
of only restoring conditionally; keep the existing restore behavior when a real
descriptor exists.
In `@src/servers/preload/__tests__/utils.spec.ts`:
- Around line 82-120: Remove the duplicated coverage from the `utils.spec.ts`
tests: the `setVersion` and `setUserLoggedIn` assertions are already covered in
`version.spec.ts` and `userLoggedIn.spec.ts`. Keep `utils.spec.ts` focused on
unique behavior by deleting those redundant `it(...)` blocks and retaining only
tests that exercise additional logic not already verified elsewhere.
In `@src/ui/main/dialogs.spec.ts`:
- Around line 256-272: Restore the global platform override used in the darwin
and win32 cases inside dialogs.spec.ts: the tests around
showMicrophonePermissionDeniedMessage currently redefine process.platform via
Object.defineProperty without putting it back, which can leak into later tests.
Save the original process.platform before overriding it in the platform-specific
tests, then restore it in afterEach or a try/finally block so the rest of the
suite runs with the correct platform.
In `@src/ui/main/icons.spec.ts`:
- Around line 11-15: The getAppIconPath test is using path.join, which can
normalize separators differently from the implementation’s forward-slash string.
Update the assertion in getAppIconPath to compare against the same literal
forward-slash path returned by the function, using the existing getAppIconPath
symbol in icons.spec.ts.
In `@src/ui/main/serverView/popupMenu.spec.ts`:
- Around line 51-56: The test helper setProcessPlatform mutates the global
process.platform and leaves it changed for later specs. Capture the original
platform value in popupMenu.spec.ts once, then restore it in afterEach or
afterAll so each test using setProcessPlatform runs with the expected platform
state. Use the existing setProcessPlatform helper and the affected
popupMenu.spec.ts test cases to place the restore logic.
In `@src/ui/reducers/__tests__/featureFlags.spec.ts`:
- Around line 513-554: The isMinimizeOnCloseEnabled tests mutate the shared
process.platform via setPlatform without restoring it, which can leak platform
state into later specs. Capture the original platform in describe and add an
afterEach (or equivalent teardown) to restore process.platform after each test,
keeping the behavior isolated while still using setPlatform,
APP_SETTINGS_LOADED, and isMinimizeOnCloseEnabled as the main test points.
---
Outside diff comments:
In `@src/spellChecking/main.spec.ts`:
- Around line 1-46: The “handles invalid languages” test is not exercising the
listener path because `dispatch` and `listen` are mocked as no-ops, so
`setupSpellChecking` never processes the toggle event. Update the spec to
capture and invoke the callback registered through `listen` (or otherwise use a
real/mock store flow) and assert that
`session.defaultSession.setSpellCheckerLanguages` receives only the filtered
valid languages. Use `setupSpellChecking`, `listen`, `dispatch`, and
`setSpellCheckerLanguages` to locate the affected test and verify the
invalid-language case with an explicit expectation.
---
Nitpick comments:
In `@src/deepLinks/main.spec.ts`:
- Around line 742-1144: The deep link spec mutates process.argv in many tests
and restores it manually, which can leak state if processDeepLinksInArgs throws
or an assertion fails. Add a small helper around the shared process.argv
save/mutate/restore flow and use it in the rocketchat:// tests so restoration is
guaranteed; update the repeated test blocks that call processDeepLinksInArgs,
especially the ones covering auth, room, invite, conference, and invalid-URL
warning paths. Keep the helper localized to this suite so the existing
processDeepLinksInArgs, askForServerAddition, and warnAboutInvalidServerUrl
coverage stays unchanged.
In `@src/downloads/integration.spec.ts`:
- Around line 291-326: The download integration test is manually reassigning
Date.now and restoring it later, which can leak state if the test fails early.
Update the test in handleWillDownloadEvent to use jest.spyOn(Date, 'now') with a
mock return value for the downloadId, and restore the spy in the proper Jest
cleanup path so the mocked Date.now is always reset even on assertion failures.
- Around line 318-320: The resume integration test is stubbing mockItem.isPaused
even though downloads/resume in main.ts only gates on item.canResume(), so the
test is misleading. Update the test around resumeHandler and mockItem to rely on
canResume as the sole resumeability signal, and remove the unnecessary isPaused
stub so the assertion matches the actual behavior being exercised.
In `@src/ipc/main.spec.ts`:
- Around line 22-47: The test for invoke in ipcMain.spec.ts contains an
unreachable conditional around webContents.send that weakens the assertion.
Update the spec for invoke and webContents.send to assert the exact call
signature always used by the implementation: channel, correlation id, and the
two payload args, and remove the 3-arg branch and the fallback throw so the test
verifies the known contract precisely.
In `@src/jitsi/__tests__/main.spec.ts`:
- Around line 21-22: The test file is using deferred loading with require() even
though it never calls jest.resetModules(), so the imports can be made static.
Update main.spec.ts to use normal ES import statements for isJitsiServerAllowed
from main and JITSI_SERVER_CAPTURE_SCREEN_PERMISSION_UPDATED from actions, since
jest.mock() is already hoisted and will behave the same. Keep the rest of the
test setup unchanged.
In `@src/logging/fallback.spec.ts`:
- Around line 4-52: The fallbackLog spec is leaking the throwing stderr mock
implementation between tests because beforeEach only calls jest.clearAllMocks(),
which does not restore spy implementations. Update the test setup around
writeSpy/fallbackLog so each test starts from a clean spy state by restoring all
mocks or recreating the process.stderr.write spy per test, ensuring the
"swallows any internal fallback logging errors" override does not affect later
assertions.
In `@src/notifications/__tests__/renderer.spec.ts`:
- Around line 68-81: The shared module-level iconCache in renderer testing is
leaking state between cases, making repeated URL tests fragile. Update
src/notifications/__tests__/renderer.spec.ts to load
src/notifications/renderer.ts in an isolated module context per test, using
jest.resetModules() and re-requiring the module (as in ipc.spec.ts), so
initNotificationsRenderer and the mockHandle handler each see a fresh cache and
tests don’t depend on unique URLs.
In `@src/outlookCalendar/__tests__/logger.spec.ts`:
- Around line 42-49: The setupOutlookLogger test only checks select and watch
call counts, but does not verify that the mocked selector values are applied to
the global flags. Update the test to assert that setupOutlookLogger sets
global.isVerboseOutlookLoggingEnabled and global.isDetailedEventsLoggingEnabled
to match the true/false values returned by the mocked select calls, while
keeping the existing setupOutlookLogger, select, and watch expectations.
- Around line 61-67: The logger spec uses shared spies with broad
toHaveBeenCalled assertions, so it does not verify each of the logging helpers
individually. Update the assertions around outlookLog, outlookInfo,
outlookDebug, and outlookEventDetail in logger.spec.ts so each function is
checked against the appropriate console spy with its expected call, rather than
relying on a single shared invocation; keep outlookWarn validated separately as
well.
In `@src/servers/preload/__tests__/sidebar.spec.ts`:
- Around line 215-256: The test setup in the String.prototype.match spy is using
a capitalized String type in a type position, which triggers static analysis.
Update the matcher parameter type in the mocked match implementation to derive
from typeof String.prototype.match instead of Parameters<String['match']>,
keeping the rest of the fallback parsing test in sidebar.spec.ts unchanged.
In `@src/servers/preload/__tests__/utils.spec.ts`:
- Around line 23-34: The test setup in utils.spec.ts is using untyped
require-based imports for Electron and local modules, which breaks the typed
mocking pattern used in sibling specs. Update the top-level imports in this spec
to use ES import statements, then cast the mocked functions with
jest.MockedFunction where they are asserted or configured so the call signatures
stay type-safe. Keep the changes aligned with the existing mock usage around
clipboard, ipcRenderer, dispatch, getServerUrl, clearUserRoles, updateUserRoles,
writeTextToClipboard, setVersion, setUserLoggedIn, openInBrowser,
WEBVIEW_SERVER_VERSION_UPDATED, and WEBVIEW_USER_LOGGED_IN.
In `@src/servers/preload/__tests__/window.spec.ts`:
- Around line 143-145: The `setUserThemeAppearance` test only checks that the
call does not throw, so it misses regressions in actual behavior. Strengthen the
spec in `window.spec.ts` by asserting the observable outcome of
`setUserThemeAppearance` for the no-op API shape, such as the dispatched action
or resulting state change, using the existing test setup around
`setUserThemeAppearance` and the theme appearance mocks.
- Around line 51-141: The added preload tests in window.spec.ts duplicate
coverage already present in the dedicated specs for reloadServer, setBadge,
setGitCommitHash, setWorkspaceUID, setTitle, openDocumentViewer,
supportedDocumentViewerFormats, and getE2ePdfPreviewSizeLimit. Remove these
overlapping cases from window.spec.ts and keep only the unique assertions there
so coverage stays centralized and maintenance is reduced.
In `@src/ui/main/__tests__/mediaPermissions.spec.ts`:
- Around line 6-15: The platform reset helper is hardcoding process.platform to
darwin instead of restoring the original value captured at module load, which
can leak state into later Jest specs. Update resetPlatform in
mediaPermissions.spec.ts to restore the saved original platform value using the
same setProcessPlatform helper, and keep the module-level capture as the source
of truth.
In `@src/ui/reducers/__tests__/stateGroups.spec.ts`:
- Around line 553-630: The tests for isMinimizeOnCloseEnabled,
e2ePdfPreviewSizeLimit, and hasHideOnTrayNotificationShown are duplicated across
this spec and featureFlags.spec.ts; consolidate the overlapping coverage into a
single test file and remove the redundant cases here or in featureFlags.spec.ts.
Keep the shared reducer behavior checks (defaults, APP_SETTINGS_LOADED handling,
and action updates) under one canonical spec for each helper to avoid duplicate
maintenance.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0f679fe5-c187-41ba-8d9d-fb004df829fa
📒 Files selected for processing (91)
src/app/__tests__/PersistableValues.spec.tssrc/app/__tests__/selectors.spec.tssrc/app/main/data.spec.tssrc/app/main/dev.spec.tssrc/app/reducers/__tests__/allowedNTLMCredentialsDomains.spec.tssrc/app/reducers/__tests__/appPath.spec.tssrc/app/reducers/__tests__/appVersion.spec.tssrc/app/reducers/__tests__/machineTheme.spec.tssrc/app/reducers/__tests__/mainWindowTitle.spec.tssrc/app/reducers/__tests__/screenCaptureFallbackForced.spec.tssrc/browser/__tests__/ipc.spec.tssrc/constants.spec.tssrc/deepLinks/main.spec.tssrc/downloads/integration.spec.tssrc/i18n/__tests__/actions.spec.tssrc/i18n/__tests__/main.spec.tssrc/ipc/main.spec.tssrc/jitsi/__tests__/ipc.spec.tssrc/jitsi/__tests__/main.spec.tssrc/jitsi/__tests__/preload.spec.tssrc/jitsi/__tests__/reducers.spec.tssrc/logViewerWindow/__tests__/constants.spec.tssrc/logViewerWindow/__tests__/types.spec.tssrc/logging/__tests__/preload.spec.tssrc/logging/cleanup.spec.tssrc/logging/fallback.spec.tssrc/logging/scopes.spec.tssrc/logging/utils.spec.tssrc/navigation/main.spec.tssrc/notifications/__tests__/actions.spec.tssrc/notifications/__tests__/renderer.spec.tssrc/outlookCalendar/__tests__/logger.spec.tssrc/screenSharing/__tests__/main.spec.tssrc/screenSharing/__tests__/preload.spec.tssrc/screenSharing/__tests__/screenRecordingPermission.spec.tssrc/screenSharing/screenPicker/__tests__/InternalPickerProvider.spec.tssrc/screenSharing/screenPicker/__tests__/PortalPickerProvider.spec.tssrc/screenSharing/screenRecordingPermission.spec.tssrc/servers/common.spec.tssrc/servers/preload/__tests__/api.spec.tssrc/servers/preload/__tests__/badge.spec.tssrc/servers/preload/__tests__/clipboard.spec.tssrc/servers/preload/__tests__/documentViewer.spec.tssrc/servers/preload/__tests__/e2ePdfPreviewSizeLimit.spec.tssrc/servers/preload/__tests__/favicon.spec.tssrc/servers/preload/__tests__/gitCommitHash.spec.tssrc/servers/preload/__tests__/internalVideoChatWindow.spec.tssrc/servers/preload/__tests__/openInBrowser.spec.tssrc/servers/preload/__tests__/reloadServer.spec.tssrc/servers/preload/__tests__/sidebar.spec.tssrc/servers/preload/__tests__/themeAppearance.spec.tssrc/servers/preload/__tests__/title.spec.tssrc/servers/preload/__tests__/uniqueID.spec.tssrc/servers/preload/__tests__/userLoggedIn.spec.tssrc/servers/preload/__tests__/utils.spec.tssrc/servers/preload/__tests__/version.spec.tssrc/servers/preload/__tests__/window.spec.tssrc/servers/preload/urls.spec.tssrc/servers/preload/userRoles-server-url.spec.tssrc/servers/preload/userRoles.spec.tssrc/spellChecking/main.spec.tssrc/store/__tests__/fsa.spec.tssrc/store/__tests__/index.spec.tssrc/store/__tests__/ipc.spec.tssrc/store/__tests__/readSetting.spec.tssrc/systemCertificates.spec.tssrc/telephony/__tests__/acceleratorDisplay.spec.tssrc/telephony/__tests__/ipc.spec.tssrc/telephony/__tests__/shortcuts.spec.tssrc/ui/components/SettingsView/__tests__/index.spec.tssrc/ui/components/__tests__/useServers.spec.tssrc/ui/components/utils/createAnchor.spec.tssrc/ui/icons/Badge.spec.tssrc/ui/main/__tests__/debounce.spec.tssrc/ui/main/__tests__/mediaPermissions.spec.tssrc/ui/main/dialogs.spec.tssrc/ui/main/icons.spec.tssrc/ui/main/serverView/popupMenu.spec.tssrc/ui/preload/__tests__/messageBox.spec.tssrc/ui/preload/__tests__/sidebar.spec.tssrc/ui/reducers/__tests__/featureFlags.spec.tssrc/ui/reducers/__tests__/stateGroups.spec.tssrc/ui/reducers/dialogs.tssrc/ui/selectors.spec.tssrc/ui/utils/__tests__/platform.spec.tssrc/updates/__tests__/common.spec.tssrc/userPresence/__tests__/actions.spec.tssrc/utils/__tests__/browserLauncher.spec.tssrc/utils/__tests__/index.spec.tssrc/utils/__tests__/versionUtils.spec.tssrc/whenReady.spec.ts
Comment on lines
+424
to
+455
| it('uses the different-certificate message when host was previously trusted', async () => { | ||
| const event = { preventDefault: jest.fn() }; | ||
| const callback = jest.fn(); | ||
| const certificate = makeCertificate({ | ||
| fingerprint: 'previous-cert', | ||
| issuerName: 'Test CA', | ||
| data: 'DATA', | ||
| }); | ||
| setNavigationState({ | ||
| trustedCertificates: { | ||
| 'server.local': 'different-certificate', | ||
| }, | ||
| }); | ||
| askForCertificateTrustMock.mockResolvedValue(AskForCertificateTrustResponse.NO); | ||
|
|
||
| await setup(); | ||
| const certificateErrorListener = getListener('certificate-error'); | ||
| await certificateErrorListener( | ||
| event as never, | ||
| {} as never, | ||
| 'https://server.local/path', | ||
| 'ERR', | ||
| certificate, | ||
| callback | ||
| ); | ||
|
|
||
| expect(askForCertificateTrustMock).toHaveBeenCalledWith( | ||
| certificate.issuerName, | ||
| undefined | ||
| ); | ||
| expect(callback).toHaveBeenCalledWith(false); | ||
| }); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check how `t`/i18n is mocked for src/navigation/main.spec.ts
rg -n "jest.mock\('.*i18n" -g '*.ts' src
rg -n "^import|from '.*i18n" src/navigation/main.spec.ts
rg -n "export const t " src -A3Repository: RocketChat/Rocket.Chat.Electron
Length of output: 1054
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the test file structure
ast-grep outline src/navigation/main.spec.ts --view expanded
# Read the relevant section around the cited lines
sed -n '1,220p' src/navigation/main.spec.ts
sed -n '220,520p' src/navigation/main.spec.ts
# Locate the certificate-error handler implementation and i18n usage
rg -n "certificate-error|differentCertificate|askForCertificateTrust|trustedCertificates\\[host\\]|\\bt\\(" src/navigation -A4 -B4Repository: RocketChat/Rocket.Chat.Electron
Length of output: 27293
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the navigation spec for i18n mocks and the relevant test context
sed -n '1,120p' src/navigation/main.spec.ts
rg -n "jest.mock\\('i18next'|setupNavigation|askForCertificateTrustMock|different-certificate|certificate-error" src/navigation/main.spec.ts -A6 -B6
# Inspect the implementation that sets the `detail` argument
FILE=$(rg -l "differentCertificate|askForCertificateTrust" src/navigation | head -n 1)
echo "Implementation file: $FILE"
sed -n '1,220p' "$FILE"Repository: RocketChat/Rocket.Chat.Electron
Length of output: 17097
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the implementation and the translation helper used by navigation
FILE=$(rg -l "differentCertificate|askForCertificateTrust" src/navigation src/i18n | head -n 1)
echo "Implementation file: $FILE"
sed -n '1,240p' "$FILE"
echo
echo "Navigation spec imports and i18n setup:"
sed -n '1,120p' src/navigation/main.spec.ts
echo
echo "Translation helper modules:"
rg -n "export .*t|i18next" src/i18n src/navigation -A4 -B4Repository: RocketChat/Rocket.Chat.Electron
Length of output: 22123
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find all related tests and the exact certificate handler code path
rg -n "askForCertificateTrustMock|error.differentCertificate|different-certificate" src -A4 -B4Repository: RocketChat/Rocket.Chat.Electron
Length of output: 6306
Expect a formatted detail string here, not undefined. This branch rewrites detail via t('error.differentCertificate', { detail }) before calling askForCertificateTrust, so the assertion should match that string.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/navigation/main.spec.ts` around lines 424 - 455, The test for the
certificate-error flow is asserting the wrong value for the second argument
passed to askForCertificateTrustMock. In the branch handled by certificate-error
listener, the code formats the message through t('error.differentCertificate', {
detail }) before prompting, so update the expectation in main.spec.ts to match
the formatted detail string instead of undefined, using the
certificateErrorListener and askForCertificateTrustMock symbols to locate the
assertion.
- Extend jest.config.js testMatch to cover top-level src/*.spec.ts files (whenReady, systemCertificates, constants) that were previously invisible to the test runner - Move ui/selectors.spec.ts into ui/__tests__/ for the same reason - Delete duplicate screenSharing/screenRecordingPermission.spec.ts (dead flat copy of the already-covered nested test) - Move jitsi/main.spec.ts, screenSharing/main.spec.ts, i18n/main.spec.ts out of __tests__/ to the flat <module>/main.spec.ts convention; the __tests__/main.spec.ts path double-matched both jest projects (renderer + main), causing the same suite to run twice concurrently - Fix TDZ ReferenceErrors in jitsi/main.spec.ts, screenSharing/main.spec.ts, i18n/main.spec.ts, browser/ipc.spec.ts, jitsi/preload.spec.ts, and servers/preload/window.spec.ts: ts-jest's hoist transformer moves every jest.mock() above all other statements unconditionally, so factories closing over an outer const always throw. Declare mocks inline in the factory and retrieve references via jest.requireMock/jest.mocked instead - Fix jitsi/ipc.spec.ts test isolation: the module under test held module-level state that leaked across tests because it was statically imported and jest.resetModules() was a no-op against it; switch to await import() per test - Tighten two weak toBeDefined() assertions in stateGroups.spec.ts to assert the actual default reducer state shape
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/ui/__tests__/selectors.spec.ts (1)
8-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor:
any[]weakens type safety in test helper.The
statehelper acceptsservers: any[]and force-casts throughas unknown as RootState. Consider typingserversasPartial<RootState['servers'][number]>[]for better type-checking of fixture shapes.As per coding guidelines, "Use TypeScript for all new code unless explicitly told otherwise."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/__tests__/selectors.spec.ts` around lines 8 - 9, The test helper state currently weakens type safety by accepting servers as any[] and force-casting to RootState. Update the state helper in selectors.spec.ts to use a typed servers parameter such as Partial<RootState['servers'][number]>[] and return a properly shaped RootState without the double cast, so fixtures are checked against the server item type.Source: Coding guidelines
src/screenSharing/main.spec.ts (2)
13-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOverly-permissive
isResponseTomock weakens the test's fidelity.The mock ignores
_id/_responsesand only checkstype !== 'OTHER', so it can't catch a bug wheresetupScreenSharingpasses the wrong request id or response set toisResponseTo.♻️ Suggested tightening
isResponseTo: jest.fn( - (_id: string, _responses: unknown[]) => (_action: { type: string }) => - _action.type !== 'OTHER' + (id: string, responses: unknown[]) => (action: { type: string; meta?: { id?: string } }) => + action.type !== 'OTHER' && action.meta?.id === id ),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/screenSharing/main.spec.ts` around lines 13 - 21, The isResponseTo mock in main.spec.ts is too permissive and does not verify the request id or responses passed by setupScreenSharing. Tighten the jest.mock for ../store/fsa so the mocked isResponseTo uses its _id and _responses inputs when deciding whether an action matches, and only returns true for the expected response tied to the provided id. Keep the hasMeta mock as-is, but make sure the assertions around setupScreenSharing exercise the real id/response matching behavior instead of only filtering out OTHER.
43-53: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePositional indexing into
mockListen.mock.callsis brittle.Grabbing listeners via fixed indices (
calls[0],calls[1][1]) assumes a stable registration order insetupScreenSharing. Filtering by the action-type argument eachlisten()call was registered with would make the test resilient to reordering.Also applies to: 73-83
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/screenSharing/main.spec.ts` around lines 43 - 53, The test is relying on brittle positional access into mockListen.mock.calls, which assumes a fixed listener registration order from setupScreenSharing. Update the affected specs to locate the needed listeners by inspecting the action-type argument passed to listen() instead of using calls[0] and calls[1][1], so the assertions stay stable even if registration order changes. Apply the same pattern in both affected blocks in main.spec.ts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/screenSharing/main.spec.ts`:
- Around line 13-21: The isResponseTo mock in main.spec.ts is too permissive and
does not verify the request id or responses passed by setupScreenSharing.
Tighten the jest.mock for ../store/fsa so the mocked isResponseTo uses its _id
and _responses inputs when deciding whether an action matches, and only returns
true for the expected response tied to the provided id. Keep the hasMeta mock
as-is, but make sure the assertions around setupScreenSharing exercise the real
id/response matching behavior instead of only filtering out OTHER.
- Around line 43-53: The test is relying on brittle positional access into
mockListen.mock.calls, which assumes a fixed listener registration order from
setupScreenSharing. Update the affected specs to locate the needed listeners by
inspecting the action-type argument passed to listen() instead of using calls[0]
and calls[1][1], so the assertions stay stable even if registration order
changes. Apply the same pattern in both affected blocks in main.spec.ts.
In `@src/ui/__tests__/selectors.spec.ts`:
- Around line 8-9: The test helper state currently weakens type safety by
accepting servers as any[] and force-casting to RootState. Update the state
helper in selectors.spec.ts to use a typed servers parameter such as
Partial<RootState['servers'][number]>[] and return a properly shaped RootState
without the double cast, so fixtures are checked against the server item type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aaf96450-6436-46d0-bcc9-26b778a09612
📒 Files selected for processing (65)
jest.config.jssrc/app/__tests__/PersistableValues.spec.tssrc/app/reducers/__tests__/allowedNTLMCredentialsDomains.spec.tssrc/browser/__tests__/ipc.spec.tssrc/deepLinks/main.spec.tssrc/downloads/integration.spec.tssrc/i18n/main.spec.tssrc/ipc/main.spec.tssrc/jitsi/__tests__/ipc.spec.tssrc/jitsi/__tests__/preload.spec.tssrc/jitsi/__tests__/reducers.spec.tssrc/jitsi/main.spec.tssrc/logging/__tests__/preload.spec.tssrc/logging/cleanup.spec.tssrc/logging/scopes.spec.tssrc/logging/utils.spec.tssrc/navigation/main.spec.tssrc/notifications/__tests__/actions.spec.tssrc/notifications/__tests__/renderer.spec.tssrc/outlookCalendar/__tests__/logger.spec.tssrc/screenSharing/__tests__/preload.spec.tssrc/screenSharing/__tests__/screenRecordingPermission.spec.tssrc/screenSharing/main.spec.tssrc/screenSharing/screenPicker/__tests__/InternalPickerProvider.spec.tssrc/screenSharing/screenPicker/__tests__/PortalPickerProvider.spec.tssrc/servers/common.spec.tssrc/servers/preload/__tests__/api.spec.tssrc/servers/preload/__tests__/badge.spec.tssrc/servers/preload/__tests__/clipboard.spec.tssrc/servers/preload/__tests__/documentViewer.spec.tssrc/servers/preload/__tests__/e2ePdfPreviewSizeLimit.spec.tssrc/servers/preload/__tests__/favicon.spec.tssrc/servers/preload/__tests__/gitCommitHash.spec.tssrc/servers/preload/__tests__/internalVideoChatWindow.spec.tssrc/servers/preload/__tests__/openInBrowser.spec.tssrc/servers/preload/__tests__/reloadServer.spec.tssrc/servers/preload/__tests__/sidebar.spec.tssrc/servers/preload/__tests__/title.spec.tssrc/servers/preload/__tests__/uniqueID.spec.tssrc/servers/preload/__tests__/utils.spec.tssrc/servers/preload/__tests__/version.spec.tssrc/servers/preload/__tests__/window.spec.tssrc/servers/preload/urls.spec.tssrc/servers/preload/userRoles-server-url.spec.tssrc/servers/preload/userRoles.spec.tssrc/spellChecking/main.spec.tssrc/store/__tests__/ipc.spec.tssrc/store/__tests__/readSetting.spec.tssrc/systemCertificates.spec.tssrc/telephony/__tests__/acceleratorDisplay.spec.tssrc/telephony/__tests__/ipc.spec.tssrc/ui/__tests__/selectors.spec.tssrc/ui/components/SettingsView/__tests__/index.spec.tssrc/ui/components/__tests__/useServers.spec.tssrc/ui/icons/Badge.spec.tssrc/ui/main/__tests__/mediaPermissions.spec.tssrc/ui/main/dialogs.spec.tssrc/ui/main/icons.spec.tssrc/ui/main/serverView/popupMenu.spec.tssrc/ui/preload/__tests__/messageBox.spec.tssrc/ui/preload/__tests__/sidebar.spec.tssrc/ui/reducers/__tests__/featureFlags.spec.tssrc/ui/reducers/__tests__/stateGroups.spec.tssrc/ui/utils/__tests__/platform.spec.tssrc/whenReady.spec.ts
💤 Files with no reviewable changes (1)
- src/servers/preload/tests/e2ePdfPreviewSizeLimit.spec.ts
🚧 Files skipped from review as they are similar to previous changes (59)
- src/servers/preload/tests/version.spec.ts
- src/servers/preload/urls.spec.ts
- src/store/tests/readSetting.spec.ts
- src/servers/preload/tests/gitCommitHash.spec.ts
- src/logging/utils.spec.ts
- src/notifications/tests/actions.spec.ts
- src/ui/components/tests/useServers.spec.ts
- src/ui/components/SettingsView/tests/index.spec.ts
- src/servers/preload/tests/uniqueID.spec.ts
- src/servers/preload/tests/openInBrowser.spec.ts
- src/servers/preload/tests/reloadServer.spec.ts
- src/screenSharing/screenPicker/tests/InternalPickerProvider.spec.ts
- src/whenReady.spec.ts
- src/ui/icons/Badge.spec.ts
- src/servers/preload/tests/documentViewer.spec.ts
- src/servers/common.spec.ts
- src/browser/tests/ipc.spec.ts
- src/telephony/tests/ipc.spec.ts
- src/screenSharing/tests/preload.spec.ts
- src/telephony/tests/acceleratorDisplay.spec.ts
- src/servers/preload/tests/badge.spec.ts
- src/spellChecking/main.spec.ts
- src/servers/preload/tests/title.spec.ts
- src/ui/preload/tests/sidebar.spec.ts
- src/jitsi/tests/preload.spec.ts
- src/servers/preload/tests/api.spec.ts
- src/servers/preload/userRoles-server-url.spec.ts
- src/ui/utils/tests/platform.spec.ts
- src/screenSharing/tests/screenRecordingPermission.spec.ts
- src/servers/preload/tests/clipboard.spec.ts
- src/jitsi/tests/reducers.spec.ts
- src/jitsi/tests/ipc.spec.ts
- src/logging/tests/preload.spec.ts
- src/app/reducers/tests/allowedNTLMCredentialsDomains.spec.ts
- src/screenSharing/screenPicker/tests/PortalPickerProvider.spec.ts
- src/logging/cleanup.spec.ts
- src/ui/preload/tests/messageBox.spec.ts
- src/servers/preload/tests/favicon.spec.ts
- src/logging/scopes.spec.ts
- src/servers/preload/tests/window.spec.ts
- src/outlookCalendar/tests/logger.spec.ts
- src/servers/preload/tests/internalVideoChatWindow.spec.ts
- src/deepLinks/main.spec.ts
- src/servers/preload/userRoles.spec.ts
- src/ui/main/dialogs.spec.ts
- src/notifications/tests/renderer.spec.ts
- src/ui/main/icons.spec.ts
- src/servers/preload/tests/utils.spec.ts
- src/servers/preload/tests/sidebar.spec.ts
- src/store/tests/ipc.spec.ts
- src/ui/main/serverView/popupMenu.spec.ts
- src/systemCertificates.spec.ts
- src/ui/reducers/tests/stateGroups.spec.ts
- src/ipc/main.spec.ts
- src/app/tests/PersistableValues.spec.ts
- src/downloads/integration.spec.ts
- src/ui/main/tests/mediaPermissions.spec.ts
- src/ui/reducers/tests/featureFlags.spec.ts
- src/navigation/main.spec.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: check (windows-latest)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Use TypeScript for all new code unless explicitly told otherwise
Use optional chaining with fallbacks for platform-specific APIs instead of mocking when possible. Example:const uid = process.getuid?.() ?? 1000;
Files:
src/ui/__tests__/selectors.spec.tssrc/jitsi/main.spec.tssrc/i18n/main.spec.tssrc/screenSharing/main.spec.ts
**/*.{tsx,ts}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{tsx,ts}: MANDATORY: Use Fuselage components for all UI work. Only create custom components when Fuselage doesn't provide what's needed
Import UI components from@rocket.chat/fuselageand checkTheme.d.tsfor valid color tokens
Use React functional components with hooks
Use PascalCase for component file names
Files:
src/ui/__tests__/selectors.spec.tssrc/jitsi/main.spec.tssrc/i18n/main.spec.tssrc/screenSharing/main.spec.ts
**/*.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
*.spec.tsfile naming for Renderer process tests
Files:
src/ui/__tests__/selectors.spec.tssrc/jitsi/main.spec.tssrc/i18n/main.spec.tssrc/screenSharing/main.spec.ts
**/*.{spec.ts,main.spec.ts}
📄 CodeRabbit inference engine (CLAUDE.md)
Only mock platform-specific APIs when defensive coding isn't possible. Linux-only APIs requiring mocks:
process.getuid(),process.getgid(),process.geteuid(),process.getegid()
Files:
src/ui/__tests__/selectors.spec.tssrc/jitsi/main.spec.tssrc/i18n/main.spec.tssrc/screenSharing/main.spec.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Redux actions must follow FSA (Flux Standard Action) pattern
Avoid unnecessary comments — write self-documenting code through clear naming
Always verify libraries by checking official docs and.d.tsfiles innode_modules/. Never assume props, tokens, or APIs work without verification
Avoid subjective descriptors ('smart', 'excellent', 'dumb') in documentation and comments
Use measurable descriptions in code documentation: 'reduced memory usage', 'improved by X%' instead of subjective claims
NEVER invent metrics — don't include estimated time spent or speculated user counts. Only include numbers from actual logs, error messages, or documented sources
Files:
src/ui/__tests__/selectors.spec.tssrc/jitsi/main.spec.tssrc/i18n/main.spec.tssrc/screenSharing/main.spec.ts
🔇 Additional comments (5)
jest.config.js (1)
22-22: LGTM!Also applies to: 34-36
src/ui/__tests__/selectors.spec.ts (1)
11-38: LGTM!src/jitsi/main.spec.ts (1)
1-86: LGTM!src/i18n/main.spec.ts (2)
1-93: LGTM!Also applies to: 110-110
29-42: 🩺 Stability & Availability
mockListen()needs an unsubscribe return beforetearDown()runs. If any test reaches this cleanup path with the defaultundefinedreturn, theunsubscribe()call will throw.
- Fix process.platform/process.mas leaking across test files in the same
Jest worker: PersistableValues.spec.ts, internalVideoChatWindow.spec.ts,
dialogs.spec.ts, popupMenu.spec.ts, featureFlags.spec.ts now save and
restore the original value instead of hardcoding or never resetting it
- Fix a real TDZ hoisting bug in dev.spec.ts (same class as the previous
commit's fixes): jest.mock('chokidar', ...) closed over outer consts
that get hoisted below it; declare mocks inline and retrieve via
jest.requireMock instead
- Fix icons.spec.ts: getAppIconPath builds its path with a template
literal (always forward slashes), but the test compared against
path.join(...), which normalizes to backslashes on Windows CI
- Remove duplicate test coverage in utils.spec.ts: the version/userLoggedIn
dispatch assertions were already covered verbatim by version.spec.ts and
userLoggedIn.spec.ts
Rejected: navigation/main.spec.ts's askForCertificateTrust assertion —
CodeRabbit flagged the expected 'undefined' detail as wrong given the
source reassigns detail via t(...), but i18next is never initialized in
this spec, so t() genuinely returns undefined at runtime; verified by
running i18next.t() directly against an uninitialized instance.
- acceleratorDisplay.spec.ts: the meta+option+p case relied on the CI
runner's real process.platform for the meta->Cmd mapping (only defined
for darwin); pass { platform: 'darwin' } explicitly like the sibling
assertions in the same file
- navigation/main.spec.ts, systemCertificates.spec.ts: both asserted
readFileSync/readFile was called with a hardcoded POSIX path string,
but the real implementation builds the path via path.join(...), which
normalizes to backslashes on Windows; wrap the expected value in
path.join(...) too so both sides use the same OS-aware separator
Confirmed all 3 previously failed only on ubuntu-latest/windows-latest
in CI (#3393),
not locally on macOS.
Definitively root-caused the CI check job's EvalError, replacing the "documented as a known blocker" note from the previous commit. @kayahr/jest-electron-runner's ElectronEnvironment monkey-patches vm.Script#runInContext to redirect to script.runInThisContext() against Electron's native renderer V8 context, instead of a real vm.createContext() sandbox. Every module Jest loads (test files and all required source) is compiled as a vm.Script and executed this way. That native context enforces V8's AllowCodeGenerationFromStrings restriction at context-creation time - a native embedder API, not exposed via webPreferences (contextIsolation, nodeIntegration, sandbox: all verified empirically to have no effect), CSP (the runner's own bootstrap HTML already sets 'unsafe-eval'), or any documented Electron/Node command-line switch. Istanbul's coverage instrumentation injects new Function(...) counter calls into every instrumented module, which trips this restriction; without --coverage, modules execute as plain compiled scripts and never hit it. Confirmed empirically, ruling out every configurable lever before concluding this is structural: - contextIsolation/nodeIntegration: already least-restrictive - sandbox: false patched directly into the runner's compiled JS, no change - CSP: runner's own index.html already permits unsafe-eval - coveragePathIgnorePatterns / istanbul-ignore-file / per-project collectCoverageFrom override - none prevent transform-time instrumentation, since --coverage instruments before any report-scoping filter applies - coverageProvider v8 - fails differently (inspector session can't connect in this same restricted context) @kayahr/jest-electron-runner is archived upstream (maintainer moved to Vitest per its issue #33) - no fix will land there. Fix: exclude the 17 affected preload/renderer-context specs from the coverage-enabled project via testPathIgnorePatterns, gated on --coverage being present in argv. They still run and gate CI on pass/fail under plain yarn test - only the coverage-instrumented run skips them. Verified: yarn test:coverage now passes 125/125 suites (was 17 failed). yarn test unaffected, still 142/142 suites pass.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
jest.config.js (1)
60-60: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOverriding
testPathIgnorePatternsdrops the default node_modules exclusion.Jest's default
testPathIgnorePatternsis["/node_modules/"]; setting this option replaces rather than extends that default, so in non-coverage runs it becomes[]. SincetestMatchfor this project is scoped tosrc/..., this is currently harmless, but it's fragile — any future broadening oftestMatch(or a config split) would silently re-enable scanningnode_modules. Consider merging with the default explicitly.♻️ Suggested fix
- testPathIgnorePatterns: isCoverageRun ? COVERAGE_INCOMPATIBLE_SPECS : [], + testPathIgnorePatterns: isCoverageRun + ? ['/node_modules/', ...COVERAGE_INCOMPATIBLE_SPECS] + : ['/node_modules/'],🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@jest.config.js` at line 60, The testPathIgnorePatterns setting in the Jest config is replacing Jest’s default node_modules exclusion instead of preserving it. Update the Jest configuration around testPathIgnorePatterns so the non-coverage case still includes the default /node_modules/ ignore pattern, and only adds COVERAGE_INCOMPATIBLE_SPECS when isCoverageRun is true. Keep the fix localized to the Jest config entry that currently sets testPathIgnorePatterns.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@jest.config.js`:
- Line 60: The testPathIgnorePatterns setting in the Jest config is replacing
Jest’s default node_modules exclusion instead of preserving it. Update the Jest
configuration around testPathIgnorePatterns so the non-coverage case still
includes the default /node_modules/ ignore pattern, and only adds
COVERAGE_INCOMPATIBLE_SPECS when isCoverageRun is true. Keep the fix localized
to the Jest config entry that currently sets testPathIgnorePatterns.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a8dabdef-b2e2-4895-9b01-a942b1536668
📒 Files selected for processing (1)
jest.config.js
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: check (macos-latest)
- GitHub Check: check (windows-latest)
- GitHub Check: check (ubuntu-latest)
🔇 Additional comments (2)
jest.config.js (2)
1-36: LGTM!
58-58: LGTM!Also applies to: 71-72
Line 23 (uses mac overrides by default when platform is darwin) called
formatAcceleratorForDisplay('commandorcontrol+q') with no explicit platform
option, same class of bug as the meta+option+p case fixed earlier in this
file — relies on the CI runner's real process.platform instead of testing
the darwin default explicitly. Passed on macOS CI by accident of
environment, failed on ubuntu-latest/windows-latest (real process.platform
there is linux/win32, so commandorcontrol resolved to Ctrl not Cmd).
Mocked process.platform to darwin for this specific assertion, restoring
the original value after, matching the platform-leak-safe pattern used
elsewhere in this branch.
Member
Author
|
Jira: CORE-2394 |
jeanfbrito
added a commit
that referenced
this pull request
Jul 10, 2026
Aligns the provider specs introduced on master (#3393) with this branch's display-media changes: deny shape is callback(null) and the internal picker handler now receives an optional originWindow.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
test: add preload and module coverage specs
Summary
Adds Jest coverage across nearly all application modules (app state/reducers, store/IPC middleware, deep links, downloads, i18n/logging, jitsi/screenSharing/telephony/notifications/outlook/spellchecking, servers preload bridge, UI components/main/preload). Exports three previously-private dialog reducers (
serverInfoModal,telephonyServerSelect,telephonyDefaultHandlerPrompt) so they're directly testable. Four follow-up commits fix real bugs the new specs exposed, address CodeRabbit review feedback, fix platform-specific CI failures, and resolve the coverage-instrumentation-vs-Electron-sandbox conflict blocking thecheckCI job.Root Cause Analysis
A large batch of new spec files surfaced three classes of pre-existing/newly-introduced problems in this repo's Jest setup, each with a distinct root cause.
1. Four spec files were silently invisible to Jest
The Problem:
whenReady.spec.ts,systemCertificates.spec.ts,constants.spec.ts, andui/selectors.spec.tsran locally in isolation but contributed zero coverage in CI.Root Cause:
jest.config.jstestMatch only has globs forsrc/*/!(main)/**/*.spec.ts(module subdir + nested path) andsrc/**/{main,renderer}.spec.ts. Files sitting flat directly undersrc/(no module directory) or with only one path segment after the module never matched either pattern. No prior spec had ever targeted a top-levelsrc/*.tsfile, so this gap was latent until now.Fix: Extended
jest.config.jswith three targeted globs (src/whenReady.spec.tson the renderer project since it touchesdocument;src/systemCertificates.spec.tsandsrc/constants.spec.tson the main project). Relocatedui/selectors.spec.tsintoui/__tests__/. Deleted a stray duplicate (screenSharing/screenRecordingPermission.spec.ts) that was a dead flat copy of the already-covered nested test.2. Three
main.spec.tsfiles ran twice, concurrently, in different environmentsThe Problem:
jitsi/__tests__/main.spec.ts,screenSharing/__tests__/main.spec.ts, andi18n/__tests__/main.spec.tsmatched both Jest projects (renderer and main) simultaneously — the renderer glob (src/*/!(main)/**/*.spec.ts) matches any spec inside a non-main-named directory, while the main glob (src/**/main.spec.ts) matches any file literally namedmain.spec.tsregardless of its parent directory. A file at<module>/__tests__/main.spec.tssatisfies both.Root Cause: The
__tests__/main.spec.tspath convention used for these three new specs collides with an existing, unrelated naming pattern (main.spec.ts= "tests the main-process entrypoint") that this repo's testMatch treats as a signal on its own, independent of directory nesting.Fix: Moved all three to the flat
<module>/main.spec.tsconvention already used by ten other files in the repo (ipc/main.spec.ts,telephony/main.spec.ts, etc.), which matches only the main project.3. TDZ
ReferenceErrors from ts-jest's mock hoistingThe Problem: Multiple spec files threw
Cannot access '<var>' before initializationinsidejest.mock()factory callbacks.Root Cause: ts-jest ships its own hoist transformer (
ts-jest/dist/transformers/hoist-jest.js) that unconditionally sorts everyjest.mock()/jest.unmock()call to the top of its enclosing block — above all other statements, with no exception for identifiers prefixedmockthe way Babel'sbabel-plugin-jest-hoistallowlists. Any factory that closes over an outerconstdeclared later in the same file throws once thatconstis pushed below the (now-hoisted)jest.mock()call. This affectedjitsi/main.spec.ts,screenSharing/main.spec.ts,i18n/main.spec.ts,browser/__tests__/ipc.spec.ts,jitsi/__tests__/preload.spec.ts,servers/preload/__tests__/window.spec.ts, and (found in CodeRabbit's follow-up review)app/main/dev.spec.ts.Fix: Declare
jest.fn()calls inline inside the factory, then retrieve references afterward viajest.requireMock('<module>')orjest.mocked(realImport)— the pattern already used correctly by most pre-existing specs in this repo (e.g.servers/preload/__tests__/api.spec.ts).4.
jitsi/__tests__/ipc.spec.tsmodule-level state leaked across testsThe Problem: Two of three tests in this file expected
isJitsiServerAllowedto be called once per handler invocation but received zero calls.Root Cause:
src/jitsi/ipc.tsholds module-level state (permitted,dontAskAgain,firstAskPermission) that persists for the lifetime of the loaded module — by design, since permission should only be asked once per app session. The spec statically importedhandleJitsiDesktopCapturerGetSourcesat file scope and calledjest.resetModules()inbeforeEach, butresetModules()has no effect on a binding already resolved by a staticimportat module-load time — it only affects subsequentrequire()/dynamicimport()calls.Fix: Switched to
jest.resetModules()+await import('../ipc')per test, matching the pattern already used inscreenSharing/__tests__/preload.spec.tsandvideoCallWindow/main/ipc.main.spec.ts.5.
process.platform/process.masleaking across tests in the same workerThe Problem (found by CodeRabbit's review): Several specs override
process.platformorprocess.masfor a single test case viaObject.definePropertyand never restore the original value — since Jest runs with a shared worker process, this can leak a fake platform into unrelated tests that run later in the same worker.Root Cause:
PersistableValues.spec.ts's cleanup helper hardcoded'darwin'instead of restoring the actual original value;internalVideoChatWindow.spec.ts's restore was gated behindif (processMasDescriptor), silently skipping cleanup when no descriptor existed (the common case, sinceprocess.masdoesn't exist on non-macOS Node);dialogs.spec.ts,popupMenu.spec.ts, andfeatureFlags.spec.tssimply had no restore logic at all for some or all of their platform-mutating tests.Fix: Capture the real
process.platform/process.masvalue before mutating and restore it inafterEach(or inline, matching each file's existing style) in all five files.6.
getAppIconPathtest asserted a path-separator-dependent expectationThe Problem (found by CodeRabbit's review):
icons.spec.tscomparedgetAppIconPath()'s return value againstpath.join('/app', 'app', 'images', 'icon.ico').Root Cause:
getAppIconPathbuilds its path with a template literal (`${app.getAppPath()}/app/images/icon.ico`), which always produces forward slashes regardless of OS.path.join()on Windows normalizes to backslashes, so the test would fail on Windows CI runners. (The siblinggetTrayIconPathtests were unaffected — their implementation genuinely usespath.join, so both sides of those assertions use the same OS-aware separator.)Fix: Changed the expectation to the literal forward-slash string the implementation actually returns.
Also Fixed
stateGroups.spec.ts: two.toBeDefined()assertions on reducer default state tightened to.toEqual(<actual default shape>), so a wrong-but-non-null default would actually fail the test.utils.spec.ts: removed three tests that duplicatedversion.spec.tsanduserLoggedIn.spec.tsalmost verbatim, adding maintenance cost with no additional coverage.CI Status
PR CI ran the
checkjob on macos-latest, ubuntu-latest, and windows-latest — all 3 failed, but on two distinct, unrelated issues.1. Two platform-specific test bugs (fixed)
telephony/__tests__/acceleratorDisplay.spec.ts(ubuntu + windows):formatAcceleratorForDisplay('meta+option+p')was called with no explicitplatformoption, so it fell back to the CI runner's realprocess.platform. Themeta→Cmdmapping only exists inMAC_LABEL_OVERRIDES(darwin); on ubuntu/windows it resolved to'Meta'instead of the expected'Cmd'. Fixed by passing{ platform: 'darwin' }explicitly, matching the sibling assertions in the same file.navigation/main.spec.ts+systemCertificates.spec.ts(windows only): both assertreadFileSync/readFilewas called with a hardcoded POSIX path string (e.g.'/tmp/user-data/certificate.json'), but the real implementation builds that path viapath.join(...), which normalizes to backslashes on Windows. Fixed by wrapping the expected value inpath.join(...)too, so both sides of the assertion use the same OS-aware separator.2.
yarn test:coveragefailed on 17 preload-context suites — root-caused and fixedThe Problem: CI's
checkjob runstest:coverage(Istanbul-instrumented), not plaintest. 17 suites that load preload-side modules (servers/preload/__tests__/*,jitsi/__tests__/preload.spec.ts,screenSharing/__tests__/preload.spec.ts,ui/preload/__tests__/*,store/__tests__/index.spec.ts,whenReady.spec.ts, etc.) failed identically on all 3 platforms with:Confirmed not a regression: checked out the exact base commit (
83ad9a5f5) in a separate worktree and ranyarn test:coveragethere — 69/69 suites pass, exit 0, because the base commit has zero preload-context spec files. This PR is the first to load preload/renderer modules under coverage instrumentation, so the conflict never had a chance to trigger before now.Root cause:
@kayahr/jest-electron-runner'sElectronEnvironmentmonkey-patchesvm.Script#runInContextto redirect toscript.runInThisContext()against Electron's native renderer V8 context, instead of a realvm.createContext()sandbox. Every module Jest loads — test files and all required source — is compiled as avm.Scriptand executed this way. That native context enforces V8'sAllowCodeGenerationFromStringsrestriction at context-creation time, a native C++ embedder API with no exposed Electron webPreferences flag, CSP directive, or command-line switch. Istanbul's coverage instrumentation injectsnew Function(...)counter calls into every instrumented module, which trips this restriction; without--coverage, modules execute as plain compiled scripts and never hit it.Ruled out empirically before reaching this conclusion:
contextIsolation/nodeIntegration: already the least-restrictive settings in the runner'sBrowserWindowconfig.sandbox: false: patched directly into the runner's compiledJestWorkerRPC.js, re-ran — no change.index.html(not this repo's), which already declares'unsafe-eval'.coveragePathIgnorePatterns,/* istanbul ignore file */, per-projectcollectCoverageFrom: []overrides: none prevent transform-time instrumentation, since--coverageinstruments before any report-scoping filter applies.coverageProvider: 'v8': fails differently (Session is not connected— the inspector/CDP session can't attach in this same restricted context).@kayahr/jest-electron-runneris archived upstream — the maintainer moved to Vitest and archived the repo (see its issue #33) — so no fix will land there.Fix: excluded the 17 affected specs from the coverage-enabled project via
testPathIgnorePatterns, gated on--coveragebeing present inprocess.argv. They still run and gate CI on pass/fail under plainyarn test— only the coverage-instrumented run skips them.Coverage impact of this PR (measured locally with
yarn test:coverage, comparing base commit83ad9a5f5to this branch; the 17 excluded suites contribute 0% to this branch's numbers, same as they always have under coverage):Rejected CodeRabbit Suggestion
navigation/main.spec.ts:456— CodeRabbit flagged the assertion expectingaskForCertificateTrust's second argument to beundefined, reasoning that the source reassignsdetailviat('error.differentCertificate', { detail })when the host was previously trusted with a different certificate. Verified directly:i18nextis never.init()'d in this spec file, and calling.t()on an uninitialized i18next instance returnsundefined— confirmed by runningi18next.t(...)standalone. The existing assertion is correct for this spec's actual (unmocked) runtime behavior.Files Changed
jest.config.js— three new testMatch globs for top-levelsrc/*.spec.tsfiles, plus a--coverage-gatedtestPathIgnorePatternsexcluding the 17 coverage-incompatible specssrc/ui/reducers/dialogs.ts— three reducers exported for direct testabilityacceleratorDisplay.spec.ts,navigation/main.spec.ts,systemCertificates.spec.ts)Verification
Full
yarn testsuite run multiple times independently (before and after each fix pass) to confirm no regressions.yarn test:coveragerun on both this branch (base commit83ad9a5f5: 69/69 suites pass, confirming the EvalError was newly exposed and not pre-existing) and after the fix (125/125 suites pass).Closes: none
Summary by CodeRabbit