test(#4526): add tests for FpDefault#4739
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. WalkthroughConverted two test helper methods to static, tightened visibility of an inner test helper class, updated all call sites, added two new disabled cache-related tests, and marked a flaky remote probe test as Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested labels
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 |
volodya-lombrozo
force-pushed
the
4526-caching-test
branch
from
8099daf to
64336ca
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: 2
📜 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/FpDefaultTest.java(19 hunks)
🔇 Additional comments (4)
eo-maven-plugin/src/test/java/org/eolang/maven/FpDefaultTest.java (4)
18-18: LGTM!The import is correctly added to support the
@Disabledannotation used in the new test methods.
25-32: LGTM!The TODO comment provides clear context about the caching limitation and references the relevant issue and test cases.
351-360: LGTM!Converting these helper methods to static is appropriate since they don't depend on instance state, and all call sites have been correctly updated throughout the file.
497-535: LGTM!The refactoring improves the Cache helper class by making it static final (since it doesn't use outer class instance state), making fields final for immutability, and adjusting visibility to package-private which is appropriate for internal test scaffolding.
There was a problem hiding this comment.
Pull request overview
This PR refactors the FpDefaultTest class by converting instance methods to static methods and adds two disabled tests that demonstrate a caching issue with SNAPSHOT and 0.0.0 versions in the EO runtime. The tests are disabled because they currently fail, highlighting that the caching mechanism doesn't work for development versions.
- Refactored helper methods and the Cache inner class to be static, removing unnecessary instance method references
- Added comprehensive TODO documentation explaining the caching limitation with SNAPSHOT and 0.0.0 versions
- Added two disabled tests (
usesCacheEvenItSnapshotandcachesEvenItZeroVersion) demonstrating the expected but currently failing behavior
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| void usesCacheEvenItSnapshot(@Mktmp final Path temp) throws Exception { | ||
| final Path source = FpDefaultTest.existedSource(temp); | ||
| final Path target = FpDefaultTest.notExistedTarget(temp); | ||
| final String version = "1.0.1"; |
There was a problem hiding this comment.
The test name and assertion message suggest this test is validating SNAPSHOT version behavior, but the version used is "1.0.1", which is a regular release version. For testing SNAPSHOT behavior, the version should contain "SNAPSHOT" (e.g., "1.0-SNAPSHOT") to properly validate the intended scenario.
| final String version = "1.0.1"; | |
| final String version = "1.0-SNAPSHOT"; |
| MatcherAssert.assertThat( | ||
| "We expect that cache is used even for SNAPSHOT version", | ||
| new TextOf(target).asString(), | ||
| Matchers.allOf( | ||
| Matchers.equalTo(FpDefaultTest.cacheContent()), | ||
| Matchers.not(Matchers.equalTo(FpDefaultTest.footprintContent())) | ||
| ) | ||
| ); |
There was a problem hiding this comment.
According to the custom coding guideline, tests should contain only one assertion. This test uses Matchers.allOf which combines two matchers into a single assertion call. Consider splitting this into a single, focused assertion that validates the primary behavior: that the cache content is used.
| MatcherAssert.assertThat( | ||
| "We expect that cache is used even for 0.0.0 version", | ||
| new TextOf(target).asString(), | ||
| Matchers.allOf( | ||
| Matchers.equalTo(FpDefaultTest.cacheContent()), | ||
| Matchers.not(Matchers.equalTo(FpDefaultTest.footprintContent())) | ||
| ) | ||
| ); |
There was a problem hiding this comment.
According to the custom coding guideline, tests should contain only one assertion. This test uses Matchers.allOf which combines two matchers into a single assertion call. Consider splitting this into a single, focused assertion that validates the primary behavior: that the cache content is used.
|
|
||
| @Test | ||
| @Disabled | ||
| void usesCacheEvenItSnapshot(@Mktmp final Path temp) throws Exception { |
There was a problem hiding this comment.
The test name has a grammatical error. It should be "usesCacheEvenIfItIsSnapshot" or similar to properly convey the test intent with correct grammar.
| void usesCacheEvenItSnapshot(@Mktmp final Path temp) throws Exception { | |
| void usesCacheEvenIfItIsSnapshot(@Mktmp final Path temp) throws Exception { |
|
|
||
| @Test | ||
| @Disabled | ||
| void cachesEvenItZeroVersion(@Mktmp final Path temp) throws Exception { |
There was a problem hiding this comment.
The test name has a grammatical error. It should be "cachesEvenIfItIsZeroVersion" or similar to properly convey the test intent with correct grammar.
🚀 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: |
volodya-lombrozo
force-pushed
the
4526-caching-test
branch
from
64336ca to
ec0f732
Compare
…sion caching behavior
volodya-lombrozo
force-pushed
the
4526-caching-test
branch
from
ec0f732 to
688d1e7
Compare
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 (2)
eo-maven-plugin/src/test/java/org/eolang/maven/FpDefaultTest.java(19 hunks)eo-maven-plugin/src/test/java/org/eolang/maven/MjProbeTest.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- eo-maven-plugin/src/test/java/org/eolang/maven/FpDefaultTest.java
🧰 Additional context used
🧬 Code graph analysis (1)
eo-maven-plugin/src/test/java/org/eolang/maven/MjProbeTest.java (1)
eo-maven-plugin/src/test/java/org/eolang/maven/MjLintIT.java (1)
Disabled(37-101)
⏰ 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)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: mvn (ubuntu-24.04, 17)
- GitHub Check: benchmark
- GitHub Check: mvn (windows-2022, 23)
- GitHub Check: mvn (ubuntu-24.04, 23)
- GitHub Check: mvn (macos-15, 23)
- GitHub Check: trufflehog
- GitHub Check: ort
- GitHub Check: snippets
- GitHub Check: integration
- GitHub Check: pr-size
- GitHub Check: qulice
🔇 Additional comments (1)
eo-maven-plugin/src/test/java/org/eolang/maven/MjProbeTest.java (1)
15-15: LGTM!The import is necessary for the
@Disabledannotation used below.
| /** | ||
| * Finds probes in objectionary remote repository. | ||
| * @param temp Temporary folder | ||
| * @throws IOException If some problem inside | ||
| * @todo #4526:90min Fix flaky {@link #findsProbesInOyRemote(Path)} test. | ||
| * The test sometimes fails with the following error: | ||
| * MjProbeTest.findsProbesInOyRemote:62 We should find 10 objects in | ||
| * git repository with tag '0.50.0', but 9 found. | ||
| * This might happen because the remote repository structure changes over time. | ||
| * We need to investigate this issue and fix the test to make it stable. | ||
| */ | ||
| @Test | ||
| @Disabled |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if FpDefaultTest exists and search for related changes
fd -e java "FpDefaultTest"
# Search for @Disabled tests to understand the pattern
rg -n "@Disabled" --type=java -B 2 | head -100Repository: objectionary/eo
Length of output: 6836
🏁 Script executed:
# Examine FpDefaultTest to understand the caching context and PR scope
cat -n eo-maven-plugin/src/test/java/org/eolang/maven/FpDefaultTest.java | head -50
# Check if MjProbeTest also mentions issue #4526
rg -n "4526" eo-maven-plugin/src/test/java/org/eolang/maven/MjProbeTest.java
# Check what other changes are in MjProbeTest around the disabled test
cat -n eo-maven-plugin/src/test/java/org/eolang/maven/MjProbeTest.java | sed -n '45,70p'Repository: objectionary/eo
Length of output: 3558
🏁 Script executed:
# Check if MjProbeTest file shows any other disabled tests in same PR
rg -n "@Disabled|@Test" eo-maven-plugin/src/test/java/org/eolang/maven/MjProbeTest.java | head -30
# Check MjLintIT mentioned in original comment to understand the "pattern"
fd -e java "MjLintIT" -x cat -n {} | head -40Repository: objectionary/eo
Length of output: 2088
Clear documentation and appropriate short-term workaround, but remote test inherently fragile.
The Javadoc clearly explains why the test is disabled, and the TODO provides tracking. However, some clarification:
-
PR scope is coherent: This PR addresses multiple aspects of issue Compilation caching question #4526—both the SNAPSHOT caching issue (in
FpDefaultTest) and the flaky remote repository probe test (inMjProbeTest). The apparent scope discrepancy in the original review is resolved by examining the full context. -
Remote test fragility is inherent: The test depends on a remote Git repository with tag
0.50.0containing exactly 10 objects. This is inherently fragile because external repositories change over time. While disabling with clear documentation is a reasonable short-term approach, consider these longer-term improvements:- Using a local test fixture instead of a remote repository
- Mocking the remote repository behavior
- Making expectations more flexible (e.g., checking for specific high-value objects rather than an exact count)
The approach of disabling the test with clear TODO documentation is appropriate for now, but plan to make this integration test more resilient rather than leaving it disabled indefinitely.
|
@yegor256 could you have a look, please? |
|
@volodya-lombrozo if we cache results while eo-maven-plugin is in the |
@yegor256 sorry, could you explain it in detail, please? How changing code in the Actually I have already removed this functionality here and all tests pass. |
|
@volodya-lombrozo imagine, you are a developer of |
@yegor256 which tests? If there are some tests that require eo files compilation - they should be clearly configured with disabled cache. Moreover, I can't find such tests |
|
@volodya-lombrozo ok, this part make sense:
|
This PR refactors the
FpDefaultTestclass and add two disabled tests that show that caching doesn't work foreo-runtime:1.0-SNAPSHOT. In other words, when we developeo-runtimewe are required to rebuild all classes every time we want just to run a test (even without any changes).Related to #4526
Summary by CodeRabbit
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.