feat: add Gradle and Maven support with Surefire parser (75-90% savings)#782
Conversation
📊 Automated PR Analysis
SummaryAdds Gradle and Maven command support to rtk with output filtering that achieves 75-90% token savings. Includes Gradle wrapper auto-detection, Surefire XML test parser for Maven, discovery rules for both build tools, and comprehensive test fixtures. Review Checklist
Analyzed automatically by wshm · This is an automated analysis, not a human review. |
pszymkowiak
left a comment
There was a problem hiding this comment.
Review — Gradle & Maven support
Good work — real fixtures, savings assertions, gradlew auto-detection, sub-enum pattern. A few things to fix before merge.
P0 — Must Fix
1. CHANGELOG rebase conflict
The diff deletes the Ruby feature entries and the ## [0.31.0] section. Looks like a merge artifact — the [Unreleased] block got duplicated and the old one was removed with its surrounding context. Please check that the full changelog history is intact after your upstream merge.
2. TOML filter conflict
src/filters/gradle.toml and src/filters/mvn-build.toml already exist on develop and match the same commands. The Rust modules are strictly better (Surefire parsing, gradlew detection, sub-enum routing), so please delete the two TOML filters to avoid undefined routing behavior.
P1 — Should Fix
3. Vec<Regex> for noise patterns — O(N×M) per line
Both modules define 17-19 separate Regex objects in a Vec, iterated for every output line. Collapse into a single alternation regex (like rspec_cmd.rs does). Not a blocker but measurable on large Maven reactor builds.
4. ^ - noise pattern too broad (gradle_cmd.rs)
This strips any line starting with -, which catches legitimate output beyond the Gradle welcome banner. Narrow to the welcome block context or use a more specific pattern.
5. run_other missing tee integration
Both gradle_cmd::run_other and maven_cmd::run_other skip tee::tee_and_hint() — if the filter produces wrong output for an unsupported subcommand, the raw output is lost with no recovery path. See run_test/run_build for the pattern to follow.
6. Missing test for compile-failure in filter_gradle_test
The total == 0 && !build_errors.is_empty() path in filter_gradle_test is never exercised. Add a test calling filter_gradle_test with the gradle_build_fail_raw.txt fixture.
P2 — Nice to Have
7. mvn verify falls through to Other instead of the Surefire parser — consider adding a Verify subcommand routing to run_test.
8. All fixtures from a single project — a multi-module reactor fixture would strengthen coverage.
Overall solid implementation. Fix P0 #1 and #2, and I'll re-review.
|
Hey We are cleaning up the codebase and improving the project structure for better onboarding. As part of this effort, PR #826 reorganizes No logic changes — only file moves and import path updates. What you need to doRebase your branch on git fetch origin && git rebase origin/developGit detects renames automatically. If you get import conflicts, update the paths: use crate::git; // now: use crate::cmds::git::git;
use crate::tracking; // now: use crate::core::tracking;
use crate::config; // now: use crate::core::config;
use crate::init; // now: use crate::hooks::init;
use crate::gain; // now: use crate::analytics::gain;Need help rebasing? Tag @aeppling |
0cb8825 to
edd2dcf
Compare
|
All P0 and P1 items addressed in edd2dcf: P0 #1 — CHANGELOG: Fixed during rebase onto P0 #2 — Conflicting TOML filters: Deleted P1 #3 — P1 #4 — P1 #5 — P1 #6 — Missing compile-failure test: Added All 23 Gradle/Maven unit tests pass. P2 items (#7 |
|
Probably related to #381 |
16f1d4d to
4a4187a
Compare
Code reviewFound 4 issues:
Lines 464 to 496 in b6bc8cc
rtk/src/cmds/jvm/gradle_cmd.rs Lines 343 to 351 in b6bc8cc
rtk/src/cmds/jvm/gradle_cmd.rs Lines 24 to 29 in b6bc8cc 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
33292fc to
af85aca
Compare
- Add `rtk gradle` with sub-commands: build, test, check, dependencies Filters daemon noise, download progress, UP-TO-DATE tasks; shows errors and failures only (75-90% token savings) - Add `rtk mvn` with sub-commands: compile, test, package, verify, install, dependency:tree. Includes Surefire XML parser for structured test failure output with text fallback (75-90% token savings) - Add gradlew wrapper auto-detection (./gradlew vs gradle) - Register both commands in main.rs following the Go sub-enum pattern - Delete TOML-based gradle.toml and mvn-build.toml (replaced by Rust) - Update src/discover/rules.rs and src/hooks for missed-savings detection - Add 8 real fixtures (Gradle + Maven build/test success/fail) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
af85aca to
414b48d
Compare
|
Any progress in this issue? It would be great to have at least "something" - even if it wouldn't be perfect. I also see there are multiple opened PR for adding Maven/Gradle support. So there surely is quite a demand for this feature 🙏🏼 |
`mvn verify` (the canonical goal that produces target/failsafe-reports/)
previously fell through to Passthrough — no filtering, no XML enrichment
— even though the failsafe parser was already implemented for run_test.
- New MvnCommands::Verify variant; dispatch_mvn routes to run_verify.
- Extract run_test body to private run_tests_like(binary, goal, ...);
both run_test and run_verify wrap it. Shared filter + enrichment.
- Parameterize filter_mvn_test summary prefix via filter_mvn_tests_with_goal
(goal in {"test", "verify"}). Public wrappers filter_mvn_test /
filter_mvn_verify preserve existing call sites.
- Generalize enrich_with_reports zero-tests detection + error message
(ends_with(": no tests run"), goal extracted from summary prefix).
- Discover rules: add `verify` to mvn and mvnd patterns.
- Snapshot + prefix-assertion tests for verify path; goal-awareness test
for enrichment's zero-tests branch.
Addresses P2 rtk-ai#7 from the review of sibling PR rtk-ai#782.
`mvn verify` (the canonical goal that produces target/failsafe-reports/)
previously fell through to Passthrough — no filtering, no XML enrichment
— even though the failsafe parser was already implemented for run_test.
- New MvnCommands::Verify variant; dispatch_mvn routes to run_verify.
- Extract run_test body to private run_tests_like(binary, goal, ...);
both run_test and run_verify wrap it. Shared filter + enrichment.
- Parameterize filter_mvn_test summary prefix via filter_mvn_tests_with_goal
(goal in {"test", "verify"}). Public wrappers filter_mvn_test /
filter_mvn_verify preserve existing call sites.
- Generalize enrich_with_reports zero-tests detection + error message
(ends_with(": no tests run"), goal extracted from summary prefix).
- Discover rules: add `verify` to mvn and mvnd patterns.
- Snapshot + prefix-assertion tests for verify path; goal-awareness test
for enrichment's zero-tests branch.
Addresses P2 rtk-ai#7 from the review of sibling PR rtk-ai#782.
`mvn verify` (the canonical goal that produces target/failsafe-reports/)
previously fell through to Passthrough — no filtering, no XML enrichment
— even though the failsafe parser was already implemented for run_test.
- New MvnCommands::Verify variant; dispatch_mvn routes to run_verify.
- Extract run_test body to private run_tests_like(binary, goal, ...);
both run_test and run_verify wrap it. Shared filter + enrichment.
- Parameterize filter_mvn_test summary prefix via filter_mvn_tests_with_goal
(goal in {"test", "verify"}). Public wrappers filter_mvn_test /
filter_mvn_verify preserve existing call sites.
- Generalize enrich_with_reports zero-tests detection + error message
(ends_with(": no tests run"), goal extracted from summary prefix).
- Discover rules: add `verify` to mvn and mvnd patterns.
- Snapshot + prefix-assertion tests for verify path; goal-awareness test
for enrichment's zero-tests branch.
Addresses P2 rtk-ai#7 from the review of sibling PR rtk-ai#782.
Multi-module compile output was dominated by the `Reactor Build Order:` block and per-module `Reactor Summary` status lines (~40 redundant tokens on a green 5-module build), and javac `[ERROR] <path>:[line,col]` locations were emitted twice — inline during compilation and again in the trailing help block — nearly doubling the failure-path output. Changes in filter_mvn_compile: - Skip `Reactor Build Order:` block entirely (redundant with per-module Building headers that are already stripped). - Collapse Reactor Summary: emit a compact `Reactor: N modules — M SUCCESS, K FAILURE (name, ...)` line only when something failed; all-green reactors rely on the surviving `BUILD SUCCESS` line. - Dedup `[ERROR] <path>:[line,col]` occurrences by (path, line, col). When a duplicate fires, swallow the indented `[ERROR] symbol:/location:/ required:/found:/reason:` context lines that mirror an earlier javac explanation emitted without the `[ERROR]` prefix. Also: broaden `deprecated` check to `deprecat` (catches both `deprecated` and `deprecation` variants); strip `/pom.xml` path references (previously only literal `from pom.xml` matched); drop the Help-footer line `For more information about the errors...` which wasn't in the boilerplate list. Savings on fixtures adapted from rtk-ai#782 (attribution in filenames): mvn_pr782_compile_success_raw 74.6% → 97.8% (+23pp) mvn_pr782_compile_fail_raw 49.1% → 79.7% (+31pp) mvn_pr782_test_pass_raw 98.7% → 98.7% (unchanged, correct) mvn_pr782_test_fail_raw 93.4% → 93.4% (unchanged, correct) Four new regression tests validate multi-module accumulation (20 tests across 6 modules must not report only the first module's count), failure enumeration uniqueness, reactor collapse, and javac error dedup.
Summary
rtk gradlecommand with sub-commands:build,test,check,dependencies— filtering Gradle output to errors/failures only (75-90% savings)rtk mvncommand with sub-commands:compile,test,package,verify,install,dependency:tree— with Surefire XML parser for structured test failure output (75-90% savings)src/main.rsCommands enum following the Go sub-enum patternCHANGELOG.mdandsrc/discover/rules.rsfor missed-savings detectionChanges since initial review (33292fc)
TEST_SUMMARY_REnow captures optional skipped count —passedis correctlytotal - failed - skipped(previously skipped tests were counted as passing)"Gradle test: no tests found"instead of"Gradle test: 0 passed"total_passedinstead oftotal_run, so skipped tests are not reported as passinginstasnapshot tests (8 total) for build and test success/failure fixturesTest plan
cargo check --all-targetspassesrtk gradle buildfilters Gradle build output to errors onlyrtk gradle testshows only failed tests with skipped count correctrtk mvn compilefilters Maven compile errorsrtk mvn testuses Surefire XML when available, falls back to text parser🤖 Generated with Claude Code