Open
Conversation
Fix incorrect subscription reference timetoken (used by listeners to filter old messages) caused by the server returning timetoken older than previous one because of MX. fix(subscription-set): fix event handling by subscription Fix the issue because of which all subscriptions of the subscription set have been requested to handle the received event.
…n refresh along with multiple (25)pubnub instances
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
mohitpubnub
changed the title
mohitpubnub
changed the title
mohitpubnub
changed the title
mohitpubnub
added
type: test
Contributor
Author
|
@coderabbitai review |
|
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis pull request adds three new integration test suites across separate test files: message routing between subscription sets, heartbeat behavior with duplicate channel deduplication, and token refresh across multiple PubNub instances with shared worker aggregation. Configuration values are parameterized with environment variables. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (7)
test/integration/endpoints/presence.test.ts (4)
1083-1083: Consider asserting channel groups are also deduplicated.The test subscribes to duplicate channel groups but only asserts on channels. Adding an assertion for
getSubscribedChannelGroups()would provide complete coverage.Suggested addition
assert.deepEqual(pubnubWithEE.getSubscribedChannels(), ['c1']); + assert.deepEqual(pubnubWithEE.getSubscribedChannelGroups(), ['cg1']);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/endpoints/presence.test.ts` at line 1083, Add an assertion to verify channel groups are deduplicated as well: after the existing assert.deepEqual(pubnubWithEE.getSubscribedChannels(), ['c1']), call and assert pubnubWithEE.getSubscribedChannelGroups() returns the expected deduplicated array (e.g., ['g1']) so the test covers both channels and channel groups for duplicates; update the test in presence.test.ts around the existing assertion for getSubscribedChannels().
918-920: Redundantnock.disableNetConnect()call in nestedbeforehook.The outer
beforehook at line 12-14 already callsnock.disableNetConnect(). This nestedbeforehook is redundant since nock'sdisableNetConnect()is global and persists across the entire test suite run.Suggested removal
- before(() => { - nock.disableNetConnect(); - }); - beforeEach(() => { nock.cleanAll();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/endpoints/presence.test.ts` around lines 918 - 920, The nested before hook calling nock.disableNetConnect() is redundant because the outer before (earlier in the file) already disables network connections globally; remove the inner before(() => { nock.disableNetConnect(); }); block in presence.test.ts so there is only the single global nock.disableNetConnect() invocation.
991-992: Consider adding final assertion after heartbeat wait.The test waits 4 seconds for heartbeat to trigger but doesn't assert anything afterward. Adding a final assertion would make the test's intent clearer and verify the subscription state remains consistent.
Suggested assertion
await new Promise((resolve) => setTimeout(resolve, 4000)); // wait for heartbeat to trigger + + // Verify subscription state remains deduplicated after heartbeats + assert.deepEqual(pubnubWithEE.getSubscribedChannels(), ['c1']); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/endpoints/presence.test.ts` around lines 991 - 992, After the 4s heartbeat wait add a final assertion to verify the presence subscription/state remained correct; for example, assert the subscription is still active (e.g., expect(presenceSubscription.isSubscribed).toBe(true)) and that the expected member(s) remain in the presence list (e.g., expect(presenceList).toContain(expectedUserId)) so the test confirms the heartbeat had no adverse effect on subscription state.
1155-1157: Sequential subscribes without status await may cause timing issues.Lines 1155-1156 call
subscribe()twice in rapid succession before awaitingsubscriptionChangedPromise. This may only capture one status change event, potentially making the test non-deterministic. Consider whether both calls should complete before asserting.The current approach may be intentional to test rapid subscription changes, but be aware that only one
PNSubscriptionChangedCategoryevent will resolve the promise, and the second subscribe may not have completed processing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/endpoints/presence.test.ts` around lines 1155 - 1157, The two back-to-back calls to pubnubWithEE.subscribe(...) can race and only trigger one PNSubscriptionChangedCategory resolution via subscriptionChangedPromise; fix by ensuring both subscribes complete before asserting — either combine into a single subscribe call (e.g., call pubnubWithEE.subscribe with both channels and channelGroups together), or await subscriptionChangedPromise after the first subscribe and then await a second subscriptionChangedPromise (or otherwise listen for two PNSubscriptionChangedCategory events) before proceeding; reference pubnubWithEE.subscribe and subscriptionChangedPromise (and the PNSubscriptionChangedCategory event) to locate where to add the extra await or merge the calls.test/integration/shared-worker/shared-worker.test.ts (2)
1685-1689: Listener subscription delay may be insufficient.The listener subscribes after a 1000ms delay, but the token update and subscription change sequence starts around 4000ms. If the listener isn't fully connected before messages are published, it may miss them. Consider waiting for listener's
PNConnectedCategorystatus before proceeding.Suggested approach
// Wait for listener to be connected before proceeding const listenerConnectedPromise = new Promise<void>((resolve) => { listenerInstance.addListener({ status: (statusEvent: any) => { if (statusEvent.category === PubNub.CATEGORIES.PNConnectedCategory) { resolve(); } }, message: (messageEvent: any) => { // ... existing message handler }, }); }); const listenerSubscription = listenerInstance.channel(testChannel).subscription(); listenerSubscription.subscribe(); await listenerConnectedPromise;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/shared-worker/shared-worker.test.ts` around lines 1685 - 1689, The listener may miss messages because it currently subscribes after a fixed 1s delay; instead wait for the listener to report PNConnectedCategory before proceeding. Update the setup around listenerInstance: attach an addListener handler that resolves a promise when statusEvent.category === PubNub.CATEGORIES.PNConnectedCategory (while keeping the existing message handler), call listenerInstance.channel(testChannel).subscription().subscribe() to start the connection, then await that promise before running the token update/subscription-change sequence; reference listenerInstance, addListener, PubNub.CATEGORIES.PNConnectedCategory, listenerSubscription (from channel(testChannel).subscription().subscribe()) to locate and modify the code.
1488-1498: Consider using a state machine pattern for complex test state.The test uses 9+ boolean flags (
tokenUpdateCompleted,messagePublished,messageReceived, etc.) to track state transitions. This could become difficult to maintain. A state machine or enum-based approach would make state transitions more explicit.This is acceptable for now given the complexity of what's being tested, but if this test needs modification in the future, consider refactoring to use a more structured state management approach.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/shared-worker/shared-worker.test.ts` around lines 1488 - 1498, The test currently tracks many independent boolean flags (interceptedRequests, testCompleted, errorOccurred, tokenUpdateCompleted, messagePublished, messageReceived, tokenRefreshVerified, initialHeartbeatVerified, updatedHeartbeatVerified, subscriptionChangeTriggered, initialSubscribeVerified) which makes state transitions hard to follow; replace these with a single explicit state machine or enum (e.g., TestState with values like INIT, TOKEN_UPDATED, MESSAGE_PUBLISHED, MESSAGE_RECEIVED, HEARTBEAT_INITIAL, HEARTBEAT_UPDATED, SUBSCRIPTION_CHANGED, COMPLETED, ERROR) and a single state variable (or a small state stack) and update that state in the same event handlers that currently flip the booleans; update assertions to check the enum/state transitions instead of multiple booleans and centralize transition logic (or use a tiny helper function applyTransition(currentState, event) to validate allowed transitions) so the test flow is clearer and easier to extend.test/integration/components/listeners.test.ts (1)
725-733: Listeners on individual subscriptions may never trigger.Lines 727-730 add
checkCompletionlisteners to individual subscriptions (subA,subB,subD,subscriptionC). Given the delegation behavior noted above, these listeners will not fire because the subscriptions delegate to their parent set whenparentSetsCount > 0.If the intent is to verify that individual subscriptions receive no messages when part of a set (which aligns with the SDK's current behavior), consider restructuring the test to:
- Only count messages from
subscriptionSet1(expect 3)- Verify individual subscription arrays remain empty (which the current assertions at lines 684-698 partially do)
💡 Suggested fix: Adjust message count expectation
// Track when we've received enough messages to verify the test let messageCount = 0; let testCompleted = false; const checkCompletion = () => { messageCount++; - // We expect messages for channels a, b, c - both to individual subscriptions and subscription set - // subD should receive NO messages since no message is sent to channel 'd' - if (messageCount >= 6 && !testCompleted) { - // 3 for set + 3 for individual subscriptions (a,b,c only) + // We expect messages for channels a, b, c - only on subscription set + // Individual subscriptions delegate to parent set when part of a set + if (messageCount >= 3 && !testCompleted) { testCompleted = true; try { - // Verify that each individual subscription receives messages for its respective channel - expect(channelASubscriptionMessagesReceived.length).to.equal(1); - expect((channelASubscriptionMessagesReceived[0].message as any).id).to.equal('msg-channel-a'); - expect(channelASubscriptionMessagesReceived[0].channel).to.equal('a'); - - expect(channelBSubscriptionMessagesReceived.length).to.equal(1); - expect((channelBSubscriptionMessagesReceived[0].message as any).id).to.equal('msg-channel-b'); - expect(channelBSubscriptionMessagesReceived[0].channel).to.equal('b'); - - expect(channelCSubscriptionMessagesReceived.length).to.equal(1); - expect((channelCSubscriptionMessagesReceived[0].message as any).id).to.equal('msg-channel-c'); - expect(channelCSubscriptionMessagesReceived[0].channel).to.equal('c'); + // Individual subscriptions delegate to parent set - they should receive NO messages directly + expect(channelASubscriptionMessagesReceived.length).to.equal(0, 'subA should delegate to parent set'); + expect(channelBSubscriptionMessagesReceived.length).to.equal(0, 'subB should delegate to parent set'); + expect(channelCSubscriptionMessagesReceived.length).to.equal(0, 'subscriptionC should delegate to parent set'); + expect(subDReceivedMessages.length).to.equal(0, 'subD should delegate to parent set');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/integration/components/listeners.test.ts` around lines 725 - 733, The test adds checkCompletion listeners to individual subscriptions (subA, subB, subD, subscriptionC) but those subscriptions delegate to their parent set when parentSetsCount > 0, so those listeners will never fire; update the test to only count messages from subscriptionSet1 (expecting 3) and assert that the individual subscription message arrays remain empty (verify subA/messages, subB/messages, subD/messages, subscriptionC/messages are unchanged) instead of relying on their listeners to be invoked; locate logic around subscriptionSet1.subscribe(), the checkCompletion listener and the individual subscriptions (subA, subB, subD, subscriptionC) to make the adjustments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/integration/components/listeners.test.ts`:
- Around line 725-733: The test adds checkCompletion listeners to individual
subscriptions (subA, subB, subD, subscriptionC) but those subscriptions delegate
to their parent set when parentSetsCount > 0, so those listeners will never
fire; update the test to only count messages from subscriptionSet1 (expecting 3)
and assert that the individual subscription message arrays remain empty (verify
subA/messages, subB/messages, subD/messages, subscriptionC/messages are
unchanged) instead of relying on their listeners to be invoked; locate logic
around subscriptionSet1.subscribe(), the checkCompletion listener and the
individual subscriptions (subA, subB, subD, subscriptionC) to make the
adjustments.
In `@test/integration/endpoints/presence.test.ts`:
- Line 1083: Add an assertion to verify channel groups are deduplicated as well:
after the existing assert.deepEqual(pubnubWithEE.getSubscribedChannels(),
['c1']), call and assert pubnubWithEE.getSubscribedChannelGroups() returns the
expected deduplicated array (e.g., ['g1']) so the test covers both channels and
channel groups for duplicates; update the test in presence.test.ts around the
existing assertion for getSubscribedChannels().
- Around line 918-920: The nested before hook calling nock.disableNetConnect()
is redundant because the outer before (earlier in the file) already disables
network connections globally; remove the inner before(() => {
nock.disableNetConnect(); }); block in presence.test.ts so there is only the
single global nock.disableNetConnect() invocation.
- Around line 991-992: After the 4s heartbeat wait add a final assertion to
verify the presence subscription/state remained correct; for example, assert the
subscription is still active (e.g.,
expect(presenceSubscription.isSubscribed).toBe(true)) and that the expected
member(s) remain in the presence list (e.g.,
expect(presenceList).toContain(expectedUserId)) so the test confirms the
heartbeat had no adverse effect on subscription state.
- Around line 1155-1157: The two back-to-back calls to
pubnubWithEE.subscribe(...) can race and only trigger one
PNSubscriptionChangedCategory resolution via subscriptionChangedPromise; fix by
ensuring both subscribes complete before asserting — either combine into a
single subscribe call (e.g., call pubnubWithEE.subscribe with both channels and
channelGroups together), or await subscriptionChangedPromise after the first
subscribe and then await a second subscriptionChangedPromise (or otherwise
listen for two PNSubscriptionChangedCategory events) before proceeding;
reference pubnubWithEE.subscribe and subscriptionChangedPromise (and the
PNSubscriptionChangedCategory event) to locate where to add the extra await or
merge the calls.
In `@test/integration/shared-worker/shared-worker.test.ts`:
- Around line 1685-1689: The listener may miss messages because it currently
subscribes after a fixed 1s delay; instead wait for the listener to report
PNConnectedCategory before proceeding. Update the setup around listenerInstance:
attach an addListener handler that resolves a promise when statusEvent.category
=== PubNub.CATEGORIES.PNConnectedCategory (while keeping the existing message
handler), call listenerInstance.channel(testChannel).subscription().subscribe()
to start the connection, then await that promise before running the token
update/subscription-change sequence; reference listenerInstance, addListener,
PubNub.CATEGORIES.PNConnectedCategory, listenerSubscription (from
channel(testChannel).subscription().subscribe()) to locate and modify the code.
- Around line 1488-1498: The test currently tracks many independent boolean
flags (interceptedRequests, testCompleted, errorOccurred, tokenUpdateCompleted,
messagePublished, messageReceived, tokenRefreshVerified,
initialHeartbeatVerified, updatedHeartbeatVerified, subscriptionChangeTriggered,
initialSubscribeVerified) which makes state transitions hard to follow; replace
these with a single explicit state machine or enum (e.g., TestState with values
like INIT, TOKEN_UPDATED, MESSAGE_PUBLISHED, MESSAGE_RECEIVED,
HEARTBEAT_INITIAL, HEARTBEAT_UPDATED, SUBSCRIPTION_CHANGED, COMPLETED, ERROR)
and a single state variable (or a small state stack) and update that state in
the same event handlers that currently flip the booleans; update assertions to
check the enum/state transitions instead of multiple booleans and centralize
transition logic (or use a tiny helper function applyTransition(currentState,
event) to validate allowed transitions) so the test flow is clearer and easier
to extend.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2e425cb1-2f7c-4c22-b826-ee1455ef1650
📒 Files selected for processing (3)
test/integration/components/listeners.test.tstest/integration/endpoints/presence.test.tstest/integration/shared-worker/shared-worker.test.ts
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
3 participants
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.
Summary by CodeRabbit