[ECO-5251] Presence tests (single channel) #239
Conversation
WalkthroughThis change restores and expands presence-related unit tests, introduces new test helpers, and enhances mock classes to support thorough testing of presence operations. It adds a thread-safe method call recorder, convenience initializers, and deep equality utilities, and updates test annotations for specification traceability. No production code is modified except for a documentation comment update. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Case
participant MockPresence as MockRealtimePresence
participant Recorder as MockMethodCallRecorder
Test->>MockPresence: Call presence operation (e.g., enter, update, get)
MockPresence->>Recorder: addRecord(signature, arguments)
MockPresence-->>Test: Return stubbed result (success or empty)
Test->>Recorder: hasRecord(signature, arguments) (assertion)
Assessment against linked issues
Suggested reviewers
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)Tests/AblyChatTests/DefaultPresenceTests.swift (1)⏰ Context from checks skipped due to timeout of 90000ms (34)
🔇 Additional comments (11)
✨ Finishing Touches
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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
maratal
changed the title
DefaultRoomLifecycleManager.DefaultRoomLifecycleManager.
maratal
changed the title
DefaultRoomLifecycleManager.DefaultRoomLifecycleManager.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
Tests/AblyChatTests/Helpers/Helpers.swift (1)
54-61: Helpful extension for test iterationsThe static
allproperty for presence event types provides a convenient way to test across all event types. Consider adding a descriptive comment to explain its testing use case.extension [PresenceEventType] { + /// Contains all possible presence event types for use in comprehensive testing static let all = [ PresenceEventType.present, PresenceEventType.enter, PresenceEventType.leave, PresenceEventType.update, ] }Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift (2)
8-8: Fix trailing whitespaceRemove the trailing whitespace on these lines to satisfy style guidelines.
private let resultOfWaitToBeAbleToPerformPresenceOperations: Result<Void, ARTErrorInfo>? - + private(set) var attachCallCount = 0 private(set) var detachCallCount = 0 private(set) var releaseCallCount = 0 private(set) var waitCallCount = 0 - +Also applies to: 13-13
🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 8-8: Lines should not have trailing whitespace
(trailing_whitespace)
17-21: Fix multiline parameters brackets styleAccording to SwiftLint, multiline parameters should have their surrounding brackets on new lines.
- init(attachResult: Result<Void, ARTErrorInfo>? = nil, - detachResult: Result<Void, ARTErrorInfo>? = nil, - roomStatus: RoomStatus? = nil, - resultOfWaitToBeAblePerformPresenceOperations: Result<Void, ARTErrorInfo>? = nil - ) { + init( + attachResult: Result<Void, ARTErrorInfo>? = nil, + detachResult: Result<Void, ARTErrorInfo>? = nil, + roomStatus: RoomStatus? = nil, + resultOfWaitToBeAblePerformPresenceOperations: Result<Void, ARTErrorInfo>? = nil + ) {🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 17-17: Multiline parameters should have their surrounding brackets in a new line
(multiline_parameters_brackets)
Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (3)
8-12: Consider makingmockPresencea regular optional or initializing it in the constructor.
UsingMockRealtimePresence!is convenient, but if it remainsnilat runtime, calls topresencewill crash. A safer approach is to use a regular optional and handle nil cases, or ensuremockPresenceis always initialized in the initializer.
36-37: Refine default parameters in initializer.
You currently have optional parametersmessageToEmitOnSubscribeandmockPresenceset tonilby default. If these remainnilduring test usage, referencing them could cause unexpected behavior. Consider either requiring non-nil arguments for stricter typing or adding checks to ensure safe usage.Also applies to: 45-45
190-192: Consider extending the mock to track channel state changes.
Returning a newARTEventListener()is fine for a basic mock, but if you eventually need to verify state transitions, storing or invoking the provided closure could be beneficial for testing.Tests/AblyChatTests/Mocks/MockRealtimePresence.swift (1)
4-129: Guard against potential data races with shared state.
This mock class is marked@unchecked Sendableand manipulatesmembersandsubscribeCallbackwithout synchronization. If these methods are called concurrently from multiple tasks, you risk data races. Though it may be sufficient for single-threaded test code, consider adding locking or concurrency-safe mechanisms if test scenarios require concurrency correctness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
Sources/AblyChat/RoomFeature.swift(1 hunks)Tests/AblyChatTests/DefaultRoomPresenceTests.swift(1 hunks)Tests/AblyChatTests/Helpers/Helpers.swift(1 hunks)Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift(5 hunks)Tests/AblyChatTests/Mocks/MockRealtimePresence.swift(2 hunks)Tests/AblyChatTests/Mocks/MockRoomLifecycleContributor.swift(2 hunks)Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift(2 hunks)
🧰 Additional context used
🪛 SwiftLint (0.57.0)
Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift
[Error] 17-17: Multiline parameters should have their surrounding brackets in a new line
(multiline_parameters_brackets)
[Error] 8-8: Lines should not have trailing whitespace
(trailing_whitespace)
[Error] 13-13: Lines should not have trailing whitespace
(trailing_whitespace)
⏰ 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,
releaseconfiguration, tvOS (Xcode 16) - GitHub Check: Example app, macOS (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 (9)
Tests/AblyChatTests/Mocks/MockRoomLifecycleContributor.swift (1)
4-4: Protocol conformance added appropriatelyGood addition of the
EmitsDiscontinuitiesprotocol to the mock contributor, which aligns with the changes inDefaultFeatureChannel.Sources/AblyChat/RoomFeature.swift (1)
66-66: Good type abstraction improvementChanging the contributor type from a concrete implementation to a protocol composition (
any RoomLifecycleContributor & EmitsDiscontinuities) improves flexibility and testability. This allows for better dependency injection and supports the goal of testing without usingDefaultRoomLifecycleManager.Tests/AblyChatTests/Helpers/Helpers.swift (2)
25-32: Well-designed convenience initializerThe convenience initializer for
ARTPresenceMessageis well-structured with appropriate default values, making test setup much cleaner.
34-52: Useful helper function for test setupThe
createMockContributorfunction is a good addition that will simplify test code by reducing boilerplate. It provides sensible defaults while allowing test-specific customization.Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift (2)
30-36: Improved attach operation logicThe changes to
performAttachOperationcorrectly emit a status change and improve error handling by checking for the presence operation result.
66-72: Good implementation of waitToBeAbleToPerformPresenceOperationsThis implementation properly tracks call counts and handles the result of waiting for presence operations. The fatal error provides a clear message when the mock is not configured correctly.
Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (2)
77-77: Verify fallback to.failedstate whenattachResultisnil.
Right now, ifattachResultis uninitialized ornil, the channel defaults to.failed. Confirm that behavior aligns with your testing requirements, since it could obscure whether the attach result was simply unset.
92-95: Equatable conformance is a good addition.
MarkingAttachOrDetachResultasEquatablehelps compare outcomes in tests without extra overhead.Tests/AblyChatTests/DefaultRoomPresenceTests.swift (1)
1-478: Comprehensive test coverage is commendable.
These tests systematically cover presence scenarios—enter, update, leave, and error handling—ensuring reliability and correctness of theDefaultPresenceworkflow. As a whole, the suite appears logically consistent with your real-time constraints and concurrency model.
maratal
force-pushed
the
fix/235-presence-tests
branch
from
2b6500d to
be42380
Compare
maratal
force-pushed
the
fix/235-presence-tests
branch
from
be42380 to
dcb9d8b
Compare
|
@maratal please can you also:
|
maratal
force-pushed
the
fix/235-presence-tests
branch
from
ae86971 to
3d4eb27
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
Tests/AblyChatTests/DefaultRoomPresenceTests.swift (4)
40-44:⚠️ Potential issueFix array check in presence filtering.
Here,
let client2 = presenceMembers.filter { ... }returns an array, soclient2 != nilwill always be true. To confirm the presence is found, check for a non-empty array instead.- #expect(client2 != nil) + #expect(!client2.isEmpty)
157-161:⚠️ Potential issueCorrect the presence filtering condition.
As with the earlier logic,
filterreturns[PresenceMember]. Checkingclient1 != nilis ineffective. Instead, verify that the array is non-empty.- #expect(client1 != nil) + #expect(!client1.isEmpty)
40-44:⚠️ Potential issueFix array check in presence filtering
Here,
let client2 = presenceMembers.filter { ... }returns an array, soclient2 != nilwill always be true. To confirm the presence is found, check for a non-empty array instead.- #expect(client2 != nil) + #expect(!client2.isEmpty)
157-161:⚠️ Potential issueCorrect the presence filtering condition
As with the earlier logic,
filterreturns[PresenceMember]. Checkingclient1 != nilis ineffective. Instead, verify that the array is non-empty.- #expect(client1 != nil) + #expect(!client1.isEmpty)
🧹 Nitpick comments (2)
Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift (2)
17-22: Consider addressing parameter name inconsistency.The parameter name
resultOfWaitToBeAblePerformPresenceOperationsis missing the word "To" between "Able" and "Perform", while the property name includes it correctly.init( attachResult: Result<Void, ARTErrorInfo>? = nil, detachResult: Result<Void, ARTErrorInfo>? = nil, roomStatus: RoomStatus? = nil, - resultOfWaitToBeAblePerformPresenceOperations: Result<Void, ARTErrorInfo>? = nil + resultOfWaitToBeAbleToPerformPresenceOperations: Result<Void, ARTErrorInfo>? = nil ) {
21-21: Parameter name doesn't match property nameThere's a slight inconsistency in naming between the parameter and the property it sets:
- resultOfWaitToBeAblePerformPresenceOperations: Result<Void, ARTErrorInfo>? = nil + resultOfWaitToBeAbleToPerformPresenceOperations: Result<Void, ARTErrorInfo>? = nilThe parameter is missing the word "To" between "Able" and "Perform".
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (8)
Sources/AblyChat/RoomFeature.swift(1 hunks)Tests/AblyChatTests/DefaultMessagesTests.swift(6 hunks)Tests/AblyChatTests/DefaultRoomPresenceTests.swift(1 hunks)Tests/AblyChatTests/Helpers/Helpers.swift(1 hunks)Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift(5 hunks)Tests/AblyChatTests/Mocks/MockRealtimePresence.swift(2 hunks)Tests/AblyChatTests/Mocks/MockRoomLifecycleContributor.swift(2 hunks)Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- Tests/AblyChatTests/Mocks/MockRoomLifecycleContributor.swift
- Tests/AblyChatTests/DefaultMessagesTests.swift
- Tests/AblyChatTests/Helpers/Helpers.swift
- Sources/AblyChat/RoomFeature.swift
- Tests/AblyChatTests/Mocks/MockRealtimeChannel.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,
releaseconfiguration, tvOS (Xcode 16) - GitHub Check: Example app, macOS (Xcode 16)
- GitHub Check: Xcode, tvOS (Xcode 16)
- GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16) - GitHub Check: Xcode, iOS (Xcode 16)
- GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16) - GitHub Check: Xcode, macOS (Xcode 16)
- GitHub Check: SPM,
releaseconfiguration (Xcode 16) - GitHub Check: SPM (Xcode 16)
🔇 Additional comments (37)
Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift (7)
7-8: Good addition of the result property for presence operations testing.The new property enables testing of presence operation scenarios, which aligns with the PR objective of implementing presence tests without using
DefaultRoomLifecycleManager.
31-41: Good implementation of status change emission before checking results.The addition of emitting a status change before proceeding with attachment logic correctly simulates the behavior of a real lifecycle manager, which is important for testing presence operations.
75-84: Well-implemented waiting mechanism with proper error handling.The implementation correctly tracks wait calls and provides appropriate error handling, which is essential for testing presence operations in different room states.
7-7: Added property to support presence operationsThe addition of
resultOfWaitToBeAbleToPerformPresenceOperationsenables the mock to simulate presence operation results, aligning with the PR's goal of implementing presence tests without usingDefaultRoomLifecycleManager.
25-25: Ensure property assignment matches parameter nameThis line assigns the value from the parameter to the property, but since the parameter name differs slightly from the property name, it might lead to confusion for future maintainers.
31-41: Enhanced performAttachOperation with status change emissionThe implementation now emits status changes before proceeding with attachment logic and adds conditional processing based on the presence operation capability state. This creates a more realistic simulation of the real component's behavior.
75-85: Implementation of waitToBeAbleToPerformPresenceOperationsGood implementation that follows the same pattern as the existing operations in this class. It correctly tracks call counts, validates required parameters, and properly handles errors by converting them to InternalError.
Tests/AblyChatTests/DefaultRoomPresenceTests.swift (15)
5-20: Good basic test for channel name verification.This test verifies that the channel name matches the expected format, which is an important foundation for presence functionality.
46-72: Well-structured test for presence entry during attachment.This test effectively simulates the real-world scenario of entering presence while a room is in the attaching state, which is an important use case to verify.
74-115: Comprehensive error handling test for failed attachment.The test thoroughly verifies the error details when attempting presence operations during a failed attachment, ensuring proper error propagation.
117-138: Good test for invalid room state handling.This test verifies that attempts to enter presence when the room is in an invalid state are properly rejected with appropriate error information.
163-189: Good test for update presence during attachment.This test mirrors the structure used for testing enter presence during attachment, providing consistent verification of both operations.
191-232: Thorough test for update failure during attachment.The test properly verifies error details when attempting to update presence during a failed attachment.
338-364: Well-implemented test for retrieving presence during attachment.The test follows the same pattern as the enter and update tests, providing consistent verification of all presence operations during attachment.
409-455: Comprehensive test for presence event subscription.This test verifies that all types of presence events (present, enter, update, leave) are properly received through the subscription, which is crucial for real-time presence functionality.
459-476: Well-structured test for discontinuity handling.The test properly verifies that discontinuity events are propagated through the presence system, which is important for handling connection issues gracefully.
1-20: Good test structure for channel name verificationThe test correctly validates that the channel name is set as expected when initializing DefaultPresence. The test follows a clear given-when-then structure which improves readability.
50-72: Well-structured test for presence operations during attachmentThis test effectively validates that users can enter presence while the room is attaching. The use of mock components and async let for concurrent operations creates a realistic test scenario.
76-115: Comprehensive test for handling attachment failuresThe test thoroughly examines error handling when entering presence during a failed attachment. It validates the correct error codes and ensures the lifecycle manager was called correctly.
275-294: Good validation of presence checking functionalityThe test effectively validates the
isUserPresentmethod by checking both positive and negative cases. The assertion structure is clear and confirms expected behavior.
338-407: Thorough testing of presence retrieval during attachmentThese tests provide comprehensive coverage of retrieving presence members while the room is in an attaching state, including both success and failure scenarios. The error validation is particularly well-implemented.
457-476: Proper testing of discontinuity eventsThis test ensures that discontinuity events from the feature channel are correctly propagated through the DefaultPresence implementation, which is an important aspect of robust presence management.
Tests/AblyChatTests/Mocks/MockRealtimePresence.swift (15)
4-9: Good implementation of mock presence with proper memory management.The class now properly inherits from NSObject and conforms to @unchecked Sendable, which allows it to be safely used in async contexts while maintaining the expected behavior of Ably's native classes.
11-16: Well-structured initializer with sensible defaults.The initializer properly sets up the mock state with appropriate defaults, making the mock easy to use in tests with minimal configuration.
18-24: Effective implementation of presence retrieval methods.These methods correctly handle both unfiltered and filtered retrieval of presence members, supporting the test cases that verify presence state.
34-41: Simple but effective update implementation.The implementation correctly updates the current member's data and handles callbacks, which is sufficient for testing the update functionality.
52-65: Complete implementation of enter client methods.The methods properly create and add presence members, trigger callbacks, and set the appropriate action, fully supporting the enter presence test cases.
71-79: Good error handling in update client method.The precondition check ensures that attempting to update a non-existent client fails fast with a clear error message, which helps maintain test integrity.
89-95: Well-implemented subscribe method with notification of existing members.The method correctly sets up the callback and notifies it about all existing members, which mirrors the behavior of the real Ably presence implementation.
4-9: Enhanced mock implementation with proper state managementThe updated mock now inherits from NSObject and conforms to @unchecked Sendable, with proper state management properties. This facilitates more realistic behavior simulation and enables thorough testing of presence functionality.
11-16: Well-structured initializerThe initializer properly sets up the mock state, including a smart handling of the current member (taking the first member if there's only one). This allows for flexible test configurations.
18-24: Implemented get methods for presence retrievalBoth get methods are now implemented to return members based on query parameters, supporting the test scenarios in DefaultRoomPresenceTests. The error handling in the first method is particularly well done.
34-41: Added update method implementationThe update methods now properly modify the current member's data and call the callback if provided, allowing tests to validate update operations.
43-50: Added leave method implementationThe leave methods now properly remove the current member from the members collection and call the callback if provided, supporting leave operation testing.
52-65: Added enterClient method implementationThe enterClient methods correctly create and add a new member, set the action to enter, and notify subscribers. The implementation ensures proper state for presence tests.
71-79: Good error handling in updateClientThe implementation properly validates that the client exists before attempting the update, with a clear precondition failure message if not. This helps catch test setup issues early.
89-95: Implemented subscribe methodThe subscribe method now stores the callback and immediately invokes it for all existing members, which matches the behavior of the real implementation. This is essential for proper presence notification testing.
Now compiler fails for the reason I can't immediately recognize: Couldn't you have a look? @lawrence-forooghian |
|
Will take a look |
maratal
force-pushed
the
fix/235-presence-tests
branch
from
3d4eb27 to
f53befd
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
Tests/AblyChatTests/Mocks/MockRealtimePresence.swift (5)
22-24: Consider returningpresenceGetErrorin allgetcalls.Currently, the query-based
getmethod ignorespresenceGetError. Consistency might require returning this error if set, as with the othergetmethod.func get(_ query: ARTRealtimePresenceQuery, callback: @escaping ARTPresenceMessagesCallback) { + if let error = presenceGetError { + callback(nil, error) + return + } callback(members.filter { $0.clientId == query.clientId }, nil) }
26-32: Consider implementing or removing these unimplemented enter methods.They currently call
fatalError, which may disrupt tests. If they are not required, removing them can avoid accidental usage. If needed, implement them to maintain consistency.
97-99: Attach-aware subscription is stubbed out.Up to you whether to implement or remove if not used by the tests.
101-103: Filtered subscription byARTPresenceActionis also stubbed.If not needed for these tests, removing might keep the code cleaner.
105-107: Attachment-based subscription with presence action is also stubbed.Same suggestion: implement or remove based on test requirements.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (8)
Sources/AblyChat/RoomFeature.swift(1 hunks)Tests/AblyChatTests/DefaultMessagesTests.swift(6 hunks)Tests/AblyChatTests/DefaultRoomPresenceTests.swift(1 hunks)Tests/AblyChatTests/Helpers/Helpers.swift(3 hunks)Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift(5 hunks)Tests/AblyChatTests/Mocks/MockRealtimePresence.swift(2 hunks)Tests/AblyChatTests/Mocks/MockRoomLifecycleContributor.swift(2 hunks)Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- Sources/AblyChat/RoomFeature.swift
- Tests/AblyChatTests/Mocks/MockRoomLifecycleContributor.swift
- Tests/AblyChatTests/DefaultMessagesTests.swift
- Tests/AblyChatTests/Helpers/Helpers.swift
- Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift
- Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Xcode, iOS (Xcode 16)
🔇 Additional comments (26)
Tests/AblyChatTests/DefaultRoomPresenceTests.swift (17)
5-20: Test struct initialization looks consistent.The overall structure of this
DefaultRoomPresenceTestssuite is well-organized, providing a clear setup for verifying presence-related functionality.
22-44: Use a non-empty array check instead of!= nil.The variable
client2is an array, so checkingclient2 != nilwill always pass. Consider verifying!client2.isEmptyto confirm that the member is present.
46-72: Presence enter while attaching is thoroughly tested.This test correctly simulates the attaching state scenario and verifies that presence
enteris deferred until the room is ready.
74-112: Good coverage of failure scenario during attach.This test properly confirms that an attachment failure leads to the correct error when trying to enter presence.
114-134: Failure path test accurately captures invalid state behavior.The test ensures a proper error is thrown and helps confirm robust error handling.
136-157: Check for array emptiness instead of!= nil.Same as the earlier comment, the variable
client1is an array. Verifying!client1.isEmptyis more accurate thanclient1 != nil.
159-185: Presence update while attaching is validated correctly.The logic verifies that the test waits for the room to be ready before allowing presence updates.
187-225: Failure during attaching is correctly tested for updates.This checks that an attach error interrupts the update flow as expected.
227-247: Invalid state error handling for presence update.The test accurately confirms the correct error is thrown when the room state is invalid.
249-266: Coverage for leaving presence is complete.Removing the member from presence is verified, ensuring correct logic under normal conditions.
270-286: Presence-check functionality appears solid.Ensures that the presence system accurately detects whether a given user ID is present.
290-305: All-members retrieval logic is tested properly.Verifies that all present members are returned, covering normal operation.
307-327: Failure during retrieval in invalid state is correctly handled.Ensures the error path is tested when the room is not in an appropriate state.
329-355: Retrieval while attaching is thoroughly validated.Checks that the attach flow and wait logic function properly before retrieving presence data.
357-395: Attach failure scenario for retrieval.Verifies an attach error leads to a matching error on presence retrieval, maintaining consistency.
399-443: Comprehensive subscription coverage for all presence events.Captures the .present, .enter, .update, and .leave events, confirming the subscription logic is correct.
447-465: Discontinuity handling test is succinct and accurate.Confirms that emitted discontinuities are surfaced as expected through
onDiscontinuity().Tests/AblyChatTests/Mocks/MockRealtimePresence.swift (9)
4-10: Class setup and concurrency marking look appropriate.Adopting
@unchecked Sendableis reasonable for this test mock when used carefully in concurrency contexts.
11-16: Constructor provides robust initialization.Ensures
syncComplete,members, andpresenceGetErrorare all correctly captured.
34-41: Update methods correctly modify the current member.Mimics real presence updates by setting
data; the callback approach helps confirm asynchronous logic is used in tests.
43-50: Leaving presence properly removes the current member.This approach matches the expected behavior of leaving an active presence session.
52-65: Enter client logic is straightforward and effective.Valid for simulating presence entries, and the subscription callback is appropriately invoked.
67-79: Precondition failure might warrant more graceful handling.Crashing on missing members can be suitable for tests, but confirm this is desired behavior.
Would you like to keep terminating the test when a client does not exist, or handle it with a custom error? If you’d prefer verification, I can provide a script or code snippet to confirm usage across the test suite.
81-88: Client leave logic straightforwardly removes the member.Properly ensures that once a client leaves, it is no longer in the array.
89-95: Subscription replays existing members.This is a valid design choice, though it might differ from real-world presence defaults. The approach is acceptable for test scenarios.
109-118: Unsubscribe methods differ in implementation.
func unsubscribe()is unimplemented (fatalError), butunsubscribe(_:)zeroes out the callback. Clarify if you want both to behave consistently.
Thanks @lawrence-forooghian, it's now ready. |
lawrence-forooghian
left a comment
lawrence-forooghian
left a comment
There was a problem hiding this comment.
Haven't finished reviewing this but will leave some comments based on what I've looked at so far
maratal
force-pushed
the
fix/235-presence-tests
branch
from
f53befd to
5a9cba5
Compare
Resolves #242. The API changes are based on the JS API changes from branch integration/single-channel-integration at 0924cd5. The behavioural changes are based on [1] at 27ddbb3. This commit is quite large, and combines API and behavioural changes, because the new specification points are written with reference to the new API and so it was hard to disentangle the two. Key changes to public API: - Updated room options API: - there's no concept of a feature being turned "on" any more, and correspondingly no error when you try and use a feature that's not turned on - removed `allFeaturesEnabled`, since the equivalent has been removed from JS - you can now fetch a room without passing a room options; this uses the default options - Removed feature-specific error codes - Discontinuities now always have an error - Extended the set of RoomStatus that can have an error (this does not have an equivalent in the JS API; it's a result of the room lifecycle manager statuses that mean we're now more driven by the errors emitted by ably-cocoa; we still need to revisit this in #12) Key changes to interactions with Realtime: - Switched to v3 of the REST API - Use a single channel for all room features; this simplifies the room lifecycle manager and the DefaultRoom class - Room reactions now use ephemeral messages - Resolves #133 (setting of presence-related channel modes). Internal changes: - Removed FeatureChannel and RoomLifecycleContributor - Switch to using callbacks for ably-cocoa state changes (i.e. end the experiment of using AsyncSequence in the ably-cocoa wrapper) and enforce that ably-cocoa deliver its state changes on the main thread; together with fc83fc1 this allows us to no longer have to work around the fact that the spec's method for detecting a discontinuity assumes a single-threaded environment, hence resolving #239. [1] ably/specification#282
Resolves #242. The API changes are based on the JS API changes from branch integration/single-channel-integration at 0924cd5. The behavioural changes are based on [1] at 27ddbb3. This commit is quite large, and combines API and behavioural changes, because the new specification points are written with reference to the new API and so it was hard to disentangle the two. Key changes to public API: - Updated room options API: - there's no concept of a feature being turned "on" any more, and correspondingly no error when you try and use a feature that's not turned on - removed `allFeaturesEnabled`, since the equivalent has been removed from JS - you can now fetch a room without passing a room options; this uses the default options - Removed feature-specific error codes - Discontinuities now always have an error - Extended the set of RoomStatus that can have an error (this does not have an equivalent in the JS API; it's a result of the room lifecycle manager statuses that mean we're now more driven by the errors emitted by ably-cocoa; we still need to revisit this in #12) Key changes to interactions with Realtime: - Switched to v3 of the REST API - Use a single channel for all room features; this simplifies the room lifecycle manager and the DefaultRoom class - Room reactions now use ephemeral messages - Resolves #133 (setting of presence-related channel modes). Internal changes: - Removed FeatureChannel and RoomLifecycleContributor - Switch to using callbacks for ably-cocoa state changes (i.e. end the experiment of using AsyncSequence in the ably-cocoa wrapper) and enforce that ably-cocoa deliver its state changes on the main thread; together with fc83fc1 this allows us to no longer have to work around the fact that the spec's method for detecting a discontinuity assumes a single-threaded environment, hence resolving #239. [1] ably/specification#282
Resolves #242. The API changes are based on the JS API changes from branch integration/single-channel-integration at 0924cd5. The behavioural changes are based on [1] at 2f88b1b. This commit is quite large, and combines API and behavioural changes, because the new specification points are written with reference to the new API and so it was hard to disentangle the two. Key changes to public API: - Updated room options API: - there's no concept of a feature being turned "on" any more, and correspondingly no error when you try and use a feature that's not turned on - removed `allFeaturesEnabled`, since the equivalent has been removed from JS - you can now fetch a room without passing a room options; this uses the default options - Removed feature-specific error codes - Discontinuities now always have an error - Extended the set of RoomStatus that can have an error (this does not have an equivalent in the JS API; it's a result of the room lifecycle manager statuses that mean we're now more driven by the errors emitted by ably-cocoa; we still need to revisit this in #12) Key changes to interactions with Realtime: - Switched to v3 of the REST API - Use a single channel for all room features; this simplifies the room lifecycle manager and the DefaultRoom class - Room reactions now use ephemeral messages - Resolves #133 (setting of presence-related channel modes). Internal changes: - Removed FeatureChannel and RoomLifecycleContributor - Switch to using callbacks for ably-cocoa state changes (i.e. end the experiment of using AsyncSequence in the ably-cocoa wrapper) and enforce that ably-cocoa deliver its state changes on the main thread; together with fc83fc1 this allows us to no longer have to work around the fact that the spec's method for detecting a discontinuity assumes a single-threaded environment, hence resolving #239. [1] ably/specification#282
maratal
force-pushed
the
fix/235-presence-tests
branch
from
b3427a1 to
c74b428
Compare
maratal
changed the title
DefaultRoomLifecycleManager.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Tests/AblyChatTests/Mocks/MockRealtimePresence.swift (1)
61-64:⚠️ Potential issueFix force unwrapping of optional
clientId.The force unwrap of
clientId!could cause a runtime crash ifclientIdis nil.- var callRecorderDescription: String { - "clientId=\(clientId!)" + var callRecorderDescription: String { + "clientId=\(clientId ?? "nil")"Note: I see this issue was previously flagged in a past review and supposedly addressed, but the fix doesn't appear to be reflected in this code.
🧹 Nitpick comments (5)
Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift (2)
14-20: Property can be non-optional, which removes the run-timefatalErrorguard.
resultOfWaitToBeAbleToPerformPresenceOperationsis declared as optional but always initialised (default parameter provides.success(())).
Turning it into a non-optional removes the need for theguard let … else fatalErrorat the call-site and makes the intent clearer (tests that really want the “unset → crash” behaviour can still passnilexplicitly by changing the parameter type toResult<Void, ARTErrorInfo>?).- private let resultOfWaitToBeAbleToPerformPresenceOperations: Result<Void, ARTErrorInfo>? + private let resultOfWaitToBeAbleToPerformPresenceOperations: Result<Void, ARTErrorInfo>and in the initializer:
- init(..., - resultOfWaitToBeAbleToPerformPresenceOperations: Result<Void, ARTErrorInfo> = .success(())) { + init(..., + resultOfWaitToBeAbleToPerformPresenceOperations: Result<Void, ARTErrorInfo> = .success(())) {…followed by removing the
guardinsidewaitToBeAbleToPerformPresenceOperations.
This simplifies the mock and avoids surprising crashes in future refactors.
66-79: Minor nit: recorded argument can keep the enum value instead of its string description.Storing
"\(requestedByFeature)"means the test must compare against the exactCustomStringConvertible
implementation. Keeping the raw enum makes comparisons type-safe and immune to description changes:- arguments: ["requestedByFeature": "\(requestedByFeature)"] + arguments: ["requestedByFeature": requestedByFeature]
MockMethodCallRecorder.compareAnyalready handles enums (viaStringcurrently); adding a genericEquatablefallback would make this change work seamlessly.Tests/AblyChatTests/Helpers/Helpers.swift (2)
97-122:compareAnymisses several common literal types and could short-circuit earlier.The helper is very handy, but:
Double,Float,Date,UUID, etc. are not covered – comparisons will silently fall through tofalse.- The recursive array comparison performs
countcheck inside the loop; moving it outside is cheaper.- A generic
Equatablebranch would catch most simple cases without hand-listing every type.Illustrative refactor:
+ // Generic fast-path for any Equatable pair of the same concrete type + if let lhs = any1 as? (any Equatable), + let rhs = any2 as? (any Equatable), + type(of: lhs) == type(of: rhs) { + return lhs == rhs + }Followed by type-specific fallbacks only where required (e.g.
NSDictionarydeep equality for[String:Any]).
This keeps the helper future-proof and reduces maintenance.
146-164: Lock scope can be reduced to minimise contention.
hasRecordholdsmutexfor the entire search as well as the equality work.
Copying the array under lock and performing the expensive search after releasing the lock keeps the recorder thread-safe while improving parallelism, e.g.:- mutex.lock() - let result = records.contains { record in + mutex.lock() + let snapshot = records mutex.unlock() + let result = snapshot.contains { record in ... } - mutex.unlock() return resultThe pattern mirrors the write-heavy
addRecord, but optimises read paths used by assertions in heavily concurrent tests.Tests/AblyChatTests/DefaultPresenceTests.swift (1)
81-88: Factor out the “work-around-closure” once the compiler bug is gone.The repeated
TODOblocks add indirection and noise to each negative-test case.
Consider extracting a helper like#expectAsyncThrows(or remove the closure wrapper entirely) when upgrading past the compiler issue referenced in #233 to keep the tests concise.No action required right now, but worth tracking so dead code doesn’t linger.
Also applies to: 204-213, 358-367, 415-424
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (5)
Tests/AblyChatTests/DefaultPresenceTests.swift(1 hunks)Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift(4 hunks)Tests/AblyChatTests/Helpers/Helpers.swift(1 hunks)Tests/AblyChatTests/Mocks/MockRealtimePresence.swift(1 hunks)Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift
🧰 Additional context used
🧬 Code Graph Analysis (1)
Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift (3)
Tests/AblyChatTests/Helpers/Helpers.swift (1)
addRecord(146-150)Tests/AblyChatTests/Mocks/MockRealtimePresence.swift (2)
get(23-29)get(31-37)Sources/AblyChat/InternalError.swift (6)
toInternalError(35-37)toInternalError(41-43)toInternalError(47-49)toInternalError(53-55)toInternalError(59-61)toInternalError(65-67)
⏰ Context from checks skipped due to timeout of 90000ms (34)
- GitHub Check: Example app, tvOS (Xcode 16.3)
- GitHub Check: Example app, macOS (Xcode 16.2)
- GitHub Check: Example app, tvOS (Xcode 16.1)
- GitHub Check: Example app, tvOS (Xcode 16.2)
- GitHub Check: Example app, iOS (Xcode 16.1)
- GitHub Check: Example app, iOS (Xcode 16.2)
- GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16.3) - GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16.1) - GitHub Check: Example app, macOS (Xcode 16.3)
- GitHub Check: Example app, iOS (Xcode 16.3)
- GitHub Check: Example app, macOS (Xcode 16.1)
- GitHub Check: Xcode, iOS (Xcode 16.3)
- GitHub Check: Xcode, macOS (Xcode 16.1)
- GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16.2) - GitHub Check: Xcode, tvOS (Xcode 16.3)
- GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16.3) - GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16.2) - GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16.2) - GitHub Check: Xcode, iOS (Xcode 16.1)
- GitHub Check: Xcode, macOS (Xcode 16.2)
- GitHub Check: Xcode, tvOS (Xcode 16.1)
- GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16.1) - GitHub Check: Xcode, iOS (Xcode 16.2)
- GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16.1) - GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16.3) - GitHub Check: Xcode, macOS (Xcode 16.3)
- GitHub Check: SPM,
releaseconfiguration (Xcode 16.3) - GitHub Check: Xcode, tvOS (Xcode 16.2)
- GitHub Check: SPM,
releaseconfiguration (Xcode 16.1) - GitHub Check: SPM (Xcode 16.3)
- GitHub Check: SPM,
releaseconfiguration (Xcode 16.2) - GitHub Check: SPM (Xcode 16.1)
- GitHub Check: SPM (Xcode 16.2)
- GitHub Check: Generate code coverage
🔇 Additional comments (3)
Tests/AblyChatTests/Mocks/MockRealtimePresence.swift (3)
7-9: Improved mock implementation provides better testability.The mock now returns dummy
ARTEventListenerinstances for subscribe methods and implements a no-op for unsubscribe, making it more useful for testing without crashing with fatal errors.Also applies to: 11-13, 15-17
23-29: Enhanced testing capabilities with call recording.The implementation now records method calls with their arguments instead of failing with fatal errors. This pattern allows tests to verify which methods were called with what arguments, greatly improving testability while reducing the logic within the mock itself.
Also applies to: 31-37, 39-44, 46-51, 53-58
5-6:✅ Verification successful
Verify thread safety of the
callRecorder.The
callRecorderis used to track method invocations across multiple methods, which could potentially be called from different threads. Make sure thatMockMethodCallRecorderimplements proper synchronization mechanisms to avoid race conditions.
🏁 Script executed:
#!/bin/bash # Check if MockMethodCallRecorder is thread-safe rg -A 20 "class MockMethodCallRecorder" --type swiftLength of output: 1473
🏁 Script executed:
#!/bin/bash # Locate the file containing MockMethodCallRecorder and display its implementation along with lock usage file=$(rg -l "class MockMethodCallRecorder" --type swift) echo "Inspecting file: $file" echo "----- Class Definition and Methods -----" sed -n '1,200p' "$file" echo echo "----- Lock Usage -----" rg -n "mutex" -A3 "$file" echo echo "----- Methods Operating on records -----" rg -nE "func (record|callCount|reset)" -A5 "$file"Length of output: 7328
Thread Safety of
MockMethodCallRecorderVerifiedThe
addRecordandhasRecordmethods both lock access to the sharedrecordsarray using anNSLock, ensuring serialized reads and writes and preventing race conditions.Optionally, you could replace the explicit
lock()/unlock()calls with adefer { mutex.unlock() }pattern for added safety in future early-exit scenarios, but the current implementation is thread-safe as is.
maratal
force-pushed
the
fix/235-presence-tests
branch
from
c74b428 to
ce99af3
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (6)
Sources/AblyChat/RoomLifecycleManager.swift(1 hunks)Tests/AblyChatTests/DefaultPresenceTests.swift(1 hunks)Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift(4 hunks)Tests/AblyChatTests/Helpers/Helpers.swift(1 hunks)Tests/AblyChatTests/Mocks/MockRealtimePresence.swift(1 hunks)Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- Sources/AblyChat/RoomLifecycleManager.swift
🚧 Files skipped from review as they are similar to previous changes (4)
- Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift
- Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift
- Tests/AblyChatTests/Helpers/Helpers.swift
- Tests/AblyChatTests/Mocks/MockRealtimePresence.swift
🔇 Additional comments (10)
Tests/AblyChatTests/DefaultPresenceTests.swift (10)
8-9: Clear specification mapping with @SPEC and @specOneOf annotationsGood use of specification annotations to clearly trace test coverage back to specific requirements. This practice enhances maintainability by providing explicit documentation of which specification items are being tested.
11-33: LGTM: Well-structured test for entering presenceThis test effectively verifies that entering presence sends the correct data to the Ably SDK. The Given-When-Then structure provides clear separation of test phases.
36-60: Good test for entering presence during attachmentThis test properly verifies that the presence operations coordinate with the room lifecycle manager, which is crucial for ensuring proper state management during transitions.
117-126: Test pattern for error case is consistentThe error verification pattern is consistent with other tests, which is good for maintainability. The use of
isChatErrorhelper ensures proper error validation.
131-156: LGTM: Well-structured test for updating presenceThis test effectively validates that updating presence sends the correct data to the Ably SDK. The pattern is consistent with the enter test which aids readability.
254-279: Good implementation of leave presence testThis test properly verifies that leaving presence invokes the correct Ably SDK method with the expected parameters.
283-308: Effective test for isUserPresent functionalityThe test properly verifies that the query is constructed correctly when checking if a user is present.
312-338: Good implementation of presence retrieval testThe test correctly verifies that getting all members of the presence set invokes the right Ably SDK method.
436-487: Comprehensive test for presence event subscriptionExcellent test coverage for subscribing to presence events. The test verifies all four event types (present, enter, update, leave) and confirms the correct event details are received.
1-490: Overall excellent test coverage for presence functionalityThis file provides comprehensive test coverage for all presence-related operations including enter, update, leave, query, and subscribe functionality. Tests cover happy paths, error cases, and state transitions. The structure is consistent and follows good testing practices.
Some minor suggestions:
- Consider extracting common setup code to reduce duplication, as many tests share similar initialization patterns
- Follow up on the TODOs once the compiler issue is fixed and specification PR is merged
maratal
force-pushed
the
fix/235-presence-tests
branch
from
ce99af3 to
410e4ae
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
Tests/AblyChatTests/DefaultPresenceTests.swift (2)
489-489: Remove @specUntested annotation.Remove this entire line with the @specUntested annotation and TODO comment once specification PR 282 is merged, as noted in past review comments.
- // @specUntested CHA-PR8 - Discontinuity is now only within `Room` itself. TODO: Remove this once specification PR 282 is merged.
81-91: 🛠️ Refactor suggestionRemove compiler crash workaround.
The TODO comments about avoiding compiler crash (issue #233) with the workaround pattern should be removed now that Xcode 16.3+ is available, as noted in past review comments. The tests should call the API directly inside the
#expectblocks.Apply this pattern across all instances:
- // TODO: avoids compiler crash (https://github.com/ably/ably-chat-swift/issues/233), revert once Xcode 16.3 released - let doIt = { - // When - try await defaultPresence.enter() - } - await #expect { - try await doIt() - } /* Then */ throws: { error in + await #expect { + try await defaultPresence.enter() + } throws: { error in isChatError(error, withCodeAndStatusCode: .variableStatusCode(.roomInInvalidState, statusCode: 500), cause: attachError) }Also applies to: 117-127, 204-214, 240-250, 357-367, 415-425
🧹 Nitpick comments (1)
Tests/AblyChatTests/Helpers/Helpers.swift (1)
173-178: Consider adding a comment explaining the implementation.Line 176 has a comment "Probably there is a better way of doing this" but doesn't explain what exactly this conversion does or why it's implemented this way. Consider adding more details about the implementation and its purpose.
extension [String: Any] { func toAblyCocoaData() -> Any { - // Probaly there is a better way of doing this + // Converts a Swift dictionary to Ably Cocoa-compatible data format + // by wrapping it in PresenceDataDTO and then converting to JSONValue + // This ensures compatibility with Ably Cocoa's serialization expectations PresenceDataDTO(userCustomData: JSONValue(ablyCocoaData: self)).toJSONValue.toAblyCocoaData } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (6)
Sources/AblyChat/RoomLifecycleManager.swift(1 hunks)Tests/AblyChatTests/DefaultPresenceTests.swift(1 hunks)Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift(4 hunks)Tests/AblyChatTests/Helpers/Helpers.swift(1 hunks)Tests/AblyChatTests/Mocks/MockRealtimePresence.swift(1 hunks)Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- Sources/AblyChat/RoomLifecycleManager.swift
- Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift
- Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift
- Tests/AblyChatTests/Mocks/MockRealtimePresence.swift
⏰ Context from checks skipped due to timeout of 90000ms (34)
- GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16.2) - GitHub Check: Example app, iOS (Xcode 16.2)
- GitHub Check: Xcode, tvOS (Xcode 16.3)
- GitHub Check: Example app, macOS (Xcode 16.1)
- GitHub Check: Example app, iOS (Xcode 16.3)
- GitHub Check: Example app, tvOS (Xcode 16.3)
- GitHub Check: Xcode, iOS (Xcode 16.3)
- GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16.3) - GitHub Check: Xcode, iOS (Xcode 16.2)
- GitHub Check: Example app, tvOS (Xcode 16.1)
- GitHub Check: Example app, macOS (Xcode 16.3)
- GitHub Check: Example app, macOS (Xcode 16.2)
- GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16.3) - GitHub Check: Example app, tvOS (Xcode 16.2)
- GitHub Check: Example app, iOS (Xcode 16.1)
- GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16.2) - GitHub Check: Xcode, tvOS (Xcode 16.2)
- GitHub Check: Xcode, tvOS (Xcode 16.1)
- GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16.1) - GitHub Check: Xcode, macOS (Xcode 16.2)
- GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16.3) - GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16.2) - GitHub Check: Xcode, macOS (Xcode 16.1)
- GitHub Check: Xcode, iOS (Xcode 16.1)
- GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16.1) - GitHub Check: Xcode, macOS (Xcode 16.3)
- GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16.1) - GitHub Check: SPM (Xcode 16.2)
- GitHub Check: SPM,
releaseconfiguration (Xcode 16.3) - GitHub Check: SPM (Xcode 16.1)
- GitHub Check: SPM (Xcode 16.3)
- GitHub Check: SPM,
releaseconfiguration (Xcode 16.2) - GitHub Check: SPM,
releaseconfiguration (Xcode 16.1) - GitHub Check: Generate code coverage
🔇 Additional comments (3)
Tests/AblyChatTests/Helpers/Helpers.swift (2)
124-165: Well-implemented thread-safe mock method call recorder.The
MockMethodCallRecorderclass is well designed with proper thread safety using mutex locks for both reads and writes. This implementation correctly addresses the data race issues that were previously flagged in reviews.
97-122: Comprehensive deep equality check implementation.The
compareAnyfunction provides thorough type-checking and comparison for various data types, which is essential for testing heterogeneous values. The recursive implementation for arrays is particularly well done.Tests/AblyChatTests/DefaultPresenceTests.swift (1)
5-487: Well-structured and comprehensive presence test suite.The test suite provides thorough coverage of presence functionality including enter, update, leave, query, and subscription operations. Tests appropriately verify both success scenarios and error handling. The use of mock objects with call recorders allows for precise verification of interactions with dependencies.
maratal
force-pushed
the
fix/235-presence-tests
branch
from
410e4ae to
6cf2821
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Tests/AblyChatTests/DefaultPresenceTests.swift (1)
80-90: 🛠️ Refactor suggestionRemove the compiler-crash workaround closure once Xcode 16.3+ is in CI
The
doItwrapper and the surrounding TODO comment were temporary to avoid
SR-16440 (issue #233).
CI now runs Xcode 16.3; the workaround can be safely removed for cleaner tests:- // TODO: avoids compiler crash (https://github.com/ably/ably-chat-swift/issues/233), revert once Xcode 16.3 released - let doIt = { - try await defaultPresence.enter() - } - await #expect { - try await doIt() - } /* Then */ throws: { error in + await #expect { + try await defaultPresence.enter() + } throws: { error in isChatError(error, withCodeAndStatusCode: .variableStatusCode(.roomInInvalidState, statusCode: 500), cause: attachError) }Apply the same removal to all six occurrences highlighted above.
Also applies to: 118-126, 204-214, 241-249, 358-366, 415-424
🧹 Nitpick comments (1)
Tests/AblyChatTests/DefaultPresenceTests.swift (1)
12-23: Consider extracting a reusablemakeDefaultPresence()test helperEvery test repeats ~15 lines of boiler-plate to build a
DefaultPresence
instance. A small factory function would:
- cut >120 duplicated lines,
- surface the intent of the “Given” section,
- make future changes (e.g. new ctor parameter) one-liner.
Example:
func makePresence( roomLifecycleManager: MockRoomLifecycleManager = .init(), channelName: String = "basketball::$chat::$chatMessages" ) async -> DefaultPresence { let channel = await MockRealtimeChannel(name: channelName) return await DefaultPresence( channel: channel, roomLifecycleManager: roomLifecycleManager, roomID: "basketball", clientID: "client1", logger: TestLogger(), options: .init() ) }Tests then become:
let defaultPresence = await makePresence() try await defaultPresence.enter()Also applies to: 38-50, 135-147, 160-172, 256-268, 285-298, 315-327, 371-383, 439-451
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (6)
Sources/AblyChat/RoomLifecycleManager.swift(1 hunks)Tests/AblyChatTests/DefaultPresenceTests.swift(1 hunks)Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift(4 hunks)Tests/AblyChatTests/Helpers/Helpers.swift(1 hunks)Tests/AblyChatTests/Mocks/MockRealtimePresence.swift(1 hunks)Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- Tests/AblyChatTests/DefaultRoomLifecycleManagerTests.swift
- Sources/AblyChat/RoomLifecycleManager.swift
- Tests/AblyChatTests/Mocks/MockRoomLifecycleManager.swift
- Tests/AblyChatTests/Mocks/MockRealtimePresence.swift
- Tests/AblyChatTests/Helpers/Helpers.swift
⏰ Context from checks skipped due to timeout of 90000ms (34)
- GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16.2) - GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16.2) - GitHub Check: Example app, iOS (Xcode 16.3)
- GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16.2) - GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16.3) - GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16.3) - GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16.3) - GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16.1) - GitHub Check: Example app, iOS (Xcode 16.2)
- GitHub Check: Xcode, tvOS (Xcode 16.2)
- GitHub Check: Example app, tvOS (Xcode 16.3)
- GitHub Check: Example app, tvOS (Xcode 16.1)
- GitHub Check: Example app, tvOS (Xcode 16.2)
- GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16.1) - GitHub Check: SPM (Xcode 16.3)
- GitHub Check: Example app, macOS (Xcode 16.3)
- GitHub Check: Xcode, macOS (Xcode 16.1)
- GitHub Check: Xcode, tvOS (Xcode 16.3)
- GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16.1) - GitHub Check: Example app, macOS (Xcode 16.2)
- GitHub Check: Example app, iOS (Xcode 16.1)
- GitHub Check: Example app, macOS (Xcode 16.1)
- GitHub Check: Xcode, iOS (Xcode 16.2)
- GitHub Check: Xcode, iOS (Xcode 16.3)
- GitHub Check: Xcode, macOS (Xcode 16.2)
- GitHub Check: Xcode, tvOS (Xcode 16.1)
- GitHub Check: Xcode, iOS (Xcode 16.1)
- GitHub Check: Xcode, macOS (Xcode 16.3)
- GitHub Check: SPM (Xcode 16.1)
- GitHub Check: SPM,
releaseconfiguration (Xcode 16.3) - GitHub Check: SPM,
releaseconfiguration (Xcode 16.2) - GitHub Check: SPM,
releaseconfiguration (Xcode 16.1) - GitHub Check: SPM (Xcode 16.2)
- GitHub Check: Generate code coverage
🔇 Additional comments (1)
Tests/AblyChatTests/DefaultPresenceTests.swift (1)
28-32:❌ Incorrect review comment
Verify argument key – should it be
clientIdinstead ofname?
enterClient(_:data:)takes the clientId as its first unlabeled parameter.
The call-recorder assertion is matching the key"name", which looks out of line with the API and with the rest of the file (everywhere else you use"clientID"/"clientId").A mismatched key will cause the expectation to pass even when the client id is wrong, defeating the purpose of the test.
- arguments: ["name": "client1", + arguments: ["clientId": "client1",Please check the exact key used by
MockRealtimePresence’scallRecorderforenterClientand adjust all similar assertions if necessary.
🏁 Script executed:
#!/bin/bash # Locate the recording call in the mock to confirm the argument key rg -n 'record\(.*enterClient' -t swiftLength of output: 40
🏁 Script executed:
#!/bin/bash echo "## Searching for MockRealtimePresence class definition" rg -n 'class .*MockRealtimePresence' -t swift echo -e "\n## Listing Swift files matching '*Mock*Presence*'" fd -e swift | grep -i 'Mock.*Presence' echo -e "\n## Searching for callRecorder usage" rg -n 'callRecorder' -t swift echo -e "\n## Searching for recordCall usage" rg -n 'recordCall' -t swift echo -e "\n## Searching for hasRecord usage" rg -n 'hasRecord' -t swift || true echo -e "\n## Searching for enterClient function definitions" rg -n 'func enterClient' -t swift echo -e "\n## Searching for enterClient calls" rg -n 'enterClient(' -t swiftLength of output: 5835
🏁 Script executed:
#!/bin/bash # Show the enterClient implementation in MockRealtimePresence rg -n 'func enterClient' -C5 Tests/AblyChatTests/Mocks/MockRealtimePresence.swift # Show how arguments are recorded in addRecord for enterClient rg -n 'addRecord' -C5 Tests/AblyChatTests/Mocks/MockRealtimePresence.swiftLength of output: 1809
Ignore argument key change – “name” is correct in mock
MockRealtimePresence’s enterClient(_ name:data:) records the argument under the key"name", so the test’s expectation using"name"matches the mock implementation. No change required.Likely an incorrect or invalid review comment.
maratal
changed the title
|
Pushed a change to use |
lawrence-forooghian
force-pushed
the
fix/235-presence-tests
branch
from
9d404f5 to
89d3929
Compare
Part of #235
Results of running unit tests 100 times in Xcode:
Summary by CodeRabbit