[ECO-5132] Added typing tests #217
Conversation
WalkthroughThe pull request enhances the Changes
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
temporarily deployed
to
staging/pull/217/AblyChat
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
Sources/AblyChat/DefaultTyping.swift (2)
9-9: Fix typo in property name.The property name contains a typo: "Presense" should be "Presence".
- private let maxPresenseGetRetryDuration: TimeInterval // Max duration as specified in CHA-T6c1 + private let maxPresenceGetRetryDuration: TimeInterval // Max duration as specified in CHA-T6c1
242-252: Prevent memory leaks in test subscriptions.The test subscription arrays can grow indefinitely as new subscriptions are added. Consider adding cleanup logic to remove terminated subscriptions.
Add a method to clean up terminated subscriptions:
private func cleanupTerminatedSubscriptions() { testStartTypingEventSubscriptions.removeAll(where: { $0.isTerminated }) testStopTypingEventSubscriptions.removeAll(where: { $0.isTerminated }) testPresenceGetTypingEventSubscriptions.removeAll(where: { $0.isTerminated }) testPresenceGetRetryTypingEventSubscriptions.removeAll(where: { $0.isTerminated }) }Call this method before adding new subscriptions in each
testsOnly_subscribeTo*method.Tests/AblyChatTests/DefaultRoomTypingTests.swift (3)
298-298: Implement missing test for CHA-T6c2 specification.The test
ifMultiplePresenceEventsReceivedThenOnlyTheLatestEventIsEmittedis currently unimplemented.Would you like me to help implement this test case? I can provide a complete implementation that verifies only the latest event is emitted when multiple presence events are received.
268-268: Document the rationale for the arbitrary timeout value.The comment mentions the timeout value is arbitrary and references an issue. Consider adding more context about why this specific value was chosen.
- let maxPresenseGetRetryDuration = 3.0 // arbitrary, TODO: improve https://github.com/ably/ably-chat-swift/issues/216 + let maxPresenseGetRetryDuration = 3.0 // Chosen to balance between quick test execution and reliable presence get retries + // TODO: Make this configurable or determine dynamically - https://github.com/ably/ably-chat-swift/issues/216
133-134: Extract magic numbers into named constants.The timeout value of 0.5 seconds is hardcoded without explanation. Consider extracting this and other similar values into named constants with clear documentation.
+ // Default timeout is 5 seconds, using 0.5 for faster test execution + private static let testTimeout: TimeInterval = 0.5 + - let timeout = 0.5 // default is 5 (seconds) + let timeout = Self.testTimeout
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Sources/AblyChat/DefaultTyping.swift(5 hunks)Tests/AblyChatTests/DefaultRoomTypingTests.swift(1 hunks)Tests/AblyChatTests/Helpers/Helpers.swift(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- 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: SPM,
releaseconfiguration (Xcode 16) - GitHub Check: SPM (Xcode 16)
- GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16) - GitHub Check: Xcode, macOS (Xcode 16)
- GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16) - GitHub Check: Xcode, tvOS (Xcode 16)
🔇 Additional comments (1)
Sources/AblyChat/DefaultTyping.swift (1)
Line range hint
43-82: Consider potential race condition in presence get retry logic.The retry logic uses a shared
totalElapsedTimevariable that could be affected by concurrent presence get operations. Consider using a dedicated task for managing retries.Consider refactoring the retry logic into a dedicated async task that can be cancelled when needed. This would help prevent race conditions and make the retry state more manageable.
maratal
force-pushed
the
114-tests
branch
4 times, most recently
from
ddb3e18 to
3fdad4d
Compare
maratal
force-pushed
the
140-typing-tests
branch
from
612bbc8 to
5466b2f
Compare
github-actions
bot
temporarily deployed
to
staging/pull/217/AblyChat
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
Sources/AblyChat/DefaultTyping.swift (1)
Line range hint
43-82: Ensure Retry Logic Correctly Respects Maximum Retry DurationIn the retry loop for
presence.get(), verify that thetotalElapsedTimecalculation correctly accounts for the delays, including jitter, and that the loop exits after reachingmaxPresenceGetRetryDuration.Review and adjust the calculation of
totalElapsedTimeto ensure it accurately reflects the accumulated wait times. This ensures the retry mechanism adheres to the specified maximum duration.
🧹 Nitpick comments (7)
Tests/AblyChatTests/DefaultRoomTypingTests.swift (6)
25-52: Ensure Proper Synchronization When Testing Attachment StateIn
retrieveCurrentlyTypingClientIDsWhileAttaching(), the test sets up an attaching state and waits for the room to become attaching before callingdefaultTyping.get(). To prevent flaky tests, ensure that the test reliably waits for the expected state.Consider adding explicit synchronization or wait mechanisms to ensure the room status is in the attaching state before proceeding. This can improve the reliability of the test.
122-149: Potential Race Condition in Timing-Sensitive TestIn
usersMayConfigureTimeoutForTyping(), the test checks if the typing stopped event is emitted after a custom timeout interval. Timing-based tests can lead to flaky results due to execution environment variability.Consider using mock timers or expectations to control the timing more deterministically. This approach can enhance the reliability of the test across different environments.
194-217: Adjust Tolerance for Timing ComparisonIn
ifTypingIsAlreadyInProgressThenTimeoutIsExtended(), the test compares intervals using a tolerance of0.1. Due to potential execution speed variations, consider increasing the tolerance to prevent intermittent test failures.Adjust the tolerance value to a higher number, such as
0.2, or utilize mock timers for better reliability in timing comparisons.
241-259: Ensure Proper Cleanup of Subscriptions in TestsIn
whenPresenceEventReceivedClientWillPerformPresenceGet(), verify that any subscriptions or event listeners are properly cleaned up after the test to prevent side effects on other tests.Add code to unsubscribe or cancel any ongoing subscriptions at the end of the test. This practice ensures test isolation and reliability.
263-293: Assist with Implementing the Presence Get Retry TestThe test
ifPresenceGetFailsItShallBeRetriedUsingBackoffWithJitter()includes a TODO comment referencing issue #216.Would you like assistance in implementing this test case? I can help draft the test code or provide guidance on handling the retry logic with backoff and jitter.
297-300: Complete the Test Implementation for Multiple Presence EventsThe test
ifMultiplePresenceEventsReceivedThenOnlyTheLatestEventIsEmitted()is marked as TODO.Do you need help in implementing this test? I can assist in writing the test code to verify that only the latest typing event is emitted when multiple presence events are received.
Sources/AblyChat/DefaultTyping.swift (1)
236-282: Organize Test-Only Code for Better MaintainabilityThe test-only code under
#if DEBUGincludes several subscription methods and structures. For improved maintainability, consider organizing these methods together and providing clear documentation of their purpose.Grouping the test-only code into an extension or a separate file (if appropriate) can enhance code readability and maintenance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Sources/AblyChat/DefaultTyping.swift(5 hunks)Tests/AblyChatTests/DefaultRoomTypingTests.swift(1 hunks)Tests/AblyChatTests/Helpers/Helpers.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Tests/AblyChatTests/Helpers/Helpers.swift
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: Example app, tvOS (Xcode 16)
- GitHub Check: Xcode, tvOS (Xcode 16)
- GitHub Check: Example app, iOS (Xcode 16)
- GitHub Check: Xcode, iOS (Xcode 16)
- GitHub Check: Example app, macOS (Xcode 16)
- GitHub Check: Xcode, macOS (Xcode 16)
- GitHub Check: SPM (Xcode 16)
- GitHub Check: Xcode,
releaseconfiguration, tvOS (Xcode 16) - GitHub Check: Xcode,
releaseconfiguration, iOS (Xcode 16) - GitHub Check: Xcode,
releaseconfiguration, macOS (Xcode 16) - GitHub Check: SPM,
releaseconfiguration (Xcode 16)
🔇 Additional comments (9)
Tests/AblyChatTests/DefaultRoomTypingTests.swift (5)
9-21: Good Coverage for Retrieving Typing Client IDsThe test
retrieveCurrentlyTypingClientIDs()correctly verifies that thedefaultTyping.get()method retrieves the list of currently typing client IDs. The use of mock presence with predefined members ensures the test is reliable and effective.
56-95: Accurate Error Handling in Failure ScenariosThe test
retrieveCurrentlyTypingClientIDsWhileAttachingWithFailure()correctly handles exceptions when attachment fails. The error handling in lines 82~ to 91~ captures and asserts the expected status code and error code appropriately.
97-118: Proper Validation of Error Handling When Room Is in Invalid StateThe test
failToRetrieveCurrentlyTypingClientIDsWhenRoomInInvalidState()effectively verifies that an error is thrown when attempting to retrieve typing client IDs while the room is in an invalid state. The assertions correctly check for the expected error conditions.
156-189: Effective Testing of Start and Stop Typing IndicatorsThe test
usersMayIndicateThatTheyHaveStartedOrStoppedTyping()effectively verifies that the typing indicators work as expected when starting and stopping typing. The assertions confirm that the typing client IDs are correctly updated.
221-237: Comprehensive Test for Subscribing to Typing EventsThe test
usersMaySubscribeToTypingEvents()successfully verifies that subscribing to typing events works and that the correct data is received. The use of subscriptions ensures that typing events are emitted and captured as expected.Sources/AblyChat/DefaultTyping.swift (4)
12-18: ConfigurablemaxPresenceGetRetryDurationin InitializerThe initializer for
DefaultTypingnow accepts themaxPresenceGetRetryDurationparameter with a default value of30.0. This change enhances configurability and allows for better control over the retry mechanism.
75-79: Proper Emission of Test Events During Retry AttemptsThe code under
#if DEBUGemits test events during retry attempts. This practice is appropriate for testing purposes.Ensure that these test-only events are properly guarded and do not impact production code.
173-177: Test Events Emitted When Stopping Typing IndicatorsIn the
stop()method, test events are emitted under#if DEBUGwhen typing is stopped. This aids in testing the stop functionality.
227-231: Test Events Emitted When Starting Typing IndicatorsSimilarly, in the
startTyping()method, test events are emitted under#if DEBUGwhen typing starts. This facilitates testing the start functionality.
maratal
force-pushed
the
140-typing-tests
branch
from
5466b2f to
267694d
Compare
github-actions
bot
temporarily deployed
to
staging/pull/217/AblyChat
|
Spec coverage test issue PR - ably/specification#272 |
maratal
force-pushed
the
140-typing-tests
branch
from
267694d to
50dbbe6
Compare
github-actions
bot
temporarily deployed
to
staging/pull/217/AblyChat
maratal
force-pushed
the
140-typing-tests
branch
from
50dbbe6 to
3e11ea0
Compare
github-actions
bot
temporarily deployed
to
staging/pull/217/AblyChat
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Sources/AblyChat/DefaultTyping.swift (2)
237-282: Enhance documentation for test-only APIs.The test infrastructure is well-organized, but the documentation could be more descriptive about the purpose and usage of each subscription type.
Apply this diff to enhance the documentation:
#if DEBUG - /// The `DefaultTyping` emits a `TestTypingEvent` each time ``start`` or ``stop`` is called. + /// Represents a typing-related test event with a timestamp. + /// Used to track when various typing-related operations occur during testing. internal struct TestTypingEvent: Equatable { internal let timestamp = Date() } - /// Subscription of typing start events for testing purposes. + /// Collection of subscriptions that receive events when typing starts. + /// These events are emitted after successfully entering presence. private var testStartTypingEventSubscriptions: [Subscription<TestTypingEvent>] = [] - /// Subscription of typing stop events for testing purposes. + /// Collection of subscriptions that receive events when typing stops. + /// These events are emitted after successfully leaving presence. private var testStopTypingEventSubscriptions: [Subscription<TestTypingEvent>] = [] - /// Subscription of presence get events for testing purposes. + /// Collection of subscriptions that receive events when presence.get() succeeds. + /// These events are emitted after successfully retrieving the presence set. private var testPresenceGetTypingEventSubscriptions: [Subscription<TestTypingEvent>] = [] - /// Subscription of retry presence get events for testing purposes. + /// Collection of subscriptions that receive events when presence.get() fails and is retried. + /// These events are emitted before each retry attempt. private var testPresenceGetRetryTypingEventSubscriptions: [Subscription<TestTypingEvent>] = []
285-287: Document the rationale for @unchecked Sendable conformance.Add documentation explaining why the conformance is needed and why it's safe to use @unchecked.
Apply this diff:
#if DEBUG +/// Conformance to Sendable is required for test-only features. +/// Using @unchecked because the class contains mutable state that is only accessed +/// in a controlled manner through test-only methods. extension DefaultTyping: @unchecked Sendable {} #endif
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Sources/AblyChat/DefaultTyping.swift(5 hunks)Tests/AblyChatTests/DefaultRoomTypingTests.swift(1 hunks)Tests/AblyChatTests/Helpers/Helpers.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Tests/AblyChatTests/Helpers/Helpers.swift
- Tests/AblyChatTests/DefaultRoomTypingTests.swift
🔇 Additional comments (4)
Sources/AblyChat/DefaultTyping.swift (4)
9-9: Fix typo in property name and enhance documentation.The property name has a typo: "Presense" should be "Presence". Additionally, consider adding more descriptive documentation using Swift's documentation comments.
Apply this diff:
-private let maxPresenceGetRetryDuration: TimeInterval // Max duration as specified in CHA-T6c1 +/// Maximum duration to retry presence get operations when they fail. +/// Specification: CHA-T6c1 +private let maxPresenceGetRetryDuration: TimeInterval
Line range hint
43-82: LGTM! Well-implemented retry mechanism.The retry logic is well-implemented with:
- Configurable maximum retry duration
- Exponential backoff with jitter
- Clear error logging
- Proper test instrumentation
173-177: LGTM! Test instrumentation properly added.Test event emission is correctly placed after the presence leave operation and properly guarded with DEBUG flag.
227-231: LGTM! Test instrumentation properly added.Test event emission is correctly placed after the timer setup and properly guarded with DEBUG flag.
maratal
force-pushed
the
140-typing-tests
branch
from
3e11ea0 to
cce8978
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
Tests/AblyChatTests/DefaultRoomTypingTests.swift (4)
148-148: Consider reducing the timing tolerance for more precise testing.The current tolerance (0.4s) is quite large compared to the expected timeout (0.5s), allowing a test to pass even if the timing is off by up to 80%. While the comment indicates this is based on CI failures, consider if there's a way to make the test more reliable without such a large tolerance.
- #expect(interval.isEqual(to: timeout, tolerance: 0.4)) // tolerance value is based on CI fails + #expect(interval.isEqual(to: timeout, tolerance: 0.2)) // reduced tolerance for more precise testing
216-216: Same timing tolerance issue as before.Consider reducing the tolerance for more precise testing.
- #expect(interval.isEqual(to: timeout + timeoutExtension, tolerance: 0.4)) + #expect(interval.isEqual(to: timeout + timeoutExtension, tolerance: 0.2))
287-287: Remove debug print statement.Debug print statements should be removed from test code before merging to maintain clean test output.
- print("Retrying presence.get() at \(event.timestamp)") + // Retrying presence.get() at \(event.timestamp)
261-293: Test implementation for retry logic could be improved.The test for the retry backoff mechanism with jitter is implemented in a way that could be brittle. The test manually loops through retries and has an arbitrary stop condition. Consider restructuring to make it more deterministic.
I would suggest modifying the test to count the number of retries rather than timing them, which would make the test more reliable:
- // Then: It shall be retried using a backoff with jitter, up to a max timeout - let retryStartedAt = Date() - let retryShouldStopBefore = retryStartedAt + maxPresenceGetRetryDuration - 1 // TODO: improve - for await event in typingPresenceGetRetrySubscription { - print("Retrying presence.get() at \(event.timestamp)") - if event.timestamp >= retryShouldStopBefore { - break - } - } - #expect(Date().distance(to: retryStartedAt) <= maxPresenceGetRetryDuration) + // Then: It shall be retried using a backoff with jitter, up to a max timeout + var retryCount = 0 + let startTime = Date() + + for await _ in typingPresenceGetRetrySubscription { + retryCount += 1 + if Date().timeIntervalSince(startTime) >= maxPresenceGetRetryDuration { + break + } + } + + // Verify we had multiple retries but stopped within the max duration + #expect(retryCount > 1) + #expect(Date().timeIntervalSince(startTime) <= maxPresenceGetRetryDuration)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Sources/AblyChat/DefaultTyping.swift(5 hunks)Tests/AblyChatTests/DefaultRoomTypingTests.swift(1 hunks)Tests/AblyChatTests/Helpers/Helpers.swift(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Tests/AblyChatTests/Helpers/Helpers.swift
- Sources/AblyChat/DefaultTyping.swift
⏰ Context from checks skipped due to timeout of 90000ms (12)
- 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, 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)
- GitHub Check: check-documentation
🔇 Additional comments (11)
Tests/AblyChatTests/DefaultRoomTypingTests.swift (11)
1-4: Good test setup with proper imports and access to testable components.The imports look good, including the
@testable import AblyChatwhich allows testing internal components. The use of the Testing framework is appropriate for Swift testing.
5-21: Well-structured test for basic typing functionality.This test verifies the core functionality of retrieving currently typing client IDs. The test follows a good pattern with clear Given-When-Then sections and appropriate expectations.
23-52: Thorough test for retrieving typing indicators during channel attachment.Good test implementation that covers the behavior while a channel is in the attaching state. The test properly waits for the room to become attaching before proceeding with the test.
54-95: Comprehensive test for attachment failures with proper error checking.The test correctly verifies error handling during attachment failures and checks for the expected error codes and statuses.
97-118: Good error handling test for invalid room states.This test properly validates that appropriate errors are thrown with the correct status codes when attempting to retrieve typing information in an invalid room state.
155-189: Well-organized test covering multiple specifications in a single test.This test effectively validates that users can start and stop typing indicators. The test is clearly annotated with which specifications each section is testing.
193-217: Good test for timeout extension behavior.This test verifies that the typing timeout is extended appropriately when typing is already in progress. The calculation for the expected interval is correct.
220-237: Good test for typing event subscription.This test validates that users can subscribe to typing events and receive notifications when typing status changes.
240-259: Well-implemented test for presence get operation.The test properly verifies that a presence.get() operation is performed when a presence event is received.
302-318: Good implementation of discontinuity handling test.The test properly verifies that discontinuity events from the feature channel are propagated through the DefaultTyping instance.
5-319: Overall excellent test coverage for typing indicator functionality.The test suite provides comprehensive coverage of the DefaultTyping class functionality, covering normal operations, error handling, timing, and event propagation. The tests follow a consistent pattern with clear Given-When-Then sections and appropriate expectations.
There are a few minor improvements suggested, primarily around timing tolerances and the completion of the TODO items, but overall this is a high-quality test implementation.
Closes #140
Summary by CodeRabbit
New Features
Tests
Improvements