fix: deduplicate ReaderTestSupport between reader and builtins (fixes #1807) by gnodet · Pull Request #1815 · jline/jline3 (original) (raw)
Note
Reviews paused
It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.
Use the following commands to manage reviews:
@coderabbitai resumeto resume automatic reviews.@coderabbitai reviewto trigger a single review.
Use the checkboxes below for quick actions:
- ▶️ Resume reviews
- 🔍 Trigger review
📝 Walkthrough
Walkthrough
Consolidates duplicate test utilities by generating a jline-reader test-jar from the reader module, removes the builtins copy of ReaderTestSupport, and configures builtins to consume the test-jar with JPMS patching and runtime opens so tests compile and run against module org.jline.reader. (45 words)
Changes
| Cohort / File(s) | Summary |
|---|---|
| Reader test-jar generation reader/pom.xml | Adds maven-jar-plugin execution to produce a test JAR containing ReaderTestSupport classes. |
| Dependency management (root) pom.xml | Adds org.jline:jline-reader as a test-jar entry in dependencyManagement to allow resolving the reader test-jar. |
| Builtins build & JPMS test wiring builtins/pom.xml | Adds property for reader.test.jar, adds test-scoped org.jline:jline-reader dependency of type test-jar, configures maven-compiler-plugin --patch-module for test compile and sets surefire.argLine to patch modules, add opens and add-reads for tests. |
| Builtins test imports builtins/src/test/java/org/jline/builtins/OptionCompleterTest.java, builtins/src/test/java/org/jline/builtins/TreeCompleterTest.java | Replace local dependency on internal test class by importing org.jline.reader.impl.ReaderTestSupport from the reader test-jar (imports added). |
| Duplicate removal builtins/src/test/java/org/jline/builtins/ReaderTestSupport.java | Removes the local duplicate ReaderTestSupport (and its nested helper types and test helpers); functionality now provided by the reader test-jar. |
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
🐇 I hopped between modules, found one true bed,
Shared tests bundled up, no duplicates spread.
Patch flags set, jars snug and tight,
One ReaderTestSupport, everything right. ✨
🚥 Pre-merge checks | ✅ 5 ✅ Passed checks (5 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title clearly and concisely describes the main change: deduplicating ReaderTestSupport between two modules and fixing issue #1807. |
| Linked Issues check | ✅ Passed | All coding objectives from issue #1807 are met: reader module produces test-jar [reader/pom.xml], builtins depends on it [builtins/pom.xml], duplicate ReaderTestSupport is removed [builtins/src/test/java], tests updated to import from reader [OptionCompleterTest, TreeCompleterTest], and JPMS configuration is in place [--patch-module, --add-reads]. |
| Out of Scope Changes check | ✅ Passed | All changes are directly related to deduplicating ReaderTestSupport: POM configurations for test-jar generation and consumption, removal of duplicate class, import updates in dependent tests, and JPMS compatibility setup. No extraneous changes present. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches 🧪 Generate unit tests (beta)
- Create PR with unit tests
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.
❤️ Share
Comment @coderabbitai help to get the list of available commands and usage tips.