ci(#4777): move several integration test classes#4778
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. 📝 WalkthroughWalkthroughAdds integration-test utilities and adjusts tests for the EO Maven plugin: two new package-private helper classes, test simplifications (fixed paths, in-method EO program), and a workflow update to exclude the new test files from simian analysis. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (12)
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 |
…to eo-integration-tests
volodya-lombrozo
force-pushed
the
4777-move-tests
branch
from
f1a42f0 to
415140b
Compare
🚀 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 moves several integration test classes (with *IT suffix) from eo-maven-plugin to eo-integration-tests module, resolving puzzle #4777. The tests are refactored to remove external dependencies like RandomProgramResolver and replace constant references with hardcoded strings.
Key changes:
- Moved and refactored
MjTranspileITandMjAssembleITtest classes to the integration tests module - Added helper classes
ContainsFilesandAppendedPlugin(duplicated from eo-maven-plugin as temporary solution) - Inlined random program generation logic to eliminate dependency on
RandomProgramResolver
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| eo-integration-tests/src/test/java/org/eolang/maven/package-info.java | Adds package documentation describing integration tests using Farea framework |
| eo-integration-tests/src/test/java/org/eolang/maven/MjTranspileIT.java | Updates test documentation and removes RandomProgramResolver dependency by inlining test program |
| eo-integration-tests/src/test/java/org/eolang/maven/MjAssembleIT.java | Replaces constant references (MjParse.DIR, MjPull.DIR, MjAssemble.XMIR/EO) with hardcoded strings |
| eo-integration-tests/src/test/java/org/eolang/maven/ContainsFiles.java | Adds Hamcrest matcher for file glob patterns (duplicate from eo-maven-plugin) |
| eo-integration-tests/src/test/java/org/eolang/maven/AppendedPlugin.java | Adds helper class to configure EO Maven plugin in Farea tests (duplicate from eo-maven-plugin) |
| .github/workflows/simian.yml | Adds exclusions for duplicated test helper classes |
Comments suppressed due to low confidence (9)
eo-integration-tests/src/test/java/org/eolang/maven/MjTranspileIT.java:137
- This test method contains two assertions, violating the project guideline that each test should contain only one assertion. Consider splitting this test into two separate test methods.
eo-integration-tests/src/test/java/org/eolang/maven/MjAssembleIT.java:34 - The hardcoded string "eo" replaces the constant MjAssemble.EO. This reduces maintainability because if the file extension changes in the MjAssemble class, these tests will not reflect that change. Consider adding a reference to MjAssemble.EO or defining a test constant.
eo-integration-tests/src/test/java/org/eolang/maven/MjAssembleIT.java:61 - This test method contains two assertions, violating the project guideline that each test should contain only one assertion. Consider splitting this test into two separate test methods.
eo-integration-tests/src/test/java/org/eolang/maven/MjTranspileIT.java:137 - This test method contains two assertions, violating the project guideline that each test should contain only one assertion. Consider splitting this test into two separate test methods.
eo-integration-tests/src/test/java/org/eolang/maven/MjAssembleIT.java:33 - The hardcoded string "1-parse" replaces the constant MjParse.DIR. This reduces maintainability because if the directory name changes in the MjParse class, these tests will not reflect that change. Consider adding a reference to MjParse.DIR or defining a test constant.
eo-integration-tests/src/test/java/org/eolang/maven/MjAssembleIT.java:34 - The hardcoded string "2-pull" replaces the constant MjPull.DIR. This reduces maintainability because if the directory name changes in the MjPull class, these tests will not reflect that change. Consider adding a reference to MjPull.DIR or defining a test constant.
eo-integration-tests/src/test/java/org/eolang/maven/MjAssembleIT.java:33 - The hardcoded string "xmir" replaces the constant MjAssemble.XMIR. This reduces maintainability because if the file extension changes in the MjAssemble class, these tests will not reflect that change. Consider adding a reference to MjAssemble.XMIR or defining a test constant.
eo-integration-tests/src/test/java/org/eolang/maven/MjAssembleIT.java:86 - The hardcoded string "1-parse" replaces the constant MjParse.DIR. This reduces maintainability because if the directory name changes in the MjParse class, these tests will not reflect that change. Consider adding a reference to MjParse.DIR or defining a test constant.
eo-integration-tests/src/test/java/org/eolang/maven/MjAssembleIT.java:87 - The hardcoded string "xmir" replaces the constant MjAssemble.XMIR. This reduces maintainability because if the file extension changes in the MjAssemble class, these tests will not reflect that change. Consider adding a reference to MjAssemble.XMIR or defining a test constant.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| java-version: 17 | ||
| - run: wget --quiet http://public.yegor256.com/simian.jar -O /tmp/simian.jar | ||
| - run: java -jar /tmp/simian.jar -threshold=15 "-excludes=**/EOsocketTest.java" "-excludes=**/gen" "-excludes=**/it" "**/*.java" | ||
| - run: java -jar /tmp/simian.jar -threshold=15 "-excludes=**/EOsocketTest.java" "-excludes=**/gen" "-excludes=**/it" "**/*.java" "-excludes=**/AppendedPlugin.java" "-excludes=**/ContainsFiles.java" |
There was a problem hiding this comment.
The exclusion patterns for AppendedPlugin.java and ContainsFiles.java should use path patterns (e.g., "**AppendedPlugin.java") to properly exclude files. The current syntax may not correctly match these files in subdirectories.
| - run: java -jar /tmp/simian.jar -threshold=15 "-excludes=**/EOsocketTest.java" "-excludes=**/gen" "-excludes=**/it" "**/*.java" "-excludes=**/AppendedPlugin.java" "-excludes=**/ContainsFiles.java" | |
| - run: java -jar /tmp/simian.jar -threshold=15 "-excludes=**/EOsocketTest.java" "-excludes=**/gen" "-excludes=**/it" "**/*.java" "-excludes=**AppendedPlugin.java" "-excludes=**ContainsFiles.java" |
![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
🧹 Nitpick comments (3)
eo-integration-tests/src/test/java/org/eolang/maven/AppendedPlugin.java (1)
16-21: Track the TODO for removing duplicate classes.The TODO indicates that this class is duplicated in the
eo-maven-pluginmodule and should be removed after all integration tests are moved. Ensure this is tracked in the issue management system to avoid maintaining duplicate code long-term.Do you want me to create a follow-up issue to track the removal of duplicate
AppendedPluginclasses once all integration tests are migrated?eo-integration-tests/src/test/java/org/eolang/maven/ContainsFiles.java (2)
18-22: Track the TODO for removing duplicate classes.The TODO indicates this class is duplicated in the
eo-maven-pluginmodule. Consider creating a shared test utilities module or keeping a single copy in a common location to avoid maintaining duplicate code.Do you want me to create a follow-up issue to track the consolidation of duplicate test utility classes?
65-88: Consider performance implications for large directory trees.The
Files.walk(item)traversal processes the entire directory tree for each glob pattern. While acceptable for typical integration test scenarios with small directory structures, this could be slow if test directories grow large.💡 Optional optimization for large directories
Consider using
Files.find()with maxDepth limits or implementing early termination:private static boolean matchesGlob(final Path item, final String glob) { try { - return Files.walk(item) + return Files.find( + item, + Integer.MAX_VALUE, + (path, attrs) -> FileSystems + .getDefault() + .getPathMatcher(String.format("glob:%s", glob)) + .matches(path) + ) + .findFirst() + .isPresent(); - .anyMatch( - FileSystems - .getDefault() - .getPathMatcher( - String.format( - "glob:%s", - glob - ) - )::matches - ); } catch (final IOException ex) {This allows early termination when the first match is found.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/simian.ymleo-integration-tests/src/test/java/org/eolang/maven/AppendedPlugin.javaeo-integration-tests/src/test/java/org/eolang/maven/ContainsFiles.javaeo-integration-tests/src/test/java/org/eolang/maven/MjAssembleIT.javaeo-integration-tests/src/test/java/org/eolang/maven/MjTranspileIT.javaeo-integration-tests/src/test/java/org/eolang/maven/package-info.java
🧰 Additional context used
🧬 Code graph analysis (1)
eo-integration-tests/src/test/java/org/eolang/maven/MjTranspileIT.java (1)
eo-maven-plugin/src/test/java/org/eolang/maven/MjTranspileIT.java (1)
Test(93-133)
⏰ 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). (11)
- GitHub Check: pr-size
- GitHub Check: qulice
- GitHub Check: mvn (windows-2022, 23)
- GitHub Check: snippets
- GitHub Check: benchmark
- GitHub Check: integration
- GitHub Check: mvn (ubuntu-24.04, 17)
- GitHub Check: mvn (ubuntu-24.04, 23)
- GitHub Check: mvn (macos-15, 23)
- GitHub Check: trufflehog
- GitHub Check: ort
🔇 Additional comments (7)
.github/workflows/simian.yml (1)
24-24: LGTM! Appropriate exclusions for test utilities.The exclusions for
AppendedPlugin.javaandContainsFiles.javaare correctly added to prevent duplication warnings for these test helper classes, which align with the TODOs in those files acknowledging the temporary duplication.eo-integration-tests/src/test/java/org/eolang/maven/package-info.java (1)
1-10: LGTM! Clear package documentation.The package-info provides helpful context about the integration tests and the Farea framework usage.
eo-integration-tests/src/test/java/org/eolang/maven/MjAssembleIT.java (2)
33-34: Verify that hard-coded directory names are stable conventions.The code now uses hard-coded strings
"1-parse","xmir","2-pull", and"eo"instead of referencing constants. Ensure these directory names are stable conventions in the EO Maven plugin and unlikely to change, as changes would require updating these string literals across integration tests.
86-87: LGTM! Appropriate use of ContainsFiles matcher.The test correctly uses the new
ContainsFilesmatcher to verify that the parsed XMIR file exists even when the EO program is invalid.eo-integration-tests/src/test/java/org/eolang/maven/MjTranspileIT.java (3)
21-21: LGTM! Updated documentation is clearer.The comment now explicitly references the
eo-maven-plugin:transpilegoal, making the test's purpose more obvious.
26-26: LGTM! Test simplification.Removing
RandomProgramResolversimplifies the test infrastructure while maintaining test coverage, especially if the program content was not meaningfully randomized.
88-99: LGTM! Program construction moved inline.Moving the EO program construction into the test method makes the test more self-contained and easier to understand.
| public Execution value() throws IOException { | ||
| return this.farea.build() | ||
| .plugins() | ||
| .append( | ||
| "org.eolang", | ||
| "eo-maven-plugin", | ||
| System.getProperty( | ||
| "eo.version", | ||
| Manifests.read("EO-Version") | ||
| ) | ||
| ).execution(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check for eo.version property definitions and EO-Version manifest references
rg "eo\.version" -C3Repository: objectionary/eo
Length of output: 5843
🏁 Script executed:
# Check for EO-Version manifest entries
rg "EO-Version" -C3Repository: objectionary/eo
Length of output: 13757
🏁 Script executed:
# Look for the Manifests class to understand how it reads manifest files
fd -e java | xargs rg -l "class Manifests|Manifests.read" | head -5Repository: objectionary/eo
Length of output: 361
🏁 Script executed:
# Check pom.xml for version properties and manifest configuration
find . -name "pom.xml" | xargs rg -l "eo.version|EO-Version" | head -5Repository: objectionary/eo
Length of output: 99
🏁 Script executed:
# Find and examine eo-integration-tests pom.xml for version and manifest configuration
find . -path "*/eo-integration-tests/pom.xml" -exec cat {} \;Repository: objectionary/eo
Length of output: 8034
🏁 Script executed:
# Find the Manifests class to understand how it reads manifest entries
fd -e java -type f | xargs rg -l "class Manifests|public static.*read"Repository: objectionary/eo
Length of output: 231
🏁 Script executed:
# Check if eo-integration-tests inherits system property from parent
find . -path "*/eo-integration-tests/pom.xml" -o -path "*/pom.xml" | head -1 | xargs cat | grep -A 20 "systemPropertyVariables"Repository: objectionary/eo
Length of output: 41
🏁 Script executed:
# Search for Manifests class definition and usage pattern
rg -l "class Manifests" --type=javaRepository: objectionary/eo
Length of output: 41
🏁 Script executed:
# Check the manifest file in eo-integration-tests test resources
cat eo-integration-tests/src/test/resources/META-INF/MANIFEST.MFRepository: objectionary/eo
Length of output: 138
🏁 Script executed:
# Verify that eo-integration-tests test manifest is accessible during tests
find eo-integration-tests -name "MANIFEST.MF" -o -name "pom.xml" | grep -E "test/resources|pom.xml"Repository: objectionary/eo
Length of output: 194
🏁 Script executed:
# Check if Maven filters test resources for eo-integration-tests
grep -r "filtering" eo-integration-tests/pom.xmlRepository: objectionary/eo
Length of output: 41
🏁 Script executed:
# Check the parent pom (eo-parent) for system property or resource configuration
find . -path "*/eo-parent/pom.xml" -exec head -100 {} \; | grep -A 30 "systemPropertyVariables"Repository: objectionary/eo
Length of output: 41
🏁 Script executed:
# Verify if the manifest placeholder is properly handled during build
rg "filtering.*true" eo-integration-testsRepository: objectionary/eo
Length of output: 41
🏁 Script executed:
# Find and examine the parent pom.xml (eo-parent)
find . -name "eo-parent" -type d -o -name "pom.xml" | xargs grep -l "eo-parent" | head -1 | xargs dirname | xargs find . -name "pom.xml" -path "*/eo-parent/*" | head -1 | xargs cat | head -150Repository: objectionary/eo
Length of output: 177
🏁 Script executed:
# Check how parent pom configures test resources and system properties
rg "eo-parent" -A 200 | head -100Repository: objectionary/eo
Length of output: 4330
🏁 Script executed:
# Look at the actual parent pom location
git ls-files | grep "pom.xml" | grep -i parentRepository: objectionary/eo
Length of output: 41
🏁 Script executed:
# Check if parent pom configures test resource filtering
cat pom.xml | grep -A 50 "build"Repository: objectionary/eo
Length of output: 6664
🏁 Script executed:
# Search for test resources configuration in parent or child
rg "testResources|filtering" pom.xml -A 5Repository: objectionary/eo
Length of output: 41
🏁 Script executed:
# Check if there are any issues or comments about the manifest in the codebase
rg "manifest|EO-Version" eo-integration-tests/pom.xml -C 5Repository: objectionary/eo
Length of output: 377
🏁 Script executed:
# Check if test resources are configured with filtering in any pom
rg "testResources" -A 10 pom.xml eo-integration-tests/pom.xmlRepository: objectionary/eo
Length of output: 41
🏁 Script executed:
# Check if Maven properties filtering is configured for the project
rg "maven-resources-plugin" -C 5Repository: objectionary/eo
Length of output: 612
🏁 Script executed:
# Check eo-maven-plugin's pom for system property configuration
cat eo-maven-plugin/pom.xml | grep -A 20 "systemPropertyVariables"Repository: objectionary/eo
Length of output: 915
🏁 Script executed:
# Verify the AppendedPlugin in eo-maven-plugin module to see if it inherits system property from there
cat eo-maven-plugin/src/test/java/org/eolang/maven/AppendedPlugin.java | head -60Repository: objectionary/eo
Length of output: 1081
🏁 Script executed:
# Check if there's a shared test base or superclass for AppendedPlugin
rg "class AppendedPlugin" -A 5 eo-maven-plugin/Repository: objectionary/eo
Length of output: 591
Version resolution will fail in test execution due to unfiltered manifest placeholder.
The eo.version system property is not set in the eo-integration-tests module build, leaving Manifests.read("EO-Version") as the sole fallback. However, the test manifest at src/test/resources/META-INF/MANIFEST.MF contains an unfiltered placeholder (EO-Version: ${project.version}) that will return the literal string "${project.version}" instead of an actual version number. This will cause test failures when the plugin version is resolved.
Either configure Maven resource filtering to replace the placeholder in test resources, or add eo.version to the module's systemPropertyVariables in the surefire/failsafe plugin configuration.
🤖 Prompt for AI Agents
In eo-integration-tests/src/test/java/org/eolang/maven/AppendedPlugin.java
around lines 38 to 49, the code falls back to reading EO-Version from the test
MANIFEST which contains an unfiltered placeholder "${project.version}", causing
version resolution to fail; fix by ensuring the plugin version is provided at
test runtime — either add a concrete eo.version system property to the module's
surefire/failsafe configuration (systemPropertyVariables.eo.version) or enable
Maven resource filtering for src/test/resources/META-INF/MANIFEST.MF so the
EO-Version placeholder is replaced, and then update the test build configuration
accordingly so Manifests.read("EO-Version") returns a real version string.
|
@yegor256 Could you have a look, please? If the reason isn't clear, please let me know. I will try to improve the description. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This PR resolves puzzle
4718-a792fc10by moving several*ITintegration tests fromeo-maven-plugintoeo-integration-tests.Explanation:
We have
*ITtests ineo-maven-plugin. They useFareato testeo-maven-plugin. Here is the issue. When we runmvn clean test(insideeo-maven-plugin) ->*ITtest starts -> they look foreo-maven-plugininstalled on the system -> they test against the previously installedeo-maven-plugin, not the current one. Thus, all integration tests ineo-maven-plugintest nothing for now.Fixes #4777
Summary by CodeRabbit
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.