fix: deduplicate ReaderTestSupport between reader and builtins (fixes #1807)#1815
Conversation
…#1807) Use a test-jar dependency from the reader module instead of maintaining a separate copy in builtins. Configure --patch-module for JPMS compatibility during test compilation and execution.
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughConsolidates duplicate test utilities by generating a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@builtins/pom.xml`:
- Around line 27-28: The pom currently hardcodes reader.test.jar to
../reader/target/... and uses ${reader.test.jar} in the surefire.argLine and
compilerArgs, which breaks when the test-jar is resolved from the local repo;
replace this by using maven-dependency-plugin:properties to resolve the artifact
org.jline:jline-reader:jar:tests during initialize so Maven sets a property (the
standard property name will be ${org.jline:jline-reader:jar:tests}), then update
surefire.argLine and the default-testCompile compilerArgs to reference that
resolved property instead of ${reader.test.jar}; ensure the plugin execution has
<phase>initialize</phase> and the <goal>properties</goal> so the property is
available when Surefire and javac args are evaluated.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0552c34d-cf1f-4190-b2ca-cf291d9d1300
📒 Files selected for processing (6)
builtins/pom.xmlbuiltins/src/test/java/org/jline/builtins/OptionCompleterTest.javabuiltins/src/test/java/org/jline/builtins/ReaderTestSupport.javabuiltins/src/test/java/org/jline/builtins/TreeCompleterTest.javapom.xmlreader/pom.xml
💤 Files with no reviewable changes (1)
- builtins/src/test/java/org/jline/builtins/ReaderTestSupport.java
| <reader.test.jar>${project.basedir}/../reader/target/jline-reader-${project.version}-tests.jar</reader.test.jar> | ||
| <surefire.argLine>--add-opens java.base/java.io=org.jline.nativ --add-opens java.base/java.io=org.jline.terminal --patch-module org.jline.reader=${reader.test.jar} --add-reads org.jline.reader=ALL-UNNAMED</surefire.argLine> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the hardcoded path and check whether any CI workflow relies on the sibling target/ layout.
rg -nP 'reader\.test\.jar|\.\./reader/target' -C2
fd -t f -e yml -e yaml . .github 2>/dev/null | xargs -r rg -nP 'mvn|maven' -lRepository: jline/jline3
Length of output: 1097
🏁 Script executed:
# Check the test-jar dependency declaration around lines 52-57
sed -n '45,65p' builtins/pom.xmlRepository: jline/jline3
Length of output: 764
🏁 Script executed:
# Get full context around lines 95-103 for the compiler configuration
sed -n '90,110p' builtins/pom.xmlRepository: jline/jline3
Length of output: 846
🏁 Script executed:
# Check GitHub workflows to see how they invoke Maven
cat .github/workflows/master-build.yml
cat .github/workflows/release.ymlRepository: jline/jline3
Length of output: 7218
🏁 Script executed:
# Verify maven-dependency-plugin:properties capability and check if it's used elsewhere in project
rg -n 'maven-dependency-plugin' -A3Repository: jline/jline3
Length of output: 1456
🏁 Script executed:
# Check if any of these usages employ the properties goal
rg -n 'maven-dependency-plugin' -A10 | grep -E '(goal|properties)'Repository: jline/jline3
Length of output: 926
Hardcoded relative path to reader's target/ bypasses Maven resolution.
${reader.test.jar} points at ../reader/target/jline-reader-${project.version}-tests.jar (line 27), so --patch-module only works when the reader module's target/ is freshly built in the same checkout. This breaks in common scenarios:
- Running
mvn -pl builtins testagainst an already-installed reader (the test-jar is in~/.m2, not../reader/target/). - Any consumer building
builtinswithout a siblingreader/directory. - Changes to
finalNameor artifact naming conventions.
The declared test-jar dependency (lines 52–57) is already resolved by Maven — prefer letting Maven give you the actual artifact path rather than guessing it.
♻️ Suggested approach using maven-dependency-plugin:properties
Use the properties goal to bind the resolved artifact path into a property (e.g., org.jline:jline-reader:jar:tests) during initialize, then reference that in --patch-module:
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-dependency-plugin</artifactId>
<executions>
<execution>
<id>resolve-test-jar</id>
<phase>initialize</phase>
<goals><goal>properties</goal></goals>
</execution>
</executions>
</plugin>Then replace ${reader.test.jar} with ${org.jline:jline-reader:jar:tests} in both surefire.argLine (line 28) and the default-testCompile compilerArgs (line 100).
Also applies to: lines 95–103
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@builtins/pom.xml` around lines 27 - 28, The pom currently hardcodes
reader.test.jar to ../reader/target/... and uses ${reader.test.jar} in the
surefire.argLine and compilerArgs, which breaks when the test-jar is resolved
from the local repo; replace this by using maven-dependency-plugin:properties to
resolve the artifact org.jline:jline-reader:jar:tests during initialize so Maven
sets a property (the standard property name will be
${org.jline:jline-reader:jar:tests}), then update surefire.argLine and the
default-testCompile compilerArgs to reference that resolved property instead of
${reader.test.jar}; ensure the plugin execution has <phase>initialize</phase>
and the <goal>properties</goal> so the property is available when Surefire and
javac args are evaluated.
|
Re: CodeRabbit suggestion to use I investigated this approach. Unfortunately it doesn't work in this project for two reasons:
The relative path via Claude Code on behalf of Guillaume Nodet |
gnodet
force-pushed
the
ci-issue-1807
branch
2 times, most recently
from
a14ec41 to
5dfb537
Compare
Use ${maven.multiModuleProjectDirectory} instead of
${project.basedir}/.. for a more robust reference to the
project root directory.
|
1 participant
Summary
Eliminate the duplicated
ReaderTestSupportclass betweenreaderandbuiltinsmodules by sharing it via a Maven test-jar.readermodule to produce a test-jar viamaven-jar-plugin(scoped toReaderTestSupportonly)builtinsOptionCompleterTestandTreeCompleterTestto importReaderTestSupportfromorg.jline.reader.implReaderTestSupportfrombuiltins/src/test/java/org/jline/builtins/--patch-moduleand--add-readsfor JPMS compatibility during test compilation and executionFixes #1807
Benefits
ReaderTestSupportonly need to happen in one placeReaderTestSupportand its inner classes are included (4 classes), not all reader testsDownsides
ReaderTestSupportwill be published alongside the main jar (this is standard Maven behavior for test-jars)--patch-moduleand--add-readsflags for both compilation and test execution, sinceReaderTestSupportmust be patched into theorg.jline.readermodule-pl builtins) requires that the reader module (including its test-jar) has been built firstmaven.multiModuleProjectDirectory— this is a Maven internal property (not part of the public API), used to construct the test-jar path for--patch-module; it works reliably in Maven 3.x and 4.x but is technically undocumentedTest plan
OptionCompleterTestandTreeCompleterTestwhich use the sharedReaderTestSupport)