test: extract-and-test moderate modules + coverage to 26% (Phase 2) - #3364
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
WalkthroughNew Jest test suites are added for six modules: IPC renderer helpers, main-process logging, navigation protocol handling, Outlook Calendar EWS utilities, screen sharing request tracking, and browser launcher. The existing Outlook spec is deleted and replaced by an expanded version moved to a subdirectory. ChangesTest Coverage Expansion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/outlookCalendar/__tests__/getOutlookEvents.spec.ts (1)
714-728: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winRename the “connectivity” test block to match actual behavior
Lines 714-728 describe and comment these cases as connectivity checks, but
sanitizeExchangeUrlonly normalizes/validates URLs (no network probing). Please rename this block/tests to URL-construction behavior to avoid misleading coverage intent.Suggested rename
-describe('Connectivity testing features', () => { - it('handles connectivity testing gracefully when it fails', () => { - // Test that even if connectivity testing fails, the function still returns a valid URL +describe('URL normalization resilience', () => { + it('returns a valid EWS URL for unreachable-looking hosts', () => { const result = sanitizeExchangeUrl('https://unreachable.example.com'); expect(result).toBe( 'https://unreachable.example.com/ews/exchange.asmx' ); }); - it('runs connectivity testing automatically on this debugging branch', () => { - // Connectivity testing runs automatically, validating the URL construction works + it('normalizes a standard HTTPS host to the EWS endpoint', () => { const result = sanitizeExchangeUrl('https://mail.company.com'); expect(result).toBe('https://mail.company.com/ews/exchange.asmx'); }); });As per coding guidelines, “Avoid unnecessary comments — write self-documenting code through clear naming.”
🤖 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__/getOutlookEvents.spec.ts` around lines 714 - 728, Rename the describe block from "Connectivity testing features" to accurately reflect that it tests URL construction behavior (not network connectivity). Rename both test cases within this block to describe what sanitizeExchangeUrl actually does: the first test should reflect that it returns a normalized URL path regardless of whether the domain exists, and the second test should reflect basic URL normalization. Remove the misleading comments about connectivity testing and replace them with clear descriptions of the URL normalization behavior being validated, ensuring test names and descriptions accurately represent that sanitizeExchangeUrl only constructs and normalizes URLs without performing any network connectivity checks.Source: Coding guidelines
🤖 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/navigation/main.spec.ts`:
- Around line 1-163: The test file currently named main.spec.ts does not follow
the Main-process spec naming convention. Rename the file from
src/navigation/main.spec.ts to src/navigation/navigation.main.spec.ts to match
the required *.main.spec.ts pattern for Main-process tests, where the feature
name precedes .main.spec.ts.
---
Nitpick comments:
In `@src/outlookCalendar/__tests__/getOutlookEvents.spec.ts`:
- Around line 714-728: Rename the describe block from "Connectivity testing
features" to accurately reflect that it tests URL construction behavior (not
network connectivity). Rename both test cases within this block to describe what
sanitizeExchangeUrl actually does: the first test should reflect that it returns
a normalized URL path regardless of whether the domain exists, and the second
test should reflect basic URL normalization. Remove the misleading comments
about connectivity testing and replace them with clear descriptions of the URL
normalization behavior being validated, ensuring test names and descriptions
accurately represent that sanitizeExchangeUrl only constructs and normalizes
URLs without performing any network connectivity checks.
🪄 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: 8e21b8bb-d6c7-4262-8dbb-33c7b8bae3a2
📒 Files selected for processing (9)
jest.config.jssrc/ipc/renderer.spec.tssrc/logging/main/index.main.spec.tssrc/navigation/main.spec.tssrc/navigation/main.tssrc/outlookCalendar/__tests__/getOutlookEvents.spec.tssrc/outlookCalendar/getOutlookEvents.spec.tssrc/screenSharing/main/ScreenSharingRequestTracker.main.spec.tssrc/utils/__tests__/browserLauncher.spec.ts
💤 Files with no reviewable changes (1)
- src/outlookCalendar/getOutlookEvents.spec.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (7)
**/*.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/navigation/main.tssrc/navigation/main.spec.tssrc/screenSharing/main/ScreenSharingRequestTracker.main.spec.tssrc/ipc/renderer.spec.tssrc/utils/__tests__/browserLauncher.spec.tssrc/outlookCalendar/__tests__/getOutlookEvents.spec.tssrc/logging/main/index.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/navigation/main.tssrc/navigation/main.spec.tssrc/screenSharing/main/ScreenSharingRequestTracker.main.spec.tssrc/ipc/renderer.spec.tssrc/utils/__tests__/browserLauncher.spec.tssrc/outlookCalendar/__tests__/getOutlookEvents.spec.tssrc/logging/main/index.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/navigation/main.tssrc/navigation/main.spec.tssrc/screenSharing/main/ScreenSharingRequestTracker.main.spec.tssrc/ipc/renderer.spec.tssrc/utils/__tests__/browserLauncher.spec.tssrc/outlookCalendar/__tests__/getOutlookEvents.spec.tssrc/logging/main/index.main.spec.ts
**/*.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
*.spec.tsfile naming for Renderer process tests
Files:
src/navigation/main.spec.tssrc/screenSharing/main/ScreenSharingRequestTracker.main.spec.tssrc/ipc/renderer.spec.tssrc/utils/__tests__/browserLauncher.spec.tssrc/outlookCalendar/__tests__/getOutlookEvents.spec.tssrc/logging/main/index.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/navigation/main.spec.tssrc/screenSharing/main/ScreenSharingRequestTracker.main.spec.tssrc/ipc/renderer.spec.tssrc/utils/__tests__/browserLauncher.spec.tssrc/outlookCalendar/__tests__/getOutlookEvents.spec.tssrc/logging/main/index.main.spec.ts
**/*.main.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
*.main.spec.tsfile naming for Main process tests
Files:
src/screenSharing/main/ScreenSharingRequestTracker.main.spec.tssrc/logging/main/index.main.spec.ts
src/outlookCalendar/**/*.{ts,tsx}
📄 CodeRabbit inference engine (src/outlookCalendar/AGENTS.md)
src/outlookCalendar/**/*.{ts,tsx}: UsecreateClassifiedError()fromerrorClassification.tsfor user-facing errors to provide error categorization, user-friendly messages, and structured error context
Always use outlookError() for errors as it logs regardless of verbose mode settings, ensuring errors are always visible to users
Files:
src/outlookCalendar/__tests__/getOutlookEvents.spec.ts
🔇 Additional comments (12)
src/ipc/renderer.spec.ts (1)
1-188: LGTM!jest.config.js (1)
15-18: LGTM!src/navigation/main.ts (1)
41-41: LGTM!src/screenSharing/main/ScreenSharingRequestTracker.main.spec.ts (1)
1-325: LGTM!src/utils/__tests__/browserLauncher.spec.ts (1)
1-219: LGTM!src/logging/main/index.main.spec.ts (7)
1-125: LGTM!
127-223: LGTM!
224-277: LGTM!
279-376: LGTM!
378-457: LGTM!
459-670: LGTM!
673-689: LGTM!
| import type { Certificate } from 'electron'; | ||
|
|
||
| import { select, dispatch } from '../store'; | ||
| import { askForOpeningExternalProtocol } from '../ui/main/dialogs'; | ||
| import { EXTERNAL_PROTOCOL_PERMISSION_UPDATED } from './actions'; | ||
| import { serializeCertificate, isProtocolAllowed } from './main'; | ||
|
|
||
| jest.mock('electron', () => ({ | ||
| app: { | ||
| addListener: jest.fn(), | ||
| getPath: jest.fn(), | ||
| }, | ||
| })); | ||
| jest.mock('../store'); | ||
| jest.mock('../ui/main/dialogs'); | ||
|
|
||
| const selectMock = select as jest.MockedFunction<typeof select>; | ||
| const dispatchMock = dispatch as jest.MockedFunction<typeof dispatch>; | ||
| const askForOpeningExternalProtocolMock = | ||
| askForOpeningExternalProtocol as jest.MockedFunction< | ||
| typeof askForOpeningExternalProtocol | ||
| >; | ||
|
|
||
| const makeCertificate = (overrides: Partial<Certificate> = {}): Certificate => | ||
| ({ | ||
| issuerName: 'Example CA', | ||
| data: 'CERT-DATA', | ||
| ...overrides, | ||
| }) as unknown as Certificate; | ||
|
|
||
| describe('navigation/main.ts', () => { | ||
| beforeEach(() => { | ||
| jest.clearAllMocks(); | ||
| }); | ||
|
|
||
| describe('serializeCertificate', () => { | ||
| it('serializes as "<issuerName>\\n<data>"', () => { | ||
| const certificate = makeCertificate({ | ||
| issuerName: 'Example CA', | ||
| data: 'CERT-DATA', | ||
| }); | ||
|
|
||
| expect(serializeCertificate(certificate)).toBe('Example CA\nCERT-DATA'); | ||
| }); | ||
|
|
||
| it('calls toString() on the data field', () => { | ||
| const toString = jest.fn(() => 'STRINGIFIED'); | ||
| const certificate = makeCertificate({ | ||
| issuerName: 'Issuer', | ||
| data: { toString } as unknown as Certificate['data'], | ||
| }); | ||
|
|
||
| expect(serializeCertificate(certificate)).toBe('Issuer\nSTRINGIFIED'); | ||
| expect(toString).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it('handles empty issuer name', () => { | ||
| const certificate = makeCertificate({ issuerName: '', data: 'D' }); | ||
|
|
||
| expect(serializeCertificate(certificate)).toBe('\nD'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('isProtocolAllowed', () => { | ||
| const mockExternalProtocols = ( | ||
| externalProtocols: Record<string, boolean> | ||
| ) => { | ||
| selectMock.mockImplementation((selector: any) => | ||
| selector({ externalProtocols }) | ||
| ); | ||
| }; | ||
|
|
||
| it.each(['http://example.com', 'https://example.com', 'mailto:a@b.com'])( | ||
| 'allows intrinsic protocol %s without prompting', | ||
| async (url) => { | ||
| mockExternalProtocols({}); | ||
|
|
||
| await expect(isProtocolAllowed(url)).resolves.toBe(true); | ||
| expect(askForOpeningExternalProtocolMock).not.toHaveBeenCalled(); | ||
| expect(dispatchMock).not.toHaveBeenCalled(); | ||
| } | ||
| ); | ||
|
|
||
| it('allows a persisted protocol that is marked allowed', async () => { | ||
| mockExternalProtocols({ 'custom:': true }); | ||
|
|
||
| await expect(isProtocolAllowed('custom://thing')).resolves.toBe(true); | ||
| expect(askForOpeningExternalProtocolMock).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('does not treat a persisted protocol marked as disallowed as intrinsic', async () => { | ||
| mockExternalProtocols({ 'custom:': false }); | ||
| askForOpeningExternalProtocolMock.mockResolvedValue({ | ||
| allowed: false, | ||
| dontAskAgain: false, | ||
| }); | ||
|
|
||
| await expect(isProtocolAllowed('custom://thing')).resolves.toBe(false); | ||
| expect(askForOpeningExternalProtocolMock).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| it('prompts for an unknown protocol and returns the dialog result', async () => { | ||
| mockExternalProtocols({}); | ||
| askForOpeningExternalProtocolMock.mockResolvedValue({ | ||
| allowed: true, | ||
| dontAskAgain: false, | ||
| }); | ||
|
|
||
| await expect(isProtocolAllowed('custom://thing')).resolves.toBe(true); | ||
| expect(askForOpeningExternalProtocolMock).toHaveBeenCalledTimes(1); | ||
| expect(askForOpeningExternalProtocolMock.mock.calls[0][0]).toBeInstanceOf( | ||
| URL | ||
| ); | ||
| expect(dispatchMock).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('returns false when the dialog denies and dontAskAgain is false', async () => { | ||
| mockExternalProtocols({}); | ||
| askForOpeningExternalProtocolMock.mockResolvedValue({ | ||
| allowed: false, | ||
| dontAskAgain: false, | ||
| }); | ||
|
|
||
| await expect(isProtocolAllowed('custom://thing')).resolves.toBe(false); | ||
| expect(dispatchMock).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('persists the permission when dontAskAgain is true (allowed)', async () => { | ||
| mockExternalProtocols({}); | ||
| askForOpeningExternalProtocolMock.mockResolvedValue({ | ||
| allowed: true, | ||
| dontAskAgain: true, | ||
| }); | ||
|
|
||
| await expect(isProtocolAllowed('custom://thing')).resolves.toBe(true); | ||
| expect(dispatchMock).toHaveBeenCalledWith({ | ||
| type: EXTERNAL_PROTOCOL_PERMISSION_UPDATED, | ||
| payload: { protocol: 'custom:', allowed: true }, | ||
| }); | ||
| }); | ||
|
|
||
| it('persists the denial when dontAskAgain is true (denied)', async () => { | ||
| mockExternalProtocols({}); | ||
| askForOpeningExternalProtocolMock.mockResolvedValue({ | ||
| allowed: false, | ||
| dontAskAgain: true, | ||
| }); | ||
|
|
||
| await expect(isProtocolAllowed('custom://thing')).resolves.toBe(false); | ||
| expect(dispatchMock).toHaveBeenCalledWith({ | ||
| type: EXTERNAL_PROTOCOL_PERMISSION_UPDATED, | ||
| payload: { protocol: 'custom:', allowed: false }, | ||
| }); | ||
| }); | ||
|
|
||
| it('rejects on a malformed URL', async () => { | ||
| mockExternalProtocols({}); | ||
|
|
||
| await expect(isProtocolAllowed('not a url')).rejects.toThrow(); | ||
| expect(askForOpeningExternalProtocolMock).not.toHaveBeenCalled(); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Rename this test file to follow the Main-process spec naming convention.
This suite targets a Main-process module (src/navigation/main.ts), so it should be named with the *.main.spec.ts pattern (for example, navigation.main.spec.ts) rather than main.spec.ts.
As per coding guidelines, "**/*.main.spec.ts: Use *.main.spec.ts file naming for Main process tests".
🤖 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 1 - 163, The test file currently
named main.spec.ts does not follow the Main-process spec naming convention.
Rename the file from src/navigation/main.spec.ts to
src/navigation/navigation.main.spec.ts to match the required *.main.spec.ts
pattern for Main-process tests, where the feature name precedes .main.spec.ts.
Source: Coding guidelines
54013d1 to
c46c552
Compare
Adds unit tests for modules needing light Electron mocking or a minimal extraction, continuing the coverage push from Phase 1. - navigation/main.ts: export serializeCertificate (one keyword, no behavior change) + test it and the already-exported isProtocolAllowed - screenSharing/ScreenSharingRequestTracker.ts: timeout/listener state machine via fake timers (~96%) - utils/browserLauncher.ts: async lazy-load + promise caching (~93%) - ipc/renderer.ts: invoke/handle/retry wrappers (100% lines) - logging/index.ts: logger factory, hooks, console override, IPC + web- contents logging (~93%, 157 lines were uncovered — largest single gain) - outlookCalendar/getOutlookEvents.ts: relocate the spec into __tests__/ so the jest glob actually discovers it (it was silently never running), fix 3 stale assertions to match real source behavior, add appointment- mapping + connectivity coverage (0% -> ~95%) Ratchets coverageThreshold to lines/statements 25, branches 22, functions 18. Coverage: lines 22.34% -> 26.34%, statements 22.91% -> 26.76%, branches 20.39% -> 23.61%, functions 16.11% -> 19.17%. 802 tests pass.
35bacdd to
c263950
Compare
1 participant
What
Phase 2 of the coverage push: tests for moderate-complexity modules that needed light Electron mocking or a minimal extraction. Stacked on Phase 1.
Coverage delta
802 tests pass, 2 skipped, 0 failures. Lint clean.
New / changed specs
logging/index.tsoutlookCalendar/getOutlookEvents.tsscreenSharing/ScreenSharingRequestTracker.tsutils/browserLauncher.tsipc/renderer.tsnavigation/main.ts(serializeCertificate,isProtocolAllowed)Latent bug fixed (getOutlookEvents)
The existing
getOutlookEvents.spec.tslived at a path that matched neither jest project glob, so it was silently never executed (0% coverage despite ~340 lines of tests). Moving it into__tests__/makes jest discover it. On first execution, 3 pre-existing assertions failed — they encoded behavior the source never had; reconciled to actual (correct) source behavior with explanatory comments. No source logic changed.This is exactly the failure mode the Phase 1 placement-pitfall guard was written for — now caught.
Source change (1 keyword)
navigation/main.ts: addedexporttoserializeCertificateso it can be unit-tested (it had 1 internal caller, unchanged). Impact analysis: LOW risk.isProtocolAllowedwas already exported (CRITICAL blast radius — 6 callers — so left untouched, just tested). No logic moved, no signatures changed.Ratchet gate
Bumps
coverageThresholdto lines/statements 25, branches 22, functions 18 (just under the new baseline). Active in CI viavalidate-pr.yml.Toward 50%
The remaining gap to a 50% goal is concentrated in
ui/(React components, ~3000 lines) and integration-only files (videoCallWindow/ipc.ts,ui/main/serverView,logViewerWindow.tsx). Those need a renderer-component testing harness (Fuselage + Redux + Electron-renderer mocking) — a larger Phase 3 initiative, planned separately.Summary by CodeRabbit
Tests
Chores