ADFA-5263: Delete copyToTestDir, which copied a jar nothing reads - #1739
Conversation
The task copied tooling-api-model.jar to <root>/tests/test-home/.cg/init/model.jar. Nothing reads that file, and nothing reads that directory. The only consumer of a "test home" is gradle-plugin's test helper, which resolves FileProvider.testHomeDir() -- testing/resources/test-home, a different directory -- and then *writes its own* init script there, with a classpath from Gradle's PluginUnderTestMetadataReading. It never asks for a model jar. A grep for model.jar across the repo returned only the task that produced it. The destination had drifted before: 2a84174 (Feb 2023) is "fix: invalid path specified in copyToTestDir", and #1161 renamed .androidide to .cg inside it. Removing it takes three problems with it: - into(rootProject.mkdir(...)) ran at configuration time, so merely realizing the task created directories in the source tree -- on --dry-run, and again after every clean. That is why tests/test-home kept reappearing. - outputs.upToDateWhen { false } on both the copy and jar meant any build touching this module re-jarred and re-copied unconditionally. - Its output being a directory inside the source tree is what tripped Gradle's implicit-dependency validation against Spotless (ADFA-5244). That was worked around at the consumer by excluding the directory from the Spotless walk. Verified: the jar still builds; tests/ is no longer created at configuration time; :app:assembleV8Debug succeeds with the task absent from the graph; and `:common:compileV8DebugKotlin spotlessCheck` -- the exact invocation ADFA-5244 was filed for -- now passes on this branch, which carries no Spotless exclude at all. The two .gitignore entries that existed only for this task's output go too. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![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.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (2)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. 📝 Walkthrough
WalkthroughThe change removes test artifact ignore rules from ChangesTest artifact cleanup
Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to This change removes an unused test-home jar copy and its configuration-time directory side effects while preserving the jar build and affected checks; no actionable merge-blocking risk remains. Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. ✨ Finishing Touches📝 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 |
|
Heads-up for anyone with a clone that predates this merge: you likely have a stale Why it appears: I checked that this is only a leftover and not still being produced, since I initially suspected the opposite. On a fresh
So the premise of this PR holds. The only residue is on disk in existing checkouts, and the fix is the one-liner above. (For the record on how I misread it: a jar dated after this merge turned up in one of my worktrees, which looked like live creation. It was built ~9 minutes after this PR merged, but by a branch whose own |
Brings in ADFA-5263 (#1739), which deleted copyToTestDir independently, and ADFA-5264 (#1740), which ignores the test project's .cg/ cache. With both in, this branch's diff is the three files it actually changes. Conflict was in .gitignore: stage removed both tests/ entries as part of #1739. Kept this branch's tests/test-home line -- the gradle-plugin tests still refill that directory -- and left the model.jar entry deleted, since the task that wrote it is gone. Moved the entry out from under the "Generated files for tooling API" header, which heads the composite-build output dirs and not this. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01M4sTwYg47aK8VB9kRKZicU
2 participants
:subprojects:tooling-api-model:copyToTestDircopied the module's jar to<root>/tests/test-home/.cg/init/model.jar. Nothing reads that file, and nothing reads that directory.The only consumer of a "test home" is gradle-plugin's test helper, and it resolves
FileProvider.testHomeDir()—testing/resources/test-home, a different directory — then writes its own init script there fromPluginUnderTestMetadataReading.readImplementationClasspath(). It never asks for a model jar. A grep formodel.jaracross the repo returns only the task that produced it.The destination has drifted before:
2a841748b(Feb 2023) is literally "fix: invalid path specified in copyToTestDir", and #1161 renamed.androidide→.cginside it.Three problems leave with it
into(rootProject.mkdir(...))ran at configuration time. Merely realizing the task created directories in the source tree — on--dry-run, and again after everyclean. That is whytests/test-homekept reappearing when nothing had run.outputs.upToDateWhen { false }on both the copy andjarmeant any build touching this module re-jarred and re-copied unconditionally.Verified
tests/is no longer created at configuration time — deleted it, ran a configuration pass, it stayed gone.:app:assembleV8Debugsucceeds with the task absent from the graph.:common:compileV8DebugKotlin spotlessCheck— the exact invocation ADFA-5244 was filed for — passes on this branch, which carries no Spotless exclude at all. That is the point: fixing the producer removes the need for the workaround rather than making it permanent.Relationship to #1730
#1730 excludes
tests/from the Spotless walk to stop the symptom. If this lands, that exclude is dead weight and should be reverted; if #1730 lands first, this makes it removable. Whichever goes second should drop it — I'll do that as soon as the order is known. The CI guard #1730 adds (running a compile task together withspotlessCheck) is worth keeping either way, since it catches this whole class of failure.