Add test coverage for notification system #116
Conversation
Co-authored-by: acarlson33 <216757359+acarlson33@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive test coverage for the notification system, addressing a previously untested area of the codebase. The tests validate notification settings management and trigger logic, including edge cases and error handling scenarios.
Key Changes
- Added 26 tests for notification settings (calculateMuteExpiration, isMuteExpired, getEffectiveNotificationLevel, isInQuietHours, database operations)
- Added 27 tests for notification triggers (extractMentionedUserIds, isReplyToUser, buildNotificationPayload, shouldNotifyUser)
- Implemented proper Appwrite dependency mocking consistent with the codebase
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/tests/notification-triggers.test.ts | New test file covering notification trigger logic including event classification, filtering, user preferences, payload generation, and edge cases like self-notification prevention |
| src/tests/notification-settings.test.ts | New test file covering notification settings management including mute expiration calculations, notification level hierarchy, quiet hours handling, and database CRUD operations with JSON parsing |
| it("should extract user IDs from mentions", async () => { | ||
| const { extractMentionedUserIds } = await import("../lib/notification-triggers"); | ||
|
|
||
| const messageContent = "Hey <@user123> and <@user456>, check this out!"; | ||
| const result = extractMentionedUserIds(messageContent); | ||
|
|
||
| expect(result).toEqual(["user123", "user456"]); | ||
| }); |
There was a problem hiding this comment.
The dynamic import pattern using await import() inside each test is unnecessary and adds complexity. Since the module mocking is already set up at the top of the file with vi.mock(), you can import the functions normally at the module level alongside other imports (e.g., import { extractMentionedUserIds, isReplyToUser, buildNotificationPayload, shouldNotifyUser } from "../lib/notification-triggers"). This pattern is repeated throughout the file and should be refactored to use static imports for better consistency with standard testing practices.
There was a problem hiding this comment.
Fixed in commit 647272f. Replaced all dynamic imports with static imports at the module level.
| it("should return undefined for 'forever' duration", async () => { | ||
| const { calculateMuteExpiration } = await import("../lib/notification-settings"); | ||
| const result = calculateMuteExpiration("forever"); | ||
| expect(result).toBeUndefined(); | ||
| }); |
There was a problem hiding this comment.
The dynamic import pattern using await import() inside each test is unnecessary and adds complexity. Since the module mocking is already set up at the top of the file with vi.mock(), you can import the functions normally at the module level alongside other imports (e.g., import { calculateMuteExpiration, isMuteExpired, getEffectiveNotificationLevel, isInQuietHours, getNotificationSettings } from "../lib/notification-settings"). This pattern is repeated throughout the file and should be refactored to use static imports for better consistency with standard testing practices.
There was a problem hiding this comment.
Fixed in commit 647272f. Replaced all dynamic imports with static imports at the module level.
| const { calculateMuteExpiration } = await import("../lib/notification-settings"); | ||
| const before = new Date(); | ||
| const result = calculateMuteExpiration("15m"); | ||
| const after = new Date(); |
| // Test at a specific time | ||
| const now = new Date(); | ||
| const currentHour = now.getHours(); | ||
| const currentMin = now.getMinutes(); |
acarlson33
left a comment
acarlson33
left a comment
There was a problem hiding this comment.
@copilot please fix the copilot code review comments.
Co-authored-by: acarlson33 <216757359+acarlson33@users.noreply.github.com>
All code review comments have been addressed in commit 647272f:
All 53 tests continue to pass. The changes reduced code by 85 lines and improved consistency with standard testing practices. |
Testing Plan for Notification System ✅ Complete
Summary
Successfully added comprehensive test coverage for the notification system with 53 passing tests across 2 test files.
Code Review Fixes Applied
await import()with static imports at module levelafterin notification-settings.test.tscurrentMinin notification-settings.test.ts📄 notification-settings.test.ts (26 tests)
Tests core notification settings management including:
📄 notification-triggers.test.ts (27 tests)
Tests notification trigger logic including:
All tests follow existing patterns in the codebase and use proper mocking for Appwrite dependencies. The test suite passes successfully with no code quality or security issues.
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.