Skip to content

Commit f79f814

Browse files
committed
feat(mvn): unify stdout failure format with XML enrichment path
When XML is missing and the text parser is the only failure source, its output now matches the XML-enriched format: 1. Shorten exception FQN on the first detail line (`org.junit.ComparisonFailure:` -> `ComparisonFailure:`) via `shorten_exception_header`, mirroring `failure_kind_label`'s `rsplit('.').next()` logic. 2. Drop non-app stack frames when pom groupId is known (`app_packages` threaded through `filter_mvn_tests_with_goal`). `org.junit.Assert.*`, `org.hamcrest.*`, and similar assertion-lib frames were leaking through the legacy whitelist — now gated by `is_framework_frame_ext` which also rejects any `at <pkg>...` frame not in `app_packages`. Empty `app_packages` falls back to the legacy whitelist — no regression for fixtures and callers that do not detect a groupId.
1 parent 8b002c0 commit f79f814

2 files changed

Lines changed: 206 additions & 15 deletions

File tree

Cargo.lock

Lines changed: 47 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

src/cmds/java/mvn_cmd.rs

Lines changed: 159 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -228,13 +228,6 @@ impl TestLikeGoal {
228228
Self::Verify => "verify",
229229
}
230230
}
231-
232-
fn filter(self) -> fn(&str) -> String {
233-
match self {
234-
Self::Test => filter_mvn_test,
235-
Self::Verify => filter_mvn_verify,
236-
}
237-
}
238231
}
239232

240233
/// Build the `(tool_name, tee_label)` pair used for tracking a run of
@@ -301,15 +294,17 @@ fn run_tests_like(
301294
let app_pkgs = crate::cmds::java::pom_groupid::detect(&cwd);
302295

303296
let cwd_for_filter = cwd.clone();
304-
let filter = goal.filter();
305297

306298
let (tool_name, tee_label) = mvn_labels(binary, goal_str, goal_str);
307299
runner::run_filtered(
308300
cmd,
309301
&tool_name,
310302
&args.join(" "),
311303
move |raw: &str| {
312-
let filtered = filter(raw);
304+
// Thread `app_packages` into the stdout parser so its framework
305+
// frame filtering matches the XML enrichment's behavior — keeps
306+
// the fallback (no XML reports) format consistent with XML output.
307+
let filtered = filter_mvn_tests_with_goal(raw, goal_str, &app_pkgs);
313308
enrich_with_reports(&filtered, &cwd_for_filter, started_at, &app_pkgs, goal_str)
314309
},
315310
runner::RunOptions::with_tee(&tee_label),
@@ -777,12 +772,14 @@ fn counts(r: Option<&SurefireResult>) -> (usize, usize, usize) {
777772
}
778773

779774
/// Filter `mvn test` output using a state machine parser.
775+
#[cfg(test)]
780776
pub(crate) fn filter_mvn_test(output: &str) -> String {
781-
filter_mvn_tests_with_goal(output, "test")
777+
filter_mvn_tests_with_goal(output, "test", &[])
782778
}
783779

780+
#[cfg(test)]
784781
pub(crate) fn filter_mvn_verify(output: &str) -> String {
785-
filter_mvn_tests_with_goal(output, "verify")
782+
filter_mvn_tests_with_goal(output, "verify", &[])
786783
}
787784

788785
/// Shared state machine parser for test-producing goals (`test`, `verify`).
@@ -792,7 +789,7 @@ pub(crate) fn filter_mvn_verify(output: &str) -> String {
792789
/// - Testing: collect failure details from [ERROR] headers and assertion lines
793790
/// - Summary: parse final "Tests run:" line, BUILD SUCCESS/FAILURE, Total time
794791
/// - Done: stop at Help boilerplate
795-
fn filter_mvn_tests_with_goal(output: &str, goal: &str) -> String {
792+
fn filter_mvn_tests_with_goal(output: &str, goal: &str, app_packages: &[String]) -> String {
796793
let clean = strip_ansi(output);
797794
let mut state = TestParseState::Preamble;
798795

@@ -877,7 +874,7 @@ fn filter_mvn_tests_with_goal(output: &str, goal: &str) -> String {
877874
if f.details.len() >= MAX_DETAIL_LINES {
878875
continue;
879876
}
880-
if is_framework_frame(stripped)
877+
if is_framework_frame_ext(stripped, app_packages)
881878
|| is_maven_boilerplate(trimmed)
882879
|| stripped.is_empty()
883880
|| (trimmed.starts_with(ERROR_TAG) && stripped.contains("<<<"))
@@ -964,8 +961,16 @@ fn filter_mvn_tests_with_goal(output: &str, goal: &str) -> String {
964961
}
965962
for (i, failure) in failures.iter().enumerate() {
966963
writeln!(result, "{}. {}", i + 1, failure.name).ok();
967-
for detail in &failure.details {
968-
writeln!(result, " {}", truncate(detail, MAX_LINE_LENGTH)).ok();
964+
for (di, detail) in failure.details.iter().enumerate() {
965+
// First detail line is the exception header (e.g.
966+
// `org.junit.ComparisonFailure: expected:<X> but was:<Y>`).
967+
// Shorten the FQN to match the XML path's format.
968+
let rendered = if di == 0 {
969+
shorten_exception_header(detail)
970+
} else {
971+
detail.clone()
972+
};
973+
writeln!(result, " {}", truncate(&rendered, MAX_LINE_LENGTH)).ok();
969974
}
970975
}
971976
if total_failures_seen > MAX_FAILURES_SHOWN {
@@ -1016,6 +1021,64 @@ fn is_framework_frame(line: &str) -> bool {
10161021
line.starts_with("...") && line.contains("more")
10171022
}
10181023

1024+
/// Extended framework-frame check driven by the legacy whitelist plus
1025+
/// `app_packages` when known. Any `at <pkg>...` frame whose package is not
1026+
/// in `app_packages` is treated as framework noise, matching the XML
1027+
/// enrichment path's filtering behavior.
1028+
///
1029+
/// When `app_packages` is empty, falls back to the whitelist only — this
1030+
/// preserves the pre-app_packages behavior for fixtures and callers that
1031+
/// do not supply a pom groupId.
1032+
fn is_framework_frame_ext(line: &str, app_packages: &[String]) -> bool {
1033+
if is_framework_frame(line) {
1034+
return true;
1035+
}
1036+
if app_packages.is_empty() {
1037+
return false;
1038+
}
1039+
// Only `at ...` frames are framework-gated. Exception headers and
1040+
// assertion messages (e.g. "expected:<X> but was:<Y>") never start with
1041+
// `at ` after `strip_maven_prefix`, so they pass through unchanged.
1042+
let Some(after_at) = line.strip_prefix("at ") else {
1043+
return false;
1044+
};
1045+
!app_packages
1046+
.iter()
1047+
.any(|pkg| after_at.starts_with(pkg.as_str()))
1048+
}
1049+
1050+
/// Strip the package prefix from an exception header, mirroring the XML
1051+
/// path's `failure_kind_label` (`rsplit('.').next()`). Turns
1052+
/// `"org.junit.ComparisonFailure: expected:<X> but was:<Y>"` into
1053+
/// `"ComparisonFailure: expected:<X> but was:<Y>"`. Passes non-exception
1054+
/// lines through unchanged.
1055+
fn shorten_exception_header(line: &str) -> String {
1056+
let Some((fqn, rest)) = line.split_once(':') else {
1057+
return line.to_string();
1058+
};
1059+
let fqn_trimmed = fqn.trim();
1060+
// Must look like a Java FQN: non-empty, no spaces, contains a dot.
1061+
if fqn_trimmed.is_empty()
1062+
|| fqn_trimmed.contains(char::is_whitespace)
1063+
|| !fqn_trimmed.contains('.')
1064+
{
1065+
return line.to_string();
1066+
}
1067+
// Every segment must be a valid Java identifier-ish token (letters,
1068+
// digits, `_`, `$`). This rejects message bodies like "expected:<X>".
1069+
let valid = fqn_trimmed.split('.').all(|seg| {
1070+
!seg.is_empty()
1071+
&& seg
1072+
.chars()
1073+
.all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '$')
1074+
});
1075+
if !valid {
1076+
return line.to_string();
1077+
}
1078+
let short = fqn_trimmed.rsplit('.').next().unwrap_or(fqn_trimmed);
1079+
format!("{short}:{rest}")
1080+
}
1081+
10191082
/// Returns true for Maven boilerplate lines that should be stripped.
10201083
/// Expects pre-trimmed input from callers.
10211084
fn is_maven_boilerplate(line: &str) -> bool {
@@ -2838,6 +2901,87 @@ mod tests {
28382901
);
28392902
}
28402903

2904+
#[test]
2905+
fn shorten_exception_header_strips_fqn_package() {
2906+
// Standard Java exception header: `org.junit.ComparisonFailure: msg`
2907+
assert_eq!(
2908+
super::shorten_exception_header(
2909+
"org.junit.ComparisonFailure: expected:<He[llo]!> but was:<He[re I am]!>",
2910+
),
2911+
"ComparisonFailure: expected:<He[llo]!> but was:<He[re I am]!>",
2912+
);
2913+
assert_eq!(
2914+
super::shorten_exception_header("java.lang.NullPointerException: x"),
2915+
"NullPointerException: x",
2916+
);
2917+
}
2918+
2919+
#[test]
2920+
fn shorten_exception_header_passthrough_for_non_fqn() {
2921+
// Messages without a package-qualified prefix stay untouched.
2922+
assert_eq!(
2923+
super::shorten_exception_header("expected:<200> but was:<404>"),
2924+
"expected:<200> but was:<404>",
2925+
);
2926+
// Simple class name (no dots) — passthrough.
2927+
assert_eq!(
2928+
super::shorten_exception_header("AssertionError: boom"),
2929+
"AssertionError: boom",
2930+
);
2931+
// FQN token with whitespace in it — not a class, passthrough.
2932+
assert_eq!(
2933+
super::shorten_exception_header("not fqn: value"),
2934+
"not fqn: value",
2935+
);
2936+
}
2937+
2938+
#[test]
2939+
fn text_filter_shortens_exception_fqn_in_first_detail() {
2940+
// Regression: before unification, stdout parser emitted the full FQN
2941+
// (`org.junit.ComparisonFailure:`). XML path only shows the short
2942+
// class name — text fallback must match so both sources render
2943+
// identically.
2944+
let input = include_str!("../../../tests/fixtures/mvn_test_reactor_fail.txt");
2945+
let output = filter_mvn_test(input);
2946+
assert!(
2947+
!output.contains("org.junit.ComparisonFailure:"),
2948+
"text filter leaked FQN — must render short `ComparisonFailure:`:\n{output}"
2949+
);
2950+
}
2951+
2952+
#[test]
2953+
fn text_filter_drops_non_app_frames_when_app_packages_known() {
2954+
// With `app_packages=["com.edeal.frontline"]`, `org.junit.Assert.*`
2955+
// frames are not app code and must be dropped — same rule the XML
2956+
// `stack_trace::process` path applies. Without this, the stdout
2957+
// fallback kept `at org.junit.Assert.assertEquals(Assert.java:117)`
2958+
// noise while XML output was clean.
2959+
let input = include_str!("../../../tests/fixtures/mvn_test_reactor_fail.txt");
2960+
let output =
2961+
super::filter_mvn_tests_with_goal(input, "test", &pkgs("com.edeal.frontline"));
2962+
assert!(
2963+
!output.contains("org.junit.Assert.assertEquals"),
2964+
"kept `org.junit.Assert` framework frame with app_packages known:\n{output}"
2965+
);
2966+
}
2967+
2968+
#[test]
2969+
fn text_filter_preserves_existing_behavior_without_app_packages() {
2970+
// Empty app_packages falls back to the legacy whitelist — fixtures
2971+
// that today rely on certain frames surviving must keep working.
2972+
let input = include_str!("../../../tests/fixtures/mvn_test_reactor_fail.txt");
2973+
let with_empty = filter_mvn_test(input); // app_packages = &[]
2974+
let with_pkgs =
2975+
super::filter_mvn_tests_with_goal(input, "test", &pkgs("com.edeal.frontline"));
2976+
// Empty-packages mode keeps frames the legacy whitelist doesn't cover.
2977+
assert!(
2978+
with_empty.contains("org.junit.Assert.assertEquals"),
2979+
"empty app_packages must NOT regress the legacy behavior:\n{with_empty}"
2980+
);
2981+
// App-packages mode drops them (covered above); sanity-check divergence.
2982+
assert_ne!(with_empty, with_pkgs);
2983+
}
2984+
28412985
#[test]
28422986
fn enrich_drops_text_failures_block_when_xml_has_failures() {
28432987
// Regression: before deduplication the user saw two "Failures"

0 commit comments

Comments
 (0)