core/test: revert occupancy test changes #383
Conversation
Now that underlying changes have been effected on the server, we no longer need to account for the +1 in occupancy, so these tests can be revereted to their previous state.
WalkthroughThe changes involve modifications to the expected occupancy values in integration tests for the occupancy functionality within chat rooms. Specifically, adjustments were made to the expected number of connections and presence members in two test files: one for general occupancy tests and another for the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ChatRoom
participant OccupancySystem
User->>ChatRoom: Join
ChatRoom->>OccupancySystem: Update occupancy (connections: 2)
OccupancySystem-->>ChatRoom: Confirm occupancy updated
ChatRoom-->>User: Notify occupancy updated (connections: 2)
User->>ChatRoom: Leave
ChatRoom->>OccupancySystem: Update occupancy (connections: 0)
OccupancySystem-->>ChatRoom: Confirm occupancy updated
ChatRoom-->>User: Notify occupancy updated (connections: 0)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
Coverage Report
File CoverageNo changed files found. |
![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
🧹 Outside diff range and nitpick comments (1)
test/react/hooks/use-occupancy.integration.test.tsx (1)
64-64: LGTM! The occupancy expectations are correctly adjusted.The changes align with the PR objectives to revert the occupancy test changes following server-side updates.
Consider adding a comment explaining how the expected occupancy values (3 connections, 2 presence members) are calculated:
+ // Expected occupancy: + // - connections: 3 (one connection each from chatClient, chatClientTwo, and chatClientThree) + // - presenceMembers: 2 (from roomTwo and roomThree presence.enter()) const expectedOccupancy = { connections: 3, presenceMembers: 2 };Also applies to: 70-70, 73-74
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
test/core/occupancy.integration.test.ts(3 hunks)test/react/hooks/use-occupancy.integration.test.tsx(1 hunks)
🔇 Additional comments (3)
test/core/occupancy.integration.test.ts (3)
101-105: LGTM: Clean state after detachment.
The expected count of 0 connections after all clients detach is correct, ensuring proper cleanup.
143-146: LGTM: Consistent with previous test case.
The expected connection count of 2 maintains consistency with the earlier test case and correctly reflects the number of attached clients.
90-92: LGTM: Connection count matches expected behavior.
The expected connection count of 2 is correct:
- Main client connection (1)
- Subscriber client connection (1)
Let's verify this behavior is consistent with the server implementation:
✅ Verification successful
Let me analyze the test files to understand the connection count patterns better.
Connection count of 2 is consistent across integration tests
The verification confirms that the connection count of 2 is correct and consistent with the test scenario where:
- A main client attaches to the channel
- A subscriber client attaches and enters presence
The test assertions align with similar scenarios in other integration tests, where each client connection is counted individually. The change from 3 to 2 connections reflects the correct behavior after server-side updates.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any other tests that might be asserting different connection counts
# to ensure consistency across the test suite
rg -l "connections.*:" test/ | xargs rg "connections:.*[0-9]+"
Length of output: 2858
Script:
#!/bin/bash
# Get more context around the integration tests to understand the test scenarios
rg -B 5 -A 5 "connections: [23]," test/core/occupancy.integration.test.ts
Length of output: 686
Context
N/A
Description
Now that underlying changes have been effected on the server, we no longer need to account for the +1 in occupancy, so these tests can be revereted to their previous state.
Checklist
Testing Instructions (Optional)
N/A
Summary by CodeRabbit
useOccupancyhook to ensure correct assertions and waiting conditions for occupancy events.