[WIP] 149 fix occupancy tests #179
Conversation
Upon our integration tests starting to fail, we noticed that Realtime seems to have renamed the `latestAction` property to `action`. This change has not yet been reflected in the spec, but has been in JS [1]. I’ve created an issue to get the spec updated [2] but since this test failure is stopping us from merging anything else I am going to rename this property until we get a more definitive answer. Part of #169. [1] ably/ably-chat-js#427 [2] ably/specification#254
This is intermittently failing with > ✘ Test basicIntegrationTest() recorded an issue at IntegrationTests.swift:162:9: Expectation failed: (rxOccupancyEventFromSubscription.presenceMembers → 0) == 1 This is presumably because the arbitrary 2 second wait isn’t enough. So, instead, wait until the subscription gives us the presence count that we’re expecting. Resolves #149.
Noticed that this seemed to be half-done (the comment next to the `Task.sleep` suggested there was meant to be an occupancy test to follow the `leave`).
WalkthroughThe pull request introduces a series of changes primarily focused on renaming the Changes
Possibly related PRs
Suggested reviewers
Poem
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 (
|
![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
🧹 Outside diff range and nitpick comments (4)
Tests/AblyChatTests/IntegrationTests.swift (1)
182-184: Correct indentation for better readabilityThe indentation inside the closure for
rxOccupancySubscription.firstis misaligned, which affects readability.Apply this diff to fix the indentation:
_ = try #require(await rxOccupancySubscription.first { occupancyEvent in - occupancyEvent.presenceMembers == 1 // 1 for txClient entering presence + occupancyEvent.presenceMembers == 1 // 1 for txClient entering presence })Example/AblyChatExample/Mocks/Misc.swift (1)
Line range hint
13-20: Ensure unique serials for mock messages to avoid duplicationUsing
Date().timeIntervalSince1970for theserialproperty may result in duplicate serials if messages are created quickly. This could cause issues in tests that rely on unique message serials.Consider using
UUID().uuidStringto ensure uniqueness:Array(repeating: 0, count: numberOfMockMessages).map { _ in Message( - serial: "\(Date().timeIntervalSince1970)", + serial: UUID().uuidString, action: .create, clientID: self.clientID, roomID: self.roomID, text: MockStrings.randomPhrase(), createdAt: Date(), metadata: [:], headers: [:] ) }Tests/AblyChatTests/Mocks/MockHTTPPaginatedResponse.swift (1)
Line range hint
73-78: Fix incorrect status code in successSendMessageThe mock response has a status code of 500 which indicates a server error, but the variable name suggests this should represent a successful response.
static let successSendMessage = MockHTTPPaginatedResponse( items: [ [ "serial": "3446456", "createdAt": 1_631_840_000_000, "text": "hello", ], ], - statusCode: 500, + statusCode: 200, headers: [:] )Sources/AblyChat/DefaultMessages.swift (1)
86-86: LGTM: Property rename with robust error handlingThe property rename is consistently applied in the production code. The surrounding error handling for required fields provides good validation before message creation.
Consider adding debug logging for the action value to help with troubleshooting:
+ logger.log(message: "Creating message with action: \(action)", level: .debug) let message = Message( serial: serial, action: action,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
Example/AblyChatExample/Mocks/Misc.swift(1 hunks)Example/AblyChatExample/Mocks/MockClients.swift(2 hunks)Sources/AblyChat/ChatAPI.swift(1 hunks)Sources/AblyChat/DefaultMessages.swift(1 hunks)Sources/AblyChat/Message.swift(2 hunks)Tests/AblyChatTests/ChatAPITests.swift(3 hunks)Tests/AblyChatTests/IntegrationTests.swift(2 hunks)Tests/AblyChatTests/MessageSubscriptionTests.swift(1 hunks)Tests/AblyChatTests/MessageTests.swift(4 hunks)Tests/AblyChatTests/Mocks/MockHTTPPaginatedResponse.swift(2 hunks)
🔇 Additional comments (12)
Tests/AblyChatTests/IntegrationTests.swift (1)
193-196: Verify occupancy updates after presence leave
After calling txRoom.presence.leave(data: nil), ensure that the occupancy count reflects the change. Verifying this confirms that the leave action is properly processed.
You can add an assertion to check the occupancy count:
// (8) Check that we received an updated presence count on the subscription
_ = try #require(await rxOccupancySubscription.first { occupancyEvent in
occupancyEvent.presenceMembers == 0 // 0 for txClient leaving presence
})
+// Verify occupancy count after leave
+let occupancyAfterLeave = try await rxRoom.occupancy.get()
+#expect(occupancyAfterLeave.presenceMembers == 0)Sources/AblyChat/Message.swift (1)
12-12: Ensure all references to latestAction are updated to action
The property latestAction has been renamed to action. Please verify that all usages of latestAction in the codebase have been updated to action to prevent any inconsistencies or runtime errors.
Run the following script to find any remaining references to latestAction:
Also applies to: 20-22, 33-33
✅ Verification successful
Property rename from latestAction to action has been properly implemented
The verification shows that there are no remaining references to latestAction in the codebase, and the new action property is correctly used in the Message.swift file for property declaration, initialization, and pattern matching. All references have been successfully updated.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any occurrences of 'latestAction' in the codebase
rg 'latestAction'
Length of output: 17
Script:
#!/bin/bash
# Let's verify both the old and new property names in Swift files
echo "Searching for 'latestAction' in Swift files:"
rg -t swift 'latestAction'
echo -e "\nSearching for 'action' property in Message.swift:"
rg -t swift 'action' Sources/AblyChat/Message.swift
Length of output: 565
Tests/AblyChatTests/MessageTests.swift (1)
7-7: Updates to action property are consistent
The test cases have been correctly updated to reflect the renaming of latestAction to action, maintaining consistency with the Message struct changes.
Also applies to: 18-18, 29-29, 73-73
Tests/AblyChatTests/MessageSubscriptionTests.swift (1)
29-29: Instantiation of Message updated appropriately
The change from latestAction to action in the Message instantiation aligns with the updated Message struct.
Tests/AblyChatTests/Mocks/MockHTTPPaginatedResponse.swift (1)
102-102: LGTM: Property renaming is consistent
The renaming of "latestAction" to "action" in the mock responses aligns with the broader refactoring changes across the codebase.
Also applies to: 112-112
Tests/AblyChatTests/ChatAPITests.swift (1)
44-44: LGTM: Test cases properly updated
The test cases have been correctly updated to use the new action property name while maintaining comprehensive test coverage for both success and error scenarios.
Also applies to: 94-94, 104-104
Sources/AblyChat/ChatAPI.swift (3)
Line range hint 49-56: LGTM: Message initialization updated correctly
The Message initialization has been properly updated to use the new action property name.
Line range hint 1-124: Verify PR title and changes alignment
The PR title mentions fixing occupancy tests, but the changes primarily focus on renaming the action property in message-related code. Consider updating the PR title to better reflect the actual changes, or verify if occupancy test fixes are missing from this PR.
✅ Verification successful
PR title accurately reflects the changes
The PR title mentioning occupancy test fixes is accurate. The diff shows significant changes to occupancy-related tests in IntegrationTests.swift, including:
- Improved test structure for occupancy verification
- Added more specific assertions for presence member counts
- Removed arbitrary sleep delay in favor of proper event verification
- Enhanced test readability with better variable naming
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for occupancy-related changes
rg -l "occupancy" --type swift
# Check commit messages for context
git log --oneline | grep -i "occupancy"
Length of output: 568
Script:
#!/bin/bash
# Check the diff to see actual changes in occupancy-related files
git diff HEAD~1 -- $(rg -l "occupancy" --type swift)
# Look for test files with occupancy in their name
fd -e swift occupancy
Length of output: 2378
Line range hint 5-6: Address API version transition TODO
There's a TODO comment about removing v1 after full transition to v2. This should be tracked and addressed to avoid maintaining two API versions indefinitely.
Would you like me to create a GitHub issue to track the API version transition task?
Example/AblyChatExample/Mocks/MockClients.swift (2)
107-107: LGTM: Property rename is consistent
The renaming of latestAction to action aligns with the broader refactoring changes across the codebase.
133-133: LGTM: Consistent property rename in message creation
The property rename is consistently applied in the message creation logic of the send method.
Sources/AblyChat/DefaultMessages.swift (1)
Line range hint 1-1: Verify PR scope and title
The PR title mentions fixing occupancy tests, but the changes reviewed are primarily about renaming the latestAction property to action. Consider updating the PR title to better reflect these changes, or verify if there are additional changes related to occupancy tests that should be included.
| let rxMessagesBeforeSubscribing = try await { | ||
| while true { | ||
| let messages = try await rxMessageSubscription.getPreviousMessages(params: .init()) | ||
| if !messages.items.isEmpty { | ||
| return messages | ||
| } | ||
| // Wait 1 second before retrying the history fetch | ||
| try await Task.sleep(nanoseconds: NSEC_PER_SEC) | ||
| } | ||
| }() |
There was a problem hiding this comment.
Prevent infinite loop by adding a timeout or maximum retries
The while true loop in your retry mechanism could cause the test to hang indefinitely if the condition is never met. To prevent this, consider adding a timeout or a maximum number of retries.
Apply this diff to introduce a maximum retry limit:
let rxMessagesBeforeSubscribing = try await {
+ let maxRetries = 10
+ var retries = 0
while true {
let messages = try await rxMessageSubscription.getPreviousMessages(params: .init())
if !messages.items.isEmpty {
return messages
}
// Wait 1 second before retrying the history fetch
try await Task.sleep(nanoseconds: NSEC_PER_SEC)
+ retries += 1
+ if retries >= maxRetries {
+ throw NSError(domain: "IntegrationTests", code: 1, userInfo: [NSLocalizedDescriptionKey: "Failed to retrieve messages after \(maxRetries) attempts"])
+ }
}
}()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let rxMessagesBeforeSubscribing = try await { | |
| while true { | |
| let messages = try await rxMessageSubscription.getPreviousMessages(params: .init()) | |
| if !messages.items.isEmpty { | |
| return messages | |
| } | |
| // Wait 1 second before retrying the history fetch | |
| try await Task.sleep(nanoseconds: NSEC_PER_SEC) | |
| } | |
| }() | |
| let rxMessagesBeforeSubscribing = try await { | |
| let maxRetries = 10 | |
| var retries = 0 | |
| while true { | |
| let messages = try await rxMessageSubscription.getPreviousMessages(params: .init()) | |
| if !messages.items.isEmpty { | |
| return messages | |
| } | |
| // Wait 1 second before retrying the history fetch | |
| try await Task.sleep(nanoseconds: NSEC_PER_SEC) | |
| retries += 1 | |
| if retries >= maxRetries { | |
| throw NSError(domain: "IntegrationTests", code: 1, userInfo: [NSLocalizedDescriptionKey: "Failed to retrieve messages after \(maxRetries) attempts"]) | |
| } | |
| } | |
| }() |
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
Documentation