[ECO-5014] feat: basic sandbox setup #47
Conversation
WalkthroughThe changes in this pull request include the addition of a new dependency for testing in the Changes
Assessment against linked issues
Possibly related issues
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: 6
🧹 Outside diff range and nitpick comments (4)
chat-android/src/test/java/com/ably/chat/SandboxTest.kt (2)
29-41: Improve helper functions with constants and validation.Consider the following improvements:
- Move hardcoded values to constants
- Add API key validation
- Generate unique client IDs to prevent conflicts in parallel tests
Consider this implementation:
+private companion object { + const val SANDBOX_ENV = "sandbox" + const val API_KEY_PATTERN = "^[^:]+:[^:]+\$" +} private fun createSandboxChatClient(apiKey: String): DefaultChatClient { + require(apiKey.matches(Regex(API_KEY_PATTERN))) { "Invalid API key format" } val realtime = createSandboxRealtime(apiKey) return DefaultChatClient(realtime, ClientOptions()) } -private fun createSandboxRealtime(chatApiKey: String, chatClientId: String = "sandbox-client"): AblyRealtime = +private fun createSandboxRealtime( + chatApiKey: String, + chatClientId: String = "sandbox-client-${System.currentTimeMillis()}" +): AblyRealtime = AblyRealtime( PubSubClientOptions().apply { key = chatApiKey - environment = "sandbox" + environment = SANDBOX_ENV clientId = chatClientId }, )
11-11: Add KDoc documentation for the test class.Since this is a sandbox test setup that others might use as a reference, consider adding documentation explaining:
- The purpose of these tests
- Prerequisites for running them
- Expected behavior
- Any limitations or considerations
Add this documentation:
+/** + * Integration tests using Ably's sandbox environment. + * + * Prerequisites: + * - Network access to Ably's sandbox environment + * - Valid API key creation permissions + * + * Note: These tests interact with actual sandbox services and may take longer to execute + * than unit tests. They should not be mixed with fast-running unit tests. + */ class SandboxTest {gradle/libs.versions.toml (2)
54-55: Consider adding additional Ktor modules for testing.For sandbox testing, you might need additional Ktor modules:
ktor-client-content-negotiationfor JSON handlingktor-serialization-gsonfor integration with your existing Gson dependencyktor-client-core = { module = "io.ktor:ktor-client-core", version.ref = "ktor" } ktor-client-cio = { module = "io.ktor:ktor-client-cio", version.ref = "ktor" } +ktor-client-content-negotiation = { module = "io.ktor:ktor-client-content-negotiation", version.ref = "ktor" } +ktor-serialization-gson = { module = "io.ktor:ktor-client-gson", version.ref = "ktor" }
57-58: Update bundle if additional Ktor modules are added.If you add the suggested Ktor modules, update the bundle accordingly:
-ktor-client = ["ktor-client-core", "ktor-client-cio"] +ktor-client = ["ktor-client-core", "ktor-client-cio", "ktor-client-content-negotiation", "ktor-serialization-gson"]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
chat-android/build.gradle.kts(1 hunks)chat-android/src/test/java/com/ably/chat/SandboxTest.kt(1 hunks)chat-android/src/test/java/com/ably/chat/SandboxUtils.kt(1 hunks)gradle/libs.versions.toml(2 hunks)
🔇 Additional comments (4)
chat-android/src/test/java/com/ably/chat/SandboxTest.kt (1)
1-10: LGTM: Imports are well-organized and necessary.
The imports cover all required functionality for sandbox testing, including Ably's realtime capabilities, coroutine testing, and JUnit assertions.
chat-android/build.gradle.kts (1)
52-52: LGTM! The Ktor client dependency is appropriately scoped.
The addition of the Ktor client bundle for testing purposes aligns well with the PR objectives of setting up a sandbox for integration testing.
Let's verify the Ktor client bundle definition in the version catalog:
✅ Verification successful
Ktor client bundle is properly defined in the version catalog
The verification confirms that:
- The Ktor version is defined:
ktor = "3.0.1" - The required Ktor client libraries are properly declared:
ktor-client-corektor-client-cio
- The bundle
ktor-clientcorrectly includes both dependencies
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the Ktor client bundle definition exists in the version catalog
# Expected: Should find the bundle definition and its version
# Test: Search for Ktor-related configurations
rg -A 5 'ktor' gradle/libs.versions.toml
Length of output: 770
chat-android/src/test/java/com/ably/chat/SandboxUtils.kt (1)
1-32: Address security concerns in sandbox setup.
Several security considerations need to be addressed:
- Implement rate limiting to prevent API abuse
- Ensure API keys are not logged
- Consider implementing secure storage for sensitive data
- Add request timeouts to prevent hanging tests
Let's verify the current usage of this utility:
gradle/libs.versions.toml (1)
23-23: Consider grouping related version declarations.
The Ktor version declaration could be grouped with other networking-related dependencies for better organization.
Let's verify Ktor's compatibility with the project's Kotlin version:
| @Test | ||
| fun basicIntegrationTest() = runTest { | ||
| val chatClient = createSandboxChatClient(sandboxApiKey) | ||
| val room = chatClient.rooms.get("basketball") | ||
| room.attach() | ||
| assertEquals(ChannelState.attached, room.messages.channel.state) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage with additional scenarios and proper cleanup.
The current test only covers the basic "happy path". Consider:
- Adding timeout handling for attach operation
- Testing error scenarios
- Adding proper resource cleanup
Consider this enhanced implementation:
@Test
fun basicIntegrationTest() = runTest {
val chatClient = createSandboxChatClient(sandboxApiKey)
- val room = chatClient.rooms.get("basketball")
- room.attach()
- assertEquals(ChannelState.attached, room.messages.channel.state)
+ try {
+ val room = chatClient.rooms.get("basketball")
+ withTimeout(5000) {
+ room.attach()
+ }
+ assertEquals(ChannelState.attached, room.messages.channel.state)
+ } finally {
+ chatClient.close()
+ }
}
+@Test
+fun shouldHandleAttachFailure() = runTest {
+ val chatClient = createSandboxChatClient("invalid.key")
+ try {
+ val room = chatClient.rooms.get("basketball")
+ withTimeout(5000) {
+ room.attach()
+ }
+ fail("Expected attachment to fail")
+ } catch (e: Exception) {
+ assertTrue(e.message?.contains("invalid key") ?: false)
+ } finally {
+ chatClient.close()
+ }
}Committable suggestion skipped: line range outside the PR's diff.
| import io.ktor.http.ContentType | ||
| import io.ktor.http.contentType | ||
|
|
||
| val client = HttpClient(CIO) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving HttpClient configuration and lifecycle management.
The global HttpClient instance could cause issues in parallel tests and lacks important configurations:
- Consider making it private and/or scoped to test instances
- Add timeout configurations
- Implement proper resource cleanup
-val client = HttpClient(CIO)
+private val client = HttpClient(CIO) {
+ engine {
+ requestTimeout = 5000
+ }
+ install(HttpTimeout) {
+ requestTimeoutMillis = 5000
+ connectTimeoutMillis = 5000
+ }
+}📝 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.
| val client = HttpClient(CIO) | |
| private val client = HttpClient(CIO) { | |
| engine { | |
| requestTimeout = 5000 | |
| } | |
| install(HttpTimeout) { | |
| requestTimeoutMillis = 5000 | |
| connectTimeoutMillis = 5000 | |
| } | |
| } |
|
|
||
| val client = HttpClient(CIO) | ||
|
|
||
| suspend fun loadAppCreationRequestBody(): JsonElement = |
There was a problem hiding this comment.
Can we create a sandbox wrapper class instead to hide internal complexity, we expose only two util methods that deal with sandbox app.
suspend fun setup() : Sandbox {
// creates a test sandbox app
// key is set as a property of sandbox
}
suspend fun tearDown(app: Sandbox) {
// deletes the given sandbox app
}
}
We need to make sure we safely close sandbox app resource after usage in the test case
There was a problem hiding this comment.
Do we really need to delete an app? I tried to find reference to this in the docs or in other SDKs and can't find
There was a problem hiding this comment.
We occasionally prune the temp apps on sandbox anyway (they're all _tmp*) - its probably beneficial to keep the same app available for a while anyway, incase you want to debug against it / check logs over time.
ttypic
force-pushed
the
ECO-5014/simple-sandbox-setup
branch
2 times, most recently
from
f37a148
to
0b019ad
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (3)
chat-android/src/test/java/com/ably/chat/SandboxTest.kt (1)
35-47: Enhance helper functions with validation and configuration optionsThe helper functions could be improved in several ways:
- Add API key format validation
- Make environment configurable rather than hard-coding "sandbox"
- Allow configuration of other important client options (e.g., logLevel for debugging)
Consider this enhanced implementation:
-private fun createSandboxChatClient(apiKey: String): DefaultChatClient { +private fun createSandboxChatClient( + apiKey: String, + options: ClientOptions = ClientOptions(), +): DefaultChatClient { + require(apiKey.contains(":")) { "Invalid API key format" } val realtime = createSandboxRealtime(apiKey) - return DefaultChatClient(realtime, ClientOptions()) + return DefaultChatClient(realtime, options) } -private fun createSandboxRealtime(chatApiKey: String, chatClientId: String = "sandbox-client"): AblyRealtime = +private fun createSandboxRealtime( + chatApiKey: String, + chatClientId: String = "sandbox-client", + environment: String = "sandbox", + logLevel: Int? = null, +): AblyRealtime = AblyRealtime( PubSubClientOptions().apply { key = chatApiKey - environment = "sandbox" + environment = environment clientId = chatClientId + logLevel?.let { this.logLevel = it } }, )chat-android/src/test/java/com/ably/chat/Sandbox.kt (2)
18-23: Consider making URLs configurableThe sandbox URLs should be configurable to support different environments or testing scenarios.
Consider adding configuration parameters:
class Sandbox { + companion object { + private const val DEFAULT_SANDBOX_URL = "https://sandbox-rest.ably.io" + } + + private val sandboxUrl: String + + constructor(sandboxUrl: String = DEFAULT_SANDBOX_URL) { + this.sandboxUrl = sandboxUrl + } private lateinit var appId: String lateinit var apiKey: String private set
1-45: Consider improving test isolationThe current sandbox implementation might affect other tests running in parallel. Consider implementing proper test isolation:
- Use separate sandbox instances for different test suites
- Implement proper cleanup in case of test failures
- Consider using test containers or similar technology for true isolation
Would you like assistance in implementing a more isolated testing approach?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
chat-android/build.gradle.kts(1 hunks)chat-android/src/test/java/com/ably/chat/Sandbox.kt(1 hunks)chat-android/src/test/java/com/ably/chat/SandboxTest.kt(1 hunks)gradle/libs.versions.toml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- chat-android/build.gradle.kts
- gradle/libs.versions.toml
🔇 Additional comments (1)
chat-android/src/test/java/com/ably/chat/SandboxTest.kt (1)
26-32: 🛠️ Refactor suggestion
Enhance test robustness and resource management
While I agree with the previous review comment about adding timeout handling and error scenarios, there are additional concerns:
- The chat client should be properly closed to avoid resource leaks
- The test assumes the "basketball" room exists and is accessible
- The test doesn't verify if messages can actually be sent/received
Let's verify if there are any similar tests in the codebase that might provide patterns for proper resource management:
#!/bin/bash
# Search for similar test patterns in the codebase
rg -l "AblyRealtime.*close\(\)" --type kotlin
rg "fun.*Test.*runTest" --type kotlin -A 5| suspend fun tearDown() { | ||
| client.delete("https://sandbox-rest.ably.io/apps/$appId") | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling to tearDown method
The tearDown method should handle potential failures gracefully.
Consider this implementation:
suspend fun tearDown() {
- client.delete("https://sandbox-rest.ably.io/apps/$appId")
+ try {
+ val response = client.delete("$sandboxUrl/apps/$appId")
+ if (!response.status.isSuccess()) {
+ throw SandboxTeardownException("Failed to delete sandbox app: ${response.status}")
+ }
+ } catch (e: Exception) {
+ throw SandboxTeardownException("Failed to delete sandbox app", e)
+ }
}📝 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.
| suspend fun tearDown() { | |
| client.delete("https://sandbox-rest.ably.io/apps/$appId") | |
| } | |
| suspend fun tearDown() { | |
| try { | |
| val response = client.delete("$sandboxUrl/apps/$appId") | |
| if (!response.status.isSuccess()) { | |
| throw SandboxTeardownException("Failed to delete sandbox app: ${response.status}") | |
| } | |
| } catch (e: Exception) { | |
| throw SandboxTeardownException("Failed to delete sandbox app", e) | |
| } | |
| } |
| suspend fun setUp() { | ||
| val response: HttpResponse = client.post("https://sandbox-rest.ably.io/apps") { | ||
| contentType(ContentType.Application.Json) | ||
| setBody(loadAppCreationRequestBody().toString()) | ||
| } | ||
| val body = JsonParser.parseString(response.bodyAsText()) | ||
| // From JS chat repo at 7985ab7 — "The key we need to use is the one at index 5, which gives enough permissions to interact with Chat and Channels" | ||
| apiKey = body.asJsonObject["keys"].asJsonArray[5].asJsonObject["keyStr"].asString | ||
| appId = body.asJsonObject["appId"].asString | ||
| } |
There was a problem hiding this comment.
Add error handling and response validation
The setUp method lacks proper error handling and response validation:
- No handling of HTTP errors
- No validation of response structure
- Magic number (index 5) used for key selection
Consider this implementation:
suspend fun setUp() {
- val response: HttpResponse = client.post("https://sandbox-rest.ably.io/apps") {
+ val response: HttpResponse = try {
+ client.post("$sandboxUrl/apps") {
+ contentType(ContentType.Application.Json)
+ setBody(loadAppCreationRequestBody().toString())
+ }
+ } catch (e: Exception) {
+ throw SandboxSetupException("Failed to create sandbox app", e)
+ }
+
+ if (!response.status.isSuccess()) {
+ throw SandboxSetupException("Failed to create sandbox app: ${response.status}")
+ }
+
val body = JsonParser.parseString(response.bodyAsText())
- apiKey = body.asJsonObject["keys"].asJsonArray[5].asJsonObject["keyStr"].asString
- appId = body.asJsonObject["appId"].asString
+ try {
+ val keys = body.asJsonObject["keys"]?.asJsonArray
+ ?: throw SandboxSetupException("Missing 'keys' in response")
+
+ if (keys.size() <= REQUIRED_KEY_INDEX) {
+ throw SandboxSetupException("Insufficient keys in response")
+ }
+
+ apiKey = keys[REQUIRED_KEY_INDEX].asJsonObject["keyStr"]?.asString
+ ?: throw SandboxSetupException("Invalid key structure")
+
+ appId = body.asJsonObject["appId"]?.asString
+ ?: throw SandboxSetupException("Missing 'appId' in response")
+ } catch (e: Exception) {
+ throw SandboxSetupException("Failed to parse sandbox response", e)
+ }
}
+private companion object {
+ // The key at index 5 gives enough permissions to interact with Chat and Channels
+ private const val REQUIRED_KEY_INDEX = 5
+}
+class SandboxSetupException : RuntimeException {
+ constructor(message: String) : super(message)
+ constructor(message: String, cause: Throwable) : super(message, cause)
+}📝 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.
| suspend fun setUp() { | |
| val response: HttpResponse = client.post("https://sandbox-rest.ably.io/apps") { | |
| contentType(ContentType.Application.Json) | |
| setBody(loadAppCreationRequestBody().toString()) | |
| } | |
| val body = JsonParser.parseString(response.bodyAsText()) | |
| // From JS chat repo at 7985ab7 — "The key we need to use is the one at index 5, which gives enough permissions to interact with Chat and Channels" | |
| apiKey = body.asJsonObject["keys"].asJsonArray[5].asJsonObject["keyStr"].asString | |
| appId = body.asJsonObject["appId"].asString | |
| } | |
| suspend fun setUp() { | |
| val response: HttpResponse = try { | |
| client.post("$sandboxUrl/apps") { | |
| contentType(ContentType.Application.Json) | |
| setBody(loadAppCreationRequestBody().toString()) | |
| } | |
| } catch (e: Exception) { | |
| throw SandboxSetupException("Failed to create sandbox app", e) | |
| } | |
| if (!response.status.isSuccess()) { | |
| throw SandboxSetupException("Failed to create sandbox app: ${response.status}") | |
| } | |
| val body = JsonParser.parseString(response.bodyAsText()) | |
| try { | |
| val keys = body.asJsonObject["keys"]?.asJsonArray | |
| ?: throw SandboxSetupException("Missing 'keys' in response") | |
| if (keys.size() <= REQUIRED_KEY_INDEX) { | |
| throw SandboxSetupException("Insufficient keys in response") | |
| } | |
| apiKey = keys[REQUIRED_KEY_INDEX].asJsonObject["keyStr"]?.asString | |
| ?: throw SandboxSetupException("Invalid key structure") | |
| appId = body.asJsonObject["appId"]?.asString | |
| ?: throw SandboxSetupException("Missing 'appId' in response") | |
| } catch (e: Exception) { | |
| throw SandboxSetupException("Failed to parse sandbox response", e) | |
| } | |
| } | |
| private companion object { | |
| // The key at index 5 gives enough permissions to interact with Chat and Channels | |
| private const val REQUIRED_KEY_INDEX = 5 | |
| } | |
| class SandboxSetupException : RuntimeException { | |
| constructor(message: String) : super(message) | |
| constructor(message: String, cause: Throwable) : super(message, cause) | |
| } |
| suspend fun loadAppCreationRequestBody(): JsonElement = | ||
| JsonParser.parseString( | ||
| client.get("https://raw.githubusercontent.com/ably/ably-common/refs/heads/main/test-resources/test-app-setup.json") { | ||
| contentType(ContentType.Application.Json) | ||
| }.bodyAsText(), | ||
| ).asJsonObject.get("post_apps") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve configuration loading reliability
The current implementation has several potential issues:
- Tests depend on external GitHub availability
- Configuration is fetched for every test
- No error handling for network issues
Consider caching the configuration and adding fallback:
+private var cachedConfig: JsonElement? = null
+
suspend fun loadAppCreationRequestBody(): JsonElement =
- JsonParser.parseString(
- client.get("https://raw.githubusercontent.com/ably/ably-common/refs/heads/main/test-resources/test-app-setup.json") {
- contentType(ContentType.Application.Json)
- }.bodyAsText(),
- ).asJsonObject.get("post_apps")
+ cachedConfig ?: try {
+ val response = client.get(CONFIG_URL) {
+ contentType(ContentType.Application.Json)
+ }
+ if (!response.status.isSuccess()) {
+ throw ConfigLoadException("Failed to load config: ${response.status}")
+ }
+ JsonParser.parseString(response.bodyAsText())
+ .asJsonObject
+ .get("post_apps")
+ ?.also { cachedConfig = it }
+ ?: throw ConfigLoadException("Missing 'post_apps' in config")
+ } catch (e: Exception) {
+ // Fallback to embedded config if available
+ loadEmbeddedConfig()
+ ?.also { cachedConfig = it }
+ ?: throw ConfigLoadException("Failed to load config", e)
+ }
+
+private companion object {
+ private const val CONFIG_URL =
+ "https://raw.githubusercontent.com/ably/ably-common/refs/heads/main/test-resources/test-app-setup.json"
+}Consider also embedding a copy of the configuration in the test resources for reliability.
Committable suggestion skipped: line range outside the PR's diff.
| @Test | ||
| fun basicIntegrationTest() = runTest { | ||
| val chatClient = createSandboxChatClient(sandbox.apiKey) | ||
| val room = chatClient.rooms.get("basketball") |
There was a problem hiding this comment.
I'd definitely recommend the room id here (and anything you do in tests) being random - if you're running an IDE that caches the sandbox config (or just running multiple tests in a suite), it gets flakey :D
|
|
||
| val client = HttpClient(CIO) | ||
|
|
||
| class Sandbox { |
There was a problem hiding this comment.
Perhaps not urgent, but would definitely recommend a setting/mode that can work with a local development realtime environment, will make future debugging a lot easier
added basic sandbox setup for simple happy-path testing
ttypic
force-pushed
the
ECO-5014/simple-sandbox-setup
branch
from
0b019ad
to
2b17388
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
chat-android/src/test/java/com/ably/chat/SandboxTest.kt (1)
10-26: Consider implementing a test fixture patternTo improve test maintainability and reduce duplication in future test cases, consider:
- Creating a base test class with common setup/teardown logic
- Implementing test fixtures for common scenarios
- Adding helper methods for verification and assertions
Example structure:
abstract class BaseSandboxTest { protected lateinit var sandbox: Sandbox protected lateinit var chatClient: ChatClient @Before fun setUp() = runTest { // Common setup logic } @After fun tearDown() = runTest { // Common cleanup logic } protected suspend fun createAndAttachRoom(roomId: String = UUID.randomUUID().toString()): Room { // Common room creation and attachment logic } } class SandboxTest : BaseSandboxTest() { // Current test methods }chat-android/src/test/java/com/ably/chat/Sandbox.kt (1)
18-21: Consider making retry configuration more flexibleWhile the retry logic is good, consider making it configurable through constructor parameters or environment variables to allow different retry strategies in different test scenarios.
- install(HttpRequestRetry) { - retryOnServerErrors(maxRetries = 4) - exponentialDelay() - } + install(HttpRequestRetry) { + retryOnServerErrors(maxRetries = getEnvOrDefault("SANDBOX_MAX_RETRIES", 4)) + exponentialDelay( + base = getEnvOrDefault("SANDBOX_RETRY_BASE_DELAY", 500L), + maxDelayMs = getEnvOrDefault("SANDBOX_RETRY_MAX_DELAY", 5000L) + ) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
chat-android/build.gradle.kts(1 hunks)chat-android/src/test/java/com/ably/chat/Sandbox.kt(1 hunks)chat-android/src/test/java/com/ably/chat/SandboxTest.kt(1 hunks)gradle/libs.versions.toml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- chat-android/build.gradle.kts
- gradle/libs.versions.toml
🔇 Additional comments (2)
chat-android/src/test/java/com/ably/chat/SandboxTest.kt (1)
19-25: 🛠️ Refactor suggestion
Enhance test robustness and coverage
The current test implementation needs improvements in several areas:
- Timeout handling for the attach operation
- Resource cleanup for the chat client
- Coverage of error scenarios
@Test
fun basicIntegrationTest() = runTest {
- val chatClient = sandbox.createSandboxChatClient()
- val room = chatClient.rooms.get(UUID.randomUUID().toString())
- room.attach()
- assertEquals(ChannelState.attached, room.messages.channel.state)
+ val chatClient = sandbox.createSandboxChatClient()
+ try {
+ val room = chatClient.rooms.get(UUID.randomUUID().toString())
+ withTimeout(5000) {
+ room.attach()
+ }
+ assertEquals(ChannelState.attached, room.messages.channel.state)
+ } finally {
+ chatClient.close()
+ }
}
+
+@Test
+fun shouldHandleAttachFailure() = runTest {
+ val chatClient = sandbox.createSandboxChatClient()
+ try {
+ val room = chatClient.rooms.get("invalid-room-id")
+ assertThrows<AblyException> {
+ withTimeout(5000) {
+ room.attach()
+ }
+ }
+ } finally {
+ chatClient.close()
+ }
}chat-android/src/test/java/com/ably/chat/Sandbox.kt (1)
42-45:
Fix incorrect parameter usage in createSandboxChatClient
The function passes apiKey instead of chatClientId to createSandboxRealtime.
- val realtime = createSandboxRealtime(apiKey)
+ val realtime = createSandboxRealtime()Likely invalid or redundant comment.
| @Before | ||
| fun setUp() = runTest { | ||
| sandbox = Sandbox.createInstance() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and cleanup to test lifecycle methods
The current setup lacks error handling and proper cleanup. Consider adding:
- Error handling in setUp
- A tearDown method to clean up resources
- Timeout handling for sandbox creation
@Before
fun setUp() = runTest {
- sandbox = Sandbox.createInstance()
+ try {
+ withTimeout(5000) {
+ sandbox = Sandbox.createInstance()
+ }
+ } catch (e: Exception) {
+ throw IllegalStateException("Failed to set up sandbox environment: ${e.message}", e)
+ }
}
+
+@After
+fun tearDown() = runTest {
+ try {
+ sandbox.close()
+ } catch (e: Exception) {
+ println("Warning: Failed to tear down sandbox environment: ${e.message}")
+ throw e
+ }
+}Committable suggestion skipped: line range outside the PR's diff.
| val response: HttpResponse = client.post("https://sandbox-rest.ably.io/apps") { | ||
| contentType(ContentType.Application.Json) | ||
| setBody(loadAppCreationRequestBody().toString()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make sandbox URL configurable
The sandbox URL should be configurable to support different environments (e.g., local development as suggested in past reviews).
+ companion object {
+ private const val DEFAULT_SANDBOX_URL = "https://sandbox-rest.ably.io"
+ private val SANDBOX_URL = System.getenv("SANDBOX_URL") ?: DEFAULT_SANDBOX_URL
+ }
- val response: HttpResponse = client.post("https://sandbox-rest.ably.io/apps") {
+ val response: HttpResponse = client.post("$SANDBOX_URL/apps") {📝 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.
| val response: HttpResponse = client.post("https://sandbox-rest.ably.io/apps") { | |
| contentType(ContentType.Application.Json) | |
| setBody(loadAppCreationRequestBody().toString()) | |
| } | |
| companion object { | |
| private const val DEFAULT_SANDBOX_URL = "https://sandbox-rest.ably.io" | |
| private val SANDBOX_URL = System.getenv("SANDBOX_URL") ?: DEFAULT_SANDBOX_URL | |
| } | |
| val response: HttpResponse = client.post("$SANDBOX_URL/apps") { | |
| contentType(ContentType.Application.Json) | |
| setBody(loadAppCreationRequestBody().toString()) | |
| } |
| private suspend fun loadAppCreationRequestBody(): JsonElement = | ||
| JsonParser.parseString( | ||
| client.get("https://raw.githubusercontent.com/ably/ably-common/refs/heads/main/test-resources/test-app-setup.json") { | ||
| contentType(ContentType.Application.Json) | ||
| }.bodyAsText(), | ||
| ).asJsonObject.get("post_apps") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add timeout for configuration loading
Configuration loading from external URL should have a reasonable timeout to prevent tests from hanging.
+private const val CONFIG_TIMEOUT_MS = 5000L
private suspend fun loadAppCreationRequestBody(): JsonElement =
JsonParser.parseString(
client.get("https://raw.githubusercontent.com/ably/ably-common/refs/heads/main/test-resources/test-app-setup.json") {
+ timeout {
+ requestTimeoutMillis = CONFIG_TIMEOUT_MS
+ }
contentType(ContentType.Application.Json)
}.bodyAsText(),
).asJsonObject.get("post_apps")📝 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 suspend fun loadAppCreationRequestBody(): JsonElement = | |
| JsonParser.parseString( | |
| client.get("https://raw.githubusercontent.com/ably/ably-common/refs/heads/main/test-resources/test-app-setup.json") { | |
| contentType(ContentType.Application.Json) | |
| }.bodyAsText(), | |
| ).asJsonObject.get("post_apps") | |
| private const val CONFIG_TIMEOUT_MS = 5000L | |
| private suspend fun loadAppCreationRequestBody(): JsonElement = | |
| JsonParser.parseString( | |
| client.get("https://raw.githubusercontent.com/ably/ably-common/refs/heads/main/test-resources/test-app-setup.json") { | |
| timeout { | |
| requestTimeoutMillis = CONFIG_TIMEOUT_MS | |
| } | |
| contentType(ContentType.Application.Json) | |
| }.bodyAsText(), | |
| ).asJsonObject.get("post_apps") |
| internal fun Sandbox.createSandboxRealtime(chatClientId: String = "sandbox-client"): AblyRealtime = | ||
| AblyRealtime( | ||
| io.ably.lib.types.ClientOptions().apply { | ||
| key = apiKey | ||
| environment = "sandbox" | ||
| clientId = chatClientId | ||
| }, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Generate unique client IDs for parallel test safety
Using a fixed default client ID could cause issues in parallel test execution.
-internal fun Sandbox.createSandboxRealtime(chatClientId: String = "sandbox-client"): AblyRealtime =
+internal fun Sandbox.createSandboxRealtime(
+ chatClientId: String = "sandbox-client-${System.currentTimeMillis()}-${Random.nextInt(1000)}"
+): AblyRealtime =📝 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.
| internal fun Sandbox.createSandboxRealtime(chatClientId: String = "sandbox-client"): AblyRealtime = | |
| AblyRealtime( | |
| io.ably.lib.types.ClientOptions().apply { | |
| key = apiKey | |
| environment = "sandbox" | |
| clientId = chatClientId | |
| }, | |
| ) | |
| internal fun Sandbox.createSandboxRealtime( | |
| chatClientId: String = "sandbox-client-${System.currentTimeMillis()}-${Random.nextInt(1000)}" | |
| ): AblyRealtime = | |
| AblyRealtime( | |
| io.ably.lib.types.ClientOptions().apply { | |
| key = apiKey | |
| environment = "sandbox" | |
| clientId = chatClientId | |
| }, | |
| ) |
added basic sandbox setup for simple happy-path testing
Summary by CodeRabbit
Sandboxclass for managing Ably applications in a sandbox environment.