test(push): add tests for direct publish via clientId and batch push publish #2031
Conversation
WalkthroughThe browser push test suite was expanded with three new tests covering direct push by device ID, direct push by client ID, and batch push notifications. The setup logic was refactored to reinitialize the message channel before each test, and a client ID was added to the REST client initialization. All changes are confined to test logic. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Runner
participant Browser as Browser
participant SW as Service Worker
participant Ably as Ably REST API
Test->>Browser: Initialize message channel (beforeEach)
Test->>Ably: Initialize AblyRest client (with clientId)
Test->>Browser: Activate push and register SW
Browser->>SW: Register and activate SW
Test->>Ably: Send push notification (direct or batch)
Ably->>Browser: Deliver push notification
Browser->>SW: Forward push message via message channel
SW->>Test: Post received push payload
Test->>Test: Assert payload correctness
Suggested reviewers
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
temporarily deployed
to
staging/pull/2031/bundle-report
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
test/browser/push.test.js (4)
158-159: Fix typo in console.log message.There's a spelling error in the console.log message.
- console.log('recieved event', event); + console.log('received event', event);
169-169: Fix typo in error message.There's a spelling error in the error message.
- reject(new Error('Batch publish recieved error: ' + res.errorMessage)); + reject(new Error('Batch publish received error: ' + res.errorMessage));
176-184: Enhance batch payload verification.The current verification only checks the title but doesn't verify the notification body. Additionally, lines 178 and 180 check for data properties that aren't defined in the batch payload.
for (const receivedPushPayload of receivedPayloads) { if (receivedPushPayload.notification.title === 'SingleRecipient') { - expect(receivedPushPayload.data).to.deep.equal(batchPayload[0].payload.data); + expect(receivedPushPayload.notification.body).to.equal('batch_publish_1'); + // Only check data if it was included in the payload + if (batchPayload[0].payload.data) { + expect(receivedPushPayload.data).to.deep.equal(batchPayload[0].payload.data); + } } else if (receivedPushPayload.notification.title === 'MultipleRecipients') { - expect(receivedPushPayload.data).to.deep.equal(batchPayload[1].payload.data); + expect(receivedPushPayload.notification.body).to.equal('batch_publish_2'); + // Only check data if it was included in the payload + if (batchPayload[1].payload.data) { + expect(receivedPushPayload.data).to.deep.equal(batchPayload[1].payload.data); + } } else { expect.fail('Unexpected payload received'); } }
160-160: Consider making the expected notification count dynamic.The hardcoded value of 3 expected payloads is brittle if the batch configuration changes.
- if (receivedPayloads.length === 3) { + // Calculate expected count: sum of individual recipients plus recipients in arrays + const expectedCount = batchPayload.reduce((count, entry) => { + return count + (Array.isArray(entry.recipient) ? entry.recipient.length : 1); + }, 0); + if (receivedPayloads.length === expectedCount) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/browser/push.test.js(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-node (20.x)
- GitHub Check: test-node (16.x)
- GitHub Check: test-browser (chromium)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-node (18.x)
- GitHub Check: test-npm-package
🔇 Additional comments (5)
test/browser/push.test.js (5)
16-16: Good refactoring of the messageChannel initialization.Moving the messageChannel initialization to the beforeEach hook ensures a fresh channel for each test, preventing potential state contamination between tests.
Also applies to: 46-47
34-34: LGTM: Added clientId to REST client initialization.The addition of clientId is necessary for the new client ID-based push notification tests.
65-93: Well-structured new test for device ID push notifications.This test effectively verifies direct publishing to a device ID, with proper async handling and comprehensive payload verification.
95-122: Well-structured new test for client ID push notifications.This test correctly demonstrates targeting push notifications by client ID, complementing the device ID test case.
124-185: Good implementation of batch publish test.The test effectively verifies the batch publishing functionality with different recipient configurations (single recipient and multiple recipients).
owenpearson
force-pushed
the
push-batch-test
branch
from
ca6922f to
56aa1d3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
test/browser/push.test.js (3)
66-93: LGTM: Well-structured test for device ID targetingThis test effectively verifies direct push publishing to a specific device ID. The test properly:
- Activates push notifications
- Creates a push recipient targeting the device ID
- Sets up a message channel to receive the notification
- Publishes and waits for the notification
- Verifies both notification content and data payload
For improved readability, consider adding a comment explaining the overall flow of the test and how the service worker communication works.
124-184: Consider refining the batch publish test implementationWhile the test is functional, there are some opportunities for improvement:
The test directly uses
rest.request('POST', '/push/batch/publish', ...)instead of using an abstraction likerest.push.admin.publishBatch()that might be available (or could be created for consistency)The test expects exactly 3 payloads, but it might be better to make this expectation dynamic based on the batch payload structure (single recipient + 2 multiple recipients = 3)
The payload verification logic differs from the direct publish tests - consider making it more consistent across all tests
Add comments explaining the expected number of notifications and why (1 from single recipient + 2 from multiple recipients)
174-183: Enhance verification for batch publishing notificationsThe verification logic could be strengthened by also checking the notification body and ensuring all expected payloads are received:
expect(receivedPayloads.length).to.equal(3); + +// Create a map to track which expected notifications we've received +const receivedNotifications = { + 'batch_publish_1': 0, + 'batch_publish_2': 0 +}; + for (const receivedPushPayload of receivedPayloads) { if (receivedPushPayload.notification.title === 'SingleRecipient') { expect(receivedPushPayload.data).to.deep.equal(batchPayload[0].payload.data); + expect(receivedPushPayload.notification.body).to.equal('batch_publish_1'); + receivedNotifications['batch_publish_1']++; } else if (receivedPushPayload.notification.title === 'MultipleRecipients') { expect(receivedPushPayload.data).to.deep.equal(batchPayload[1].payload.data); + expect(receivedPushPayload.notification.body).to.equal('batch_publish_2'); + receivedNotifications['batch_publish_2']++; } else { expect.fail('Unexpected payload received'); } } + +// Verify we got 1 of the first type and 2 of the second type +expect(receivedNotifications['batch_publish_1']).to.equal(1); +expect(receivedNotifications['batch_publish_2']).to.equal(2);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
test/browser/push.test.js(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
test/browser/push.test.js (1)
test/rest/push.test.js (20)
rest(396-396)rest(456-456)rest(476-476)rest(496-496)rest(517-517)rest(555-555)rest(575-575)rest(595-595)rest(634-634)rest(678-678)rest(717-717)rest(734-734)pushRecipient(421-426)pushRecipient(524-526)pushRecipient(602-604)pushPayload(409-412)pushPayload(528-531)pushPayload(606-609)pushPayload(643-646)pushPayload(687-690)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: test-node (16.x)
- GitHub Check: test-node (20.x)
- GitHub Check: test-browser (firefox)
- GitHub Check: test-browser (chromium)
- GitHub Check: test-browser (webkit)
- GitHub Check: test-node (18.x)
- GitHub Check: test-npm-package
🔇 Additional comments (3)
test/browser/push.test.js (3)
16-16: LGTM: Good change to initialize messageChannel in beforeEachChanging
messageChannelfrom a constant to a variable and initializing it fresh before each test ensures proper isolation between test cases, preventing any potential state leakage.Also applies to: 46-47
34-34: LGTM: Adding clientId to the REST clientThe addition of the clientId parameter is necessary for the new client ID-based push notification tests.
95-122: LGTM: Well-structured test for client ID targetingThis test correctly verifies push publishing to a client ID, following the same pattern as the device ID test. The structure is clear and the verification is thorough.
Summary by CodeRabbit