[ECO-5338] Liveobjects: Setup tests #1092
Conversation
1. Added test specific dependencies, updated junit, added mockk 2. Added test utils for mocking private classes, fields etc
WalkthroughThis change introduces a comprehensive test setup for LiveObjects, including new unit and integration tests, supporting test utilities, and Gradle task configuration. It also updates dependency versions and refactors how the Changes
Sequence Diagram(s)sequenceDiagram
participant CI as GitHub Actions
participant Gradle
participant Tests as Test Suite
CI->>Gradle: Run check (includes runLiveObjectUnitTests)
Gradle->>Tests: Execute unit tests (filtered by package)
Tests-->>Gradle: Report results
Gradle-->>CI: Return check status
CI->>Gradle: Run check-liveobjects (runLiveObjectIntegrationTests)
Gradle->>Tests: Execute integration tests (filtered by package)
Tests-->>Gradle: Report results
Gradle-->>CI: Return integration test status
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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 (
|
1. Updated separate gradlew task for both unit and integration tests 2. Updated check.yml and integration-test.yml file to run respective tests
sacOO7
force-pushed
the
chore/setup-liveobjects-tests
branch
from
c605937 to
0b44a36
Compare
sacOO7
force-pushed
the
chore/setup-liveobjects-tests
branch
from
f48c58b to
2e3a11e
Compare
sacOO7
force-pushed
the
chore/setup-liveobjects-tests
branch
from
2e3a11e to
911f57e
Compare
sacOO7
force-pushed
the
chore/setup-liveobjects-tests
branch
from
911f57e to
98bbb2a
Compare
sacOO7
force-pushed
the
chore/setup-liveobjects-tests
branch
from
98bbb2a to
08b8e76
Compare
1. Added ktor http dependencies and updated kotlin tests bundle 2. Added IntegrationTest parameterized class to run tests for both msgpack and json 3. Updated LiveObjectTest extending IntegrationTest class
sacOO7
force-pushed
the
chore/setup-liveobjects-tests
branch
from
08b8e76 to
af9deda
Compare
1. Updated LiveObjectTest to extend UnitTest class 2. Marked test utility methods as internal
sacOO7
force-pushed
the
chore/setup-liveobjects-tests
branch
from
25bce5e to
1adf275
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: 6
🧹 Nitpick comments (4)
live-objects/src/test/kotlin/io/ably/lib/objects/unit/setup/UnitTest.kt (1)
23-28: Improve API key configuration for security and maintainability.The hardcoded API key
"keyName:Value"should be externalized to avoid potential security issues and improve maintainability.Consider using a test-specific constant or configuration:
+ companion object { + private const val TEST_API_KEY = "test:key" + } + private val realtimeClients = mutableMapOf<String, AblyRealtime>() internal fun getMockRealtimeChannel(channelName: String, clientId: String = "client1"): Channel { val client = realtimeClients.getOrPut(clientId) { AblyRealtime(ClientOptions().apply { autoConnect = false - key = "keyName:Value" + key = TEST_API_KEY this.clientId = clientId }) }live-objects/src/test/kotlin/io/ably/lib/objects/TestUtils.kt (2)
10-19: Improve assertWaiter flexibility and error handling.The
assertWaiterfunction is useful for async testing, but has some limitations:Consider these improvements:
-suspend fun assertWaiter(timeoutInMs: Long = 10_000, block: suspend () -> Boolean) { +suspend fun assertWaiter( + timeoutInMs: Long = 10_000, + intervalMs: Long = 100, + message: String = "Assertion failed within timeout", + block: suspend () -> Boolean +) { withContext(Dispatchers.Default) { - withTimeout(timeoutInMs) { + try { + withTimeout(timeoutInMs) { do { val success = block() - delay(100) + if (!success) delay(intervalMs) } while (!success) } + } catch (e: TimeoutCancellationException) { + throw AssertionError(message, e) + } } }This adds configurable intervals, better error messages, and proper timeout handling.
34-46: Improve field traversal error handling.The field traversal logic is good but the error handling could be more informative:
private fun Class<*>.findField(name: String): Field { var result = kotlin.runCatching { getDeclaredField(name) } var currentClass = this while (result.isFailure && currentClass.superclass != null) { currentClass = currentClass.superclass!! result = kotlin.runCatching { currentClass.getDeclaredField(name) } } - if (result.isFailure) { - throw result.exceptionOrNull() as Exception - } - return result.getOrNull() as Field + return result.getOrElse { + throw NoSuchFieldException("Field '$name' not found in class hierarchy of ${this.simpleName}") + } }live-objects/src/test/kotlin/io/ably/lib/objects/integration/setup/IntegrationTest.kt (1)
87-92: Remove the empty tearDownAfterClass method.The empty
tearDownAfterClass()method serves no purpose and should 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 (16)
.github/workflows/check.yml(1 hunks).github/workflows/integration-test.yml(1 hunks)gradle/libs.versions.toml(3 hunks)lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java(3 hunks)lib/src/main/java/io/ably/lib/realtime/Connection.java(1 hunks)lib/src/main/java/io/ably/lib/transport/ConnectionManager.java(2 hunks)lib/src/test/java/io/ably/lib/test/realtime/ConnectionManagerTest.java(1 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/setup/UnitTest.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/setup/UnitTest.kt (1)
getMockRealtimeChannel(21-43)
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 (9)
- GitHub Check: check-realtime-okhttp
- GitHub Check: check-liveobjects
- GitHub Check: check-rest-okhttp
- GitHub Check: check-rest
- GitHub Check: check-realtime
- GitHub Check: check (19)
- GitHub Check: check (24)
- GitHub Check: check (29)
- GitHub Check: check (21)
🔇 Additional comments (33)
live-objects/src/main/kotlin/io/ably/lib/objects/ErrorCodes.kt (1)
1-12: LGTM! Clean error code definitions.The enum implementations follow Kotlin best practices with appropriate internal visibility, clear naming, and readable numeric formatting using underscores for large numbers. The error code values align well with standard HTTP and application-level error conventions.
.github/workflows/check.yml (1)
22-22: Good integration of live object unit tests into CI.The addition of
runLiveObjectUnitTeststo the Gradle command properly integrates the new unit tests into the main check workflow, ensuring they run on every PR and push to main.lib/src/test/java/io/ably/lib/test/realtime/ConnectionManagerTest.java (1)
140-140: Correct constructor update for refactored ConnectionManager.This change appropriately removes the LiveObjectsPlugin parameter from the ConnectionManager constructor call, aligning with the broader refactoring mentioned in the PR summary where LiveObjectsPlugin access was moved to be available via AblyRealtime instead of being passed to individual constructors.
.github/workflows/integration-test.yml (1)
94-110: Excellent addition of live object integration tests to CI.The new
check-liveobjectsjob follows the established workflow pattern perfectly, using consistent JDK version (17), distribution (temurin), and setup steps. This properly separates integration tests from unit tests and ensures comprehensive CI coverage for the live objects module.live-objects/src/test/kotlin/io/ably/lib/objects/integration/LiveObjectTest.kt (1)
8-17: Well-structured integration test for live objects setup.This integration test follows good practices:
- Properly extends the
IntegrationTestbase class- Uses
runTestfor coroutine-based testing- Leverages helper methods (
generateChannelName(),getRealtimeChannel()) from the base class- Has a focused scope - verifying the
objectsproperty is accessible on channels- Uses appropriate assertions with
assertNotNullThe test is simple but effective for validating the basic live objects integration in a real environment.
lib/src/main/java/io/ably/lib/realtime/Connection.java (1)
125-128: LGTM! Clean refactoring of plugin dependency management.The removal of
LiveObjectsPluginfrom the constructor parameters simplifies theConnectionclass and aligns with the broader refactoring to centralize plugin access viaAblyRealtime. This reduces coupling betweenConnectionand the plugin system.lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java (1)
76-86:✅ Verification successful
Verify initialization timing doesn't create dependency issues.
The initialization order has changed so that
connectionis created beforeliveObjectsPluginis initialized. SinceConnectionManagernow accesses the plugin viaably.liveObjectsPlugin, ensure this doesn't create timing issues where the plugin is accessed before initialization.Verify that all plugin access in
ConnectionManageroccurs after plugin initialization:
🏁 Script executed:
#!/bin/bash # Search for liveObjectsPlugin usage in ConnectionManager to verify access patterns rg -A 3 -B 3 "liveObjectsPlugin" lib/src/main/java/io/ably/lib/transport/ConnectionManager.javaLength of output: 508
Initialization order is safe; plugin access is null-guarded
The
ConnectionManageronly invokesably.liveObjectsPlugin.handle(...)ifably.liveObjectsPlugin != null, so creating the connection before initializing the plugin cannot cause NPEs or race-condition errors. No changes required.lib/src/main/java/io/ably/lib/transport/ConnectionManager.java (2)
768-768: LGTM! Constructor simplification aligns with architectural changes.Removing the
LiveObjectsPluginparameter from the constructor is consistent with the refactoring to centralize plugin access via theAblyRealtimeinstance. This reduces the coupling betweenConnectionManagerand plugin initialization.
1226-1228: Good defensive programming with null safety checks.The change from using a local
liveObjectsPluginfield to accessingably.liveObjectsPluginis implemented safely with proper null checking. This ensures the code handles cases where the LiveObjects plugin is not available gracefully.The existing error handling (catching
Throwableand logging) provides additional robustness against plugin implementation issues.live-objects/build.gradle.kts (4)
1-1: Good addition for detailed test logging.The import for
TestExceptionFormatenables comprehensive test failure reporting, which will be valuable for debugging test issues.
20-32: Excellent comprehensive test configuration.The test configuration includes several best practices:
- Full exception format for detailed error reporting
- JVM args to open Java modules for reflection (essential for modern Java versions)
- Lifecycle logging for test visibility
- Forcing tests to run (preventing false cache hits)
- Disabling tests for release builds to avoid duplication
This setup will facilitate effective testing and debugging.
34-46:✅ Verification successful
Well-structured test task separation.
The separation of unit and integration tests is excellent for CI/CD pipelines. The package-based filtering and exclusion of base test classes is appropriate.
However, verify that the excluded base classes (
UnitTest.classandIntegrationTest.class) are indeed intended to be abstract base classes and not runnable tests themselves.
🏁 Script executed:
#!/bin/bash # Description: Verify base test classes are abstract or contain no test methods # Expected: These should be base classes without @Test methods # Check UnitTest class structure ast-grep --pattern 'class UnitTest { $$$ }' # Check IntegrationTest class structure ast-grep --pattern 'class IntegrationTest { $$$ }' # Look for any @Test annotations in these base classes rg -A 2 -B 2 "@Test" live-objects/src/test/kotlin/io/ably/lib/objects/unit/setup/UnitTest.kt live-objects/src/test/kotlin/io/ably/lib/objects/integration/setup/IntegrationTest.ktLength of output: 281
🏁 Script executed:
#!/usr/bin/env bash set -e echo "== Locating UnitTest.kt ==" fd UnitTest.kt echo "== Inspecting UnitTest.kt (first 50 lines) ==" fd UnitTest.kt -x sed -n '1,50p' {} echo "== Locating IntegrationTest.kt ==" fd IntegrationTest.kt echo "== Inspecting IntegrationTest.kt (first 50 lines) ==" fd IntegrationTest.kt -x sed -n '1,50p' {} echo "== Searching for @Test annotations in both files ==" rg -n "@Test" $(fd UnitTest.kt) $(fd IntegrationTest.kt) || echo "No @Test annotations found"Length of output: 4349
Tests Task Configuration Approved
Confirmed thatUnitTest.ktandIntegrationTest.ktcontain no@Testmethods or runnable test cases, so excludingUnitTest.classandIntegrationTest.classis safe.
16-18:✅ Verification successful
Verify the kotlin-tests bundle contents.
The addition of the
kotlin-testsbundle is good for consistent dependency management. Please ensure this bundle includes all necessary testing dependencies.
🏁 Script executed:
#!/bin/bash # Description: Verify the kotlin-tests bundle contains expected dependencies # Expected: Bundle should include junit, mockk, coroutine-test, and other testing libs # Check what's included in the kotlin-tests bundle rg -A 10 "kotlin-tests.*=" gradle/libs.versions.tomlLength of output: 773
Bundle verification successful
The
kotlin-testsbundle ingradle/libs.versions.tomlincludes:
- junit
- mockk
- coroutine-test
- nanohttpd
- turbine
- ktor-client-cio
- ktor-client-core
This covers all required testing libraries—no further changes needed.
live-objects/src/test/kotlin/io/ably/lib/objects/unit/setup/UnitTest.kt (2)
30-43: Well-structured mock setup with comprehensive behavior coverage.The spy-based mocking approach is excellent, providing real object behavior while allowing specific method overrides. The mock covers all essential channel operations (attach, detach, subscribe variants) and properly manages state transitions.
The forced state setting to
attachedat the end ensures consistent test conditions.
45-48: Proper test isolation with cleanup.The
@Aftermethod ensures proper test isolation by clearing client instances between tests, preventing state leakage.gradle/libs.versions.toml (3)
52-55: Good addition of testing dependencies.The new MockK and Ktor client dependencies support the enhanced testing infrastructure. The library definitions follow the proper format and reference the correct version variables.
61-61: Well-structured kotlin-tests bundle.The new
kotlin-testsbundle provides a good collection of testing dependencies including JUnit, MockK, coroutine testing, and HTTP client libraries. This promotes consistent dependency management across test modules.
3-3:✅ Verification successful
Verify JUnit version for latest security updates.
The upgrade from JUnit 4.12 to 4.13.2 is good, but please verify this is the latest stable version to ensure any security fixes are included.
🌐 Web query:
What is the latest stable version of JUnit 4.x and are there any known security vulnerabilities in version 4.13.2?💡 Result:
The latest stable version of JUnit 4.x is 4.13.2. (junit.org)
Regarding security vulnerabilities, JUnit 4.13.2 addresses a known issue in the
TemporaryFolderrule. Prior to version 4.13.1, this rule could inadvertently expose sensitive information when writing to temporary directories on Unix-like systems, as these directories are typically shared among users. This vulnerability was identified as CVE-2020-15250. (acunetix.com)In JUnit 4.13.1, the
TemporaryFolderrule was updated to limit access to temporary folders, mitigating this information disclosure risk. Therefore, JUnit 4.13.2, being a subsequent release, includes this fix and does not have known security vulnerabilities.Citations:
- 1: https://junit.org/junit4/dependency-info.html?utm_source=openai
- 2: https://www.acunetix.com/vulnerabilities/sca/cve-2020-15250-vulnerability-in-maven-package-junit-junit/?utm_source=openai
Verified JUnit version is current and secure
JUnit 4.13.2 is the latest stable 4.x release and includes the fix for CVE-2020-15250; there are no known security vulnerabilities in this version.
live-objects/src/main/kotlin/io/ably/lib/objects/Utils.kt (5)
6-14: LGTM! Well-designed utility function with proper parameter defaults.The function signature and implementation look correct. The default
HttpStatusCode.BadRequestfor client errors is appropriate, and the delegation to helper functions maintains good separation of concerns.
16-19: LGTM! Clean overload for ErrorInfo-based exception creation.This overload provides a nice convenience method when you already have an
ErrorInfoobject, maintaining consistency with the primary function.
21-25: LGTM! Correct ErrorInfo constructor usage.The parameter order matches the
ErrorInfo(message, statusCode, code)constructor signature from the Java class as shown in the relevant code snippets.
27-32: LGTM! Proper null-safe exception creation.The Elvis operator correctly handles both cases - with and without a cause. The use of
AblyException.fromErrorInfovariants is appropriate.
33-35: LGTM! Convenient error creation functions with appropriate mappings.The
clientErrorandserverErrorfunctions provide good defaults:
clientError: BadRequest (400) error code and statusserverError: InternalError (500) error code and statusThese mappings are semantically correct for their intended use cases.
live-objects/src/test/kotlin/io/ably/lib/objects/integration/setup/IntegrationTest.kt (4)
15-22: LGTM! Proper parameterized test setup with appropriate timeout.The parameterized test configuration and 10-second timeout rule are well-configured for integration testing scenarios.
36-47: LGTM! Well-designed client and channel management with proper async handling.The function correctly:
- Uses
getOrPutfor lazy client creation- Applies protocol selection based on test parameters
- Ensures client connection and channel attachment using suspend functions
- Properly chains the operations
The use of
ensureConnected()andensureAttached()from the sandbox utilities ensures proper async state management.
53-55: LGTM! Good practice for test isolation.Using UUID-based channel names prevents test interference and state collisions, which is essential for reliable integration testing.
57-67: LGTM! Comprehensive resource cleanup.The cleanup logic properly:
- Removes all event listeners with
channel.off()- Releases channels from the client
- Closes all realtime clients
- Clears the client map
This ensures no resource leaks between test runs.
live-objects/src/test/kotlin/io/ably/lib/objects/integration/setup/Sandbox.kt (6)
25-38: LGTM! Well-configured HTTP client with comprehensive retry logic.The retry configuration is robust:
- Appropriate max retries (5)
- Retries on HTTP failures and common timeout exceptions
- Exponential backoff strategy
This should handle transient network issues effectively during integration testing.
42-46: LGTM! Proper JSON configuration loading.The file path correctly references the shared test resources, and the JSON parsing extracts the right configuration section for app creation.
65-72: LGTM! Clean client creation with proper configuration.The extension function properly applies the provided options while ensuring the sandbox API key and environment are set correctly.
74-90: LGTM! Robust connection state management.The suspend function correctly:
- Returns early if already connected
- Waits for connection events using CompletableDeferred
- Handles both success (connected) and failure cases
- Cleans up event listeners properly
- Closes the client on connection failure
The error handling using
ablyException(it.reason)integrates well with the utility functions from Utils.kt.
92-107: LGTM! Consistent channel attachment state management.The function mirrors the connection logic appropriately:
- Returns early if already attached
- Waits for attachment events
- Handles success and failure cases
- Proper event listener cleanup
The implementation is consistent with the connection management function above.
48-62:❓ Verification inconclusive
Fix API key selection discrepancy.
There's a mismatch between the comment and implementation:
- Comment mentions "index 5" with specific permissions for Chat and Channels
- Code uses
index 0Please verify which index is correct for the required permissions.
🏁 Script executed:
#!/bin/bash # Check the test-app-setup.json file to understand the keys structure cat ../lib/src/test/resources/ably-common/test-resources/test-app-setup.json | jq '.post_apps.keys[] | {keyStr: .keyStr, capability: .capability}' 2>/dev/null || echo "jq not available or file structure different"Length of output: 253
#!/bin/bash
set -eecho "1. Locate loadAppCreationJson definition in Kotlin tests"
rg -n "loadAppCreationJson" -g "*.kt" || echo "No loadAppCreationJson found"echo
echo "2. List all JSON files under test resources"
find . -type f -path "/src/test/resources/.json" -printecho
echo "3. Search JSON files for 'post_apps' structure"
grep -RIl '"post_apps"' -n . | while read -r file; do
echo "---- Inspecting $file ----"
if command -v jq >/dev/null 2>&1; then
jq '.post_apps.keys | to_entries[] | {index: .key, keyStr: .value.keyStr, capability: .value.capability}' "$file"
else
echo "jq not available; raw content:"
sed -n '1,50p' "$file"
fi
done
Verify API key index against test-app JSON structure
There’s still a mismatch between the inline comment and the code:
- Comment says “index 5” is needed for Chat/Channels permissions
- Code uses
keys[0]Please confirm which key index in your app-creation JSON actually grants the required permissions and update the code or comment accordingly.
• File:
live-objects/src/test/kotlin/io/ably/lib/objects/integration/setup/Sandbox.kt
– Lines 48–62: adjust thebody.asJsonObject["keys"].asJsonArray[…]index
live-objects/src/test/kotlin/io/ably/lib/objects/unit/setup/UnitTest.kt
Outdated
Show resolved
Hide resolved
sacOO7
force-pushed
the
chore/setup-liveobjects-tests
branch
from
37eedc2 to
5d3316f
Compare
| import org.junit.Test | ||
| import kotlin.test.assertNotNull | ||
|
|
||
| class LiveObjectTest : UnitTest() { |
There was a problem hiding this comment.
Don't like inheritance in the tests, could you take a look at this SO answer I think we can setup common parts the same way
Fixed #1089
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Refactor