Fix integration tests #177
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
|
Warning Rate limit exceeded@lawrence-forooghian has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 1 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe pull request introduces changes primarily focused on renaming the Changes
Possibly related PRs
Suggested reviewers
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 (
|
lawrence-forooghian
force-pushed
the
fix-integration-tests
branch
from
e7e5e3b to
74cbbf4
Compare
![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 (5)
Sources/AblyChat/Message.swift (1)
12-12: LGTM! Property renaming is consistent.The renaming from
latestActiontoactionis applied consistently across the property declaration, initializer, and CodingKeys.Consider adding a comment to document that this is a temporary change pending resolution of issue #254 in the Ably specification repository.
Also applies to: 20-22, 33-33
Tests/AblyChatTests/MessageSubscriptionTests.swift (1)
Line range hint
4-24: Consider improving the mock implementation.The
MockPaginatedResultclass currently usesfatalErrorfor all its methods. While this might be sufficient for the current tests, consider implementing meaningful behavior for commonly used methods to make the tests more robust.Consider implementing at least the basic properties:
class MockPaginatedResult<T: Equatable>: PaginatedResult { private let mockItems: [T] private let mockHasNext: Bool init(items: [T] = [], hasNext: Bool = false) { self.mockItems = items self.mockHasNext = hasNext } var items: [T] { mockItems } var hasNext: Bool { mockHasNext } var isLast: Bool { !mockHasNext } // Keep fatalError for less commonly used methods var next: (any AblyChat.PaginatedResult<T>)? { fatalError("Not implemented") } var first: any AblyChat.PaginatedResult<T> { fatalError("Not implemented") } var current: any AblyChat.PaginatedResult<T> { fatalError("Not implemented") } }Sources/AblyChat/ChatAPI.swift (1)
Line range hint
4-5: Document the API version transition planThe coexistence of v1 and v2 API versions should be documented to help users understand the migration path.
Consider adding:
- A comment explaining the differences between v1 and v2
- Deprecation notice for v1
- Migration guide in README.md
Tests/AblyChatTests/IntegrationTests.swift (2)
120-138: Well-documented temporary fix with clear tracking!The documentation thoroughly explains the reason for the retry mechanism and includes a reference to the tracking issue #175 for future removal. This is a good practice for temporary workarounds.
Consider adding an estimated timeline or condition for removal in the comment, if known. For example: "Revert this once the materialised history system supports live history (tracked in #175)."
139-148: Add safeguards to the retry mechanismWhile this is a reasonable temporary workaround, consider adding some safeguards to prevent potential issues:
- Add a maximum retry count to prevent infinite loops
- Consider making the sleep duration configurable
- Add a total timeout duration
Here's a suggested improvement:
+ let maxRetries = 10 // Adjust as needed + let sleepDuration: UInt64 = NSEC_PER_SEC // 1 second + var retryCount = 0 let rxMessagesBeforeSubscribing = try await { while true { + if retryCount >= maxRetries { + throw NSError(domain: "IntegrationTests", + code: 1, + userInfo: [NSLocalizedDescriptionKey: "Failed to fetch messages after \(maxRetries) retries"]) + } 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) + retryCount += 1 + try await Task.sleep(nanoseconds: sleepDuration) } }()
📜 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(1 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/MessageTests.swift (1)
7-7: LGTM! Test cases updated consistently.
All test message instantiations have been correctly updated to use the new action property name while maintaining the original test coverage and functionality.
Also applies to: 18-18, 29-29, 73-73
Tests/AblyChatTests/MessageSubscriptionTests.swift (1)
29-29: LGTM! Message creation updated correctly.
The message instantiation in the map function has been properly updated to use the new action property name.
Example/AblyChatExample/Mocks/Misc.swift (1)
15-15: LGTM: Property rename is consistent with PR objectives
The change from latestAction to action aligns with the temporary fix mentioned in issue #169.
Let's verify the consistency of this change across the codebase:
✅ Verification successful
Property rename from latestAction to action is consistent across the codebase
The verification shows that:
- No instances of
latestActionremain in the codebase - The
actionproperty is consistently used in Message struct initialization across all files - The Message struct definition and all its usages align with the new property name
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining instances of 'latestAction' to ensure complete migration
rg "latestAction" --type swift
Length of output: 30
Script:
#!/bin/bash
# Let's first check if there are any Swift files in the repository
fd "\.swift$"
# Then search for both 'latestAction' and 'action' in Swift files to understand the context better
echo "=== Searching for 'action' property usage ==="
rg "action:" --type swift -B 2 -A 2
# Also check for Message type/struct definition
echo "=== Searching for Message definition ==="
ast-grep --pattern 'struct Message {
$$$
}'
Length of output: 22530
Tests/AblyChatTests/Mocks/MockHTTPPaginatedResponse.swift (1)
102-102: LGTM: Mock response data updated consistently
The mock response data correctly reflects the property rename from latestAction to action while maintaining the expected action type string format "message.create".
Also applies to: 112-112
Tests/AblyChatTests/ChatAPITests.swift (2)
44-44: LGTM: Test expectations updated consistently
The test expectations have been properly updated to use action instead of latestAction, maintaining test coverage for both message sending and retrieval scenarios.
Also applies to: 94-94, 104-104
Line range hint 94-108: Verify timestamp handling consistency
There's an inconsistency in timestamp handling between the two test messages - one has a specific timestamp while the other has nil. Consider documenting why this difference exists or standardizing the approach.
Let's check if this timestamp pattern is consistent with the actual implementation:
Sources/AblyChat/ChatAPI.swift (2)
Line range hint 49-57: LGTM: Message construction looks correct
The property renaming from latestAction to action is consistent with the changes across the codebase.
Line range hint 71-75: LGTM: Robust error handling implementation
The error handling correctly implements requirement CHA-M3e by converting REST API errors to ARTErrorInfo.
Example/AblyChatExample/Mocks/MockClients.swift (2)
Line range hint 107-114: LGTM: Mock message construction aligns with API changes
The property renaming is consistent with the core API changes.
Line range hint 133-140: LGTM: Mock message sending implementation is correct
The property renaming is consistent and the mock implementation properly simulates message sending.
Sources/AblyChat/DefaultMessages.swift (2)
Line range hint 71-85: LGTM: Comprehensive error handling for message validation
The error handling is thorough with specific error messages for each missing field.
Line range hint 86-94: LGTM: Message construction is complete and consistent
The property renaming is consistent with the API changes and includes all required fields.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
Tests/AblyChatTests/IntegrationTests.swift (1)
119-148: Consider adding safety bounds to the retry mechanism.The retry implementation is a good temporary solution for handling empty history results. However, to prevent potential infinite loops in edge cases, consider adding:
- A maximum retry count
- A total timeout duration
- A configurable sleep duration
Example improvement:
let rxMessagesBeforeSubscribing = try await { + let maxRetries = 10 // Maximum number of retry attempts + var attempts = 0 while true { + attempts += 1 + if attempts > maxRetries { + throw ChatError.custom(message: "Failed to fetch messages after \(maxRetries) attempts") + } let messages = try await rxMessageSubscription.getPreviousMessages(params: .init()) if !messages.items.isEmpty { return messages } try await Task.sleep(nanoseconds: NSEC_PER_SEC) } }()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
Tests/AblyChatTests/IntegrationTests.swift(1 hunks)
🔇 Additional comments (1)
Tests/AblyChatTests/IntegrationTests.swift (1)
137-137: Verify issue tracking for this temporary fix.
The comment references issue #175 for tracking the removal of this workaround.
✅ Verification successful
Issue #175 is still open and tracking the temporary fix
The referenced issue #175 "Remove wait for historical messages in integration tests" exists and is currently open, properly tracking the temporary workaround. The issue is also linked to a Jira task (ECO-5169) for internal tracking.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and status of the tracking issue
# Check if the issue exists and is still open
gh issue view 175 --json state,title,body
Length of output: 351
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`).
Fixes integration tests so that we can start merging other PRs.
latestActionkey #169 (renamingMessage.latestActiontoaction) until we get more clarity in Chat spec does not reflect latest message field names specification#254.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
latestActiontoactionin theMessagestruct.