[ECO-5338] Liveobject unit/integration test setup#1095
Conversation
1. Added test specific dependencies, updated junit, added mockk 2. Added test utils for mocking private classes, fields etc 3. Added IntegrationTest parameterized class along with sample test 4. Added sample unit test along with mocked realtime channel
1. Updated separate gradlew task for both unit and integration tests 2. Updated check.yml and integration-test.yml file to run respective tests
WalkthroughThis update introduces a comprehensive test infrastructure for the LiveObjects module. It adds new unit and integration tests, test utilities, and custom Gradle test tasks. CI workflows are updated to run these tests, and dependencies for testing and HTTP clients are expanded. Supporting code for error handling and test setup is also included. Changes
Sequence Diagram(s)sequenceDiagram
participant CI as GitHub Actions
participant Gradle as Gradle Build
participant UnitTests as runLiveObjectUnitTests
participant IntegrationTests as runLiveObjectIntegrationTests
CI->>Gradle: Run check job
Gradle->>UnitTests: Execute LiveObject unit tests
CI->>Gradle: Run check-liveobjects job
Gradle->>IntegrationTests: Execute LiveObject integration tests
sequenceDiagram
participant Test as Test Class
participant Sandbox as Sandbox Utility
participant Ably as AblyRealtime
participant Channel as Channel
Test->>Sandbox: createInstance()
Sandbox->>Ably: createRealtimeClient()
Ably->>Channel: get channel
Test->>Ably: ensureConnected()
Ably->>Channel: ensureAttached()
Test->>Channel: Access objects property
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (6)
⏰ Context from checks skipped due to timeout of 90000ms (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 (
|
![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 (9)
live-objects/src/test/kotlin/io/ably/lib/objects/unit/TestHelpers.kt (1)
11-31: Add documentation and consider making the test key more explicit.The mock channel utility function is well-implemented but would benefit from KDoc documentation explaining its purpose and behavior. Additionally, consider making the dummy API key more obviously a test value.
+/** + * Creates a mocked Ably Realtime channel for unit testing. + * + * @param channelName The name of the channel to create + * @param clientId The client ID to use (defaults to "client1") + * @return A spied Channel instance with stubbed attach/detach/subscribe methods + */ internal fun getMockRealtimeChannel(channelName: String, clientId: String = "client1"): Channel { val client = AblyRealtime(ClientOptions().apply { autoConnect = false - key = "keyName:Value" + key = "test-key:test-value" this.clientId = clientId })live-objects/src/test/kotlin/io/ably/lib/objects/TestUtils.kt (1)
1-9: Consider adding file-level documentation for reflection utilities.This file contains powerful reflection-based utilities that bypass normal access controls. Consider adding KDoc at the file level to document the purpose and security implications of these utilities.
+/** + * Test utilities for accessing private members via reflection. + * + * These utilities should only be used in test code and provide access to: + * - Private fields (get/set) + * - Private methods (regular and suspend) + * - Polling utilities for async testing + * + * Note: These utilities bypass normal access controls and should be used carefully. + */ package io.ably.lib.objectslive-objects/src/test/kotlin/io/ably/lib/objects/unit/LiveObjectTest.kt (2)
9-9: Improve test method naming.The method name
testChannelObjectGetterTestis redundant and unclear. Consider a more descriptive name that follows testing conventions.- fun testChannelObjectGetterTest() = runTest { + fun `should return non-null objects when accessing channel objects property`() = runTest {Or using conventional naming:
- fun testChannelObjectGetterTest() = runTest { + fun testChannelObjectsPropertyIsNotNull() = runTest {
9-13: Consider enhancing test coverage.While this test verifies that
channel.objectsreturns a non-null value, it could be more comprehensive by testing the actual type or functionality of the returned objects.Consider adding assertions to verify the type or basic functionality:
@Test - fun testChannelObjectGetterTest() = runTest { + fun testChannelObjectsPropertyIsNotNull() = runTest { val channel = getMockRealtimeChannel("test-channel") val objects = channel.objects assertNotNull(objects) + // Additional assertions could verify the type or basic functionality + // assertTrue(objects is ExpectedType) }live-objects/src/main/kotlin/io/ably/lib/objects/ErrorCodes.kt (2)
3-6: Consider removing redundantpublicmodifier.The
publicmodifier on enum properties is redundant since it's the default visibility in Kotlin enums.-internal enum class ErrorCode(public val code: Int) { +internal enum class ErrorCode(val code: Int) { BadRequest(40_000), InternalError(50_000), }
8-11: Consider removing redundantpublicmodifier.Same as above - the
publicmodifier is redundant for enum properties.-internal enum class HttpStatusCode(public val code: Int) { +internal enum class HttpStatusCode(val code: Int) { BadRequest(400), InternalServerError(500), }live-objects/src/test/kotlin/io/ably/lib/objects/integration/LiveObjectTest.kt (2)
11-11: Improve test method naming for consistency.Similar to the unit test, the method name
testChannelObjectGetterTestis redundant and unclear. Consider using a more descriptive name that clearly indicates this is an integration test.- fun testChannelObjectGetterTest() = runTest { + fun `should return non-null objects when accessing real channel objects property`() = runTest {Or using conventional naming:
- fun testChannelObjectGetterTest() = runTest { + fun testRealChannelObjectsPropertyIsNotNull() = runTest {
11-16: Consider adding integration-specific test scenarios.While the basic null check is valuable, integration tests could verify more complex scenarios like channel state management, real connectivity, or live object synchronization behaviors.
Consider adding tests that verify integration-specific behaviors:
@Test fun `should handle channel attachment lifecycle with objects`() = runTest { val channelName = generateChannelName() val channel = getRealtimeChannel(channelName) // Verify channel is attached and objects are accessible assertTrue(channel.state == ChannelState.attached) assertNotNull(channel.objects) // Test object lifecycle or synchronization scenarios }live-objects/src/test/kotlin/io/ably/lib/objects/integration/setup/IntegrationTest.kt (1)
90-91: Remove empty tearDownAfterClass method.The empty
tearDownAfterClassmethod serves no purpose and can be removed to clean up the code.- @JvmStatic - @AfterClass - @Throws(Exception::class) - fun tearDownAfterClass() { - }🧰 Tools
🪛 detekt (1.23.8)
[warning] 90-91: This empty block of code can be removed.
(detekt.empty-blocks.EmptyFunctionBlock)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
.github/workflows/check.yml(1 hunks).github/workflows/integration-test.yml(1 hunks)gradle/libs.versions.toml(3 hunks)live-objects/build.gradle.kts(2 hunks)live-objects/src/main/kotlin/io/ably/lib/objects/ErrorCodes.kt(1 hunks)live-objects/src/main/kotlin/io/ably/lib/objects/Utils.kt(1 hunks)live-objects/src/test/kotlin/io/ably/lib/objects/TestUtils.kt(1 hunks)live-objects/src/test/kotlin/io/ably/lib/objects/integration/LiveObjectTest.kt(1 hunks)live-objects/src/test/kotlin/io/ably/lib/objects/integration/setup/IntegrationTest.kt(1 hunks)live-objects/src/test/kotlin/io/ably/lib/objects/integration/setup/Sandbox.kt(1 hunks)live-objects/src/test/kotlin/io/ably/lib/objects/unit/LiveObjectTest.kt(1 hunks)live-objects/src/test/kotlin/io/ably/lib/objects/unit/TestHelpers.kt(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
live-objects/src/test/kotlin/io/ably/lib/objects/unit/LiveObjectTest.kt (1)
live-objects/src/test/kotlin/io/ably/lib/objects/unit/TestHelpers.kt (1)
getMockRealtimeChannel(11-31)
live-objects/src/test/kotlin/io/ably/lib/objects/integration/LiveObjectTest.kt (1)
live-objects/src/test/kotlin/io/ably/lib/objects/integration/setup/IntegrationTest.kt (2)
generateChannelName(53-55)getRealtimeChannel(36-47)
live-objects/src/test/kotlin/io/ably/lib/objects/integration/setup/IntegrationTest.kt (1)
live-objects/src/test/kotlin/io/ably/lib/objects/integration/setup/Sandbox.kt (2)
ensureConnected(74-90)ensureAttached(92-107)
live-objects/src/main/kotlin/io/ably/lib/objects/Utils.kt (1)
lib/src/main/java/io/ably/lib/types/ErrorInfo.java (1)
ErrorInfo(17-197)
🪛 detekt (1.23.8)
live-objects/src/test/kotlin/io/ably/lib/objects/integration/setup/IntegrationTest.kt
[warning] 90-91: This empty block of code can be removed.
(detekt.empty-blocks.EmptyFunctionBlock)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: check-realtime-okhttp
- GitHub Check: check-realtime
🔇 Additional comments (22)
live-objects/src/test/kotlin/io/ably/lib/objects/TestUtils.kt (1)
10-19: The assertWaiter implementation looks solid.Good use of coroutines with proper timeout handling and context switching. The 100ms polling interval is reasonable for test scenarios.
.github/workflows/check.yml (1)
22-22: Good integration of LiveObject unit tests into CI pipeline.The addition of
runLiveObjectUnitTeststo the gradle command properly integrates the new tests into the existing CI workflow..github/workflows/integration-test.yml (1)
94-110: Well-structured integration test job.The new
check-liveobjectsjob follows the established patterns in the workflow and properly sets up the environment for running LiveObject integration tests. The configuration is consistent with other jobs in the file.live-objects/src/main/kotlin/io/ably/lib/objects/ErrorCodes.kt (1)
3-11: Well-structured error code definitions.The enum design provides a clean, type-safe way to handle error codes and HTTP status codes. The use of underscore separators in large numbers follows Kotlin conventions and improves readability.
live-objects/src/test/kotlin/io/ably/lib/objects/integration/LiveObjectTest.kt (1)
8-17: Good integration test structure.The test correctly extends
IntegrationTestand uses real channels viagetRealtimeChannel()instead of mocked ones. This provides valuable integration testing coverage complementing the unit tests.gradle/libs.versions.toml (3)
3-3: Good security update for JUnit.Updating JUnit from 4.12 to 4.13.2 addresses multiple security vulnerabilities and bug fixes. This is a recommended upgrade.
52-55: Well-structured dependency additions.The new MockK and Ktor client dependencies are appropriately defined with consistent naming and versioning patterns.
61-61: Comprehensive testing bundle.The new
kotlin-testsbundle effectively groups modern testing dependencies (MockK, coroutines-test, Turbine, Ktor clients) alongside traditional ones (JUnit, nanohttpd), providing a solid foundation for the LiveObjects testing infrastructure.live-objects/src/main/kotlin/io/ably/lib/objects/Utils.kt (4)
6-14: Well-structured exception utility function.The function correctly delegates to helper functions and provides sensible defaults. The parameter order and types align well with the ErrorInfo constructor.
16-19: Clean overload for ErrorInfo-based exception creation.This overload provides a clean interface when you already have an ErrorInfo object, maintaining consistency with the other overload.
27-31: Proper handling of optional cause parameter.The use of
letwith Elvis operator correctly handles both cases - with and without a cause throwable. This follows Kotlin idioms well.
33-35: Convenient error factory functions.These functions provide good defaults for common error scenarios, making the API more user-friendly.
live-objects/build.gradle.kts (3)
16-18: Good dependency organization.Replacing individual test dependencies with a bundle and adding
kotlin("test")provides better dependency management and consistency.
20-32: Comprehensive test configuration with appropriate JVM settings.The configuration covers essential aspects:
- Full exception logging for better debugging
- JVM module opens for reflection access (needed for mocking frameworks)
- Test lifecycle logging
- Forcing test execution to ensure fresh runs
- Smart exclusion of release builds to avoid duplication
34-45: Well-designed test task separation.The separation between unit and integration tests with appropriate filtering is excellent practice. The exclusion of the base
IntegrationTestclass from the integration test task prevents running the abstract base class.live-objects/src/test/kotlin/io/ably/lib/objects/integration/setup/IntegrationTest.kt (3)
36-47: Robust channel management with proper state handling.The function correctly:
- Uses
getOrPutfor efficient client caching- Ensures client connection before channel operations
- Properly attaches and waits for channel attachment
- Handles the protocol selection based on test parameters
53-55: Effective collision avoidance strategy.Using UUID for channel names is an excellent approach to prevent test interference and state collisions across different test runs.
57-67: Thorough resource cleanup.The cleanup logic properly:
- Removes event listeners from channels
- Releases channels from the client
- Closes realtime connections
- Clears the client map
This prevents resource leaks and test interference.
live-objects/src/test/kotlin/io/ably/lib/objects/integration/setup/Sandbox.kt (4)
25-38: Robust HTTP client configuration with appropriate retry logic.The retry configuration handles common network issues:
- Connection timeouts
- Request timeouts
- Socket timeouts
- Non-success HTTP responses
- Exponential backoff for intelligent retry spacing
57-58: Clarify API key index discrepancy.The comment mentions using "index 5" but the code uses
[0]. This inconsistency should be resolved - either the comment or the code needs correction.Please verify which index is correct for the API key selection. The comment suggests index 5 provides sufficient permissions, but the implementation uses index 0.
74-90: Excellent async state management.Both
ensureConnectedandensureAttachedfunctions properly:
- Check current state before waiting
- Use CompletableDeferred for async coordination
- Handle both success and failure scenarios
- Clean up event listeners appropriately
- Close connections on failure to prevent resource leaks
Also applies to: 92-107
42-46:Details
❓ Verification inconclusive
Verify relative file path dependency.
The relative path
"../lib/src/test/resources/ably-common/test-resources/test-app-setup.json"could be fragile if the working directory changes during test execution.
🏁 Script executed:
#!/bin/bash # Verify the test-app-setup.json file exists at the expected location fd -t f "test-app-setup.json" --exec ls -la {}Length of output: 48
🏁 Script executed:
#!/usr/bin/env bash # Search for test-app-setup.json anywhere in the repository find . -type f -name "test-app-setup.json" -printLength of output: 49
Verify existence and robustness of the relative test resource path
I wasn’t able to locate
test-app-setup.jsonunder../lib/src/test/resources/ably-common/test-resources/in this module’s codebase—not finding evidence doesn’t confirm it lives there in your monorepo. Please manually verify:
- That the file actually exists at that relative location when running tests.
- That your build/test runner’s working directory will always resolve this path correctly.
If this path spans sibling modules, consider loading the JSON as a classpath resource (e.g., via
ClassLoader.getResourceAsStream) or centralizing test fixtures to avoid brittle relative paths.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
live-objects/src/test/kotlin/io/ably/lib/objects/integration/setup/IntegrationTest.kt (2)
38-52: Fix formatting and consider error handling improvements.The method logic is sound, but there are a couple of improvements to consider:
- Formatting issue: There's an extra space before the dot on line 43.
- Error handling: Consider wrapping the channel operations in try-catch blocks for better error reporting.
Apply this diff to fix the formatting:
- }. apply { ensureConnected() } + }.apply { ensureConnected() }Consider adding error handling for better diagnostics:
return client.channels.get(channelName, channelOpts).apply { - attach() - ensureAttached() + try { + attach() + ensureAttached() + } catch (e: Exception) { + throw Exception("Failed to attach channel '$channelName' for client '$clientId'", e) + } }
95-96: Remove empty tearDownAfterClass method.The static analysis tool correctly identified that this empty method serves no purpose and should be removed.
Apply this diff to remove the empty method:
- @JvmStatic - @AfterClass - @Throws(Exception::class) - fun tearDownAfterClass() { - }🧰 Tools
🪛 detekt (1.23.8)
[warning] 95-96: This empty block of code can be removed.
(detekt.empty-blocks.EmptyFunctionBlock)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/src/main/java/io/ably/lib/objects/Adapter.java(1 hunks)lib/src/main/java/io/ably/lib/objects/LiveObjectsAdapter.java(1 hunks)live-objects/src/test/kotlin/io/ably/lib/objects/integration/setup/IntegrationTest.kt(1 hunks)live-objects/src/test/kotlin/io/ably/lib/objects/unit/TestHelpers.kt(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- lib/src/main/java/io/ably/lib/objects/Adapter.java
- lib/src/main/java/io/ably/lib/objects/LiveObjectsAdapter.java
🚧 Files skipped from review as they are similar to previous changes (1)
- live-objects/src/test/kotlin/io/ably/lib/objects/unit/TestHelpers.kt
🧰 Additional context used
🪛 detekt (1.23.8)
live-objects/src/test/kotlin/io/ably/lib/objects/integration/setup/IntegrationTest.kt
[warning] 95-96: This empty block of code can be removed.
(detekt.empty-blocks.EmptyFunctionBlock)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: check-realtime-okhttp
- GitHub Check: check-liveobjects
- GitHub Check: check-rest
- GitHub Check: check-realtime
- GitHub Check: check-rest-okhttp
- GitHub Check: check (19)
- GitHub Check: check (24)
- GitHub Check: check (21)
- GitHub Check: check (29)
- GitHub Check: check
- GitHub Check: build
🔇 Additional comments (4)
live-objects/src/test/kotlin/io/ably/lib/objects/integration/setup/IntegrationTest.kt (4)
1-16: LGTM! Clean package structure and appropriate imports.The package declaration and imports are well-organized and include all necessary dependencies for the integration testing framework.
17-26: Well-structured parameterized test setup.The use of JUnit's parameterized runner to test both msgpack and json protocols is excellent. The 10-second timeout is reasonable for integration tests, though you may want to consider if this is sufficient for network operations in slower environments.
58-60: LGTM! Effective collision avoidance strategy.Using UUID for channel name generation is a solid approach to prevent test interference.
62-72: Thorough cleanup implementation.The cleanup logic properly handles all resources - channels are detached, released, clients are closed, and the map is cleared. This prevents resource leaks between tests.
live-objects/src/test/kotlin/io/ably/lib/objects/integration/setup/IntegrationTest.kt
Show resolved
Hide resolved
live-objects/src/test/kotlin/io/ably/lib/objects/integration/setup/Sandbox.kt
Outdated
Show resolved
Hide resolved
sacOO7
force-pushed
the
chore/liveobject-tests
branch
from
26d1cdc to
2d7f510
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
live-objects/build.gradle.kts (2)
20-28: Global test configuration is solid
Usingtasks.withType<Test>().configureEachfor logging, module-opens, and lifecycle logging is a good way to ensure consistency across all Test tasks.Consider restricting
outputs.upToDateWhen { false }to only the integration test task so that unit tests can still benefit from Gradle’s up-to-date checks and improve overall build performance.
30-34: Add group and description to custom test tasks
RegisteringrunLiveObjectUnitTestswithout a group or description can make it harder for devs to discover.Example:
tasks.register<Test>("runLiveObjectUnitTests") { group = "verification" description = "Executes LiveObject unit tests only" filter { includeTestsMatching("io.ably.lib.objects.unit.*") } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
lib/src/main/java/io/ably/lib/objects/Adapter.java(1 hunks)lib/src/main/java/io/ably/lib/objects/LiveObjectsAdapter.java(1 hunks)live-objects/build.gradle.kts(2 hunks)live-objects/src/test/kotlin/io/ably/lib/objects/integration/setup/IntegrationTest.kt(1 hunks)live-objects/src/test/kotlin/io/ably/lib/objects/integration/setup/Sandbox.kt(1 hunks)live-objects/src/test/kotlin/io/ably/lib/objects/unit/TestHelpers.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- lib/src/main/java/io/ably/lib/objects/Adapter.java
- lib/src/main/java/io/ably/lib/objects/LiveObjectsAdapter.java
- live-objects/src/test/kotlin/io/ably/lib/objects/unit/TestHelpers.kt
- live-objects/src/test/kotlin/io/ably/lib/objects/integration/setup/IntegrationTest.kt
- live-objects/src/test/kotlin/io/ably/lib/objects/integration/setup/Sandbox.kt
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: check-realtime-okhttp
- GitHub Check: check-liveobjects
- GitHub Check: build
- GitHub Check: check (29)
- GitHub Check: check-rest-okhttp
- GitHub Check: check (24)
- GitHub Check: check (19)
- GitHub Check: check-rest
- GitHub Check: check (21)
- GitHub Check: check
- GitHub Check: check-realtime
🔇 Additional comments (1)
live-objects/build.gradle.kts (1)
16-18: Verify test dependency redundancy
You’ve added bothkotlin("test")andlibs.bundles.kotlin.tests. Ensure that the bundle inlibs.versions.tomldoesn’t already includekotlin("test"). If it does, you can safely remove the standalone declaration to avoid duplication.
1. Fixed liveobject channel options while initializing test channel 2. Fixed test utils as per review comments 3. Fixed adapter NotNull annotation for send method
sacOO7
force-pushed
the
chore/liveobject-tests
branch
from
2d7f510 to
1a7fafd
Compare
2 participants
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Tests
Chores