Conversation
This reverts commit 91a7da1. There are a few things that we need to fix here: - adding a conformance of ARTMessage to Sendable (it's not, and even if this were just for our internal use, there's no way to hide this conformance from users of the library) - using an instance of DefaultRoomLifecycleManager instead of a mock - using delays - I've noticed at least one intermittently failing test Created issue #235 for restoring these tests.
WalkthroughThe changes remove several debug-only and redundant code sections from the chat source and test files. Error handling in message subscriptions is reworked to use guard statements instead of do-catch blocks. Debug logging and test-only subscription methods are eliminated. In tests, methods have been renamed, removed, or added to better reflect error conditions and subscription behavior. Additionally, several mocks and helpers have been simplified by removing unused extensions, enums, and state management to streamline functionality and enforce stricter type declarations. Changes
Sequence Diagram(s)sequenceDiagram
participant S as Subscriber
participant DM as DefaultMessages
participant G as Guard Checks
participant E as Error Handler
S->>DM: Request to subscribe for message events
DM->>G: Validate message properties (data, extras, serial, clientID, version)
alt Validation Fails
G-->>DM: Return error condition
DM->>S: Throw error
else Validation Passes
G-->>DM: All properties valid
DM->>S: Create and return Message object
end
Possibly related PRs
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ 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:
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 (2)
Tests/AblyChatTests/Mocks/MockRealtimePresence.swift (1)
65-67: Consider adding descriptive comment for this implementation choiceSince this is a significant change to the mock implementation strategy, it might be helpful to add a class-level comment explaining why all methods are intentionally non-functional.
final class MockRealtimePresence: RealtimePresenceProtocol { + // This is an intentional stub implementation. Tests should not call these methods directly. + // If specific behavior is needed, consider using a test-specific mock or spy. var syncComplete: Bool { fatalError("Not implemented") }Tests/AblyChatTests/DefaultMessagesTests.swift (1)
25-26: Function name repetition.
Consider renaming for clarity:- func get_getMessagesIsExposedFromChatAPI() async throws { + func testGetMessagesIsExposedFromChatAPI() async throws {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
Sources/AblyChat/DefaultMessages.swift(2 hunks)Sources/AblyChat/DefaultRoomReactions.swift(0 hunks)Sources/AblyChat/DefaultTyping.swift(3 hunks)Sources/AblyChat/RoomFeature.swift(1 hunks)Tests/AblyChatTests/ChatAPITests.swift(1 hunks)Tests/AblyChatTests/DefaultMessagesTests.swift(5 hunks)Tests/AblyChatTests/DefaultRoomPresenceTests.swift(0 hunks)Tests/AblyChatTests/DefaultRoomReactionsTests.swift(1 hunks)Tests/AblyChatTests/DefaultRoomTypingTests.swift(0 hunks)Tests/AblyChatTests/Helpers/Helpers.swift(0 hunks)Tests/AblyChatTests/Mocks/MockHTTPPaginatedResponse.swift(4 hunks)Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift(7 hunks)Tests/AblyChatTests/Mocks/MockRealtimePresence.swift(2 hunks)Tests/AblyChatTests/Mocks/MockRoomLifecycleContributor.swift(1 hunks)Tests/AblyChatTests/Mocks/MockRoomLifecycleContributorChannel.swift(2 hunks)
💤 Files with no reviewable changes (4)
- Tests/AblyChatTests/Helpers/Helpers.swift
- Tests/AblyChatTests/DefaultRoomTypingTests.swift
- Tests/AblyChatTests/DefaultRoomPresenceTests.swift
- Sources/AblyChat/DefaultRoomReactions.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,
releaseconfiguration, iOS (Xcode 16) - GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16) - GitHub Check: Xcode, tvOS (Xcode 16)
- GitHub Check: SPM (Xcode 16)
- GitHub Check: Xcode, iOS (Xcode 16)
- GitHub Check: Xcode, macOS (Xcode 16)
- GitHub Check: SPM,
releaseconfiguration (Xcode 16)
🔇 Additional comments (45)
Tests/AblyChatTests/Mocks/MockRealtimePresence.swift (3)
4-7: Simplified protocol stub implementationThe class has been refactored to be a simple protocol stub rather than a full mock implementation. This approach provides clear indication that the methods are intentionally not implemented by using
fatalErrorcalls.
9-15: Consistent use of fatalError for unimplemented methodsThe consistent use of
fatalError("Not implemented")across all methods makes it clear that this is now a stub implementation rather than a functional mock.
25-31:Details
✅ Verification successful
Verify that tests aren't calling these methods
Since all methods now call
fatalError, ensure that no tests are still attempting to use this mock's functionality, as they would crash at runtime.
🏁 Script executed:
#!/bin/bash # Search for any test files that might be using MockRealtimePresence methods rg "mockPresence\.(get|enter|update|leave|subscribe)" Tests/Length of output: 62
Action: Confirm that test files do not invoke fatal methods on the MockRealtimePresence mock.
The initial search for patterns like
mockPresence.(get|enter|update|leave|subscribe)in the Tests directory returned no output. However, since the inference is based on a limited search result, please run the following additional script to more generally verify that no tests reference theMockRealtimePresenceclass (and consequently call its unimplemented methods):
- If this search returns any matches, review those test files to ensure they are not invoking methods (such as
update) on this mock, as doing so will trigger afatalError.- If no matches are found, you can be confident that tests are not calling the mock’s unimplemented methods.
🏁 Script executed:
#!/bin/bash # Search for any reference to the MockRealtimePresence class in the tests directory. rg "MockRealtimePresence" Tests/Length of output: 240
Final Verification: No Runtime Crashes Expected from Mock Method Calls
The verification steps confirm that:
- The search for direct invocations of
mockPresence.(get|enter|update|leave|subscribe)in the tests returned no results, indicating that none of the tests are triggering the methods that callfatalError.- Although the
MockRealtimePresenceclass is instantiated inTests/AblyChatTests/Mocks/MockRealtimeChannel.swift, there is no evidence that its unimplemented methods (likeupdate) are ever invoked.Based on these findings, it appears safe to leave the
fatalErrorimplementations in place without risking runtime crashes during tests.Sources/AblyChat/RoomFeature.swift (1)
66-66:Details
✅ Verification successful
Type restriction change on contributor property
Changed from
any RoomLifecycleContributor & EmitsDiscontinuitiestoDefaultRoomLifecycleContributor, which is more restrictive. This change aligns with the updates inMockRoomLifecycleContributorwhereEmitsDiscontinuitiesconformance was removed.
🏁 Script executed:
#!/bin/bash # Check where DefaultRoomLifecycleContributor is defined and verify it implements required protocols rg "DefaultRoomLifecycleContributor" --type swiftLength of output: 1862
Confirmation: Concrete Type Usage is Intentional and Consistent
After verifying the code, it's clear that the change to use
DefaultRoomLifecycleContributoris consistent throughout the codebase. The definition inSources/AblyChat/DefaultRoomLifecycleContributor.swiftconfirms it conforms toRoomLifecycleContributorandEmitsDiscontinuities, and its usage in tests (e.g., inTests/AblyChatTests/Mocks/MockRoomLifecycleManagerFactory.swift) aligns with the updated type expectations—especially following the removal ofEmitsDiscontinuitiesfrom the mock contributor. This type restriction change is intentional and correctly applies the updated design.Tests/AblyChatTests/Mocks/MockRoomLifecycleContributor.swift (1)
4-4:Details
❓ Verification inconclusive
Removed EmitsDiscontinuities protocol conformance
The
MockRoomLifecycleContributoractor now only conforms toRoomLifecycleContributorand notEmitsDiscontinuities. This aligns with the type restriction inRoomFeature.swiftand simplifies the mock's responsibilities.
🏁 Script executed:
#!/bin/bash # Check if the onDiscontinuity method was removed since EmitsDiscontinuities conformance was dropped rg "onDiscontinuity" Tests/AblyChatTests/Mocks/MockRoomLifecycleContributor.swiftLength of output: 81
Simplified Protocol Conformance and Removal of
onDiscontinuity
The updatedMockRoomLifecycleContributornow conforms solely toRoomLifecycleContributor, matching the type constraint inRoomFeature.swift. Our search foronDiscontinuityin the file shows no evidence of this method, which aligns with the removal of theEmitsDiscontinuitiesprotocol conformance. Please manually verify that no dependent code still expectsonDiscontinuityor theEmitsDiscontinuitiesconformance within the project.Tests/AblyChatTests/ChatAPITests.swift (2)
174-195: Good addition of error handling testThis new test covers an important error case for the
getMessagesmethod, verifying that server errors are properly propagated asARTErrorInfoexceptions. The test is well-structured following the Given-When-Then pattern and includes the appropriate spec annotation.
175-175: Properly documented test specification referenceGood job including the CHA-M5i specification reference for this test. This helps maintain traceability between tests and requirements.
Sources/AblyChat/DefaultMessages.swift (4)
57-63: Improved error handling structureThe refactoring from a previous do-catch block to guard statements improves readability and makes the error flow more explicit. Each validation step is now clearly defined with its own error message.
65-89: Good validation of message componentsThe implementation properly validates all required message components (extras, serial, clientID, version) before processing. The error messages are descriptive and will help with debugging.
91-111: Clean message creation and emissionThe message object creation is well-structured with all necessary properties. The early return for invalid action types prevents unnecessary processing.
182-182: Updated comment referenceThe comment identifier was changed from
(CHA-M5d)to(CHA-M4d), which appears to be an alignment with updated requirements numbering.Sources/AblyChat/DefaultTyping.swift (3)
11-11: Simplified initializationThe removal of the
maxPresenceGetRetryDurationparameter from the constructor simplifies the API. This is a good change if the value is standardized across all instances.
35-35: Hardcoded timeout value with clear referenceReplacing a configurable parameter with a constant (30.0 seconds) reduces flexibility but improves consistency. The comment clarifies that this follows the CHA-T6c1 specification.
42-42: Updated retry logic to use constant valueThe retry loop and logging now reference the constant
maxRetryDurationinstead of a class property. This change maintains the functionality while simplifying the implementation.Also applies to: 72-72
Tests/AblyChatTests/Mocks/MockHTTPPaginatedResponse.swift (3)
8-8: More explicit control over pagination stateAdding the
_isLastproperty provides direct control over the pagination state rather than inferring it fromhasNext. This is more explicit and prevents potential logical errors where these values might not be perfectly complementary.Also applies to: 21-21, 46-46
14-15: Enhanced mock constructor flexibilityThe constructor now accepts both
hasNextandisLastparameters with sensible defaults, making it more flexible for testing different API response scenarios.
123-123: Simplification of mock response configurationThe removal of parameters from
successGetMessagesWithItemsand other static instances simplifies the test setup with reasonable defaults.Also applies to: 148-148
Tests/AblyChatTests/DefaultRoomReactionsTests.swift (2)
46-49: Simplified test naming and scopeThe test has been renamed from a verbose description to a more focused one that clearly states what's being tested: that
subscribe()returns a subscription. The spec reference has also been updated fromCHA-ER4atoCHA-ER4.
51-52: Focused test implementationThe test has been simplified to focus solely on verifying that a subscription is created successfully, removing the complex setup and behavior verification. This provides a clear, targeted test that verifies the core functionality without unnecessary complexity.
Also applies to: 58-61
Tests/AblyChatTests/Mocks/MockRoomLifecycleContributorChannel.swift (2)
48-49: Removes delay parameter for state changes and simplifies logic.
No issues found; this change appears consistent with the overall mock simplification.
93-99: Updates state and errorReason in a single flow.
Synchronous transition looks appropriate. Ensure that tests cover edge cases (e.g., repeated failures).Tests/AblyChatTests/DefaultMessagesTests.swift (14)
7-8: Clear test naming.
The method name accurately describes its scenario (no channel serial).
19-21: Verifies the thrown error.
Using#expect(throws:)effectively checks the exception. No concerns here.
29-29: MockRealtime initialization.
This setup is straightforward and consistent with your test approach.
31-31: Channel creation.
No issues; the mock channel enables flexible testing.
35-40: Ensures no error occurs.
Checking thatgetdoes not throw aligns with expected success.
45-46: Method name aligns with subscription behavior.
Straightforward validation; no concerns.
49-55: Channel properties setup.
Attaching predefined serials for testing is consistent with the new approach.
60-63: Zero items scenario.
Validates empty message retrieval. Good edge-case coverage.
65-66: Retrieving previous messages.
This call tests history retrieval logic effectively.
69-69: Equality check.
Comparing the fetched result to the expected wrapper is clear and concise.
73-73: Header extraction test.
Verifies correct parsing of headers from channel messages.
109-109: Metadata extraction test.
Ensures the subscription captures and stores metadata correctly.
119-130: Mock message assembly.
Setting properties (action, data, extras) is consistent with the test scenario.
149-149: Channel instantiation.
Simply tests default channel creation. No issues.Tests/AblyChatTests/Mocks/MockRealtimeChannel.swift (10)
5-6: Presence property introduced.
This aligns with typical presence testing, though usage remains minimal.
32-32: Underscore for state parameter.
Indicates unused parameter in Swift, which is acceptable in mocks.
35-35: Optional message emission parameter.
Allows test customization without complicating constructor calls.
74-74: Channel state forcibly set to.attached.
Verify if you need alternate states for tests. This could mask issues if other states are expected.
86-86: Unimplemented attach method.
ThrowsfatalError; reminder to confirm no tests rely on real attach logic.
89-89: AttachOrDetachResult enum.
Maintains the success/failure pattern with callback. Looks good.
107-107: attach(_ callback:) updates call counter.
Helps track attach attempts; no concerns.
146-146: Emit message if available.
This conditional callback approach helps simulate real-time updates.
183-184: Stubbedon(_:callback:).
Returning a simpleARTEventListener()suits basic mocking.
187-188: Another unimplementedon(...).
Approach is consistent with partial mocking.
2 participants
This reverts some tests that need changes for the sake of test stability or maintainability. See commit messages for more details. First discussed in #231 (comment).
Created #235 for restoring these.
Summary by CodeRabbit