feat: fix engine (Phase 1.6) + git-aware mv + windows-safe path output
- Edit engine: generic span edits (replace/insert/delete) with stale-index guard; mv and fix share one atomic apply/rollback/diff/--json pipeline - jmove mv|check|fix commands; java rules: unused-import, missing-import (unique-FQN insert, ambiguous -> candidates for agents), import-order (google style; cross-rule byte conflicts prune by severity and converge) - mv of tracked files goes through git mv (staged rename, --no-git opt-out, moved_via/would_move_via in --json, GIT_ERROR code) - fix java extract_package losing single-segment packages (bare identifier, not scoped_identifier) — broke the FQN index for 'package p;' projects - core::rel_str: project-relative paths rendered with '/' on every platform (human/JSON/diff headers/git pathspecs), Path::display() no longer used for output; e2e fixtures: java/fix, java/fix_missing, java/order
This commit is contained in:
parent
0216fd6523
commit
9dda5f4562
37 changed files with 2521 additions and 234 deletions
249
tests/cli_fix.rs
Normal file
249
tests/cli_fix.rs
Normal file
|
|
@ -0,0 +1,249 @@
|
|||
//! End-to-end tests for `jmove fix` against a Java fixture.
|
||||
|
||||
mod common;
|
||||
|
||||
use common::{in_root, jmove, read};
|
||||
use predicates::prelude::*;
|
||||
|
||||
fn dirty_project() -> tempfile::TempDir {
|
||||
common::copy_fixture("java", "fix")
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn fix_dry_run_previews_deletions_without_writing() {
|
||||
let tmp = dirty_project();
|
||||
jmove(&tmp, &["fix", "--dry-run"])
|
||||
.success()
|
||||
.stdout(predicate::str::contains(
|
||||
"-import com.example.unused.Ghost;",
|
||||
))
|
||||
.stdout(predicate::str::contains(" import java.util.List;"));
|
||||
// Disk untouched: the unused import is still there.
|
||||
let app = read(&in_root(
|
||||
tmp.path(),
|
||||
"src/main/java/com/example/app/App.java",
|
||||
));
|
||||
assert!(app.contains("import com.example.unused.Ghost;"), "{app}");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn fix_removes_unused_and_keeps_used_and_string_mentions() {
|
||||
let tmp = dirty_project();
|
||||
jmove(&tmp, &["fix"])
|
||||
.success()
|
||||
.stdout(predicate::str::contains("fixed 1 issue in 1 file"));
|
||||
|
||||
let app = read(&in_root(
|
||||
tmp.path(),
|
||||
"src/main/java/com/example/app/App.java",
|
||||
));
|
||||
// Unused single-type import is gone, whole line removed.
|
||||
assert!(!app.contains("com.example.unused.Ghost"), "{app}");
|
||||
// Used import stays.
|
||||
assert!(app.contains("import com.example.Text;"), "{app}");
|
||||
// `List` only appears inside a string literal => kept (safe direction).
|
||||
assert!(app.contains("import java.util.List;"), "{app}");
|
||||
assert!(app.contains("\"java.util.List\""), "{app}");
|
||||
|
||||
// Run 2: convergence — the deferred `java/import-order` fix (it
|
||||
// overlapped the deletion in run 1) now applies on the clean block.
|
||||
jmove(&tmp, &["fix"])
|
||||
.success()
|
||||
.stdout(predicate::str::contains("fixed 1 issue in 1 file"));
|
||||
let app = read(&in_root(
|
||||
tmp.path(),
|
||||
"src/main/java/com/example/app/App.java",
|
||||
));
|
||||
assert!(
|
||||
app.contains(
|
||||
"import static com.example.Text.shout;\n\nimport com.example.Text;\nimport java.util.List;\n"
|
||||
),
|
||||
"{app}"
|
||||
);
|
||||
// Run 3: fixed point — nothing left to change.
|
||||
jmove(&tmp, &["fix"])
|
||||
.success()
|
||||
.stdout(predicate::str::contains("nothing to change"));
|
||||
// The project still checks clean.
|
||||
jmove(&tmp, &["check"]).success();
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn fix_json_reports_candidates_and_applied_count() {
|
||||
let tmp = dirty_project();
|
||||
jmove(&tmp, &["fix", "--dry-run", "--json"])
|
||||
.success()
|
||||
.stdout(
|
||||
predicate::str::contains("\"status\": \"dry_run\"")
|
||||
.and(predicate::str::contains("\"operation\": \"fix\""))
|
||||
.and(predicate::str::contains("\"rule\": \"java/unused-import\""))
|
||||
.and(predicate::str::contains("\"would_fix\": 1")),
|
||||
);
|
||||
|
||||
jmove(&tmp, &["fix", "--json"]).success().stdout(
|
||||
predicate::str::contains("\"status\": \"ok\"")
|
||||
.and(predicate::str::contains("\"fixes\": 1"))
|
||||
.and(predicate::str::contains("\"files_changed\": 1")),
|
||||
);
|
||||
let app = read(&in_root(
|
||||
tmp.path(),
|
||||
"src/main/java/com/example/app/App.java",
|
||||
));
|
||||
assert!(!app.contains("Ghost"), "{app}");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn fix_unknown_rule_is_rejected() {
|
||||
let tmp = dirty_project();
|
||||
jmove(&tmp, &["fix", "--rule", "does/not-exist"])
|
||||
.failure()
|
||||
.code(1)
|
||||
.stderr(predicate::str::contains(
|
||||
"unknown fix rule 'does/not-exist'",
|
||||
))
|
||||
.stderr(predicate::str::contains("java/unused-import"));
|
||||
// Nothing on disk changed.
|
||||
let app = read(&in_root(
|
||||
tmp.path(),
|
||||
"src/main/java/com/example/app/App.java",
|
||||
));
|
||||
assert!(app.contains("Ghost"), "{app}");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn fix_scoped_to_rule_and_reports_guava_like_sibling() {
|
||||
// `Text` is referenced only by a static member import of the same type,
|
||||
// so neither import is provably dead (mirrors the Guava smoke note).
|
||||
let tmp = dirty_project();
|
||||
let app = read(&in_root(
|
||||
tmp.path(),
|
||||
"src/main/java/com/example/app/App.java",
|
||||
));
|
||||
assert!(
|
||||
app.contains("import static com.example.Text.shout;"),
|
||||
"{app}"
|
||||
);
|
||||
jmove(&tmp, &["fix", "--rule", "java/unused-import"])
|
||||
.success()
|
||||
.stdout(predicate::str::contains("fixed 1 issue in 1 file"));
|
||||
let app = read(&in_root(
|
||||
tmp.path(),
|
||||
"src/main/java/com/example/app/App.java",
|
||||
));
|
||||
assert!(app.contains("import com.example.Text;"), "{app}");
|
||||
assert!(
|
||||
app.contains("import static com.example.Text.shout;"),
|
||||
"{app}"
|
||||
);
|
||||
}
|
||||
|
||||
fn missing_project() -> tempfile::TempDir {
|
||||
common::copy_fixture("java", "fix_missing")
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn missing_import_adds_unique_and_reports_ambiguous() {
|
||||
let tmp = missing_project();
|
||||
jmove(&tmp, &["fix", "--rule", "java/missing-import", "--json"])
|
||||
.success()
|
||||
.stdout(
|
||||
predicate::str::contains("\"rule\": \"java/missing-import\"")
|
||||
.and(predicate::str::contains("\"applied\": false"))
|
||||
.and(predicate::str::contains("\"com.example.a.Config\""))
|
||||
.and(predicate::str::contains("\"com.example.b.Config\"")),
|
||||
);
|
||||
let calc = read(&in_root(
|
||||
tmp.path(),
|
||||
"src/main/java/com/example/app/Calc.java",
|
||||
));
|
||||
assert!(
|
||||
calc.contains("package com.example.app;\nimport com.example.util.Maths;\n"),
|
||||
"{calc}"
|
||||
);
|
||||
// The ambiguous `Config` is never guessed at.
|
||||
let refer = read(&in_root(
|
||||
tmp.path(),
|
||||
"src/main/java/com/example/c/Refer.java",
|
||||
));
|
||||
assert!(!refer.contains("import com.example."), "{refer}");
|
||||
jmove(&tmp, &["check"]).success();
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn missing_import_dry_run_then_stable_reapply() {
|
||||
let tmp = missing_project();
|
||||
jmove(&tmp, &["fix", "--dry-run"])
|
||||
.success()
|
||||
.stdout(predicate::str::contains("+import com.example.util.Maths;"));
|
||||
jmove(&tmp, &["fix"])
|
||||
.success()
|
||||
.stdout(predicate::str::contains("fixed 3 issues in 2 files"));
|
||||
// Only manual (ambiguous) findings remain: nothing further applies.
|
||||
jmove(&tmp, &["fix"])
|
||||
.success()
|
||||
.stdout(predicate::str::contains("nothing to change"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn unused_delete_and_missing_insert_coexist_in_one_file() {
|
||||
// Dual.java: the unused import is deleted while the missing `Maths`
|
||||
// import lands at the very byte of the deleted line's end — adjacent,
|
||||
// not overlapping, so one atomic plan carries both.
|
||||
let tmp = missing_project();
|
||||
let dual = "src/main/java/com/example/app/Dual.java";
|
||||
jmove(&tmp, &["fix"]).success();
|
||||
let text = read(&in_root(tmp.path(), dual));
|
||||
assert!(!text.contains("Gone"), "{text}");
|
||||
assert!(text.contains("import com.example.util.Maths;"), "{text}");
|
||||
jmove(&tmp, &["check"]).success();
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn three_rules_converge_on_one_project() {
|
||||
// App.java: unused import inside an out-of-order block (the order
|
||||
// rewrite overlaps the deletion, so it defers to run 2).
|
||||
// C.java: out-of-order block + a bare `Maths` reference (insertion at
|
||||
// the block boundary coexists with the rewrite in run 1).
|
||||
let tmp = common::copy_fixture("java", "order");
|
||||
jmove(&tmp, &["fix", "--dry-run", "--json"])
|
||||
.success()
|
||||
.stdout(
|
||||
predicate::str::contains("\"rule\": \"java/import-order\"")
|
||||
.and(predicate::str::contains("\"applied\": false"))
|
||||
.and(predicate::str::contains("skipped")),
|
||||
);
|
||||
let app = "src/main/java/com/example/app/App.java";
|
||||
let c = "src/main/java/com/example/app/C.java";
|
||||
jmove(&tmp, &["fix"])
|
||||
.success()
|
||||
.stdout(predicate::str::contains("fixed 3 issues in 2 files"));
|
||||
let app_text = read(&in_root(tmp.path(), app));
|
||||
// Unused is gone; the deferred order fix has not touched the block.
|
||||
assert!(!app_text.contains("Unneeded"), "{app_text}");
|
||||
assert!(
|
||||
app_text.contains("import java.util.List;\nimport com.example.util.Maths;"),
|
||||
"{app_text}"
|
||||
);
|
||||
jmove(&tmp, &["fix"])
|
||||
.success()
|
||||
.stdout(predicate::str::contains("fixed 2 issues in 2 files"));
|
||||
let c_text = read(&in_root(tmp.path(), c));
|
||||
assert!(
|
||||
c_text.contains(
|
||||
"import com.example.util.Maths;\nimport java.util.List;\nimport java.util.Map;\n"
|
||||
),
|
||||
"{c_text}"
|
||||
);
|
||||
let app_text = read(&in_root(tmp.path(), app));
|
||||
assert!(
|
||||
app_text.contains(
|
||||
"import com.example.util.Maths;\nimport java.util.List;\nimport java.util.Map;\n"
|
||||
),
|
||||
"{app_text}"
|
||||
);
|
||||
// Fixed point.
|
||||
jmove(&tmp, &["fix"])
|
||||
.success()
|
||||
.stdout(predicate::str::contains("nothing to change"));
|
||||
jmove(&tmp, &["check"]).success();
|
||||
}
|
||||
115
tests/cli_git.rs
Normal file
115
tests/cli_git.rs
Normal file
|
|
@ -0,0 +1,115 @@
|
|||
//! End-to-end tests for the git integration of `mv`: tracked files move
|
||||
//! through `git mv` (staged rename, history kept), `--no-git` opts out.
|
||||
//! Self-contained: it builds a real git repo from the `typescript/basic`
|
||||
//! fixture instead of reusing the fake `.git` marker of `copy_fixture`.
|
||||
|
||||
use std::fs;
|
||||
use std::path::Path;
|
||||
use std::process::Command as Git;
|
||||
|
||||
use assert_cmd::Command;
|
||||
use predicates::prelude::*;
|
||||
use tempfile::{TempDir, tempdir};
|
||||
|
||||
/// Copy the fixture into a tempdir, `git init` it and commit everything.
|
||||
fn git_fixture(name: &str) -> TempDir {
|
||||
let tmp = tempdir().expect("tempdir");
|
||||
let src = Path::new(env!("CARGO_MANIFEST_DIR"))
|
||||
.join("tests")
|
||||
.join("typescript")
|
||||
.join(name);
|
||||
copy_dir(&src, tmp.path());
|
||||
git(tmp.path(), &["init", "-q", "-b", "main", "."]);
|
||||
git(tmp.path(), &["config", "user.name", "jmove-test"]);
|
||||
git(tmp.path(), &["config", "user.email", "test@test"]);
|
||||
git(tmp.path(), &["add", "-A"]);
|
||||
git(tmp.path(), &["commit", "-qm", "init"]);
|
||||
tmp
|
||||
}
|
||||
|
||||
fn copy_dir(from: &Path, to: &Path) {
|
||||
fs::create_dir_all(to).expect("mkdir");
|
||||
for entry in fs::read_dir(from).expect("readdir") {
|
||||
let entry = entry.expect("entry");
|
||||
let target = to.join(entry.file_name());
|
||||
if entry.file_type().expect("filetype").is_dir() {
|
||||
copy_dir(&entry.path(), &target);
|
||||
} else {
|
||||
fs::copy(entry.path(), target).expect("copy");
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
fn jmove(root: &TempDir, args: &[&str]) -> assert_cmd::assert::Assert {
|
||||
let mut cmd = Command::cargo_bin("jmove").expect("jmove binary");
|
||||
cmd.arg("--root").arg(root.path()).args(args);
|
||||
cmd.assert()
|
||||
}
|
||||
|
||||
fn git(root: &Path, args: &[&str]) {
|
||||
let status = Git::new("git")
|
||||
.arg("-C")
|
||||
.arg(root)
|
||||
.args(args)
|
||||
.status()
|
||||
.expect("git");
|
||||
assert!(status.success(), "`git {args:?}` failed");
|
||||
}
|
||||
|
||||
fn git_output(root: &Path, args: &[&str]) -> String {
|
||||
let out = Git::new("git")
|
||||
.arg("-C")
|
||||
.arg(root)
|
||||
.args(args)
|
||||
.output()
|
||||
.expect("git");
|
||||
assert!(out.status.success(), "`git {args:?}` failed");
|
||||
String::from_utf8(out.stdout).expect("utf-8")
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn mv_in_a_git_repo_moves_via_git_mv_and_stages_it() {
|
||||
let tmp = git_fixture("basic");
|
||||
jmove(&tmp, &["mv", "lib/sum.ts", "utils/sum.ts", "--json"])
|
||||
.success()
|
||||
.stdout(predicate::str::contains("\"moved_via\": \"git\""));
|
||||
let staged = git_output(tmp.path(), &["diff", "--cached", "-M", "--name-status"]);
|
||||
assert!(
|
||||
staged.contains("lib/sum.ts") && staged.contains("utils/sum.ts"),
|
||||
"rename must be staged: {staged}"
|
||||
);
|
||||
jmove(&tmp, &["check"]).success();
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn mv_dry_run_reports_would_use_git() {
|
||||
let tmp = git_fixture("basic");
|
||||
jmove(
|
||||
&tmp,
|
||||
&["mv", "lib/sum.ts", "utils/sum.ts", "--dry-run", "--json"],
|
||||
)
|
||||
.success()
|
||||
.stdout(predicate::str::contains("\"would_move_via\": \"git\""));
|
||||
assert!(
|
||||
tmp.path().join("lib/sum.ts").is_file(),
|
||||
"dry-run writes nothing"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn mv_no_git_keeps_the_plain_rename_unstaged() {
|
||||
let tmp = git_fixture("basic");
|
||||
jmove(
|
||||
&tmp,
|
||||
&["mv", "lib/sum.ts", "utils/sum.ts", "--no-git", "--json"],
|
||||
)
|
||||
.success()
|
||||
.stdout(predicate::str::contains("\"moved_via\": \"fs\""));
|
||||
let staged = git_output(tmp.path(), &["diff", "--cached", "--name-only"]);
|
||||
assert!(
|
||||
staged.trim().is_empty(),
|
||||
"--no-git must not stage: {staged}"
|
||||
);
|
||||
let target = git_output(tmp.path(), &["status", "--porcelain", "--", "utils/sum.ts"]);
|
||||
assert!(target.starts_with("??"), "{target}");
|
||||
}
|
||||
7
tests/java/fix/src/main/java/com/example/Text.java
Normal file
7
tests/java/fix/src/main/java/com/example/Text.java
Normal file
|
|
@ -0,0 +1,7 @@
|
|||
package com.example;
|
||||
|
||||
public class Text {
|
||||
public static String shout(String s) {
|
||||
return s.toUpperCase() + "!";
|
||||
}
|
||||
}
|
||||
14
tests/java/fix/src/main/java/com/example/app/App.java
Normal file
14
tests/java/fix/src/main/java/com/example/app/App.java
Normal file
|
|
@ -0,0 +1,14 @@
|
|||
package com.example.app;
|
||||
|
||||
import com.example.Text;
|
||||
import com.example.unused.Ghost;
|
||||
import java.util.List;
|
||||
import static com.example.Text.shout;
|
||||
|
||||
public class App {
|
||||
public static void main(String[] args) {
|
||||
System.out.println(Text.shout("hi"));
|
||||
String topic = "java.util.List";
|
||||
System.out.println(topic);
|
||||
}
|
||||
}
|
||||
|
|
@ -0,0 +1,3 @@
|
|||
package com.example.a;
|
||||
|
||||
public class Config {}
|
||||
|
|
@ -0,0 +1,5 @@
|
|||
package com.example.app;
|
||||
|
||||
public class Calc {
|
||||
int v = Maths.compute();
|
||||
}
|
||||
|
|
@ -0,0 +1,7 @@
|
|||
package com.example.app;
|
||||
|
||||
import com.example.gone.Gone;
|
||||
|
||||
public class Dual {
|
||||
int v = Maths.compute();
|
||||
}
|
||||
|
|
@ -0,0 +1,3 @@
|
|||
package com.example.b;
|
||||
|
||||
public class Config {}
|
||||
|
|
@ -0,0 +1,5 @@
|
|||
package com.example.c;
|
||||
|
||||
public class Refer {
|
||||
Config field;
|
||||
}
|
||||
|
|
@ -0,0 +1,7 @@
|
|||
package com.example.util;
|
||||
|
||||
public class Maths {
|
||||
public static int compute() {
|
||||
return 42;
|
||||
}
|
||||
}
|
||||
12
tests/java/order/src/main/java/com/example/app/App.java
Normal file
12
tests/java/order/src/main/java/com/example/app/App.java
Normal file
|
|
@ -0,0 +1,12 @@
|
|||
package com.example.app;
|
||||
|
||||
import java.util.List;
|
||||
import com.example.util.Maths;
|
||||
import com.example.gone.Unneeded;
|
||||
import java.util.Map;
|
||||
|
||||
public class App {
|
||||
List<String> a;
|
||||
Map<String, String> b;
|
||||
int v = Maths.compute();
|
||||
}
|
||||
10
tests/java/order/src/main/java/com/example/app/C.java
Normal file
10
tests/java/order/src/main/java/com/example/app/C.java
Normal file
|
|
@ -0,0 +1,10 @@
|
|||
package com.example.app;
|
||||
|
||||
import java.util.Map;
|
||||
import java.util.List;
|
||||
|
||||
public class C {
|
||||
List<String> a;
|
||||
Map<String, String> b;
|
||||
int v = Maths.compute();
|
||||
}
|
||||
|
|
@ -0,0 +1,7 @@
|
|||
package com.example.util;
|
||||
|
||||
public class Maths {
|
||||
public static int compute() {
|
||||
return 42;
|
||||
}
|
||||
}
|
||||
Loading…
Add table
Add a link
Reference in a new issue