Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 13 additions & 5 deletions eo-maven-plugin/src/main/java/org/eolang/maven/MjTranspile.java
Original file line number Diff line number Diff line change
Expand Up @@ -173,26 +173,34 @@ private int transpiled(
final Supplier<String> hsh = new TojoHash(tojo);
final AtomicBoolean rewrite = new AtomicBoolean(false);
final Function<XML, XML> transform = this.transpilation(source);
final String version = this.plugin.getVersion();
final Path tail = base.relativize(target);
final Path cdir = this.cache.toPath().resolve(MjTranspile.CACHE);
new FpDefault(
Comment on lines +176 to 179

Copilot AI Feb 2, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Local name cdir is an abbreviated compound; per project guideline prefer a single-word noun for locals. Consider renaming to cache/dir (or similar) to avoid abbreviations and compound naming.

Copilot generated this review using guidance from organization custom instructions.
src -> {
rewrite.compareAndSet(false, true);
final long start = System.currentTimeMillis();
final String res = transform.apply(xmir).toString();
Logger.debug(
this,
"Transpiled %[file]s (%s) to %[file]s (%s) in %[ms]s (cache miss)",
"Transpiled %[file]s (%s) to %[file]s (%s) in %[ms]s (cache miss), version: %s, hash: %s, tail: %s, cache enabled: %b, cache dir: %[file]s",
source,
MjTranspile.info(source),
target,
MjTranspile.info(target),
System.currentTimeMillis() - start
System.currentTimeMillis() - start,
version,
hsh.get(),
tail,
this.cacheEnabled,
cdir
Comment on lines 184 to +196

Copilot AI Feb 2, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The debug message always says “(cache miss)”, but this lambda can also run when caching is disabled (cache enabled: false). This can be misleading when diagnosing builds. Consider adjusting the wording to reflect both cases (e.g., “cache miss or disabled”) or emitting different messages depending on cacheEnabled.

Copilot uses AI. Check for mistakes.
);
return res;
},
this.cache.toPath().resolve(MjTranspile.CACHE),
this.plugin.getVersion(),
cdir,
version,
hsh,
base.relativize(target),
tail,
this.cacheEnabled
).apply(source, target);
return this.javaGenerated(rewrite.get(), target, hsh.get());
Expand Down
89 changes: 89 additions & 0 deletions eo-maven-plugin/src/test/java/org/eolang/maven/FpDefaultTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import com.yegor256.Mktmp;
import com.yegor256.MktmpResolver;
import java.io.IOException;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
Expand All @@ -15,6 +16,7 @@
import org.hamcrest.MatcherAssert;
import org.hamcrest.Matchers;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;

Expand Down Expand Up @@ -336,6 +338,93 @@ void cachesEvenItIsZeroVersion(@Mktmp final Path temp) throws Exception {
);
}

/**
* This test was added to mitigate an issue with the transpilation cache.
* You can read more about it here:
* <a href="https://github.com/objectionary/eo/issues/4840">4840</a>
* If cache is missing, then transpilation should be performed.
* @param temp Temporary directory
* @throws Exception If fails
*/
@Test
void transpilesIfCacheMiss(@Mktmp final Path temp) throws Exception {
final Path cdir = temp.resolve("cache-folder").resolve("transpiled-folder");
Files.createDirectories(cdir);
final String content = "Source content - no cache";
final Path source = FpDefaultTest.existedFile(
temp.resolve("target/eo/1-parse/org/eolang/examples/fibonacci.xmir"),
"old"
);
final Path target = FpDefaultTest.notExistedTarget(
temp.resolve("target/eo/5-transpile/org/eolang/examples/fibonacci.xmir")
);
Comment on lines +358 to +360

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Target path helper hides the intended transpilation output path.

notExistedTarget(...) appends tar/get.txt, so this test doesn’t actually use the .../fibonacci.xmir target and is less representative of real output paths. Consider using the explicit path (and ensure parent dirs exist).

Suggested fix
-        final Path target = FpDefaultTest.notExistedTarget(
-            temp.resolve("target/eo/5-transpile/org/eolang/examples/fibonacci.xmir")
-        );
+        final Path target =
+            temp.resolve("target/eo/5-transpile/org/eolang/examples/fibonacci.xmir");
+        Files.createDirectories(target.getParent());
📝 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.

Suggested change
final Path target = FpDefaultTest.notExistedTarget(
temp.resolve("target/eo/5-transpile/org/eolang/examples/fibonacci.xmir")
);
final Path target =
temp.resolve("target/eo/5-transpile/org/eolang/examples/fibonacci.xmir");
Files.createDirectories(target.getParent());
🤖 Prompt for AI Agents
In `@eo-maven-plugin/src/test/java/org/eolang/maven/FpDefaultTest.java` around
lines 358 - 360, The test uses FpDefaultTest.notExistedTarget(...) which appends
"tar/get.txt" and therefore hides the intended transpilation output path; change
the test to use the explicit expected output Path (the resolved
".../fibonacci.xmir") instead of calling notExistedTarget, and ensure its parent
directories are created before use (e.g., createParents on the Path or
Files.createDirectories(target.getParent())); update references to the variable
target and remove reliance on notExistedTarget to ensure the test actually
targets the fibonacci.xmir output.

new FpDefault(
src -> content,
cdir,
"1.0-SNAPSHOT",
() -> "94641989dbaebef167cf67906a265d925bbb4e5b",
Paths.get("org/eolang/examples/fibonacci.xmir"),
true
).apply(source, target);
MatcherAssert.assertThat(
"We expect a cache miss to trigger transpilation",
new TextOf(target).asString(),
Matchers.equalTo(content)
);
}

/**
* We should use the transpilation cache if it existed before transpilation.
* Current implementation of {@link FpDefault} relies on file timestamps to decide
* whether to use the cache or not.
* If the cache file is older than the source file, then the cache is ignored, which is
* a critical issue for the transpilation step, because the source files are often
* modified (e.g. by the parser) right before transpilation, making the cache
* always older than the source files.
* @param temp Temporary directory
* @throws Exception If fails
* @todo #4840:90min Implement proper cache validation mechanism.
* Currently, FpDefault relies on file timestamps to decide whether to use
* the cache or not. This approach is not reliable in many cases.
* For example, in the transpilation step, the source files are often
* modified right before transpilation, making the cache always older
* than the source files.
*/
@Test
@Disabled

Copilot AI Feb 2, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Disabled is used without a reason string, which makes test reports harder to interpret. Consider adding a short explanation directly in the annotation (e.g., referencing #4840 / timestamp-based cache validation) so it’s visible without opening the Javadoc.

Suggested change
@Disabled
@Disabled("Disabled until #4840: implement proper timestamp-based cache validation")

Copilot uses AI. Check for mistakes.
void doesNotTranspileIfCacheHitWhenCacheExistedBefore(@Mktmp final Path temp) throws Exception {
final Path cdir = temp.resolve("cache-dir").resolve("transpiled-cache");
Files.createDirectories(cdir);
final Path cachefile = cdir
.resolve("1.0-SNAPSHOT")
.resolve("94641989dbaebef167cf67906a265d925bbb4e5b")
.resolve("org/eolang/examples/fibonacci.xmir");
Files.createDirectories(cachefile.getParent());
Comment on lines +398 to +402

Copilot AI Feb 2, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Variable name cachefile is a compound name; per project guideline prefer a single-word noun for locals. Consider renaming it to something like cache/entry (and adjust usages) to keep naming consistent with the guideline.

Copilot generated this review using guidance from organization custom instructions.
final String cached = "transpilation results from cache";
Files.write(cachefile, cached.getBytes(StandardCharsets.UTF_8));
FpDefaultTest.makeOlder(cdir, 1000);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Age the cache file itself (and in the past) for this scenario.

makeOlder(cdir, 1000) updates the directory timestamp (and with a positive shift makes it newer). If the cache decision is based on the cache file’s mtime, this won’t simulate “cache older than source.” Set the cache file mtime to the past instead.

Suggested fix
-        FpDefaultTest.makeOlder(cdir, 1000);
+        Files.setLastModifiedTime(
+            cachefile,
+            FileTime.fromMillis(System.currentTimeMillis() - 1_000)
+        );
🤖 Prompt for AI Agents
In `@eo-maven-plugin/src/test/java/org/eolang/maven/FpDefaultTest.java` at line
405, The test currently ages the directory with FpDefaultTest.makeOlder(cdir,
1000), which updates the directory mtime (and with a positive shift can make it
newer); instead, change the call to age the actual cache file mtime into the
past — locate the cache file path used in this test (the cache filename variable
or the File/Path pointing to the cache under cdir) and call
FpDefaultTest.makeOlder(cacheFilePath, -1000) (or otherwise pass a negative
shift) so the cache file itself is set to an older mtime and simulates "cache
older than source".

Copilot AI Feb 2, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this disabled test, makeOlder(cdir, 1000) only changes the directory timestamp, but FpDefault compares the cache file timestamp (the path returned by CachePath, i.e., cachefile). This means the test setup may not reliably reproduce the intended “cache older than source” scenario when you later enable it. Consider explicitly setting cachefile (and/or source) lastModifiedTime to guarantee cachefile is older than source.

Suggested change
FpDefaultTest.makeOlder(cdir, 1000);
FpDefaultTest.makeOlder(cachefile, 1000);

Copilot uses AI. Check for mistakes.
final Path source = FpDefaultTest.existedFile(
temp.resolve("target/eo/1-parse/org/eolang/examples/fibonacci.xmir"),
"Source content"
);
final Path target = temp.resolve(
"target/eo/5-transpile/org/eolang/examples/fibonacci.xmir"
);
new FpDefault(
src -> "transpiled content",
cdir,
"1.0-SNAPSHOT",
() -> "94641989dbaebef167cf67906a265d925bbb4e5b",
Paths.get("org/eolang/examples/fibonacci.xmir"),
true
).apply(source, target);
MatcherAssert.assertThat(
"We expect that cache is used",
new TextOf(target).asString(),
Matchers.equalTo(cached)
);
}

/**
* Returns the cache content.
*/
Expand Down
Loading