ADFA-4936: Fix Jacoco/SonarQube workflow (plugin-manager compile + lsp:kotlin OOM) - #1603
Merged
Merged
Conversation
The nightly Jacoco/SonarQube workflow failed at :plugin-manager:compileV8DebugUnitTestKotlin: the tests referenced a service model removed in an earlier refactor (FileServiceImpl/ProjectServiceImpl/ResourceServiceImpl and IdeResourceService). Because the sonarqube task depends on jacocoAggregateReport which depends on every subproject's testV8DebugUnitTest, this compile failure skipped the aggregate report (the "no files found for jacocoAggregateReport" upload warning) and failed the run. The impls were rewritten to a pluginId+permissions model (IdeFileServiceImpl, IdeProjectServiceImpl) that takes File args and returns raw types; the Resource service was removed outright. Rewrite the tests against the current API: - IdeFileServiceImplTest (replaces FileServiceImplTest): construct IdeFileServiceImpl with a permissive PathValidator (the production allowlist only permits fixed on-device roots) and FILESYSTEM_WRITE; cover write/read/replace/delete/list plus disallowed-path and missing-permission SecurityException. - E2EIntegrationTest: file-workflow-through-registry against the current API. - PluginManagerIntegrationTest: service registration/retrieval + functional file ops + path-security. - Delete ResourceServiceImplTest: the Resource service no longer exists, nothing to rewrite to. Verified: :plugin-manager:testV8DebugUnitTest -> 13 tests, 0 failures.
The nightly's second failure was :lsp:kotlin:testV8DebugUnitTest exceeding its 10-minute per-task timeout. Root cause: KtLspTestRule built a heavy KtLspTestEnvironment (IntelliJ project, application env, FIR infra, background index workers) per test method but env.close() was disabled, so environments accumulated for the JVM's lifetime. Locally this reproduces as java.lang.OutOfMemoryError after ~130 tests; in CI (larger heap) it manifests as GC-thrash plus a global-analysis-lock waiter crawling past the 10-minute timeout. Either way the run fails and the aggregate report is skipped.
- KtLspTestRule: re-enable env.close() in the finally block. The original "fails in test cases" was caused by calling it inside project.write { }; close() manages its own threading/locking, so call it directly.
- AnalysisSerializationTest: add @test(timeout = 60_000) to the two heavy concurrency tests that previously had no timeout, so a genuine liveness stall fails fast instead of burning the task budget (defense in depth; ~130x their observed runtime).
No production code changed. Verified: :lsp:kotlin:testV8DebugUnitTest -> 169 tests, 0 failures (previously OOM/timeout).
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
Contributor
📝 Walkthrough
WalkthroughChangesTest infrastructure and service coverage
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@plugin-manager/src/test/kotlin/com/itsaky/androidide/plugins/manager/services/IdeFileServiceImplTest.kt`:
- Around line 38-40: Replace the raw absolute-path prefix checks in
IdeFileServiceImplTest.kt (lines 38-40), E2EIntegrationTest.kt (lines 42-44),
and PluginManagerIntegrationTest.kt (lines 104-106) with canonical
Path.startsWith containment against the canonical project root. In
IdeFileServiceImplTest, add coverage rejecting both sibling-prefix paths and
traversal paths while preserving acceptance of paths within the root.
- Around line 4-10: Migrate the tests in
plugin-manager/src/test/kotlin/com/itsaky/androidide/plugins/manager/services/IdeFileServiceImplTest.kt
(lines 4-10),
plugin-manager/src/test/kotlin/com/itsaky/androidide/plugins/manager/E2EIntegrationTest.kt
(lines 7-13), and
plugin-manager/src/test/kotlin/com/itsaky/androidide/plugins/manager/core/PluginManagerIntegrationTest.kt
(lines 11-18) from JUnit 4 to JUnit Jupiter and Truth: replace imports,
annotations, helpers, and assertions with their Jupiter/Truth equivalents, and
convert any `@Test`(expected = ...) cases to Jupiter exception assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 97e0016c-a7b1-4018-9faf-27a0b140cbf4
📒 Files selected for processing (7)
lsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/compiler/modules/AnalysisSerializationTest.ktlsp/kotlin/src/test/java/com/itsaky/androidide/lsp/kotlin/fixtures/KtLspTestRule.ktplugin-manager/src/test/kotlin/com/itsaky/androidide/plugins/manager/E2EIntegrationTest.ktplugin-manager/src/test/kotlin/com/itsaky/androidide/plugins/manager/core/PluginManagerIntegrationTest.ktplugin-manager/src/test/kotlin/com/itsaky/androidide/plugins/manager/services/FileServiceImplTest.ktplugin-manager/src/test/kotlin/com/itsaky/androidide/plugins/manager/services/IdeFileServiceImplTest.ktplugin-manager/src/test/kotlin/com/itsaky/androidide/plugins/manager/services/ResourceServiceImplTest.kt
💤 Files with no reviewable changes (2)
- plugin-manager/src/test/kotlin/com/itsaky/androidide/plugins/manager/services/ResourceServiceImplTest.kt
- plugin-manager/src/test/kotlin/com/itsaky/androidide/plugins/manager/services/FileServiceImplTest.kt
jatezzz
approved these changes
Jul 30, 2026
…dators
Review feedback. The test PathValidator fakes used path.absolutePath.startsWith(root.absolutePath), which false-accepts a sibling-prefix path (e.g. "<root>-sibling") and an unnormalized traversal ("<root>/../escape"). Replace with canonical, component-wise containment (canonicalFile.toPath().startsWith(root.canonicalFile.toPath())) via a small File.isWithin helper, and add IdeFileServiceImplTest coverage rejecting both sibling-prefix and traversal paths (within-root acceptance stays covered by the existing read/write tests).
Verified: :plugin-manager:testV8DebugUnitTest -> 15 tests, 0 failures; spotlessCheck clean.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
plugin-manager/src/test/kotlin/com/itsaky/androidide/plugins/manager/services/IdeFileServiceImplTest.kt (1)
132-134: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCentralize the duplicated containment helper.
The same canonical
File.isWithinimplementation is defined three times.
plugin-manager/src/test/kotlin/com/itsaky/androidide/plugins/manager/services/IdeFileServiceImplTest.kt#L132-L134: move the helper to a shared test utility.plugin-manager/src/test/kotlin/com/itsaky/androidide/plugins/manager/E2EIntegrationTest.kt#L65-L67: reuse the shared helper.plugin-manager/src/test/kotlin/com/itsaky/androidide/plugins/manager/core/PluginManagerIntegrationTest.kt#L118-L120: reuse the shared helper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugin-manager/src/test/kotlin/com/itsaky/androidide/plugins/manager/services/IdeFileServiceImplTest.kt` around lines 132 - 134, Centralize the canonical File.isWithin containment helper in a shared test utility, then remove the local definitions from IdeFileServiceImplTest, E2EIntegrationTest, and PluginManagerIntegrationTest and update each test to reuse the shared extension. Apply the change at plugin-manager/src/test/kotlin/com/itsaky/androidide/plugins/manager/services/IdeFileServiceImplTest.kt:132-134, plugin-manager/src/test/kotlin/com/itsaky/androidide/plugins/manager/E2EIntegrationTest.kt:65-67, and plugin-manager/src/test/kotlin/com/itsaky/androidide/plugins/manager/core/PluginManagerIntegrationTest.kt:118-120.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@plugin-manager/src/test/kotlin/com/itsaky/androidide/plugins/manager/services/IdeFileServiceImplTest.kt`:
- Around line 132-134: Centralize the canonical File.isWithin containment helper
in a shared test utility, then remove the local definitions from
IdeFileServiceImplTest, E2EIntegrationTest, and PluginManagerIntegrationTest and
update each test to reuse the shared extension. Apply the change at
plugin-manager/src/test/kotlin/com/itsaky/androidide/plugins/manager/services/IdeFileServiceImplTest.kt:132-134,
plugin-manager/src/test/kotlin/com/itsaky/androidide/plugins/manager/E2EIntegrationTest.kt:65-67,
and
plugin-manager/src/test/kotlin/com/itsaky/androidide/plugins/manager/core/PluginManagerIntegrationTest.kt:118-120.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b37bb7dc-90e0-4ba9-873c-3fd7e4242a7f
📒 Files selected for processing (3)
plugin-manager/src/test/kotlin/com/itsaky/androidide/plugins/manager/E2EIntegrationTest.ktplugin-manager/src/test/kotlin/com/itsaky/androidide/plugins/manager/core/PluginManagerIntegrationTest.ktplugin-manager/src/test/kotlin/com/itsaky/androidide/plugins/manager/services/IdeFileServiceImplTest.kt
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
2 participants
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
Fixes the nightly Analyze Repository with Jacoco and SonarQube workflow (ADFA-4936). The "Build and analyze" step was failing with two independent task failures under
--continue, which skippedjacocoAggregateReport(the "No files were found for jacocoAggregateReport" upload warning) and failed the run with exit 1.Failure 1 — plugin-manager unit tests didn't compile
The tests referenced a service model removed in an earlier refactor (
FileServiceImpl/ProjectServiceImpl/ResourceServiceImpl,IdeResourceService). The impls were rewritten to apluginId+permissions model (IdeFileServiceImpl,IdeProjectServiceImpl) takingFileargs and returning raw types; the Resource service was removed outright — so this was a rewrite, not a rename.IdeFileServiceImplTest(replacesFileServiceImplTest) — constructsIdeFileServiceImplwith a permissivePathValidator(the production allowlist only permits fixed on-device roots) andFILESYSTEM_WRITE; covers write/read/replace/delete/list plus disallowed-path and missing-permissionSecurityException.E2EIntegrationTest— file workflow throughServiceRegistryImpl.PluginManagerIntegrationTest— registration/retrieval + functional file ops + path-security.ResourceServiceImplTest— the Resource service no longer exists.Failure 2 — :lsp:kotlin test suite exhausted memory
KtLspTestRulebuilt a heavyKtLspTestEnvironment(IntelliJ project, application env, FIR infra, background index workers) per test method, butenv.close()was disabled — so environments accumulated for the JVM's lifetime. Locally this reproduces asjava.lang.OutOfMemoryErrorafter ~130 tests; in CI (larger heap) it manifested as GC-thrash plus a global-analysis-lock waiter crawling past the 10-minute per-task timeout.KtLspTestRule— re-enableenv.close(). The original "fails in test cases" was caused by calling it insideproject.write { };close()manages its own threading/locking, so it's called directly now.AnalysisSerializationTest— add@Test(timeout = 60_000)to the two heavy concurrency tests that had no timeout, so a genuine liveness stall fails fast instead of burning the task budget (defense in depth).No production code changed — both fixes are test-only.
Verification
:plugin-manager:testV8DebugUnitTest-> 13 tests, 0 failures:lsp:kotlin:testV8DebugUnitTest-> 169 tests, 0 failures (previously OOM/timeout; the failing CI run only reached 131 before aborting)spotlessCheckcleanOut of scope (follow-up)
While investigating, I found
plugin-api's entire unit-test suite is broken by the same refactor (PluginContextTest.kt+ 3*ServiceTest.javareferencing removed*OperationResulttypes). It doesn't affect this workflow becauseplugin-apihas nov8flavor, sojacocoAggregateReport(keyed ontestV8DebugUnitTest) never compiles or runs it. Filing a separate ticket.