chore: add some missing tests #79
Conversation
WalkthroughThe changes in this pull request enhance coroutine management within the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Outside diff range and nitpick comments (8)
chat-android/src/test/java/com/ably/chat/ConnectionTest.kt (2)
111-146: Consider documenting the timing choicesThe test effectively validates the cancellation of the transient disconnect timer. Consider adding a comment explaining why 3000ms was chosen as the intermediate time check point before reconnection.
@Test fun `should cancel the transient disconnect timer IF realtime connections status changes to CONNECTED`() = runTest { + // We check at 3000ms (before the 5s disconnect threshold) to ensure the timer + // is properly cancelled when connection is restored mid-wait val testScheduler = TestCoroutineScheduler()
77-77: Consider adding tests for additional edge casesThe current tests cover the main scenarios well. Consider adding tests for:
- Multiple rapid state transitions within the 5-second window
- Error scenarios during state transitions
- Edge cases with very short/long timing values
Would you like me to provide example implementations for these test cases?
chat-android/src/test/java/com/ably/chat/SandboxTest.kt (2)
90-109: Enhance reaction test coverageThe test verifies basic reaction functionality but could be more comprehensive.
Consider adding assertions for:
- Reaction timestamp
- Reaction sender's clientId
- Multiple reactions
- Invalid reaction types
room.reactions.subscribe { reactionEvent.complete(it) } room.reactions.send("heart") assertEquals( - "heart", - reactionEvent.await().type, + Reaction( + type = "heart", + clientId = "sandbox-client", + timestamp = reactionEvent.await().timestamp + ), + reactionEvent.await() )
111-129: Expand message test coverageThe test verifies basic message functionality but could be more comprehensive.
Consider adding test cases for:
- Messages with different content types
- Message metadata
- Error cases (e.g., message too large)
- Message ordering
- Multiple messages
room.messages.subscribe { messageEvent.complete(it) } room.messages.send("hello") assertEquals( - "hello", - messageEvent.await().message.text, + Message( + text = "hello", + clientId = "sandbox-client", + timestamp = messageEvent.await().message.timestamp, + metadata = null + ), + messageEvent.await().message )chat-android/src/test/java/com/ably/chat/TypingTest.kt (2)
48-56: Consider adding error case for presence enterThe test verifies successful typing session start but doesn't handle failure cases.
Add a test case for presence enter failure:
@Test fun `when presence enter fails, typing session is not started`() = runTest { every { pubSubPresence.enterClient(DEFAULT_CLIENT_ID, any(), any()) } answers { val completionListener = arg<CompletionListener>(2) completionListener.onError(ErrorInfo("Failed to enter", 40000)) } val typing = DefaultTyping(room) typing.start() verify(exactly = 1) { pubSubPresence.enterClient("clientId", any(), any()) } verify(exactly = 0) { pubSubPresence.leaveClient("clientId", any(), any()) } }
79-105: Add test for concurrent typing sessionsThe test verifies timeout extension but doesn't cover concurrent typing scenarios.
Consider adding a test case for concurrent typing sessions to ensure proper synchronization:
@Test fun `concurrent typing sessions are properly synchronized`() = runTest { val typing = DefaultTyping(room, dispatcher) scope.launch { typing.start() } scope.launch { typing.start() } testScheduler.runCurrent() verify(exactly = 1) { pubSubPresence.enterClient("clientId", any(), any()) } }chat-android/src/main/java/com/ably/chat/Connection.kt (1)
118-118: Document the dispatcher parameter and its impactThe addition of a configurable dispatcher is good, but needs documentation.
Add KDoc to explain the dispatcher parameter and its usage:
internal class DefaultConnection( pubSubConnection: PubSubConnection, private val logger: Logger, + /** + * The dispatcher to use for connection-related coroutines. + * Defaults to Dispatchers.Default. The dispatcher will be limited + * to parallelism of 1 to ensure proper sequencing of connection events. + */ dispatcher: CoroutineDispatcher = Dispatchers.Default, )Also applies to: 121-121
chat-android/src/main/java/com/ably/chat/Typing.kt (1)
Line range hint
200-223: Consider enhancing error handling in the retry mechanismWhile the retry mechanism is well-implemented with exponential backoff, consider these improvements:
- Distinguish between different types of exceptions to avoid retrying unrecoverable errors
- Include more context in the final error log, such as the last error encountered
private suspend fun processEvent() { var numRetries = 0 + var lastError: Exception? = null while (numRetries <= PRESENCE_GET_MAX_RETRIES) { try { val currentlyTyping = get() emit(currentlyTyping) return // Exit if successful } catch (e: Exception) { + // Don't retry on certain exceptions + when (e) { + is IllegalStateException -> throw e // Don't retry on illegal state + is AblyException -> { + if (e.errorInfo.code >= 40000) throw e // Don't retry on 4xxxx errors + } + } + lastError = e numRetries++ val delayDuration = min( PRESENCE_GET_RETRY_MAX_INTERVAL_MS, PRESENCE_GET_RETRY_INTERVAL_MS * 2.0.pow(numRetries).toLong(), ) logger.debug("Retrying in $delayDuration ms... (Attempt $numRetries of $PRESENCE_GET_MAX_RETRIES)", e) delay(delayDuration) } } - logger.error("Failed to get members after $PRESENCE_GET_MAX_RETRIES retries") + logger.error("Failed to get members after $PRESENCE_GET_MAX_RETRIES retries", lastError) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
chat-android/src/main/java/com/ably/chat/Connection.kt(2 hunks)chat-android/src/main/java/com/ably/chat/Typing.kt(3 hunks)chat-android/src/test/java/com/ably/chat/ConnectionTest.kt(3 hunks)chat-android/src/test/java/com/ably/chat/SandboxTest.kt(1 hunks)chat-android/src/test/java/com/ably/chat/TypingTest.kt(1 hunks)
🧰 Additional context used
📓 Learnings (3)
chat-android/src/test/java/com/ably/chat/SandboxTest.kt (1)
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#66
File: chat-android/src/test/java/com/ably/chat/RoomReactionsTest.kt:44-50
Timestamp: 2024-11-28T11:08:42.524Z
Learning: The test cases for verifying behavior when the room is not in the ATTACHED state are covered in `chat-android/src/test/java/com/ably/chat/room/RoomEnsureAttachedTest.kt`.
chat-android/src/main/java/com/ably/chat/Typing.kt (1)
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#66
File: chat-android/src/main/java/com/ably/chat/Typing.kt:178-183
Timestamp: 2024-11-28T11:10:20.947Z
Learning: In `chat-android/src/main/java/com/ably/chat/Typing.kt`, within the `DefaultTyping` class, the `stop()` method must execute within `typingScope` (a `CoroutineScope` with parallelism set to 1) to avoid race conditions when setting and cancelling `typingJob`.
chat-android/src/test/java/com/ably/chat/TypingTest.kt (1)
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#66
File: chat-android/src/main/java/com/ably/chat/Typing.kt:178-183
Timestamp: 2024-11-28T11:10:20.947Z
Learning: In `chat-android/src/main/java/com/ably/chat/Typing.kt`, within the `DefaultTyping` class, the `stop()` method must execute within `typingScope` (a `CoroutineScope` with parallelism set to 1) to avoid race conditions when setting and cancelling `typingJob`.
🔇 Additional comments (7)
chat-android/src/test/java/com/ably/chat/ConnectionTest.kt (2)
11-11: LGTM! Appropriate test dependencies added
The added imports for coroutine testing (StandardTestDispatcher, TestCoroutineScheduler) and time handling (Duration.Companion.milliseconds) are appropriate for the new test scenarios.
Also applies to: 13-14
78-109: Well-structured test implementation for the 5-second wait requirement
The test effectively validates the 5-second delay requirement when transitioning from CONNECTED to DISCONNECTED state. Good use of TestCoroutineScheduler for controlling time progression.
chat-android/src/test/java/com/ably/chat/TypingTest.kt (1)
25-46: Well-structured test setup with mocked dependencies
The test setup is clean and follows best practices with proper mocking of dependencies.
chat-android/src/main/java/com/ably/chat/Connection.kt (1)
121-121: Verify the impact of dispatcher changes on connection handling
The change in dispatcher could affect connection state handling.
✅ Verification successful
Let me check the connection test file and dispatcher usage to better understand the impact.
The dispatcher change is safe and well-tested
The dispatcher change is verified to be safe because:
- The test suite explicitly tests connection state transitions with a custom test dispatcher
- The tests verify critical timing-dependent behaviors like the 5-second wait period for disconnection
- The
limitedParallelism(1)ensures sequential processing of connection events, maintaining the same concurrency guarantees - The dispatcher is properly injected and can be overridden in tests, showing good design for testability
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any existing connection handling issues or patterns
rg -A 5 "class.*Connection.*Dispatcher" || echo "No other connection classes with dispatcher found"
# Check for any connection-related tests that might need updating
rg -l "class.*ConnectionTest" || echo "No connection test files found"
Length of output: 241
Script:
#!/bin/bash
# Check the connection test implementation
cat chat-android/src/test/java/com/ably/chat/ConnectionTest.kt
# Check for dispatcher usage in Connection class
rg -A 5 "dispatcher" chat-android/src/main/java/com/ably/chat/Connection.kt
Length of output: 5569
chat-android/src/main/java/com/ably/chat/Typing.kt (3)
Line range hint 12-21: LGTM: Necessary coroutine imports added
The added imports are appropriate for the new coroutine functionality being introduced.
Line range hint 23-38: LGTM: Well-structured retry mechanism constants
The retry mechanism constants are well-defined with appropriate values:
- Base retry interval of 1.5s with exponential backoff
- Maximum retry interval capped at 30s
- Reasonable maximum of 5 retries
Line range hint 95-108: LGTM: Improved coroutine management with dispatcher injection
The changes effectively address potential race conditions and improve testability:
- Dispatcher injection allows for better testing control
- Using
limitedParallelism(1)ensures sequential execution of coroutines, preventing race conditions as noted in previous learnings
| private fun fireDisconnected() = pubSubConnectionStateListenerSlot.captured.onConnectionStateChanged( | ||
| ConnectionStateChange( | ||
| ConnectionState.initialized, | ||
| ConnectionState.connected, | ||
| 0, | ||
| null, | ||
| ), | ||
| ) |
There was a problem hiding this comment.
Fix incorrect state transition in fireDisconnected
The fireDisconnected method incorrectly transitions to ConnectionState.connected instead of ConnectionState.disconnected. This could lead to incorrect test behavior.
private fun fireDisconnected() = pubSubConnectionStateListenerSlot.captured.onConnectionStateChanged(
ConnectionStateChange(
- ConnectionState.initialized,
- ConnectionState.connected,
+ ConnectionState.connected,
+ ConnectionState.disconnected,
0,
null,
),
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private fun fireDisconnected() = pubSubConnectionStateListenerSlot.captured.onConnectionStateChanged( | |
| ConnectionStateChange( | |
| ConnectionState.initialized, | |
| ConnectionState.connected, | |
| 0, | |
| null, | |
| ), | |
| ) | |
| private fun fireDisconnected() = pubSubConnectionStateListenerSlot.captured.onConnectionStateChanged( | |
| ConnectionStateChange( | |
| ConnectionState.connected, | |
| ConnectionState.disconnected, | |
| 0, | |
| null, | |
| ), | |
| ) |
ttypic
force-pushed
the
chore-add-missing-tests
branch
from
4078b7c to
b127b24
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
chat-android/src/test/java/com/ably/chat/ConnectionTest.kt (2)
82-109: Consider using TRANSIENT_TIMEOUT constant and adding comments.The test effectively validates the connection state transitions, but could be improved for maintainability:
- Use
TRANSIENT_TIMEOUTconstant fromConnection.ktinstead of hardcoding5000.milliseconds- Add comments to explain the test flow and expectations
@Test fun `should wait 5 sec if the connection status transitions from CONNECTED to DISCONNECTED`() = runTest { + // Setup test environment with controlled time val testScheduler = TestCoroutineScheduler() val dispatcher = StandardTestDispatcher(testScheduler) val connection = DefaultConnection(pubSubConnection, EmptyLogger(LogContext(tag = "TEST")), dispatcher) var status = ConnectionStatus.Initialized connection.onStatusChange { status = it.current } + // Verify initial connection fireConnected() testScheduler.runCurrent() assertEquals(ConnectionStatus.Connected, status) + // Trigger disconnection fireDisconnected() testScheduler.runCurrent() + // Status should still be Connected during transient timeout assertEquals(ConnectionStatus.Connected, status) - testScheduler.advanceTimeBy(5000.milliseconds) + // Advance time by transient timeout duration + testScheduler.advanceTimeBy(TRANSIENT_TIMEOUT.milliseconds) testScheduler.runCurrent() + // Status should now be Disconnected assertEquals(ConnectionStatus.Disconnected, status) }
115-146: Consider using constants and adding comments.Similar to the previous test, this test could be improved for maintainability:
- Use constants for time values (e.g.,
TRANSIENT_TIMEOUT)- Add comments to explain the test flow and expectations
@Test fun `should cancel the transient disconnect timer IF realtime connections status changes to CONNECTED`() = runTest { + // Setup test environment with controlled time val testScheduler = TestCoroutineScheduler() val dispatcher = StandardTestDispatcher(testScheduler) val connection = DefaultConnection(pubSubConnection, EmptyLogger(LogContext(tag = "TEST")), dispatcher) var status = ConnectionStatus.Initialized connection.onStatusChange { status = it.current } + // Verify initial connection fireConnected() testScheduler.runCurrent() assertEquals(ConnectionStatus.Connected, status) + // Trigger disconnection fireDisconnected() testScheduler.runCurrent() + // Status should still be Connected during transient timeout assertEquals(ConnectionStatus.Connected, status) - testScheduler.advanceTimeBy(3000.milliseconds) + // Advance time partially through the transient timeout + val partialTimeout = (TRANSIENT_TIMEOUT * 0.6).toLong().milliseconds + testScheduler.advanceTimeBy(partialTimeout) + // Reconnect before timeout completes fireConnected() + // Advance past the original timeout duration testScheduler.advanceTimeBy(5000.milliseconds) testScheduler.runCurrent() + // Status should remain Connected as the timeout was cancelled assertEquals(ConnectionStatus.Connected, status) }chat-android/src/main/java/com/ably/chat/Typing.kt (1)
Line range hint
208-237: Well-implemented retry mechanism with exponential backoff.The retry mechanism for presence get operations is well-implemented with:
- Exponential backoff with a maximum interval
- Proper error logging
- Clear maximum retry limit
Consider adding a comment explaining the exponential backoff formula for future maintainers.
private suspend fun processEvent() { var numRetries = 0 while (numRetries <= PRESENCE_GET_MAX_RETRIES) { try { val currentlyTyping = get() emit(currentlyTyping) return // Exit if successful } catch (e: Exception) { numRetries++ + // Calculate delay using exponential backoff: baseInterval * 2^retryCount val delayDuration = min( PRESENCE_GET_RETRY_MAX_INTERVAL_MS, PRESENCE_GET_RETRY_INTERVAL_MS * 2.0.pow(numRetries).toLong(), ) logger.debug("Retrying in $delayDuration ms... (Attempt $numRetries of $PRESENCE_GET_MAX_RETRIES)", e) delay(delayDuration) } } logger.error("Failed to get members after $PRESENCE_GET_MAX_RETRIES retries") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
chat-android/src/main/java/com/ably/chat/Connection.kt(2 hunks)chat-android/src/main/java/com/ably/chat/Typing.kt(3 hunks)chat-android/src/test/java/com/ably/chat/ConnectionTest.kt(3 hunks)chat-android/src/test/java/com/ably/chat/SandboxTest.kt(1 hunks)chat-android/src/test/java/com/ably/chat/TypingTest.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- chat-android/src/test/java/com/ably/chat/SandboxTest.kt
- chat-android/src/test/java/com/ably/chat/TypingTest.kt
🧰 Additional context used
📓 Learnings (1)
chat-android/src/main/java/com/ably/chat/Typing.kt (1)
Learnt from: sacOO7
PR: ably/ably-chat-kotlin#66
File: chat-android/src/main/java/com/ably/chat/Typing.kt:178-183
Timestamp: 2024-11-28T11:10:20.947Z
Learning: In `chat-android/src/main/java/com/ably/chat/Typing.kt`, within the `DefaultTyping` class, the `stop()` method must execute within `typingScope` (a `CoroutineScope` with parallelism set to 1) to avoid race conditions when setting and cancelling `typingJob`.
🔇 Additional comments (3)
chat-android/src/test/java/com/ably/chat/ConnectionTest.kt (1)
148-164: LGTM! Helper methods correctly simulate state transitions.
The fireConnected and fireDisconnected methods are well-implemented and correctly simulate the connection state transitions. The fireDisconnected method has been properly fixed to transition from connected to disconnected state.
chat-android/src/main/java/com/ably/chat/Connection.kt (1)
118-121: LGTM! Well-implemented coroutine dispatcher injection.
The addition of the dispatcher parameter with a default value maintains backward compatibility while allowing for custom dispatchers in tests. The use of limitedParallelism(1) ensures thread-safe state updates in the connectionScope.
chat-android/src/main/java/com/ably/chat/Typing.kt (1)
Line range hint 96-108: LGTM! Consistent dispatcher injection pattern.
The addition of the dispatcher parameter follows the same pattern as Connection.kt, maintaining consistency across the codebase. The use of limitedParallelism(1) in typingScope ensures thread-safe state updates.
Code Coverage
|
There was a problem hiding this comment.
Lgtm.
Btw, since we advanced timer by exact timer expiry, it shouldn't cause test to get flaky, because of clockskew
Maybe we can add extra numerical value to reduce flakiness
i.e. instead of testScheduler.advanceTimeBy(5000.milliseconds) use
val clockSkew = 100.milliseconds
testScheduler.advanceTimeBy(5000.milliseconds + clockSkew)
Add some missing tests
Summary by CodeRabbit
New Features
Bug Fixes
Tests