feat(#4702): move fibonacci integration test to eo-integraton-tests module#4715
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. WalkthroughModifies Maven build and integration-test setup: adds maven-invoker-plugin to eo-integration-tests and removes it from eo-maven-plugin, updates fibonacci integration test pom and invoker properties, and adds SPDX header and package declaration to the fibonacci EO source. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
🚀 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 migrates the Fibonacci integration test from the eo-maven-plugin module to the new eo-integration-tests module, consolidating integration tests in a dedicated location. The test is currently disabled due to runtime issues with class/package resolution.
Key changes:
- Removed maven-invoker-plugin configuration from eo-maven-plugin that excluded the fibonacci test
- Added maven-invoker-plugin configuration to eo-integration-tests with fibonacci test temporarily excluded
- Added eo-runtime dependency to the fibonacci test to resolve ClassNotFoundException issues
Reviewed changes
Copilot reviewed 5 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| eo-maven-plugin/pom.xml | Removed maven-invoker-plugin configuration that was excluding fibonacci test |
| eo-integration-tests/pom.xml | Added maven-invoker-plugin configuration with fibonacci test temporarily excluded pending resolution of class resolution issues |
| eo-integration-tests/src/it/settings.xml | Added Maven settings configuration for integration tests with local repository setup |
| eo-integration-tests/src/it/fibonacci/pom.xml | Added eo-runtime dependency and configured Surefire to skip tests, includes documentation of known issues |
| eo-integration-tests/src/it/fibonacci/verify.groovy | Added Groovy verification script to validate test execution and generated artifacts |
| eo-integration-tests/src/it/fibonacci/invoker.properties | Removed debug flag (-X) from Maven goals |
| eo-integration-tests/src/it/fibonacci/src/main/eo/org/eolang/examples/fibonacci.eo | Reordered architect metadata line to follow package declaration |
| eo-integration-tests/src/it/fibonacci/src/main/eo/org/eolang/examples/app.eo | Added EO application source file for fibonacci calculator |
| eo-integration-tests/src/it/fibonacci/README.md | Added documentation explaining how to use the fibonacci example as a template |
| eo-integration-tests/src/it/README.md | Added overview documentation for integration tests directory |
Comments suppressed due to low confidence (2)
eo-integration-tests/src/it/fibonacci/pom.xml:48
- Capitalization inconsistency: "surefire plugin" should be "Surefire plugin" to match Maven's official plugin naming convention.
eo-integration-tests/src/it/fibonacci/pom.xml:50 - Grammar error: "don't" should be "not" in this context. Should read "We should either not include EO tests in the eo-runtime jar".
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @todo #4702:90min Enable the fibonacci integration test. | ||
| Currently, the fibonacci integration test is disabled because it | ||
| fails with the following error: | ||
| Couldn't find object 'Φ.org.eolang.examples' because there's |
There was a problem hiding this comment.
Grammar error: "there's" should be "there is" for consistency with formal documentation style.
| Couldn't find object 'Φ.org.eolang.examples' because there's | |
| Couldn't find object 'Φ.org.eolang.examples' because there is |
…ation-test module
cf560aa to
c38affd
Compare
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
eo-integration-tests/pom.xml(1 hunks)eo-integration-tests/src/it/fibonacci/invoker.properties(1 hunks)eo-integration-tests/src/it/fibonacci/pom.xml(2 hunks)eo-integration-tests/src/it/fibonacci/src/main/eo/org/eolang/examples/fibonacci.eo(1 hunks)eo-maven-plugin/pom.xml(0 hunks)
💤 Files with no reviewable changes (1)
- eo-maven-plugin/pom.xml
🚧 Files skipped from review as they are similar to previous changes (2)
- eo-integration-tests/pom.xml
- eo-integration-tests/src/it/fibonacci/invoker.properties
⏰ 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). (10)
- GitHub Check: qulice
- GitHub Check: ort
- GitHub Check: benchmark
- GitHub Check: mvn (windows-2022, 23)
- GitHub Check: integration
- GitHub Check: mvn (ubuntu-24.04, 17)
- GitHub Check: trufflehog
- GitHub Check: mvn (macos-15, 23)
- GitHub Check: mvn (ubuntu-24.04, 23)
- GitHub Check: pr-size
🔇 Additional comments (3)
eo-integration-tests/src/it/fibonacci/src/main/eo/org/eolang/examples/fibonacci.eo (1)
1-4: LGTM! Metadata additions are correct.The SPDX headers, package declaration, and architect information are properly formatted and consistent with the file path structure.
eo-integration-tests/src/it/fibonacci/pom.xml (2)
18-30: LGTM! Dependencies correctly address the ClassNotFound issue.The
eo-runtimedependency resolves the ClassNotFound exception mentioned in the PR objectives, andjunit-jupiter-apiis appropriately scoped for testing.
31-34: LGTM! Compiler properties are appropriate.Java 11 is a reasonable baseline for this project, and the source/target settings are consistent.
| <!-- | ||
| @todo #4702:90min EO tests are distributed with the eo-runtime library. | ||
| Currently, when we add 'eo-runtime' as a dependency, the surefire plugin | ||
| tries to run its tests as well, which takes significant time. | ||
| And it's rathter unexpected behavior when we just need to run an EO app. | ||
| We should either don't include EO tests in the eo-runtime jar, | ||
| or state it clearly in the documentation that when you add eo-runtime | ||
| as a dependency, you also get its tests. | ||
| --> | ||
| <skip>true</skip> | ||
| </configuration> |
There was a problem hiding this comment.
Fix typo in TODO comment.
Line 50 contains a typo: "rathter" should be "rather".
Apply this diff to fix the typo:
- And it's rathter unexpected behavior when we just need to run an EO app.
+ And it's rather unexpected behavior when we just need to run an EO app.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <!-- | |
| @todo #4702:90min EO tests are distributed with the eo-runtime library. | |
| Currently, when we add 'eo-runtime' as a dependency, the surefire plugin | |
| tries to run its tests as well, which takes significant time. | |
| And it's rathter unexpected behavior when we just need to run an EO app. | |
| We should either don't include EO tests in the eo-runtime jar, | |
| or state it clearly in the documentation that when you add eo-runtime | |
| as a dependency, you also get its tests. | |
| --> | |
| <skip>true</skip> | |
| </configuration> | |
| <!-- | |
| @todo #4702:90min EO tests are distributed with the eo-runtime library. | |
| Currently, when we add 'eo-runtime' as a dependency, the surefire plugin | |
| tries to run its tests as well, which takes significant time. | |
| And it's rather unexpected behavior when we just need to run an EO app. | |
| We should either don't include EO tests in the eo-runtime jar, | |
| or state it clearly in the documentation that when you add eo-runtime | |
| as a dependency, you also get its tests. | |
| --> | |
| <skip>true</skip> | |
| </configuration> |
🤖 Prompt for AI Agents
In eo-integration-tests/src/it/fibonacci/pom.xml around lines 46 to 56, the TODO
comment contains a typo: change "rathter" to "rather" in the comment text;
update the comment line so it reads "rather unexpected behavior" and commit the
corrected pom.xml.
|
@yegor256 Could you review this one, please? Maybe, I'm fundamentally wrong - I suppose that any EO program should depend on |
In this PR I:
fibonacciintegration test toeo-integration-testsmodule (it's necessary because thefibonacciintegration test depends oneo-runtime)eo-runtimedependency to this test, because without it we gotClassNotFoundexceptionfibonaccitest fail (see new puzzles)Closes #4702, #4701
Summary by CodeRabbit
Chores
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.