fix(#4717): enable fibonacci integration test in pom.xml and update properties #4728
Conversation
WalkthroughThis change re-enables the fibonacci integration test by removing its Maven configuration exclusion, updates the build plugin setup to use new EO Maven plugin flags, adds Maven debug mode to integration testing, and includes supporting documentation improvements for package-info handling. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
@yegor256 could you take a look, please? |
🚀 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 enables the previously disabled Fibonacci integration test by fixing the underlying issue where package-info.class files were being removed during compilation, causing runtime errors. The fix includes proper plugin configuration and enhanced logging.
Key changes:
- Removed the fibonacci test exclusion from the integration tests configuration
- Updated fibonacci pom.xml to use the
compilegoal instead ofassembleand simplified plugin configuration - Added debug logging to track package-info.java file creation
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| eo-integration-tests/pom.xml | Removed the pomExcludes section that was preventing the fibonacci integration test from running |
| eo-integration-tests/src/it/fibonacci/pom.xml | Removed maven-compiler-plugin override, changed from assemble to compile goal, and simplified eo-maven-plugin configuration |
| eo-integration-tests/src/it/fibonacci/invoker.properties | Added -X flag to enable debug logging during test execution |
| eo-runtime/src/main/java/org/eolang/PhPackage.java | Added @todo comment documenting the need to check for package-info.class existence before object lookup |
| eo-maven-plugin/src/main/java/org/eolang/maven/MjTranspile.java | Added @todo comment about refactoring and enhanced logging to track package-info.java file creation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * @return Amount of created files | ||
| * @throws IOException If fails to create a file | ||
| * @todo #4717:90min Move {@link #pinfos(Path)} method to a separate class. | ||
| * Currently, this method violates Single Responsibility Principle of |
There was a problem hiding this comment.
Grammar issue: Missing article "the" before "Single Responsibility Principle". Should be "violates the Single Responsibility Principle of".
| * Currently, this method violates Single Responsibility Principle of | |
| * Currently, this method violates the Single Responsibility Principle of |
![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
🧹 Nitpick comments (3)
eo-integration-tests/src/it/fibonacci/invoker.properties (1)
5-5: Consider removing debug flag once the test is stable.The
-Xdebug flag provides verbose Maven output, which is helpful for troubleshooting a newly enabled test. However, it can clutter CI/CD logs significantly. Once the Fibonacci integration test runs reliably, consider removing the-Xflag to reduce log verbosity.eo-runtime/src/main/java/org/eolang/PhPackage.java (1)
125-134: Verify alignment of new TODO with PR objectives.This TODO is well-documented and describes a legitimate concern about checking
package-info.classexistence before object lookup. However, the PR objectives state: "Remove the puzzle text from the source code once the problem is fixed" for issue #4717.This change adds a new TODO referencing the same issue (#4717:90min). Please clarify:
- Is this documenting a different aspect of the issue that will be addressed separately?
- Should this TODO reference a new issue number instead?
Would you like me to help implement the
package-info.classexistence check suggested in this TODO, or should it be tracked in a separate issue?eo-maven-plugin/src/main/java/org/eolang/maven/MjTranspile.java (1)
262-264: Good SRP observation, but verify issue reference.The TODO correctly identifies that the
pinfosmethod violates Single Responsibility Principle. Extracting it to a separate class with unit tests would improve maintainability.However, this references issue #4717, which this PR is supposed to fix. If this is additional work beyond the scope of enabling the Fibonacci test, consider creating a separate issue to track this refactoring.
Would you like me to generate the code for extracting
pinfosto a separate class, or should this be deferred to a follow-up issue?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
eo-integration-tests/pom.xml(0 hunks)eo-integration-tests/src/it/fibonacci/invoker.properties(1 hunks)eo-integration-tests/src/it/fibonacci/pom.xml(2 hunks)eo-maven-plugin/src/main/java/org/eolang/maven/MjTranspile.java(3 hunks)eo-runtime/src/main/java/org/eolang/PhPackage.java(1 hunks)
💤 Files with no reviewable changes (1)
- eo-integration-tests/pom.xml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-12T15:24:05.011Z
Learnt from: h1alexbel
Repo: objectionary/eo PR: 4431
File: eo-parser/src/main/resources/XMIR.xsd:80-81
Timestamp: 2025-08-12T15:24:05.011Z
Learning: In the EO parser architecture, Φ̇ is an alias for Φ.org.eolang that gets expanded during XMIR decoration by expand-qqs.xsl. The XSD schema for FQN patterns should only validate canonical forms (Φ and ξ as roots) since alias expansion happens before XSD validation.
Applied to files:
eo-runtime/src/main/java/org/eolang/PhPackage.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: mvn (ubuntu-24.04, 23)
- GitHub Check: qulice
- GitHub Check: mvn (windows-2022, 23)
- GitHub Check: mvn (macos-15, 23)
- GitHub Check: integration
- GitHub Check: mvn (ubuntu-24.04, 17)
- GitHub Check: ort
- GitHub Check: benchmark
- GitHub Check: trufflehog
- GitHub Check: pr-size
- GitHub Check: snippets
🔇 Additional comments (5)
eo-maven-plugin/src/main/java/org/eolang/maven/MjTranspile.java (2)
275-290: LGTM! Improved observability for package-info.java generation.Capturing the result in the
savedvariable and logging it at debug level provides helpful visibility into package-info.java file creation. This is particularly useful when debugging integration tests like the newly enabled Fibonacci test.
294-297: LGTM! Clear informational logging.The info-level log when no generated sources exist provides clear feedback about why package-info.java creation is skipped. This helps prevent confusion during builds where the generated directory hasn't been created yet.
eo-integration-tests/src/it/fibonacci/pom.xml (3)
79-84: LGTM! Core change enabling the Fibonacci integration test.This execution is the primary change that enables the Fibonacci integration test by actually running the EO application during the test phase. The configuration correctly:
- Targets the test phase
- Uses exec-maven-plugin to run the Java class
- Executes
org.eolang.Mainwith appropriate arguments- Includes verbose output for debugging
This aligns perfectly with the PR objective to enable the Fibonacci integration test.
68-70: Verify new EO Maven plugin configuration properties.The configuration has been updated with new properties (
ignoreRuntime,failOnWarning,skipLinting) replacing older ones. These settings appear permissive:
ignoreRuntime=true- may skip runtime dependency checksfailOnWarning=false- allows build to continue with warningsskipLinting=true- disables lintingPlease confirm:
- Are these properties correctly named and supported in the current EO Maven plugin version?
- Are these permissive settings appropriate for an integration test, or should warnings and linting be enabled to catch issues?
EO Maven plugin latest configuration properties documentation
59-64: The "compile" goal is correct for compiling EO source to Java, but verify the complete execution flow is appropriate.The
compilegoal (from eo-maven-plugin) parses EO sources, applies optimizations, and produces Java bytecode. This is a legitimate change fromassemble, which serves a different purpose in the build pipeline.However, the execution sequence shown—
register,compile,transpile—is unusual. The typical recommended flow isregister→parse→optimize→compile. Verify:
- Is
transpilestill needed alongsidecompile?- Does this configuration correctly handle the full EO → XMIR → Java → bytecode transformation for the integration test?
2 participants
This PR enables the Fibonacci integration test.
Explanation
The
jcabi-parentpom has a specificmvn-compiler-pluginthat removes all thepackage-info.classfiles during compilation.This parent
pom.xmlis used init/fibonacci/pom.xml. That said, current version ofeo-runtimedepends on these files -> execution failsFixes #4717
Summary by CodeRabbit
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.