[ECO-5126] Connection tests #231
Conversation
WalkthroughThis pull request enhances the debugging functionality in the connection management system by introducing transient disconnect timer debug events. The changes include new structures, methods, and variables in the connection class to handle and emit debug events when connection states change. Additionally, the pull request adds comprehensive tests and updates the mock connection to support asynchronous state transitions and callback management for improved testing. Changes
Sequence Diagram(s)sequenceDiagram
actor Tester as Test Suite
participant DC as DefaultConnection
participant MC as MockConnection
Tester->>DC: testsOnly_subscribeToTransientDisconnectTimerEvents()
MC->>DC: transitionToState(CONNECTED)
DC->>Tester: Emit debug event (timer inactive)
MC->>DC: transitionToState(DISCONNECTED)
DC->>Tester: Emit debug event (timer active)
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (12)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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: 0
🧹 Nitpick comments (16)
Tests/AblyChatTests/Mocks/MockHTTPPaginatedResponse.swift (1)
43-43: Clarify the negation inisLast.
Using!_hasNextis conceptually straightforward. Consider renaming_hasNextto_hasMorePagesfor improved readability.Tests/AblyChatTests/DefaultRoomReactionsTests.swift (1)
79-96: Add edge-case validation for malformed messages.
Rejecting malformed messages is covered here, which is excellent. Consider adding more complex malformation scenarios (e.g., missing keys) to ensure robust error handling.Tests/AblyChatTests/DefaultRoomTypingTests.swift (1)
295-300: Pending implementation for multiple presence events.
This is marked TODO. If needed, we can offer a test approach to ensure only the most recent event is dispatched.Tests/AblyChatTests/Mocks/MockConnection.swift (2)
71-80: Fix SwiftLint style issues with multiline parameters/argumentsThe implementation of
transitionToStateis good, but there are some style issues flagged by SwiftLint.-func transitionToState(_ state: ARTRealtimeConnectionState, - event: ARTRealtimeConnectionEvent, - error: ARTErrorInfo? = nil) { - let stateChange = ARTConnectionStateChange(current: state, - previous: self.state, - event: event, - reason: error) +func transitionToState( + _ state: ARTRealtimeConnectionState, + event: ARTRealtimeConnectionEvent, + error: ARTErrorInfo? = nil +) { + let stateChange = ARTConnectionStateChange( + current: state, + previous: self.state, + event: event, + reason: error + )🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 75-75: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
[Error] 78-78: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
[Error] 72-72: Multiline parameters should have their surrounding brackets in a new line
(multiline_parameters_brackets)
[Error] 74-74: Multiline parameters should have their surrounding brackets in a new line
(multiline_parameters_brackets)
[Error] 71-71: Lines should not have trailing whitespace
(trailing_whitespace)
71-71: Remove trailing whitespaceThere's trailing whitespace on this line.
- +🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 71-71: Lines should not have trailing whitespace
(trailing_whitespace)
Sources/AblyChat/DefaultConnection.swift (2)
110-110: Remove or properly comment the commented-out codeThere's a commented line with an explanation that could be clearer or removed entirely.
-// subscription.emit(statusChange) // this call shouldn't be here - "Not withstanding CHA-CS5a" means just that I guess. +// Note: The emit call was removed to properly respect CHA-CS5a requirement about transient disconnect suppression
125-139: Fix SwiftLint attribute placement and whitespace issuesThe debug-only functionality looks good, but there are style issues.
#if DEBUG internal struct TransientDisconnectTimerEvent: Equatable { internal let active: Bool } /// Subscription of transient disconnect timer events for testing purposes. - @MainActor - private var transientDisconnectTimerSubscriptions: [Subscription<TransientDisconnectTimerEvent>] = [] + @MainActor private var transientDisconnectTimerSubscriptions: [Subscription<TransientDisconnectTimerEvent>] = [] /// Returns a subscription which emits transient disconnect timer events for testing purposes. @MainActor internal func testsOnly_subscribeToTransientDisconnectTimerEvents() -> Subscription<TransientDisconnectTimerEvent> { let subscription = Subscription<TransientDisconnectTimerEvent>(bufferingPolicy: .unbounded) transientDisconnectTimerSubscriptions.append(subscription) return subscription } - #endif + #endif🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 130-130: Attributes should be on their own lines in functions and types, but on the same line as variables and imports
(attributes)
Tests/AblyChatTests/DefaultConnectionTests.swift (2)
41-44: Fix bracket placement in multiline argument lists.The static analysis detected multiline argument bracket placement issues. Consider updating these to follow Swift style guidelines.
- subscription.emit(.init(current: .disconnected, - previous: .connecting, - error: connectionError, - retryIn: 1)) // arbitrary values + subscription.emit(.init( + current: .disconnected, + previous: .connecting, + error: connectionError, + retryIn: 1 + )) // arbitrary valuesAlso applies to: 88-91, 137-140
🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 41-41: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
[Error] 44-44: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
65-65: Consider avoiding force casts.The static analysis flagged force casts to
DefaultConnection. While these are likely safe in test code, consider usingas?with a guard statement or assertion for better safety and to follow best practices.- let defaultConnection = client.connection as! DefaultConnection + guard let defaultConnection = client.connection as? DefaultConnection else { + XCTFail("Expected DefaultConnection instance") + return + }Also applies to: 114-114, 160-160, 198-198, 235-235
🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 65-65: Force casts should be avoided
(force_cast)
Tests/AblyChatTests/Mocks/MockRealtimePresence.swift (4)
4-8: Consider documenting the@unchecked SendableusageThe
@unchecked Sendableannotation indicates you're guaranteeing thread safety without compiler verification. It would be helpful to add a comment explaining why this is safe in this context, similar to how it's documented in the MockRealtimeChannel class.
53-56: Force unwrapping could lead to crashesThe
currentMember!force unwrapping might crash if there's an issue with member creation. Consider using optional chaining or guard statements to handle the case where currentMember might be nil.- func enterClient(_ clientId: String, data: Any?) { - currentMember = ARTPresenceMessage(clientId: clientId, data: data) - members.append(currentMember!) - currentMember!.action = .enter - subscribeCallback?(currentMember!) + func enterClient(_ clientId: String, data: Any?) { + currentMember = ARTPresenceMessage(clientId: clientId, data: data) + guard let member = currentMember else { return } + members.append(member) + member.action = .enter + subscribeCallback?(member)
59-65: Force unwrapping could lead to crashesSimilar to the previous method, this implementation also contains force unwrapping that could be made safer.
- func enterClient(_ clientId: String, data: Any?, callback: ARTCallback? = nil) { - currentMember = ARTPresenceMessage(clientId: clientId, data: data) - members.append(currentMember!) - callback?(nil) - currentMember!.action = .enter - subscribeCallback?(currentMember!) + func enterClient(_ clientId: String, data: Any?, callback: ARTCallback? = nil) { + currentMember = ARTPresenceMessage(clientId: clientId, data: data) + guard let member = currentMember else { + callback?(ARTErrorInfo.create(withCode: 0, message: "Failed to create presence message")) + return + } + members.append(member) + callback?(nil) + member.action = .enter + subscribeCallback?(member)
26-32: Consider implementing unimplemented methodsSeveral methods throw
fatalError("Not implemented"). While this works for the current tests, it could lead to unexpected crashes if any tests call these methods in the future. Consider implementing basic functionality or at least providing stubs that don't crash.Also applies to: 43-50, 110-118
Tests/AblyChatTests/DefaultMessagesTests.swift (1)
358-359: Incorrect spec reference in the commentThe comment indicates the spec reference should be CHA-M4k but the code uses CHA-M5k. This should be corrected to maintain consistency with the rest of the documentation.
- // Wrong name, should be CHA-M4k - // @spec CHA-M5k + // @spec CHA-M4kTests/AblyChatTests/Mocks/MockRealtimeChannel.swift (2)
137-139: Incorrect error message in performStateDetachCallbacksThe error message refers to "attachResult" when it should refer to "detachResult".
- fatalError("attachResult must be set before attach is called") + fatalError("detachResult must be set before detach is called")
200-217: Potentially unsafe type casting in JSON parsingThe JSON message parsing contains several unsafe operations, including force unwrapping of optional values and unchecked type casting. Consider adding more robust error handling to prevent potential runtime crashes.
if let ts = json["timestamp"] as? String { - message.timestamp = Date(timeIntervalSince1970: TimeInterval(ts)!) + if let timeInterval = TimeInterval(ts) { + message.timestamp = Date(timeIntervalSince1970: timeInterval) + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
AblyChat-Package.xctestplan(1 hunks)Sources/AblyChat/DefaultConnection.swift(4 hunks)Sources/AblyChat/DefaultMessages.swift(4 hunks)Sources/AblyChat/DefaultRoomReactions.swift(2 hunks)Sources/AblyChat/DefaultTyping.swift(5 hunks)Sources/AblyChat/Dependencies.swift(1 hunks)Sources/AblyChat/RoomFeature.swift(1 hunks)Tests/AblyChatTests/ChatAPITests.swift(0 hunks)Tests/AblyChatTests/DefaultConnectionTests.swift(1 hunks)Tests/AblyChatTests/DefaultMessagesTests.swift(5 hunks)Tests/AblyChatTests/DefaultRoomOccupancyTests.swift(1 hunks)Tests/AblyChatTests/DefaultRoomPresenceTests.swift(1 hunks)Tests/AblyChatTests/DefaultRoomReactionsTests.swift(1 hunks)Tests/AblyChatTests/DefaultRoomTypingTests.swift(1 hunks)Tests/AblyChatTests/Helpers/Helpers.swift(1 hunks)Tests/AblyChatTests/Mocks/MockConnection.swift(4 hunks)Tests/AblyChatTests/Mocks/MockHTTPPaginatedResponse.swift(4 hunks)Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift(8 hunks)Tests/AblyChatTests/Mocks/MockRealtimePresence.swift(1 hunks)Tests/AblyChatTests/Mocks/MockRoomLifecycleContributor.swift(2 hunks)Tests/AblyChatTests/Mocks/MockRoomLifecycleContributorChannel.swift(7 hunks)
💤 Files with no reviewable changes (1)
- Tests/AblyChatTests/ChatAPITests.swift
✅ Files skipped from review due to trivial changes (1)
- AblyChat-Package.xctestplan
🧰 Additional context used
🪛 SwiftLint (0.57.0)
Tests/AblyChatTests/Mocks/MockConnection.swift
[Error] 75-75: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
[Error] 78-78: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
[Error] 72-72: Multiline parameters should have their surrounding brackets in a new line
(multiline_parameters_brackets)
[Error] 74-74: Multiline parameters should have their surrounding brackets in a new line
(multiline_parameters_brackets)
[Error] 71-71: Lines should not have trailing whitespace
(trailing_whitespace)
Sources/AblyChat/DefaultConnection.swift
[Error] 130-130: Attributes should be on their own lines in functions and types, but on the same line as variables and imports
(attributes)
[Error] 140-140: Don't include vertical whitespace (empty line) before closing braces
(vertical_whitespace_closing_braces)
Tests/AblyChatTests/DefaultConnectionTests.swift
[Error] 65-65: Force casts should be avoided
(force_cast)
[Error] 114-114: Force casts should be avoided
(force_cast)
[Error] 160-160: Force casts should be avoided
(force_cast)
[Error] 198-198: Force casts should be avoided
(force_cast)
[Error] 235-235: Force casts should be avoided
(force_cast)
[Error] 41-41: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
[Error] 44-44: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
[Error] 88-88: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
[Error] 91-91: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
[Error] 137-137: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
[Error] 140-140: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
[Error] 14-14: Lines should not have trailing whitespace
(trailing_whitespace)
[Error] 18-18: Lines should not have trailing whitespace
(trailing_whitespace)
[Error] 6-6: Don't include vertical whitespace (empty line) after opening braces
(vertical_whitespace_opening_braces)
🔇 Additional comments (57)
Sources/AblyChat/Dependencies.swift (1)
28-28: New protocol added for presence functionality.The new
RealtimePresenceProtocolappears to be properly defined, conforming to bothARTRealtimePresenceProtocolandSendable. This follows the existing pattern of other protocols in this file and prepares the groundwork for connection testing.Tests/AblyChatTests/Mocks/MockRoomLifecycleContributor.swift (2)
4-4: Added EmitsDiscontinuities protocol conformance.Correctly updated the
MockRoomLifecycleContributorto conform to both protocols, which aligns with the changes inRoomFeature.swift.
19-21: Implementation needed for onDiscontinuity method.The
fatalErrorimplementation is appropriate for mocks that don't expect this method to be called during tests. However, if you plan to test connection discontinuities, you'll need to implement this method properly.Do you plan to test this functionality? If so, consider implementing a proper mock that returns a configurable subscription instead of using
fatalError.Sources/AblyChat/RoomFeature.swift (1)
66-66: Good refactoring to use protocol composition.Changing from a concrete type to
any RoomLifecycleContributor & EmitsDiscontinuitiesimproves flexibility and testability. This allows for easier mocking in tests while maintaining the required functionality.Sources/AblyChat/DefaultRoomReactions.swift (2)
81-85: Added debugging support for malformed messages.Good enhancement for testing - emitting malformed messages to special test subscriptions while properly guarding with
#if DEBUGto keep this out of production code.
106-116: Well-designed test functionality for malformed messages.The implementation for test subscriptions is clean and follows best practices:
- Clear naming with
testsOnly_prefix- Proper documentation comments
- Conditional compilation with
#if DEBUG- Simple implementation that integrates with the existing error handling
This will be valuable for connection testing scenarios.
Tests/AblyChatTests/Mocks/MockHTTPPaginatedResponse.swift (3)
13-13: Ensure consistent usage of thehasNextflag in all invocations.
Removing theisLastparameter simplifies the constructor. Just confirm that places expecting the old parameter have been updated accordingly.
117-118: Confirm thathasNext: truematches pagination expectations.
DefininghasNext: truehere is logical if the test expects subsequent pages. This looks correct.
128-149: Validate mock responses for next-page items.
The structure of next-page items (clientId, serial, action, text) appears consistent. Just ensure these item fields align with any real or expected data schema in dependent tests.Tests/AblyChatTests/DefaultRoomReactionsTests.swift (1)
46-77: Test name and logic readability improvements.
Renaming the method tosubscriptionCanBeRegisteredToReceiveReactionEvents()is more descriptive and aligns well with the tested functionality. The concurrency usage appears correct.Tests/AblyChatTests/DefaultRoomTypingTests.swift (11)
1-4: Imports and test struct setup.
The initial setup is straightforward. Marking the test struct with@testable import AblyChatis appropriate.
5-21: Check concurrency usage inretrieveCurrentlyTypingClientIDs.
This test method is clearly organized and uses async/await correctly to fetch typing client IDs.
23-52: Attach-state synchronization looks good.
Waiting for theATTACHINGstate before callingget()ensures proper lifecycle sequencing. Great approach.
54-95: Robust error handling for attach failures.
Catching and verifying the attach error is handled well. Confirm that non-attach errors also propagate as expected.
97-118: Validate presence operation preconditions.
Correctly throws an error if the room is in an invalid state. This aligns with the specification for presence operations.
120-149: Timeout configuration for typing events.
The test effectively confirms that a shorter timeout triggers a stop event promptly. Nice approach with a tolerance to handle CI timing fluctuations.
151-189: Starting and stopping typing is clearly tested.
Ensuring idempotency when stopping typing multiple times is beneficial. Great coverage.
191-217: Extending typing timeout logic is verified.
The re-invocation ofstart()resets the timer. The test checks the final stop time accurately.
219-237: Typing event subscription is validated.
This covers the main success path for receiving typing updates. Looks solid.
239-293: Presence event triggers presence.get()
Good verification that the presence retrieval is invoked upon presence events. The backoff approach is tested thoroughly.
301-319: Discontinuity coverage.
Verifying that a discontinuity is propagated ensures resilience and consistency across feature channels.Tests/AblyChatTests/Mocks/MockConnection.swift (4)
4-4: Good use of@unchecked Sendableto support concurrencyAdding
@unchecked Sendableto the class declaration is appropriate since this mock class will be used in concurrent contexts during testing.
17-17: Good addition of the stateCallback propertyThis property is essential for storing the connection state change callback and will enable mock state transitions in tests.
47-50: LGTM: Properly implemented callback registrationThe implementation correctly stores the callback and returns an event listener, which is necessary for simulating state changes in tests.
64-66: Callback cleanup is implemented correctlySetting the callback to nil when unsubscribing is the right approach.
Sources/AblyChat/DefaultMessages.swift (3)
9-11: Good use of conditional conformance to Sendable for ARTMessageUsing
@retroactive @unchecked SendableforARTMessagein DEBUG builds enables better testing while keeping it isolated to non-production code.
61-116: Improved error handling with structured do-catchRefactoring the error handling to use a structured do-catch block makes the code more maintainable and enables proper handling of malformed messages.
The addition of emitting malformed messages to debug subscriptions is a good approach for testing purposes.
135-145: Good implementation of test-only debugging functionalityThe DEBUG-conditional implementation of malformed message subscriptions provides valuable testing capabilities without affecting production code.
Tests/AblyChatTests/DefaultRoomOccupancyTests.swift (3)
5-31: Well-structured test for occupancy information retrievalThis test correctly validates the ability to fetch occupancy information with proper mocking and assertions.
The test clearly follows the Given-When-Then pattern and includes references to specifications via
@specannotations.
33-54: Good test for real-time occupancy subscriptionThis test effectively validates that users can subscribe to real-time occupancy updates, which is an important part of the connection testing functionality.
56-74: Thorough test for discontinuity event handlingThe test properly validates that discontinuity events are propagated correctly through the occupancy system.
Sources/AblyChat/DefaultConnection.swift (4)
62-66: Good implementation of debug event emission for disconnect timer cancellationEmitting debug events when the transient disconnect timer is canceled improves testability.
75-79: Proper debug event for timer activationEmitting debug events when the transient disconnect timer starts allows for better testing of connection state transitions.
84-88: Consistent timer expiry debug event emissionThe debug event for timer expiry properly notifies subscribers that the timer is no longer active.
101-105: Complete coverage of timer cancellation scenariosAll scenarios where the timer could be canceled are properly covered with debug events.
Tests/AblyChatTests/Mocks/MockRoomLifecycleContributorChannel.swift (3)
8-8: Great enhancement for mocking network interactions.The addition of the
underlyingChannelproperty and related initialization parameters allows for more realistic testing by connecting mock objects in a hierarchy. This provides better simulation capabilities for real-world network scenarios.Also applies to: 18-25, 51-51
98-101: Nice addition of network delay simulation capability.The simulated network delay feature with configurable milliseconds is a great enhancement for testing timeout and race conditions. This allows for more realistic testing of asynchronous behavior.
131-133: Proper state change propagation.Using a separate Task to perform the state change callbacks on the underlying channel ensures that the state propagation happens asynchronously, which better simulates real-world behavior. This is a good pattern for handling event propagation in testing code.
Sources/AblyChat/DefaultTyping.swift (4)
9-9: Good improvement: Parameterized retry duration.Making the maximum presence get retry duration configurable rather than hardcoded increases flexibility and testability of the component. This aligns well with the spec requirement (CHA-T6c1).
Also applies to: 12-12, 18-18
43-43: Updated retry logic to use configurable parameter.The retry loop now correctly uses the configurable parameter instead of a hardcoded value, and the log message has been updated to reflect this change. This enhances consistency and makes the code more maintainable.
Also applies to: 82-82
47-51: Well-structured test event emission.The introduction of test event subscriptions with proper DEBUG conditional compilation directives is a good approach. These event points will allow for comprehensive testing without affecting production code.
Also applies to: 75-79, 173-177, 227-231
237-282: Comprehensive test infrastructure.The addition of test-specific structures and subscription mechanisms, all properly isolated with DEBUG directives, demonstrates good separation of concerns between production and test code. The
@unchecked Sendableextension is appropriately guarded to prevent production impact.Also applies to: 285-287
Tests/AblyChatTests/Helpers/Helpers.swift (4)
25-32: Useful convenience initializer for test data.This convenience initializer for
ARTPresenceMessagesimplifies test code by reducing boilerplate when creating test instances. Good choice of defaults for optional parameters.
34-41: Handy shortcut for comprehensive presence event testing.The static
.allproperty provides a convenient way to access all presence event types, making it easier to write exhaustive tests that cover all possible events.
43-93: Excellent test helper implementation.The
RoomLifecycleHelperenum provides a comprehensive set of factory methods and utilities that will significantly reduce test code duplication and improve maintainability. The comment on line 85 about replacing duplicates elsewhere should be addressed in a future PR.Is there an issue tracking the TODO on line 85 to replace duplicates of the
waitForManagerfunction elsewhere in the codebase?
95-99: Useful numeric comparison extension.This extension on
Doubleadds a tolerance-based equality check, which is essential for reliable floating-point comparisons in tests, especially when dealing with time-based operations.Tests/AblyChatTests/DefaultConnectionTests.swift (5)
26-55: Well-structured test for connection status observation.The test correctly verifies that clients can register for connection status updates and that the emitted status change contains the expected information as per spec requirements CHA-CS4a, CHA-CS4b, and CHA-CS4c.
🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 41-41: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
[Error] 44-44: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
57-105: Good test for transient disconnect timer behavior.This test properly validates that when connection transitions from connected to disconnected, a transient disconnect timer is initiated (CHA-CS5a1) and that the appropriate status changes are emitted upon timer expiration (CHA-CS5a4).
🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 65-65: Force casts should be avoided
(force_cast)
[Error] 88-88: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
[Error] 91-91: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
107-151: Thorough test for status change during connecting state.The test effectively validates spec requirement CHA-CS5a2, confirming that no status change is emitted when connection goes from disconnected to connecting while a transient disconnect timer is active.
🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 114-114: Force casts should be avoided
(force_cast)
[Error] 137-137: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
[Error] 140-140: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
153-189: Comprehensive tests for various connection state transitions.These tests thoroughly validate spec requirement CHA-CS5a3 for different scenarios (connected, suspended, failed), ensuring that the transient disconnect timer is canceled and appropriate status changes are emitted as expected.
Also applies to: 191-227, 228-264
🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 160-160: Force casts should be avoided
(force_cast)
5-24:✅ Verification successful
Comprehensive connection status tests with good spec references.
The test properly validates the initial connection status exposure (CHA-CS2a, CHA-CS2b, CHA-CS3) with clear expectations. The TODO comment on line 21 correctly identifies a potential issue that needs to be revisited.
The test references GitHub issue #49. Please check if this issue is still open and relevant to the TODO comment on line 21:
🏁 Script executed:
#!/bin/bash gh issue view 49 --repo ably/ably-chat-swiftLength of output: 1572
Attention: Confirmed Concurrency Issue Relevance
The test correctly verifies the initial connection status (and error) as expected, given the asynchronous firing of actor events. Our check confirmed that GitHub issue #49 ("Address concurrency-related issues not addressed by feature spec") is still open—which reinforces the validity of the TODO comment on line 21. No immediate changes are required, but please be aware that this area should be revisited when addressing the concurrency concerns discussed in the issue.
- Verified that issue #49 remains open and relevant.
- The asynchronous nature of
DefaultConnectionis acknowledged, and the test expectations are appropriate for now.🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 14-14: Lines should not have trailing whitespace
(trailing_whitespace)
[Error] 18-18: Lines should not have trailing whitespace
(trailing_whitespace)
[Error] 6-6: Don't include vertical whitespace (empty line) after opening braces
(vertical_whitespace_opening_braces)
Tests/AblyChatTests/DefaultRoomPresenceTests.swift (1)
5-486: Well-structured comprehensive test suite for presence functionality!This test file thoroughly covers all the necessary presence-related functionality in a chat room context, with clear test cases for entering, leaving, updating presence states, and handling error conditions. The test structure consistently follows the Given/When/Then pattern, making the tests readable and maintainable.
Tests/AblyChatTests/DefaultMessagesTests.swift (3)
218-219: Review iOS/tvOS version constraintsThe
@available(iOS 16.0.0, tvOS 16.0.0, *)annotations limit these tests to newer OS versions. Consider if there's a way to restructure these tests to support earlier OS versions, especially if your app targets those versions.Also applies to: 275-276
13-24: Tests have been properly updated to use more specific mocksThe tests now use more specific mock implementations and have better validation of expected behavior, which improves the reliability of the test suite.
317-320: Document untestable specsThe comments indicate some specs are currently untestable. Consider creating a dedicated section in your documentation listing these cases and explaining why they can't be tested, to ensure they're not overlooked in the future.
Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (2)
8-9: Good improvement in mocking presence functionalityThe addition of the
mockPresenceproperty and implementation of thepresenceproperty is a significant improvement, replacing afatalErrorwith actual functionality that can be used in tests.Also applies to: 13-14
117-163: Well-structured state management refactoringThe refactoring of state management with the addition of callback handling methods improves the maintainability and clarity of the code.
maratal
force-pushed
the
124-connection-tests
branch
from
37a0d7a to
acb8dfd
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
Tests/AblyChatTests/Mocks/MockConnection.swift (1)
71-80: Fix formatting issues in the newtransitionToStatemethod.The method provides a crucial functionality for testing connection state transitions, but there are some formatting issues flagged by SwiftLint.
Apply these formatting fixes to follow the project style:
- - func transitionToState(_ state: ARTRealtimeConnectionState, - event: ARTRealtimeConnectionEvent, - error: ARTErrorInfo? = nil) { - let stateChange = ARTConnectionStateChange(current: state, - previous: self.state, - event: event, - reason: error) - stateCallback?(stateChange) - } + func transitionToState( + _ state: ARTRealtimeConnectionState, + event: ARTRealtimeConnectionEvent, + error: ARTErrorInfo? = nil + ) { + let stateChange = ARTConnectionStateChange( + current: state, + previous: self.state, + event: event, + reason: error + ) + stateCallback?(stateChange) + }🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 75-75: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
[Error] 78-78: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
[Error] 72-72: Multiline parameters should have their surrounding brackets in a new line
(multiline_parameters_brackets)
[Error] 74-74: Multiline parameters should have their surrounding brackets in a new line
(multiline_parameters_brackets)
[Error] 71-71: Lines should not have trailing whitespace
(trailing_whitespace)
Tests/AblyChatTests/DefaultConnectionTests.swift (2)
5-6: Good test structure setup - consider removing extra blank line.The test struct is well organized with clear spacing between test methods, but there's an extra blank line after the opening brace that could be removed according to SwiftLint.
struct DefaultConnectionTests { - // @spec CHA-CS2a🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 6-6: Don't include vertical whitespace (empty line) after opening braces
(vertical_whitespace_opening_braces)
65-65: Consider avoiding force casts.Force casts (
as!) are used multiple times to access theDefaultConnectionimplementation. While this works in a controlled test environment, consider using a safer approach.- let defaultConnection = client.connection as! DefaultConnection + guard let defaultConnection = client.connection as? DefaultConnection else { + XCTFail("Expected DefaultConnection instance") + return + }Alternatively, if you're certain about the type and want to keep the tests concise, add a helper function to the test struct that encapsulates this pattern.
Also applies to: 114-114, 160-160, 198-198, 235-235
🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 65-65: Force casts should be avoided
(force_cast)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Sources/AblyChat/DefaultConnection.swift(4 hunks)Tests/AblyChatTests/DefaultConnectionTests.swift(1 hunks)Tests/AblyChatTests/Mocks/MockConnection.swift(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/AblyChat/DefaultConnection.swift
🧰 Additional context used
🪛 SwiftLint (0.57.0)
Tests/AblyChatTests/DefaultConnectionTests.swift
[Error] 65-65: Force casts should be avoided
(force_cast)
[Error] 114-114: Force casts should be avoided
(force_cast)
[Error] 160-160: Force casts should be avoided
(force_cast)
[Error] 198-198: Force casts should be avoided
(force_cast)
[Error] 235-235: Force casts should be avoided
(force_cast)
[Error] 41-41: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
[Error] 44-44: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
[Error] 88-88: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
[Error] 91-91: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
[Error] 137-137: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
[Error] 140-140: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
[Error] 14-14: Lines should not have trailing whitespace
(trailing_whitespace)
[Error] 18-18: Lines should not have trailing whitespace
(trailing_whitespace)
[Error] 6-6: Don't include vertical whitespace (empty line) after opening braces
(vertical_whitespace_opening_braces)
Tests/AblyChatTests/Mocks/MockConnection.swift
[Error] 75-75: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
[Error] 78-78: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
[Error] 72-72: Multiline parameters should have their surrounding brackets in a new line
(multiline_parameters_brackets)
[Error] 74-74: Multiline parameters should have their surrounding brackets in a new line
(multiline_parameters_brackets)
[Error] 71-71: Lines should not have trailing whitespace
(trailing_whitespace)
🔇 Additional comments (12)
Tests/AblyChatTests/Mocks/MockConnection.swift (4)
4-4: Good addition of Sendable conformance.Adding
@unchecked Sendableconformance allows this mock to be safely used across concurrency domains in the tests. This aligns well with the PR's objective of implementing connection tests.
17-17: LGTM - well-encapsulated callback mechanism.The addition of this private property provides a clean way to store the connection state callback.
47-50: Good implementation of theoncallback method.This implementation properly stores the callback and returns an event listener, making the mock more functional for testing.
65-65: LGTM - properly clears the callback.Appropriate implementation to remove the callback when
offis called.Tests/AblyChatTests/DefaultConnectionTests.swift (8)
11-24: Well-structured test for basic connection status.This test thoroughly verifies that the chat client correctly exposes its initial connection status. Good use of comments to explain the test's Given-When-Then structure and specification references.
🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 14-14: Lines should not have trailing whitespace
(trailing_whitespace)
[Error] 18-18: Lines should not have trailing whitespace
(trailing_whitespace)
32-55: Good test for connection status observation.The test effectively verifies that clients can register for connection status events and receive updates with the correct information. The use of
#requiremakes failure cases clear.Fix the multiline formatting for better style consistency:
- subscription.emit(.init(current: .disconnected, - previous: .connecting, - error: connectionError, - retryIn: 1)) // arbitrary values + subscription.emit(.init( + current: .disconnected, + previous: .connecting, + error: connectionError, + retryIn: 1 + )) // arbitrary values🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 41-41: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
[Error] 44-44: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
60-105: Thorough test for transient disconnect timer behavior.Excellent test implementation that verifies when a connection goes from connected to disconnected, the transient disconnect timer starts appropriately. The test contains clear sections and validations.
🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 65-65: Force casts should be avoided
(force_cast)
[Error] 88-88: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
[Error] 91-91: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
109-151: Good test for verifying no status change during specific transitions.This test effectively verifies that no status change is emitted when the connection goes from disconnected to connecting while a transient disconnect timer is active.
🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 114-114: Force casts should be avoided
(force_cast)
[Error] 137-137: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
[Error] 140-140: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
155-189: Well-structured test for connection status transitioning to connected.This test properly verifies that when the connection transitions to connected state, the transient disconnect timer is canceled and the correct status change is emitted.
🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 160-160: Force casts should be avoided
(force_cast)
193-226: Good test for connection status transitioning to suspended.This test properly verifies that when the connection transitions to suspended state, the transient disconnect timer is canceled and the correct status change is emitted.
🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 198-198: Force casts should be avoided
(force_cast)
230-264: Well-implemented test for connection status transitioning to failed.This test thoroughly validates that when the connection transitions to failed state, the transient disconnect timer is canceled and the correct status change with error information is emitted.
🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 235-235: Force casts should be avoided
(force_cast)
1-265: Overall excellent test implementation for connection state handling.This comprehensive test suite thoroughly covers the connection status requirements specified in the PR objectives. The tests methodically verify different connection state transitions and their effects on the transient disconnect timer. The tests are well-structured with clear comments referencing specifications.
Consider addressing the formatting issues throughout the file for multiline argument brackets and the force casts, but otherwise, this is a solid implementation.
🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 65-65: Force casts should be avoided
(force_cast)
[Error] 114-114: Force casts should be avoided
(force_cast)
[Error] 160-160: Force casts should be avoided
(force_cast)
[Error] 198-198: Force casts should be avoided
(force_cast)
[Error] 235-235: Force casts should be avoided
(force_cast)
[Error] 41-41: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
[Error] 44-44: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
[Error] 88-88: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
[Error] 91-91: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
[Error] 137-137: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
[Error] 140-140: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
[Error] 14-14: Lines should not have trailing whitespace
(trailing_whitespace)
[Error] 18-18: Lines should not have trailing whitespace
(trailing_whitespace)
[Error] 6-6: Don't include vertical whitespace (empty line) after opening braces
(vertical_whitespace_opening_braces)
maratal
force-pushed
the
124-connection-tests
branch
from
acb8dfd to
7db847b
Compare
maratal
force-pushed
the
124-connection-tests
branch
from
07c17e5 to
7db847b
Compare
maratal
force-pushed
the
124-connection-tests
branch
from
7db847b to
a749b70
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
Sources/AblyChat/DefaultConnection.swift (2)
125-132: Move the @mainactor attribute to its own line.SwiftLint is flagging an issue with the attribute placement.
- @MainActor - private var transientDisconnectTimerSubscriptions: [Subscription<TransientDisconnectTimerEvent>] = [] + @MainActor + private var transientDisconnectTimerSubscriptions: [Subscription<TransientDisconnectTimerEvent>] = []🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 132-132: Attributes should be on their own lines in functions and types, but on the same line as variables and imports
(attributes)
125-141: New debug events mechanism enhances testability.The new
TransientDisconnectTimerEventstructure and subscription system provides a clean way to test transient disconnect timer behavior, which is properly limited to DEBUG builds only.However, there's a code style issue with the
@MainActorattribute placement:- @MainActor - private var transientDisconnectTimerSubscriptions: [Subscription<TransientDisconnectTimerEvent>] = [] + @MainActor private var transientDisconnectTimerSubscriptions: [Subscription<TransientDisconnectTimerEvent>] = []🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 132-132: Attributes should be on their own lines in functions and types, but on the same line as variables and imports
(attributes)
Tests/AblyChatTests/DefaultConnectionTests.swift (4)
64-64: Consider using conditional unwrapping instead of force casting.There are multiple instances of force casting with
as!which could be replaced with conditional unwrapping for safer code. While this may be acceptable in tests, it's generally better to use safer alternatives.For example:
-let defaultConnection = client.connection as! DefaultConnection +guard let defaultConnection = client.connection as? DefaultConnection else { + #fail("Expected DefaultConnection type") + return +}Also applies to: 113-113, 159-159, 197-197, 234-234
🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 64-64: Force casts should be avoided
(force_cast)
40-43: Fix multiline arguments brackets formatting.SwiftLint is flagging issues with the multiline arguments formatting. Consider reformatting to match the project style guidelines.
For example:
-subscription.emit(.init(current: .disconnected, - previous: .connecting, - error: connectionError, - retryIn: 1)) // arbitrary values +subscription.emit( + .init( + current: .disconnected, + previous: .connecting, + error: connectionError, + retryIn: 1 + ) +) // arbitrary valuesAlso applies to: 87-90, 136-139
🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 40-40: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
[Error] 43-43: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
64-64: Consider avoiding force casts in tests.The tests use force casts (
as!) to access theDefaultConnectioninstance. While this is common in test code, it's generally better to use conditional casts with appropriate error handling.-let defaultConnection = client.connection as! DefaultConnection +guard let defaultConnection = client.connection as? DefaultConnection else { + throw TestError("Expected connection to be DefaultConnection") +}Also applies to: 113-113, 159-159, 197-197, 234-234
🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 64-64: Force casts should be avoided
(force_cast)
40-43: Fix multiline arguments formatting.The multiline arguments in these method calls should have their surrounding brackets on new lines according to the style guidelines:
-subscription.emit(.init(current: .disconnected, - previous: .connecting, - error: connectionError, - retryIn: 1)) // arbitrary values +subscription.emit( + .init( + current: .disconnected, + previous: .connecting, + error: connectionError, + retryIn: 1 + ) +) // arbitrary valuesAlso applies to: 87-90, 136-139
🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 40-40: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
[Error] 43-43: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Sources/AblyChat/DefaultConnection.swift(4 hunks)Tests/AblyChatTests/DefaultConnectionTests.swift(1 hunks)Tests/AblyChatTests/Mocks/MockConnection.swift(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Tests/AblyChatTests/Mocks/MockConnection.swift
🧰 Additional context used
🪛 SwiftLint (0.57.0)
Tests/AblyChatTests/DefaultConnectionTests.swift
[Error] 64-64: Force casts should be avoided
(force_cast)
[Error] 113-113: Force casts should be avoided
(force_cast)
[Error] 159-159: Force casts should be avoided
(force_cast)
[Error] 197-197: Force casts should be avoided
(force_cast)
[Error] 234-234: Force casts should be avoided
(force_cast)
[Error] 40-40: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
[Error] 43-43: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
[Error] 87-87: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
[Error] 90-90: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
[Error] 136-136: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
[Error] 139-139: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
Sources/AblyChat/DefaultConnection.swift
[Error] 132-132: Attributes should be on their own lines in functions and types, but on the same line as variables and imports
(attributes)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: Example app, tvOS (Xcode 16)
- GitHub Check: Example app, iOS (Xcode 16)
- GitHub Check: Example app, macOS (Xcode 16)
- GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16) - GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16) - GitHub Check: Xcode, tvOS (Xcode 16)
- GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16) - GitHub Check: Xcode, iOS (Xcode 16)
- GitHub Check: SPM,
releaseconfiguration (Xcode 16) - GitHub Check: Xcode, macOS (Xcode 16)
- GitHub Check: SPM (Xcode 16)
🔇 Additional comments (18)
Sources/AblyChat/DefaultConnection.swift (5)
62-66: Good addition of debug events for the transient disconnect timer.The code adds debug events to track when the transient disconnect timer is activated or deactivated, which improves testability without affecting production code.
Also applies to: 75-79, 84-88, 101-105
110-110: Removing unnecessary commented-out code improves readability.Good cleanup of the commented-out code. The comment itself explains why this function call shouldn't be there.
125-141: Well-structured test-only functionality.The addition of
TransientDisconnectTimerEventand the test-only subscription method is well-designed:
- It's properly encapsulated within
#if DEBUGto keep it out of production code- The method is clearly named with
testsOnly_prefix- The subscription management is handled appropriately
This enables effective testing of timer state without affecting production code.
🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 132-132: Attributes should be on their own lines in functions and types, but on the same line as variables and imports
(attributes)
110-110: Removed commented code improves clarity.Good cleanup of the commented-out line that was causing confusion. The comment correctly points out that "Not withstanding CHA-CS5a" doesn't imply emitting the status change here.
62-66: Well-placed debug events for comprehensive timer state tracking.The new debug events are strategically placed at all timer state transition points, making it possible to track the complete lifecycle of the transient disconnect timer. Wrapping in
#if DEBUGensures these don't affect production code.Also applies to: 75-79, 84-88, 101-105
Tests/AblyChatTests/DefaultConnectionTests.swift (13)
1-4: Good test setup with proper imports.The test file has the proper imports, including the
@testable import AblyChatto access internal components.
10-23: Well-structured test for CHA-CS2a, CHA-CS2b, and CHA-CS3 specifications.The test properly verifies that the chat client exposes its current connection status and error. The comments provide good context about the expected behavior.
31-54: Good test for connection status observation.This test properly verifies that clients can register for connection status events and receive them with the correct information.
🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 40-40: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
[Error] 43-43: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
59-104: Comprehensive test for transient disconnect timer behavior.This test thoroughly verifies the behavior when a connection goes from connected to disconnected, ensuring that:
- The transient disconnect timer starts correctly
- The timer duration is appropriate (5 seconds)
- The connection status and error information is handled correctly
The test demonstrates good use of the new timer event subscription functionality.
🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 64-64: Force casts should be avoided
(force_cast)
[Error] 87-87: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
[Error] 90-90: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
108-150: Good test for CHA-CS5a2 specification.This test properly verifies that no status change is emitted when the connection goes from disconnected to connecting while a transient disconnect timer is active.
🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 113-113: Force casts should be avoided
(force_cast)
[Error] 136-136: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
[Error] 139-139: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
154-188: Good test for CHA-CS5a3 specification when connection goes to connected.This test effectively verifies that the transient disconnect timer is canceled and the correct status change is emitted when the connection transitions to connected.
🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 159-159: Force casts should be avoided
(force_cast)
192-225: Good test for CHA-CS5a3 specification when connection goes to suspended.This test properly verifies that the transient disconnect timer is canceled and the correct status change is emitted when the connection transitions to suspended.
🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 197-197: Force casts should be avoided
(force_cast)
229-263: Good test for CHA-CS5a3 specification when connection goes to failed.This test thoroughly verifies that the transient disconnect timer is canceled and the correct status change (including error information) is emitted when the connection transitions to failed.
🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 234-234: Force casts should be avoided
(force_cast)
5-23: Comprehensive tests for connection status functionality.The test verifies that the chat client correctly exposes its connection status as required by specs CHA-CS2a, CHA-CS2b, and CHA-CS3.
Note the TODO comment on line 20 correctly identifies that there might be timing issues due to the asynchronous nature of the initialization that should be revisited with issue #49.
25-54: Good test coverage for connection status observation.This test ensures that clients can register listeners for connection status events and receive all required information in those events, covering specs CHA-CS4a through CHA-CS4d.
🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 40-40: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
[Error] 43-43: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
56-104: Important timer state verification.This test verifies that when connection transitions from connected to disconnected, the transient disconnect timer starts correctly and expires after 5 seconds as specified in CHA-CS5a1 and CHA-CS5a4.
🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 64-64: Force casts should be avoided
(force_cast)
[Error] 87-87: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
[Error] 90-90: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
106-150: Proper verification of status change suppression.The test confirms that when a transient disconnect timer is active, no status change is emitted if the connection changes to connecting or disconnected states, per spec CHA-CS5a2.
🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 113-113: Force casts should be avoided
(force_cast)
[Error] 136-136: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
[Error] 139-139: Multiline arguments should have their surrounding brackets in a new line
(multiline_arguments_brackets)
152-188: Effective tests for timer cancellation scenarios.These tests thoroughly verify that the transient disconnect timer is properly cancelled when the connection status changes to connected, suspended, or failed states, and that the appropriate status change events are emitted, per spec CHA-CS5a3.
Also applies to: 190-225, 227-263
🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 159-159: Force casts should be avoided
(force_cast)
maratal
force-pushed
the
124-connection-tests
branch
from
a749b70 to
4e9f70f
Compare
maratal
force-pushed
the
124-connection-tests
branch
from
4e9f70f to
c4a2750
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
Tests/AblyChatTests/DefaultConnectionTests.swift (3)
94-94: Potential flaky test with time-dependent assertionThis assertion depends on timing and assumes the timer will have expired after the preceding code completes. This could lead to occasional test failures if execution is faster than expected.
Consider using a more deterministic approach, such as mocking the timer or using a callback approach to verify timer completion. Alternatively, you could add a small buffer to account for timing inconsistencies:
- #expect(Date().timeIntervalSince1970 - timerStartedAt >= 5) + #expect(Date().timeIntervalSince1970 - timerStartedAt >= 4.9) // slightly less than 5 to account for timing variations
86-88: Consider refactoring the artificial status change emission patternSeveral tests emit artificial status changes to unblock the test flow. While this works, it creates a somewhat convoluted test flow that might be difficult to understand.
Consider refactoring the status observation mechanism in tests to provide better control over the test flow. This could involve:
- Using a completion handler pattern
- Creating a more testable interface that doesn't require artificial events
- Implementing a test-specific mechanism to wait for status changes
179-179: Potential flaky assertion with strict timing checkThe assertion
#expect(Date().timeIntervalSince1970 - timerStartedAt < 1)could be flaky in slower execution environments.Consider a more flexible approach:
- #expect(Date().timeIntervalSince1970 - timerStartedAt < 1) + #expect(Date().timeIntervalSince1970 - timerStartedAt < 2) // Allow more time for slower execution environments
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Sources/AblyChat/DefaultConnection.swift(4 hunks)Tests/AblyChatTests/DefaultConnectionTests.swift(1 hunks)Tests/AblyChatTests/Mocks/MockConnection.swift(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Sources/AblyChat/DefaultConnection.swift
- Tests/AblyChatTests/Mocks/MockConnection.swift
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: Example app, tvOS (Xcode 16)
- GitHub Check: Example app, iOS (Xcode 16)
- GitHub Check: Example app, macOS (Xcode 16)
- GitHub Check: Xcode, tvOS (Xcode 16)
- GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16) - GitHub Check: Xcode, iOS (Xcode 16)
- GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16) - GitHub Check: Xcode, macOS (Xcode 16)
- GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16) - GitHub Check: SPM (Xcode 16)
- GitHub Check: SPM,
releaseconfiguration (Xcode 16)
🔇 Additional comments (4)
Tests/AblyChatTests/DefaultConnectionTests.swift (4)
1-262: Well-structured test suite for connection managementThis new test suite provides comprehensive coverage for the
DefaultChatClientconnection functionality. The tests follow a clear "Given-When-Then" structure and include good documentation referencing specs.
21-21: Remember to address the TODO commentThere's a TODO comment referencing issue #49 that should be revisited once that issue is resolved.
58-101: Excellent specification-driven test coverageThe test provides detailed coverage of the transient disconnect timer behavior, ensuring it starts correctly when connection transitions from connected to disconnected and emits the expected status changes.
147-184: Good test for connection status transitionsThis test thoroughly validates that the transient disconnect timer is canceled when the connection goes to connected state and that the correct status change is emitted.
|
just the one comment... also worth checking the failing tests (are they flaky or is it down to the parallel runs you mentioned in standup?) |
Sometimes they fail (up to 1/5 of the time), but I don't want to touch it while changes in that code are still ongoing. Without parallel run, yes, most likely they succeed. |
maratal
force-pushed
the
86-messages-tests
branch
from
2aeac57 to
5bf2afd
Compare
maratal
force-pushed
the
124-connection-tests
branch
from
c4a2750 to
36f77e5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
Tests/AblyChatTests/DefaultConnectionTests.swift (3)
58-101: Consider addressing potential test flakiness with timer-based assertions.This test relies on an actual 5-second timer to complete, which could make it flaky in CI environments. The assertion on line 94 checks if time has passed, but doesn't directly verify that the timer fired correctly.
Consider refactoring the test to use a controlled time source or mock the timer functionality to make the test more reliable and faster. You could:
- Inject a mock timer that can be advanced programmatically
- Or use a dependency that allows controlling the timing without waiting
This would also reduce the test execution time significantly, as you wouldn't need to wait the full 5 seconds.
167-183: Improve time-dependent assertion reliability.The assertion on line 179 checks if less than 1 second has passed, but it doesn't directly verify that the timer was canceled. This approach is time-dependent and could be flaky.
Instead of relying on elapsed time to verify timer cancellation, consider adding explicit cancellation detection. For example, check for a cancellation event or verify the timer's state directly.
-// The library shall cancel the transient disconnect timer (less than 5 seconds -> was cancelled) -#expect(Date().timeIntervalSince1970 - timerStartedAt < 1) +// Verify the transient disconnect timer was canceled +let timerCancellationEvent = try #require(await transientTimerSubscription.first { !$0.active }) +#expect(!timerCancellationEvent.active)
5-262: Consider extracting common test setup to reduce duplication.Several test methods share similar setup code for creating mock connections and subscriptions. This pattern is repeated across multiple tests.
Consider extracting the common setup into a helper method to reduce duplication and make the tests more maintainable:
private func setupTestEnvironment() async throws -> ( MockConnection, DefaultChatClient, DefaultConnection, Subscription<TransientDisconnectTimerEvent>, Subscription<ConnectionStatusChange> ) { let realtimeConnection = MockConnection(state: .connected) let realtime = MockRealtime(createWrapperSDKProxyReturnValue: .init(connection: realtimeConnection)) let client = DefaultChatClient(realtime: realtime, clientOptions: nil) let defaultConnection = try #require(client.connection as? DefaultConnection) let transientTimerSubscription = await defaultConnection.testsOnly_subscribeToTransientDisconnectTimerEvents() let statusSubscription = defaultConnection.onStatusChange() return (realtimeConnection, client, defaultConnection, transientTimerSubscription, statusSubscription) }Then use it in your tests:
@Test func whenConnectionGoesToConnectedStatusChangeShouldBeEmitted() async throws { // Given: let (realtimeConnection, _, _, transientTimerSubscription, statusSubscription) = try await setupTestEnvironment() // When: let timerStartedAt = Date().timeIntervalSince1970 realtimeConnection.transitionToState(.disconnected, event: .disconnected) // Rest of the test... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Sources/AblyChat/DefaultConnection.swift(4 hunks)Tests/AblyChatTests/DefaultConnectionTests.swift(1 hunks)Tests/AblyChatTests/Mocks/MockConnection.swift(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Tests/AblyChatTests/Mocks/MockConnection.swift
- Sources/AblyChat/DefaultConnection.swift
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: Example app, tvOS (Xcode 16)
- GitHub Check: Example app, iOS (Xcode 16)
- GitHub Check: Xcode, tvOS (Xcode 16)
- GitHub Check: Example app, macOS (Xcode 16)
- GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16) - GitHub Check: Xcode, iOS (Xcode 16)
- GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16) - GitHub Check: SPM (Xcode 16)
- GitHub Check: Xcode, macOS (Xcode 16)
- GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16) - GitHub Check: SPM,
releaseconfiguration (Xcode 16)
🔇 Additional comments (9)
Tests/AblyChatTests/DefaultConnectionTests.swift (9)
26-26: Consider using the @specUntested annotation for clarity.For consistency with other spec annotations, consider using the
@specUntestedannotation for the CHA-CS4e and CHA-CS4f specifications that are currently untestable.-// CHA-CS4e, CHA-CS4f - currently untestable due to subscription is removed once the object is removed from memory +// @specUntested CHA-CS4e, CHA-CS4f // Untestable due to subscription being removed once the object is removed from memory
94-94: Make timer verification more robust.This assertion verifies that time has passed but doesn't directly confirm that the timer fired correctly. This could lead to false positives if the test is simply slow.
Consider adding an explicit verification that the timer completed as expected, perhaps by checking for a completion event or state change that's directly caused by the timer expiration.
1-4: LGTM - Appropriate imports.The imports are appropriate for the test file, including the required framework imports and the testable import for the module under test.
5-9: Good use of spec annotations to document test coverage.The spec annotations clearly link test cases to their corresponding specifications, which is excellent for traceability.
10-24: Well-structured initial test with clear Given-When-Then pattern.This test effectively verifies the initial connection status and error state with good comments explaining the test flow.
32-53: Good test for connection status observation.The test properly verifies that clients can register for and receive connection status updates with appropriate assertions for all required properties.
103-145: Comprehensive test for connected-to-disconnected transition.This test effectively verifies that no status change is emitted when the connection goes from disconnected to connecting while a transient disconnect timer is active. The test is well-structured and includes appropriate assertions.
186-222: Robust test for suspension state transition.The test properly verifies that the transient disconnect timer is canceled and the appropriate status change is emitted when the connection transitions to suspended.
224-261: Thorough test for connection failure handling.This test properly verifies that the connection failure is handled correctly, with the timer being canceled and appropriate error information being included in the status update.
maratal
force-pushed
the
124-connection-tests
branch
from
36f77e5 to
3b2f333
Compare
There was a problem hiding this comment.
I still have 2 flaking on my local branch when running them all concurrently, however they now always fail for the same reason (they seem to have attachSerial instead of channelSerial for the first serial validation checks we do across both tests)...
I've removed these assertions as we don't really care about the initial state of the fromSerial for the purpose of these tests (we only care they're reset appropriately to attachSerial - this part of the tests always works now)...
Happy for this to be merged and I'll follow up with mine shortly :)
lawrence-forooghian
left a comment
lawrence-forooghian
left a comment
There was a problem hiding this comment.
I was surprised to find that we've been introducing flaky tests into the Chat unit tests. We need to address the existing flaky ones and not write more.
Anything that involves a timeout is going to:
- be a potential source of flakiness
- increase our test execution time
I don't think we should be writing tests that actually have a 5-second wait, or any wait for that matter. Can we follow the approach taken for the room lifecycle manager, where we use a mock clock to perform the wait?
Should I revert messages tests PR for that matter? @lawrence-forooghian FYI @umair-ably |
|
I think we need to revisit the recently-added tests (#197, #217, #219, this PR) to make sure that they do not do any of the following:
If there are flaky tests on |
|
Opened #236 for reverting. |
|
See #263 |
Closes #124
Summary by CodeRabbit
New Features
Bug Fixes
Tests