[CHA-RL2][ECO-5010] RoomLifecycle DETACH tests #52
Conversation
…ycleManager related test file names to Room
WalkthroughThe changes in this pull request focus on enhancing the Changes
Assessment against linked issues
Possibly related issues
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 (
|
sacOO7
changed the title
sacOO7
force-pushed
the
tests/roomlifecycle-detach
branch
from
b3c82e9
to
5bbaf6c
Compare
sacOO7
force-pushed
the
tests/roomlifecycle-detach
branch
from
a445735
to
297c301
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (10)
chat-android/src/test/java/com/ably/chat/room/DetachTest.kt (3)
46-54: Consider enhancing test readability with more descriptive names and assertions.While the test is well-structured, consider:
- Making the test name more descriptive of the actual behavior being tested
- Adding assertions to verify that no state transitions occurred
- fun `(CHA-RL2a) Detach success when room is already in detached state`() = runTest { + fun `(CHA-RL2a) Detach operation should succeed immediately without state transitions when room is already detached`() = runTest { val statusLifecycle = spyk<DefaultRoomLifecycle>().apply { setStatus(RoomStatus.Detached) } val roomLifecycle = spyk(RoomLifecycleManager(roomScope, statusLifecycle, createRoomFeatureMocks())) val result = kotlin.runCatching { roomLifecycle.detach() } Assert.assertTrue(result.isSuccess) + verify(exactly = 0) { statusLifecycle.setStatus(any()) } assertWaiter { roomLifecycle.atomicCoroutineScope().finishedProcessing } }
129-159: Consider structuring test data setup more clearly.The test
(CHA-RL2f, CHA-RL2g)could benefit from:
- Separating test data setup from test execution
- Using test data builders for contributors
- Making the channel name assertions more maintainable
+ private fun createTestContributors(roomId: String = "1234") = listOf( + createChatMessagesContributor(roomId), + createTypingIndicatorContributor(roomId), + createReactionsContributor(roomId) + ) + private fun assertChannelNames(expected: List<String>, actual: List<io.ably.lib.realtime.Channel>) { + Assert.assertEquals(expected.size, actual.size) + expected.zip(actual).forEach { (expectedName, actualChannel) -> + Assert.assertEquals(expectedName, actualChannel.name) + } + } @Test fun `(CHA-RL2f, CHA-RL2g) Detach op should detach each contributor channel sequentially and room should be considered DETACHED`() = runTest { // Setup val statusLifecycle = spyk<DefaultRoomLifecycle>() val capturedChannels = mutableListOf<io.ably.lib.realtime.Channel>() mockkStatic(io.ably.lib.realtime.Channel::detachCoroutine) coEvery { any<io.ably.lib.realtime.Channel>().detachCoroutine() } coAnswers { capturedChannels.add(firstArg()) } - val contributors = createRoomFeatureMocks() + val contributors = createTestContributors() Assert.assertEquals(5, contributors.size) // Execute val roomLifecycle = spyk(RoomLifecycleManager(roomScope, statusLifecycle, contributors)) val result = kotlin.runCatching { roomLifecycle.detach() } // Verify Assert.assertTrue(result.isSuccess) Assert.assertEquals(RoomStatus.Detached, statusLifecycle.status) - Assert.assertEquals(5, capturedChannels.size) - repeat(5) { - Assert.assertEquals(contributors[it].channel.name, capturedChannels[it].name) - } - Assert.assertEquals("1234::\$chat::\$chatMessages", capturedChannels[0].name) - Assert.assertEquals("1234::\$chat::\$chatMessages", capturedChannels[1].name) - Assert.assertEquals("1234::\$chat::\$chatMessages", capturedChannels[2].name) - Assert.assertEquals("1234::\$chat::\$typingIndicators", capturedChannels[3].name) - Assert.assertEquals("1234::\$chat::\$reactions", capturedChannels[4].name) + assertChannelNames( + expected = listOf( + "1234::\$chat::\$chatMessages", + "1234::\$chat::\$chatMessages", + "1234::\$chat::\$chatMessages", + "1234::\$chat::\$typingIndicators", + "1234::\$chat::\$reactions" + ), + actual = capturedChannels + )
161-205: Consider improving concurrent operation test readability.The test
(CHA-RL2i)tests concurrent operations but could be more readable with:
- Clear separation of setup, execution, and verification phases
- Helper methods for common test patterns
- More descriptive assertions for concurrent behavior
+ private suspend fun awaitStatus(statusLifecycle: DefaultRoomLifecycle, expectedStatus: RoomStatus) { + assertWaiter { statusLifecycle.status == expectedStatus } + } @Test fun `(CHA-RL2i) Detach op should wait for existing operation as per (CHA-RL7)`() = runTest { + // Setup val statusLifecycle = spyk<DefaultRoomLifecycle>() - Assert.assertEquals(RoomStatus.Initializing, statusLifecycle.status) + Assert.assertEquals(RoomStatus.Initializing, statusLifecycle.status, "Room should start in Initializing state") val roomLifecycle = spyk(RoomLifecycleManager(roomScope, statusLifecycle, createRoomFeatureMocks())) val roomReleased = Channel<Boolean>() coEvery { roomLifecycle.release() } coAnswers { roomLifecycle.atomicCoroutineScope().async { statusLifecycle.setStatus(RoomStatus.Releasing) roomReleased.receive() statusLifecycle.setStatus(RoomStatus.Released) } } + // Execute release operation launch { roomLifecycle.release() } - assertWaiter { !roomLifecycle.atomicCoroutineScope().finishedProcessing } + awaitStatus(statusLifecycle, RoomStatus.Releasing) Assert.assertEquals(0, roomLifecycle.atomicCoroutineScope().pendingJobCount) // no queued jobs, one job running - assertWaiter { statusLifecycle.status == RoomStatus.Releasing } + // Execute detach operation val roomDetachOpDeferred = async(SupervisorJob()) { roomLifecycle.detach() } assertWaiter { roomLifecycle.atomicCoroutineScope().pendingJobCount == 1 } // detach op queued - Assert.assertEquals(RoomStatus.Releasing, statusLifecycle.status) + Assert.assertEquals(RoomStatus.Releasing, statusLifecycle.status, "Room should remain in Releasing state while detach is queued") + // Complete release operation roomReleased.send(true) - assertWaiter { statusLifecycle.status == RoomStatus.Released } + awaitStatus(statusLifecycle, RoomStatus.Released) + // Verify detach operation result val result = kotlin.runCatching { roomDetachOpDeferred.await() } assertWaiter { roomLifecycle.atomicCoroutineScope().finishedProcessing } Assert.assertTrue(result.isFailure) val exception = result.exceptionOrNull() as AblyExceptionchat-android/src/test/java/com/ably/chat/room/AttachTest.kt (5)
Line range hint
40-48: Consider adding documentation for the room scope configuration.While the spec reference is good, it would be helpful to document why the room scope is configured with limited parallelism and a dedicated coroutine name. This would help other developers understand the threading model and its implications.
Line range hint
87-146: Consider extracting common test setup logic.The test setup for mocking contributors and channels is repeated across multiple tests. Consider extracting this into helper methods to improve maintainability and reduce duplication.
+ private fun mockContributors(count: Int = 5): List<ResolvedContributor> { + return createRoomFeatureMocks("1234").also { + Assert.assertEquals(count, it.size) + } + } + + private fun mockChannelAttachment(block: (io.ably.lib.realtime.Channel) -> Unit) { + mockkStatic(io.ably.lib.realtime.Channel::attachCoroutine) + coEvery { any<io.ably.lib.realtime.Channel>().attachCoroutine() } coAnswers { + block(firstArg()) + } + }
Line range hint
148-238: Consider using constants for error codes and messages.The test uses magic numbers for error codes (500, 102_102, etc.) and hardcoded error messages. Consider extracting these into constants for better maintainability.
+ private companion object { + const val HTTP_INTERNAL_ERROR = 500 + const val ROOM_RELEASING_ERROR = 102_102 + const val ROOM_RELEASED_ERROR = 102_103 + const val ERROR_MSG_ROOM_RELEASING = "unable to attach room; room is releasing" + const val ERROR_MSG_ROOM_RELEASED = "unable to attach room; room is released" + }
Line range hint
240-393: Consider splitting complex test cases.Some test methods, particularly those testing failure scenarios, are quite long and test multiple aspects. Consider splitting them into smaller, more focused test cases for better readability and maintenance.
For example,
(CHA-RL1h1, CHA-RL1h2)could be split into:
- Test for suspended state transition
- Test for error message and code
- Test for channel state verification
Missing Assertions for Retry Intervals and Maximum Attempts
- Add assertions to verify the timing between retries.
- Add assertions to check the maximum number of retry attempts.
🔗 Analysis chain
Line range hint
395-447: Verify retry mechanism more explicitly.The retry mechanism testing in
CHA-RL1h6could be more explicit about the retry intervals and maximum attempts. Consider adding assertions to verify the timing between retries.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for retry-related configurations in the codebase rg -A 5 "retry|backoff|attempt" --type kotlinLength of output: 13319
Script:
#!/bin/bash # Search for assertion statements related to timing and retry counts in AttachTest.kt rg "Assert\.(assert|verify).*" chat-android/src/test/java/com/ably/chat/room/AttachTest.kt rg "delay\s*\(" chat-android/src/test/java/com/ably/chat/room/AttachTest.kt rg "retry" chat-android/src/test/java/com/ably/chat/room/AttachTest.ktLength of output: 4814
Script:
#!/bin/bash # Search for assertions related to timing between retries in AttachTest.kt rg "Assert\.(assertTrue|assertEquals).*delay" chat-android/src/test/java/com/ably/chat/room/AttachTest.kt rg "max retry|maxAttempt|max_attempt|maximum retries" chat-android/src/test/java/com/ably/chat/room/AttachTest.kt rg "Assert\.(assertTrue|assertEquals).*retry" chat-android/src/test/java/com/ably/chat/room/AttachTest.ktLength of output: 331
chat-android/src/main/java/com/ably/chat/RoomLifecycleManager.kt (2)
Line range hint
499-526: Consolidate room status checks indetachmethodThe
detachmethod contains multiple status checks that throw exceptions for different room states. Consider refactoring these checks using awhenexpression or a separate function to enhance readability and maintainability.Example refactoring:
val status = _statusLifecycle.status when (status) { RoomStatus.Detached -> return@async RoomStatus.Releasing, RoomStatus.Released, RoomStatus.Failed -> throw AblyException.fromErrorInfo( ErrorInfo( "unable to detach room; room is ${status.name.toLowerCase()}", HttpStatusCodes.InternalServerError, when (status) { RoomStatus.Releasing -> ErrorCodes.RoomIsReleasing.errorCode RoomStatus.Released -> ErrorCodes.RoomIsReleased.errorCode RoomStatus.Failed -> ErrorCodes.RoomInFailedState.errorCode else -> ErrorCodes.RoomLifecycleError.errorCode } ) ) else -> { // Proceed with detachment } }
552-573: Implement a retry limit or exponential backoff indoDetachmethodThe
doDetachmethod retries indefinitely until all channels are detached, which could lead to infinite loops in case of persistent failures. To prevent this, consider implementing a maximum retry limit or using exponential backoff for delays between retries.Example implementation:
var retries = 0 val maxRetries = 5 var delayDuration = _retryDurationInMs while (channelWindDown.isFailure && retries < maxRetries) { // CHA-RL2h val err = channelWindDown.exceptionOrNull() if (err is AblyException && err.errorInfo?.code != -1 && firstContributorFailedError == null) { firstContributorFailedError = err // CHA-RL2h1 } delay(delayDuration) channelWindDown = kotlin.runCatching { doChannelWindDown() } retries++ delayDuration *= 2 // Exponential backoff } if (channelWindDown.isFailure) { // Handle failure after maximum retries throw firstContributorFailedError ?: AblyException.fromErrorInfo( ErrorInfo( "detach failed after maximum retries", HttpStatusCodes.InternalServerError, ErrorCodes.RoomLifecycleError.errorCode ) ) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
chat-android/src/main/java/com/ably/chat/RoomLifecycleManager.kt(6 hunks)chat-android/src/test/java/com/ably/chat/room/AttachTest.kt(2 hunks)chat-android/src/test/java/com/ably/chat/room/DetachTest.kt(1 hunks)
🔇 Additional comments (3)
chat-android/src/test/java/com/ably/chat/room/AttachTest.kt (2)
1-12: LGTM! Well-organized package structure and imports.
The package structure follows Kotlin conventions, and imports are clean and relevant to the test implementation.
Line range hint 50-85: LGTM! Well-structured basic test cases.
The basic test cases for success and failure scenarios are well-implemented with clear assertions and error handling.
chat-android/src/main/java/com/ably/chat/RoomLifecycleManager.kt (1)
468-484: 🛠️ Refactor suggestion
Use specific error codes instead of placeholder -1
The error codes -1 used in the ErrorInfo instances may not provide meaningful information for error handling. It's recommended to define and use specific error codes that align with your application's error handling policies. This will improve error diagnosis and handling.
Apply this diff to specify appropriate error codes:
} catch (throwable: Throwable) {
// ...
// CHA-RL2h3 - We throw an error so that the promise rejects
- throw AblyException.fromErrorInfo(throwable, ErrorInfo("detach failure, retry", -1, -1))
+ val errorCode = ErrorCodes.DetachFailureRetry.errorCode // Define appropriate error code
+ throw AblyException.fromErrorInfo(
+ throwable,
+ ErrorInfo("detach failure, retry", HttpStatusCodes.InternalServerError, errorCode)
+ )
}Likely invalid or redundant comment.
| fun `(CHA-RL2h3) If channel fails to detach entering another state (ATTACHED), detach will be retried until finally detached`() = runTest { | ||
| val statusLifecycle = spyk<DefaultRoomLifecycle>() | ||
| val roomEvents = mutableListOf<RoomStatusChange>() | ||
| statusLifecycle.onChange { | ||
| roomEvents.add(it) | ||
| } | ||
|
|
||
| mockkStatic(io.ably.lib.realtime.Channel::detachCoroutine) | ||
| var failDetachTimes = 5 | ||
| coEvery { any<io.ably.lib.realtime.Channel>().detachCoroutine() } coAnswers { | ||
| val channel = firstArg<io.ably.lib.realtime.Channel>() | ||
| delay(200) | ||
| if (--failDetachTimes >= 0) { | ||
| channel.setState(ChannelState.attached) | ||
| error("failed to detach channel") | ||
| } | ||
| } | ||
|
|
||
| val contributors = createRoomFeatureMocks("1234") | ||
| val roomLifecycle = spyk(RoomLifecycleManager(roomScope, statusLifecycle, contributors), recordPrivateCalls = true) | ||
|
|
||
| val result = kotlin.runCatching { roomLifecycle.detach() } | ||
| Assert.assertTrue(result.isSuccess) | ||
| Assert.assertEquals(RoomStatus.Detached, statusLifecycle.status) | ||
| assertWaiter { roomLifecycle.atomicCoroutineScope().finishedProcessing } | ||
|
|
||
| Assert.assertEquals(0, roomEvents.filter { it.current == RoomStatus.Failed }.size) // Zero failed room status events emitted | ||
| Assert.assertEquals(1, roomEvents.filter { it.current == RoomStatus.Detached }.size) // Only one detach event received | ||
|
|
||
| // Channel detach success on 6th call | ||
| coVerify(exactly = 6) { | ||
| roomLifecycle["doChannelWindDown"](any<ResolvedContributor>()) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving test reliability for timing-dependent scenarios.
The test (CHA-RL2h3) relies on timing with delay(200) which could make it flaky. Consider:
- Using
TestCoroutineSchedulerfor deterministic timing control - Extracting the retry count to a constant
- Adding assertions for the intermediate states
+ private companion object {
+ const val MAX_DETACH_RETRIES = 5
+ }
@Test
fun `(CHA-RL2h3) If channel fails to detach entering another state (ATTACHED), detach will be retried until finally detached`() = runTest {
val statusLifecycle = spyk<DefaultRoomLifecycle>()
val roomEvents = mutableListOf<RoomStatusChange>()
statusLifecycle.onChange {
roomEvents.add(it)
}
mockkStatic(io.ably.lib.realtime.Channel::detachCoroutine)
- var failDetachTimes = 5
+ var failDetachTimes = MAX_DETACH_RETRIES
coEvery { any<io.ably.lib.realtime.Channel>().detachCoroutine() } coAnswers {
val channel = firstArg<io.ably.lib.realtime.Channel>()
- delay(200)
+ currentTestScheduler.advanceTimeBy(200)
if (--failDetachTimes >= 0) {
channel.setState(ChannelState.attached)
error("failed to detach channel")
}
}Committable suggestion skipped: line range outside the PR's diff.
| while (channelWindDown.isFailure) { // CHA-RL2h | ||
| val err = channelWindDown.exceptionOrNull() | ||
| if (err is AblyException && err.errorInfo?.code != -1 && channelFailedError == null) { | ||
| channelFailedError = err | ||
| if (err is AblyException && err.errorInfo?.code != -1 && firstContributorFailedError == null) { | ||
| firstContributorFailedError = err // CHA-RL2h1- First failed contributor error is captured |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid relying on placeholder error codes in conditionals
The condition err.errorInfo?.code != -1 relies on -1 as a placeholder error code, which may not be reliable once specific error codes are implemented. Consider updating this condition to check against defined error codes to improve error handling robustness.
if (err is AblyException && err.errorInfo?.code != -1 && firstContributorFailedError == null) {
firstContributorFailedError = err // CHA-RL2h1
}Update the condition to check for specific error codes:
+ val retryableErrorCodes = setOf( /* list of retryable error codes */ )
if (err is AblyException && err.errorInfo?.code in retryableErrorCodes && firstContributorFailedError == null) {
firstContributorFailedError = err // CHA-RL2h1
}Committable suggestion skipped: line range outside the PR's diff.
Related to [CHA-RL2] Implement Room DETACH operation #22
TODO's
(CHA-RL2a) [Testable] If the room status is already DETACHED, then this operation is a no-op.
(CHA-RL2b) [Testable] If the room is in the RELEASING status, the operation shall be rejected with an ErrorInfo with the RoomIsReleasing error code from the chat-specific error codes.
(CHA-RL2c) [Testable] If the room is in the RELEASED status, the operation shall be rejected with an ErrorInfo with the RoomIsReleased error code from the chat-specific error codes.
(CHA-RL2d) [Testable] If the room is in the FAILED status, the operation shall be rejected with an ErrorInfo with RoomInFailedState error code from the chat-specific error codes.
(CHA-RL2e) [Testable] Notwithstanding the above points, when the detachment operation begins, the room shall be transitioned to the DETACHING status and any transient disconnect timeouts cleared.
(CHA-RL2f) [Testable] The underlying contributors Realtime Channels will be detached in sequence – a call to detach() must complete before the next call begins.
(CHA-RL2g) [Testable] If all channel detachments complete successfully (all calls detach() return successfully), then the room shall transition to the DETACHED status.
(CHA-RL2h) If during detachment, a channel fails to detach (i.e. the call to detach() returns an error), different operations are performed based on the nature of the detach.
(CHA-RL2h1) [Testable] If a channel enters the FAILED state during detachment (i.e. the detach() operation fails and upon subsequently checking the channel state, it is FAILED), then the room will transition to the FAILED status. The detach operation continues until, for every channel, either a call to detach() has succeeded, or the channel enters the FAILED state, with the operation throwing an ErrorInfo with the feature-specific error code of the first feature to fail, using the error returned by the call to detach() as the cause. The same ErrorInfo must accompany the FAILED room status.
(CHA-RL2h2) [Testable] If the room is already in a FAILED status during the detach operation and another channel fails, the FAILED status will not be emitted twice.
(CHA-RL2h3) [Testable] If a channel enters another state during detachment (namely ATTACHED, which happens when detach times out), then the CHA-RL2f detachment cycle will be retried after a 250ms pause. This is indicated by the detach() call returning an error, at which point the current state may be inspected on the channel object. The rooms status will not change during this process.
(CHA-RL2i) [Testable] If a room lifecycle operation is already in progress, this operation shall wait until the current operation completes before continuing, subject to CHA-RL7.
Annotate code and tests with relevant spec
Summary by CodeRabbit
New Features
Bug Fixes
Tests