Fix the code quality errors in the tests#4591
Conversation
WalkthroughRemoves many private static final constants and some static fields from test classes across eo-maven-plugin and eo-runtime, replacing them with inline literals, small private helper methods, direct singleton references, or instance fields; one test switches from Changes
Sequence Diagram(s)No sequence diagram provided — changes are test-only refactors and do not modify runtime control flow. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related issues
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
eo-maven-plugin/src/test/java/org/eolang/maven/ChTextTest.java(2 hunks)eo-maven-plugin/src/test/java/org/eolang/maven/FpDefaultTest.java(17 hunks)eo-maven-plugin/src/test/java/org/eolang/maven/MjPlaceTest.java(10 hunks)eo-maven-plugin/src/test/java/org/eolang/maven/MjPullTest.java(5 hunks)eo-maven-plugin/src/test/java/org/eolang/maven/MjRegisterTest.java(2 hunks)eo-maven-plugin/src/test/java/org/eolang/maven/MjResolveTest.java(9 hunks)eo-maven-plugin/src/test/java/org/eolang/maven/OyIndexedTest.java(3 hunks)eo-runtime/src/test/java/EOorg/EOeolang/EOnet/EOsocketTest.java(7 hunks)eo-runtime/src/test/java/EOorg/EOeolang/HeapsTest.java(4 hunks)eo-runtime/src/test/java/org/eolang/PhDefaultTest.java(13 hunks)eo-runtime/src/test/java/org/eolang/PhPackageTest.java(3 hunks)
⏰ 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). (13)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: snippets
- GitHub Check: integration
- GitHub Check: bashate
- GitHub Check: mvn (windows-2022, 23)
- GitHub Check: mvn (macos-15, 23)
- GitHub Check: ort
- GitHub Check: markdown-lint
- GitHub Check: trufflehog
- GitHub Check: ebnf
- GitHub Check: snippets
- GitHub Check: integration
- GitHub Check: trufflehog
🔇 Additional comments (8)
eo-maven-plugin/src/test/java/org/eolang/maven/MjRegisterTest.java (2)
33-34: Looks good – inline literal matches the plugin parameter.Replacing the constant with the
"sourcesDir"literal keeps the test intent intact and avoids the extra indirection.
49-51: All good here as well.Passing the sources directory literal directly keeps the setup explicit and clear without changing behavior.
eo-runtime/src/test/java/org/eolang/PhDefaultTest.java (1)
62-157: String literal replacements look consistent.All occurrences of the old
PLUS_ATT/VOID_ATTconstants are now inlined with the correct"plus"and"void"names, and the surrounding assertions still match the intended behavior.eo-maven-plugin/src/test/java/org/eolang/maven/MjPlaceTest.java (2)
37-231: No issues spotted – literals align with expected paths.The inlined
"target/classes"strings keep the assertions readable and maintain the same semantics as the previous constants.
256-313: Helpers updated correctly.The utility methods now resolve
"target/classes"directly, staying consistent with the test expectations.eo-maven-plugin/src/test/java/org/eolang/maven/FpDefaultTest.java (3)
34-39: Constructor update mirrors the new signature.The inline
"Footprint content"supplier and extraPaths.get("")argument match the revisedFpDefaultconstructor contract.
50-112: Behavioral assertions remain valid with the new literals.Replacing the constants with string literals keeps the test intent unchanged and preserves the coverage of the SNAPSHOT/no-cache scenarios.
305-309: Helper uses the same literal supplier.Using
"Footprint content"directly keeps the helper consistent with the rest of the tests after the constant removal.
🚀 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
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
eo-runtime/src/test/java/EOorg/EOeolang/EOnk/EOsocketTest.java (3)
120-125: Consider making this method static for broader accessibility.The helper method is clean and well-documented. However, since it doesn't rely on instance state, making it
staticwould allow nested classes to access it more easily (e.g.,EOsocketTest.localhost()instead of using the literal"127.0.0.1"directly as seen in lines 501, 794, and 825).Apply this diff to make the method static:
/** * Returns the localhost address. */ - private String localhost() { + private static String localhost() { return "127.0.0.1"; }
143-143: Consider using a shared Random instance for better efficiency.Creating a new
Randominstance on each call is slightly inefficient. While acceptable for tests, consider using a staticRandominstance if this method is called frequently.Apply this diff to use a shared instance:
+ /** + * Shared random instance. + */ + private static final Random RND = new Random(); + /** * Get random port. * @return Random port */ private static int randomPort() { final int min = 10_000; final int max = 20_000; - return new Random().nextInt((max - min) + 1) + min; + return EOsocketTest.RND.nextInt((max - min) + 1) + min; }
501-501: Consider using the localhost helper for consistency.These lines use the hardcoded IP literal
"127.0.0.1"directly, while the outer class usesthis.localhost(). For consistency:
- Lines 501 and 794 (in non-static nested classes) could use
EOsocketTest.this.localhost()- Line 825 (in static nested class) would require making
localhost()static firstWhile the hardcoded IP is acceptable (per the
PMD.AvoidUsingHardCodedIPsuppression), centralizing via the helper would improve maintainability.Also applies to: 794-794, 825-825
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
eo-runtime/src/test/java/EOorg/EOeolang/EOnk/EOsocketTest.java(8 hunks)
🔇 Additional comments (1)
eo-runtime/src/test/java/EOorg/EOeolang/EOnk/EOsocketTest.java (1)
62-63: LGTM! Consistent use of the localhost helper.The refactoring to use
this.localhost()improves maintainability by centralizing the localhost address definition. The usage is consistent across all test methods in the outer class.Also applies to: 91-92, 102-102
|
@yegor256 I just resolved conflicts with master branch and the get the error in actions: |
|
@uchitsa this error doesn't break the build, ignore it (it's just a message in the log) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
eo-maven-plugin/src/test/java/org/eolang/maven/ChTextTest.java (1)
17-17: Remove unused import.The
@BeforeAllimport is no longer needed since the setup method now uses@BeforeEach.-import org.junit.jupiter.api.BeforeAll;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
eo-maven-plugin/src/test/java/org/eolang/maven/ChTextTest.java(3 hunks)
⏰ 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). (4)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: benchmark
- GitHub Check: ort
- GitHub Check: trufflehog
🔇 Additional comments (3)
eo-maven-plugin/src/test/java/org/eolang/maven/ChTextTest.java (3)
35-35: Good refactoring to instance field.Changing from a static field to an instance field correctly supports the per-test setup pattern and improves test isolation.
37-43: Excellent fix for the previously reported issues.The refactoring from
@BeforeAllto@BeforeEachcorrectly addresses the concerns raised in past review comments. Each test invocation (including each parameterized test) now gets its own temporary directory and file setup, ensuring thatthis.filepoints to an actual file created for that specific test.Based on past review comments.
69-78: Correct usage of per-test file path.The parameterized test now correctly uses
this.file, which is initialized in@BeforeEachfor each test invocation. This resolves the previous issue where the test was trying to read from a file that didn't exist in its temporary directory.Based on past review comments.
|
@yegor256 hello, PR is ready, finally |
|
@yegor256 FYI |
|
@yegor256 up |
|
@uchitsa what was the motivation for this? Who was complaining? It it was JTCop, we should enable it in this PR. Otherwise, most of these changes may be reverted back tomorrow and we won't notice. |
|
@yegor256 this #4585 (comment) and this #4182 |
|
@uchitsa that's what I'm saying: you need to increase the version of jtcop in this PR |
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 (1)
pom.xml(1 hunks)
⏰ 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: Codacy Static Code Analysis
- GitHub Check: mvn (windows-2022, 23)
- GitHub Check: mvn (macos-15, 23)
- GitHub Check: mvn (ubuntu-24.04, 23)
- GitHub Check: mvn (ubuntu-24.04, 17)
- GitHub Check: trufflehog
- GitHub Check: benchmark
- GitHub Check: qulice
- GitHub Check: integration
- GitHub Check: ort
- GitHub Check: snippets
| <groupId>com.github.volodya-lombrozo</groupId> | ||
| <artifactId>jtcop-maven-plugin</artifactId> | ||
| <version>1.3.5</version> | ||
| <version>1.4.1</version> |
There was a problem hiding this comment.
🧩 Analysis chain
Verify jtcop-maven-plugin 1.4.1 compatibility.
The version bump is aligned with the PR objectives (per yegor256's feedback to increase JTCop for code quality detection). Confirm that 1.4.1 is a stable release and compatible with the project's setup.
🌐 Web query:
Is jtcop-maven-plugin version 1.4.1 a valid and stable release? What are the release notes or breaking changes?
💡 Result:
Short answer: No public evidence that jtcop-maven-plugin version 1.4.1 is a published release. I couldn’t find a 1.4.1 artifact or release notes on Maven Central / Sonatype / GitHub—public usages and archives show versions like 1.1.1, 1.2.0 and 1.3.1 instead. [1][2][3]
Details / sources:
- Example builds pulling jtcop versions: 1.1.1 mentioned in documentation/tutorials. [1]
- Public project POMs reference 1.2.0. [2]
- CI logs / repositories show downloads for jtcop artefacts up through ~1.3.1 (no 1.4.1 found). [3]
Recommendation: If you expect 1.4.1, check the plugin’s official source repository (GitHub/owner account) or the OSS Sonatype staging/hosting for a recent release or tag. If you paste the plugin’s groupId/artifactId or a link, I can look up the exact Maven Central / GitHub pages and any changelog or breaking-change notes.
Sources:
[1] DZone “Setting up jtcop” (mentions 1.1.1).
[2] Project POM referencing com.github.volodya-lombrozo:jtcop-maven-plugin:1.2.0.
[3] CI/OSS logs showing jtcop artifacts (example: 1.3.1 downloads).
I need to verify more details about the jtcop-maven-plugin. Let me search for the actual latest released versions and the plugin's coordinates.
Revert jtcop-maven-plugin version to 1.3.1—version 1.4.1 does not exist on Maven Central.
The latest released version of com.github.volodya-lombrozo:jtcop-maven-plugin available on Maven Central is 1.3.1. Version 1.4.1 has no public release and the build will fail during dependency resolution. Update the version back to 1.3.1 or verify the intended version number.
🤖 Prompt for AI Agents
In pom.xml around line 505, the jtcop-maven-plugin version was set to 1.4.1
which is not published to Maven Central; change the plugin's version back to the
released 1.3.1 (or to the correct released version you intend to use) so Maven
can resolve the dependency; update the <version> value for
com.github.volodya-lombrozo:jtcop-maven-plugin to 1.3.1 and run mvn -U validate
to verify resolution.
|
@yegor256 updated, please check |
|
@uchitsa thanks! |
2 participants
Summary by CodeRabbit