fix(#4718): add transpileTests option#4776
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughReplaces the test-transpilation toggle with a new Maven parameter Changes
Sequence Diagram(s)sequenceDiagram
participant Maven as Maven build
participant Mj as MjTranspile (plugin)
participant JP as JavaPlaced
rect rgba(0,128,96,0.08)
Note over Maven,Mj: Plugin execution with parameter
Maven->>Mj: invoke plugin (eo.transpileTests = false|true)
Mj->>JP: exec(clazz, transpileTests)
end
alt transpileTests == true
rect rgba(0,128,255,0.06)
JP->>JP: if JavaPlaced.testsPresent(clazz) then place tests
end
else transpileTests == false
rect rgba(255,128,0,0.06)
JP-->>JP: skip placing tests
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ 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 |
🚀 Performance AnalysisAll benchmarks are within the acceptable range. No critical degradation detected (threshold is 100%). Please refer to the detailed report for more information. Click to see the detailed report
✅ Performance gain: |
There was a problem hiding this comment.
Pull request overview
This PR implements a new transpileTests configuration option for the EO Maven plugin to control whether test files are transpiled alongside production code. The change addresses issue #4718, which identified that EO tests were being improperly distributed with the eo-runtime library, causing unexpected behavior when running integration tests.
Key Changes:
- Added
transpileTestsboolean parameter toMjTranspile(defaults totruefor backward compatibility) - Updated
JavaPlacedto conditionally process test files based on a boolean flag - Modified integration test configuration to disable test transpilation where unnecessary
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| eo-maven-plugin/src/main/java/org/eolang/maven/MjTranspile.java | Introduced the transpileTests parameter and passed it to JavaPlaced for controlling test transpilation |
| eo-maven-plugin/src/main/java/org/eolang/maven/JavaPlaced.java | Changed from Proc<Xnav> to BiProc<Xnav, Boolean> to accept the test transpilation flag |
| eo-maven-plugin/src/test/java/org/eolang/maven/JavaPlacedTest.java | Updated test cases to pass the new boolean parameter when calling JavaPlaced.exec() |
| eo-maven-plugin/src/test/java/org/eolang/maven/MjTranspileIT.java | Added TODO comment documenting test dependency issues |
| eo-integration-tests/src/it/fibonacci/pom.xml | Configured transpileTests=false and removed workarounds for unwanted test execution |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * Whether to transpile tests. | ||
| * | ||
| * @checkstyle MemberNameCheck (7 lines) | ||
| */ |
There was a problem hiding this comment.
The JavaDoc comment has inconsistent indentation. The first line of the comment starts with "* " while subsequent lines start with " * ". This should be consistent with surrounding code patterns where all lines in a JavaDoc comment use the same indentation pattern (either all with "* " or adjust spacing).
| * Whether to transpile tests. | |
| * | |
| * @checkstyle MemberNameCheck (7 lines) | |
| */ | |
| * Whether to transpile tests. | |
| * | |
| * @checkstyle MemberNameCheck (7 lines) | |
| */ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
eo-maven-plugin/src/main/java/org/eolang/maven/MjTranspile.java (1)
131-145: Potential inconsistency: test-compile-source-root added regardless oftranspileTests.When
addSourcesRootis true, the code addsgenerated-test-sourcesto Maven's test-compile-source-root (lines 137-144) regardless of thetranspileTestssetting. IftranspileTestsis false, no test Java files will be generated, but Maven will still be configured to compile from that directory. This could cause confusion or build warnings.Consider gating the test-compile-source-root addition on the
transpileTestsflag:🔎 Suggested fix to align test source root with transpileTests
if (this.addSourcesRoot) { this.project.addCompileSourceRoot(this.generatedDir.getAbsolutePath()); Logger.info( this, "The directory added to Maven 'compile-source-root': %[file]s", this.generatedDir ); + if (this.transpileTests) { final String gtests = this.generatedDir.toPath().getParent().resolve( "generated-test-sources" ).toAbsolutePath().toString(); this.project.addTestCompileSourceRoot(gtests); Logger.info( this, "The directory added to Maven 'test-compile-source-root': %[file]s", gtests ); + } }
🧹 Nitpick comments (1)
eo-maven-plugin/src/main/java/org/eolang/maven/JavaPlaced.java (1)
48-56: Consider renaming parameter for clarity.The parameter name
testsis somewhat generic. UsingtranspileTestswould better align with the configuration parameter name inMjTranspileand make the intent clearer at call sites.🔎 Clearer parameter naming
- public void exec(final Xnav clazz, final Boolean tests) throws IOException { + public void exec(final Xnav clazz, final Boolean transpileTests) throws IOException { if (clazz.element("java").text().isPresent()) { this.footprint.apply(Paths.get(""), this.target); } - if (tests && JavaPlaced.testsPresent(clazz)) { + if (transpileTests && JavaPlaced.testsPresent(clazz)) { this.placeJavaTests(clazz); } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
eo-integration-tests/src/it/fibonacci/pom.xmleo-maven-plugin/src/main/java/org/eolang/maven/JavaPlaced.javaeo-maven-plugin/src/main/java/org/eolang/maven/MjTranspile.javaeo-maven-plugin/src/test/java/org/eolang/maven/JavaPlacedTest.javaeo-maven-plugin/src/test/java/org/eolang/maven/MjTranspileIT.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Agent
- GitHub Check: CodeQL analysis (java)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: integration
- GitHub Check: qulice
- GitHub Check: pr-size
- GitHub Check: mvn (ubuntu-24.04, 23)
- GitHub Check: mvn (ubuntu-24.04, 17)
- GitHub Check: mvn (windows-2022, 23)
- GitHub Check: trufflehog
- GitHub Check: mvn (macos-15, 23)
- GitHub Check: benchmark
- GitHub Check: ort
- GitHub Check: snippets
🔇 Additional comments (8)
eo-maven-plugin/src/test/java/org/eolang/maven/JavaPlacedTest.java (2)
28-47: LGTM! Test correctly updated for new API.The test properly exercises the updated
execmethod signature with thefalseparameter, verifying Java code placement without test transpilation.
49-77: LGTM! Test correctly validates test transpilation.The test properly exercises the updated
execmethod with thetrueparameter, verifying that Java tests are placed in thegenerated-test-sourcesdirectory when test transpilation is enabled.eo-maven-plugin/src/test/java/org/eolang/maven/MjTranspileIT.java (1)
24-30: Documentation added for test dependency concern.The TODO comment documents that these integration tests depend on a previously installed version of
eo-maven-pluginrather than the current build, which could lead to testing stale behavior.eo-integration-tests/src/it/fibonacci/pom.xml (2)
31-51: Configuration correctly disables test transpilation.The
transpileTestssetting is appropriately set tofalsefor this example module, which aligns with the PR objective to prevent EO tests from being distributed with the runtime library.
52-57: LGTM! Surefire configuration aligns with disabled test transpilation.The
failIfNoTestssetting prevents build failures when no test classes are present, which is appropriate given thattranspileTestsis set tofalse.eo-maven-plugin/src/main/java/org/eolang/maven/MjTranspile.java (2)
111-118: LGTM! Parameter properly configured with backward-compatible default.The
transpileTestsparameter is correctly configured with Maven annotations and defaults totrue, maintaining backward compatibility for existing builds that don't specify this parameter.
235-249: LGTM! transpileTests flag correctly passed to JavaPlaced.The
execmethod invocation at line 249 correctly passes thetranspileTestsconfiguration toJavaPlaced, properly threading the flag through to control test placement logic.eo-maven-plugin/src/main/java/org/eolang/maven/JavaPlaced.java (1)
13-19: This suggestion is not feasible. Java generics do not support primitive types as type parameters — only reference types are allowed due to how generics are implemented with type erasure.BiProc<Xnav, boolean>is syntactically invalid and cannot compile.The current implementation using
BiProc<Xnav, Boolean>is correct and the idiomatic approach in Java. The Boolean wrapper class with autoboxing/unboxing is standard practice for generic type parameters, and any theoretical NPE risk from anullBoolean parameter is typically managed at the call site and is not a common concern in practice.Likely an incorrect or invalid review comment.
|
@yegor256 could you have a look, please? |
2b50aa5 to
36025f7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
eo-integration-tests/src/it/fibonacci/pom.xml (1)
52-57: Appropriate configuration for integration test without transpiled tests.The
<failIfNoTests>false</failIfNoTests>configuration is appropriate sincetranspileTests=falsemeans test classes may not be generated. This prevents build failures when no tests are present.Consider documenting why tests are disabled for this integration test, since the AI summary mentions a "lengthy comment" was removed from the previous Surefire configuration. A brief comment explaining that this is an integration test for the Fibonacci example (not a test suite) would improve maintainability.
📝 Optional documentation improvement
<plugin> <artifactId>maven-surefire-plugin</artifactId> <configuration> + <!-- No unit tests in this integration example project --> <failIfNoTests>false</failIfNoTests> </configuration> </plugin>
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
eo-integration-tests/src/it/fibonacci/pom.xmleo-maven-plugin/src/main/java/org/eolang/maven/JavaPlaced.javaeo-maven-plugin/src/main/java/org/eolang/maven/MjTranspile.javaeo-maven-plugin/src/test/java/org/eolang/maven/JavaPlacedTest.javaeo-maven-plugin/src/test/java/org/eolang/maven/MjTranspileIT.java
✅ Files skipped from review due to trivial changes (1)
- eo-maven-plugin/src/test/java/org/eolang/maven/MjTranspileIT.java
🚧 Files skipped from review as they are similar to previous changes (3)
- eo-maven-plugin/src/main/java/org/eolang/maven/JavaPlaced.java
- eo-maven-plugin/src/test/java/org/eolang/maven/JavaPlacedTest.java
- eo-maven-plugin/src/main/java/org/eolang/maven/MjTranspile.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: mvn (windows-2022, 23)
🔇 Additional comments (1)
eo-integration-tests/src/it/fibonacci/pom.xml (1)
46-46: ConfirmtranspileTests=falseprevents test transpilation as intended.The
transpileTestsparameter is properly implemented ineo-maven-plugin(line 116-118 ofMjTranspile.java) with a default value oftrue. Setting it tofalsein this pom.xml will prevent test transpilation during the build, achieving the PR objective to exclude tests from the distribution.
|
@yegor256 could you have a look one more time, please? |
The new 'transpileTests' option prevents EO tests from being included with the
eo-runtimelibrary.Fixes #4718
Summary by CodeRabbit
New Features
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.