Add Kotlin unit test infrastructure and CircleCI step#156
Closed
pavelzeman wants to merge 2 commits intomattermost:masterfrom
Closed
Add Kotlin unit test infrastructure and CircleCI step#156pavelzeman wants to merge 2 commits intomattermost:masterfrom
pavelzeman wants to merge 2 commits intomattermost:masterfrom
Conversation
Add JUnit 4, Mockito, and OkHttp MockWebServer as test dependencies. These are needed to run the existing ExponentialRetryInterceptorTest and future interceptor tests. Co-authored-by: Claude <claude@anthropic.com>
- Add JUnit 4, Mockito, and OkHttp MockWebServer to android/build.gradle test dependencies - Add standalone test-runner project with Gradle wrapper for running Kotlin unit tests independently of the React Native build - Add kotlin-tests job to CircleCI using cimg/android executor - Add smoke test to verify the test infrastructure works - Wire kotlin-tests into the build workflow as a gate for build-package This enables TDD for Android interceptor code without requiring a full React Native app build. Co-authored-by: Claude <claude@anthropic.com>
This was referenced Mar 20, 2026
📝 WalkthroughWalkthroughThis pull request adds a new Kotlin test infrastructure for the project. It introduces a test-runner module with Gradle/Kotlin build configuration, includes test dependencies in the Android module, adds a smoke test validating OkHttp MockWebServer integration, and updates CircleCI to run these tests before packaging. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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. Comment Tip You can customize the tone of the review comments and chat replies.Configure the |
![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
🧹 Nitpick comments (1)
.circleci/config.yml (1)
69-83: Consider caching Gradle artifacts inkotlin-teststo speed CI.This job will re-download Gradle deps each run. Adding cache can significantly reduce pipeline time.
⚙️ Optional CI optimisation
kotlin-tests: executor: android steps: - checkout + - restore_cache: + keys: + - gradle-test-runner-{{ checksum "test-runner/gradle/wrapper/gradle-wrapper.properties" }}-{{ checksum "test-runner/build.gradle.kts" }} + - gradle-test-runner- - run: name: Run Kotlin unit tests command: | cd test-runner ./gradlew test + - save_cache: + key: gradle-test-runner-{{ checksum "test-runner/gradle/wrapper/gradle-wrapper.properties" }}-{{ checksum "test-runner/build.gradle.kts" }} + paths: + - ~/.gradle/caches + - ~/.gradle/wrapper - store_test_results: path: test-runner/build/test-results🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.circleci/config.yml around lines 69 - 83, The kotlin-tests job currently runs ./gradlew test every time and lacks dependency caching; update the kotlin-tests job to add CircleCI cache steps (restore_cache before running Gradle and save_cache after) targeting Gradle caches like ~/.gradle and project .gradle (e.g., ~/.gradle/caches and test-runner/.gradle), use stable cache keys that include a checksum of the Gradle wrapper or gradle.properties to bust when dependencies change, and ensure the new restore_cache/save_cache steps are placed around the existing checkout and ./gradlew test steps so the ./gradlew test run benefits from the cached artifacts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test-runner/build.gradle.kts`:
- Around line 15-21: The test runner currently limits sourceSets.test to only
Kotlin sources, excluding Java tests under android/src/test/java (e.g.,
ExponentialRetryInterceptorTest); update the test source set configuration in
build.gradle.kts so test sourceSet includes both Kotlin and Java test
directories (add the Java test srcDir or configure srcDirs to include
"src/test/java" alongside "src/test/kotlin") so that tests for interceptor
implementations in android/src/main/java/... are executed by the CI; modify the
block that references sourceSets { test { kotlin { srcDir("src/test/kotlin") } }
} to include the Java source directory for tests.
In `@test-runner/src/test/kotlin/com/mattermost/networkclient/SmokeTest.kt`:
- Around line 17-30: The MockWebServer instance `server` is shut down only after
assertions, which can leak the server if an assertion fails; wrap the test body
that starts and uses `server` (the `MockWebServer` variable `server`, the
request/response flow using `client.newCall(request).execute()` and `response`)
in a try/finally so `server.shutdown()` is always called in finally; also ensure
the `Response`/`response.body` is closed in the try (or via use/close) before
leaving so resources are released.
---
Nitpick comments:
In @.circleci/config.yml:
- Around line 69-83: The kotlin-tests job currently runs ./gradlew test every
time and lacks dependency caching; update the kotlin-tests job to add CircleCI
cache steps (restore_cache before running Gradle and save_cache after) targeting
Gradle caches like ~/.gradle and project .gradle (e.g., ~/.gradle/caches and
test-runner/.gradle), use stable cache keys that include a checksum of the
Gradle wrapper or gradle.properties to bust when dependencies change, and ensure
the new restore_cache/save_cache steps are placed around the existing checkout
and ./gradlew test steps so the ./gradlew test run benefits from the cached
artifacts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c3475ae3-2982-498b-b50f-259bf0411ce6
⛔ Files ignored due to path filters (1)
test-runner/gradle/wrapper/gradle-wrapper.jaris excluded by!**/*.jar
📒 Files selected for processing (8)
.circleci/config.ymlandroid/build.gradletest-runner/build.gradle.ktstest-runner/gradle/wrapper/gradle-wrapper.propertiestest-runner/gradlewtest-runner/gradlew.battest-runner/settings.gradle.ktstest-runner/src/test/kotlin/com/mattermost/networkclient/SmokeTest.kt
Comment on lines
+15
to
+21
| sourceSets { | ||
| test { | ||
| kotlin { | ||
| srcDir("src/test/kotlin") | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify which test/source dirs are wired into test-runner
rg -n "sourceSets|srcDir|srcDirs" test-runner/build.gradle.kts -C2
# Confirm interceptor tests currently live outside test-runner
rg -n "class ExponentialRetryInterceptorTest" android/src/test/java/com/mattermost/networkclient/ExponentialRetryInterceptorTest.kt -C2Repository: mattermost/react-native-network-client
Length of output: 368
🏁 Script executed:
#!/bin/bash
# Find all test files in android/src/test/java
find android/src/test/java -name "*.java" -o -name "*.kt" | head -20
# Check if android/src/main/java exists and what it contains
ls -la android/src/main/ 2>/dev/null || echo "No android/src/main/"
# Verify current test-runner structure
cat test-runner/build.gradle.kts | head -30Repository: mattermost/react-native-network-client
Length of output: 767
🏁 Script executed:
#!/bin/bash
# Check what's in android/src/main/java
find android/src/main/java -type f -name "*.java" -o -name "*.kt" 2>/dev/null
# Check specifically for interceptor implementations
rg -n "class.*Interceptor" android/src/main/java --type java --type kotlin
# Verify if there are any imports or references between test-runner and android
rg -n "android" test-runner/build.gradle.ktsRepository: mattermost/react-native-network-client
Length of output: 3778
🏁 Script executed:
#!/bin/bash
# Check what's in test-runner/src/test/kotlin
find test-runner/src/test/kotlin -type f 2>/dev/null | head -20
# Check test-runner directory structure
tree test-runner -L 2 2>/dev/null || find test-runner -type f -name "*.kt" -o -name "*.gradle.kts"
# Check if there's any documentation or README about test-runner
cat test-runner/README.md 2>/dev/null || cat test-runner/readme.md 2>/dev/null || echo "No README found"Repository: mattermost/react-native-network-client
Length of output: 324
🏁 Script executed:
#!/bin/bash
# Check the full build.gradle.kts to understand context better
cat test-runner/build.gradle.kts
# Check if settings.gradle.kts references android module
cat test-runner/settings.gradle.kts
# Verify the actual test file in test-runner to understand what it imports
cat test-runner/src/test/kotlin/com/mattermost/networkclient/SmokeTest.ktRepository: mattermost/react-native-network-client
Length of output: 1394
CI gate currently misses existing Android interceptor tests.
With sourceSets.test limited to src/test/kotlin, tests under android/src/test/java/... (e.g., ExponentialRetryInterceptorTest) are not included by this runner. The test-runner only executes SmokeTest.kt, which validates the test infrastructure itself rather than the actual interceptor implementations available in android/src/main/java/com/mattermost/networkclient/interceptors/. This gap weakens the stated interceptor-coverage goal.
💡 Suggested direction
sourceSets {
+ main {
+ kotlin {
+ srcDir("../android/src/main/java")
+ }
+ }
test {
kotlin {
- srcDir("src/test/kotlin")
+ srcDirs(
+ "src/test/kotlin",
+ "../android/src/test/java"
+ )
}
}
}📝 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.
Suggested change
| sourceSets { | |
| test { | |
| kotlin { | |
| srcDir("src/test/kotlin") | |
| } | |
| } | |
| } | |
| sourceSets { | |
| main { | |
| kotlin { | |
| srcDir("../android/src/main/java") | |
| } | |
| } | |
| test { | |
| kotlin { | |
| srcDirs( | |
| "src/test/kotlin", | |
| "../android/src/test/java" | |
| ) | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test-runner/build.gradle.kts` around lines 15 - 21, The test runner currently
limits sourceSets.test to only Kotlin sources, excluding Java tests under
android/src/test/java (e.g., ExponentialRetryInterceptorTest); update the test
source set configuration in build.gradle.kts so test sourceSet includes both
Kotlin and Java test directories (add the Java test srcDir or configure srcDirs
to include "src/test/java" alongside "src/test/kotlin") so that tests for
interceptor implementations in android/src/main/java/... are executed by the CI;
modify the block that references sourceSets { test { kotlin {
srcDir("src/test/kotlin") } } } to include the Java source directory for tests.
Comment on lines
+17
to
+30
| val server = MockWebServer() | ||
| server.enqueue(MockResponse().setBody("ok").setResponseCode(200)) | ||
| server.start() | ||
|
|
||
| val client = OkHttpClient() | ||
| val request = Request.Builder() | ||
| .url(server.url("/test")) | ||
| .build() | ||
|
|
||
| val response = client.newCall(request).execute() | ||
| Assert.assertEquals(200, response.code) | ||
| Assert.assertEquals("ok", response.body?.string()) | ||
|
|
||
| server.shutdown() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether cleanup is protected by finally/use in the smoke test
rg -n "server.shutdown\\(|execute\\(\\)\\.use|try|finally" test-runner/src/test/kotlin/com/mattermost/networkclient/SmokeTest.kt -C2Repository: mattermost/react-native-network-client
Length of output: 190
Wrap test cleanup in finally block to prevent resource leaks on assertion failures.
The server.shutdown() call runs only if all assertions pass. If any assertion fails, the MockWebServer remains open, potentially causing test flakiness in subsequent tests.
Suggested fix
fun mockWebServer_basicRequestResponse() {
val server = MockWebServer()
server.enqueue(MockResponse().setBody("ok").setResponseCode(200))
server.start()
-
- val client = OkHttpClient()
- val request = Request.Builder()
- .url(server.url("/test"))
- .build()
-
- val response = client.newCall(request).execute()
- Assert.assertEquals(200, response.code)
- Assert.assertEquals("ok", response.body?.string())
-
- server.shutdown()
+ try {
+ val client = OkHttpClient()
+ val request = Request.Builder()
+ .url(server.url("/test"))
+ .build()
+
+ client.newCall(request).execute().use { response ->
+ Assert.assertEquals(200, response.code)
+ Assert.assertEquals("ok", response.body?.string())
+ }
+ } finally {
+ server.shutdown()
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test-runner/src/test/kotlin/com/mattermost/networkclient/SmokeTest.kt` around
lines 17 - 30, The MockWebServer instance `server` is shut down only after
assertions, which can leak the server if an assertion fails; wrap the test body
that starts and uses `server` (the `MockWebServer` variable `server`, the
request/response flow using `client.newCall(request).execute()` and `response`)
in a try/finally so `server.shutdown()` is always called in finally; also ensure
the `Response`/`response.body` is closed in the try (or via use/close) before
leaving so resources are released.
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add infrastructure for running Kotlin unit tests in CI. Currently the repo has one Kotlin test file (
ExponentialRetryInterceptorTest.kt) but no test dependencies inbuild.gradleand no CI step to run them.The Android side of this library has 36 Kotlin files totaling ~3,250 lines of code — including interceptors for auth, retry, compression, timeouts, and metrics. That's a decent amount of logic with no test coverage in CI. This PR fixes that.
Ticket Link
N/A — infrastructure improvement to support ongoing Sentry issue fixes.
Release Note
Changes
android/build.gradle: AddtestImplementationdependencies for JUnit 4, Mockito, and OkHttp MockWebServertest-runner/: Standalone Gradle project with wrapper for running Kotlin unit tests independently of the React Native build. Includes a smoke test to verify the infrastructure works..circleci/config.yml: Addkotlin-testsjob usingcimg/androidexecutor, wired as a gate forbuild-packageWhy a standalone test-runner?
The Android module (
android/build.gradle) is a library that depends on React Native at build time, making it difficult to run./gradlew teststandalone. Thetest-runnerproject provides a lightweight Kotlin+JVM environment with OkHttp dependencies — enough to test interceptor logic without the full React Native build chain.What this enables
Summary by CodeRabbit