From 997dc2e7ccfc378d60e1b98c3fd244eb6b37ba70 Mon Sep 17 00:00:00 2001 From: Brent Westbrook <36778786+ntBre@users.noreply.github.com> Date: Thu, 17 Jul 2025 18:24:13 -0400 Subject: [PATCH] Move JUnit rendering to `ruff_db` (#19370) Summary -- This PR moves the JUnit output format to the new rendering infrastructure. As I mention in a TODO in the code, there's some code that will be shared with the `grouped` output format. Hopefully I'll have that PR up too by the time this one is reviewed. Test Plan -- Existing tests moved to `ruff_db` --------- Co-authored-by: Micha Reiser --- Cargo.lock | 2 +- crates/ruff/src/printer.rs | 10 +- .../snapshots/lint__output_format_junit.snap | 2 +- crates/ruff_db/Cargo.toml | 2 + crates/ruff_db/src/diagnostic/mod.rs | 3 + crates/ruff_db/src/diagnostic/render.rs | 6 + crates/ruff_db/src/diagnostic/render/junit.rs | 195 ++++++++++++++++++ ...nostic__render__junit__tests__output.snap} | 5 +- ..._render__junit__tests__syntax_errors.snap} | 9 +- crates/ruff_linter/Cargo.toml | 3 +- crates/ruff_linter/src/message/junit.rs | 117 ----------- crates/ruff_linter/src/message/mod.rs | 2 - 12 files changed, 222 insertions(+), 134 deletions(-) create mode 100644 crates/ruff_db/src/diagnostic/render/junit.rs rename crates/{ruff_linter/src/message/snapshots/ruff_linter__message__junit__tests__output.snap => ruff_db/src/diagnostic/render/snapshots/ruff_db__diagnostic__render__junit__tests__output.snap} (90%) rename crates/{ruff_linter/src/message/snapshots/ruff_linter__message__junit__tests__syntax_errors.snap => ruff_db/src/diagnostic/render/snapshots/ruff_db__diagnostic__render__junit__tests__syntax_errors.snap} (67%) delete mode 100644 crates/ruff_linter/src/message/junit.rs diff --git a/Cargo.lock b/Cargo.lock index d97f66115e..45fa25d305 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2839,6 +2839,7 @@ dependencies = [ "insta", "matchit", "path-slash", + "quick-junit", "ruff_annotate_snippets", "ruff_cache", "ruff_diagnostics", @@ -2987,7 +2988,6 @@ dependencies = [ "pathdiff", "pep440_rs", "pyproject-toml", - "quick-junit", "regex", "ruff_annotate_snippets", "ruff_cache", diff --git a/crates/ruff/src/printer.rs b/crates/ruff/src/printer.rs index 2fa8e476cb..dc9f8c8538 100644 --- a/crates/ruff/src/printer.rs +++ b/crates/ruff/src/printer.rs @@ -15,8 +15,8 @@ use ruff_db::diagnostic::{ use ruff_linter::fs::relativize_path; use ruff_linter::logging::LogLevel; use ruff_linter::message::{ - Emitter, EmitterContext, GithubEmitter, GitlabEmitter, GroupedEmitter, JunitEmitter, - SarifEmitter, TextEmitter, + Emitter, EmitterContext, GithubEmitter, GitlabEmitter, GroupedEmitter, SarifEmitter, + TextEmitter, }; use ruff_linter::notify_user; use ruff_linter::settings::flags::{self}; @@ -252,7 +252,11 @@ impl Printer { write!(writer, "{value}")?; } OutputFormat::Junit => { - JunitEmitter.emit(writer, &diagnostics.inner, &context)?; + let config = DisplayDiagnosticConfig::default() + .format(DiagnosticFormat::Junit) + .preview(preview); + let value = DisplayDiagnostics::new(&context, &config, &diagnostics.inner); + write!(writer, "{value}")?; } OutputFormat::Concise | OutputFormat::Full => { TextEmitter::default() diff --git a/crates/ruff/tests/snapshots/lint__output_format_junit.snap b/crates/ruff/tests/snapshots/lint__output_format_junit.snap index 30b5739172..9755dedb05 100644 --- a/crates/ruff/tests/snapshots/lint__output_format_junit.snap +++ b/crates/ruff/tests/snapshots/lint__output_format_junit.snap @@ -25,7 +25,7 @@ exit_code: 1 line 2, col 5, Undefined name `y` - + line 3, col 1, SyntaxError: Cannot use `match` statement on Python 3.9 (syntax was added in Python 3.10) diff --git a/crates/ruff_db/Cargo.toml b/crates/ruff_db/Cargo.toml index 07873cbfad..9a446a1b19 100644 --- a/crates/ruff_db/Cargo.toml +++ b/crates/ruff_db/Cargo.toml @@ -34,6 +34,7 @@ glob = { workspace = true } ignore = { workspace = true, optional = true } matchit = { workspace = true } path-slash = { workspace = true } +quick-junit = { workspace = true, optional = true } rustc-hash = { workspace = true } salsa = { workspace = true } schemars = { workspace = true, optional = true } @@ -56,6 +57,7 @@ tempfile = { workspace = true } [features] cache = ["ruff_cache"] +junit = ["dep:quick-junit"] os = ["ignore", "dep:etcetera"] serde = ["camino/serde1", "dep:serde", "dep:serde_json", "ruff_diagnostics/serde"] # Exposes testing utilities. diff --git a/crates/ruff_db/src/diagnostic/mod.rs b/crates/ruff_db/src/diagnostic/mod.rs index ade0a9402d..947075becb 100644 --- a/crates/ruff_db/src/diagnostic/mod.rs +++ b/crates/ruff_db/src/diagnostic/mod.rs @@ -1282,6 +1282,9 @@ pub enum DiagnosticFormat { Rdjson, /// Print diagnostics in the format emitted by Pylint. Pylint, + /// Print diagnostics in the format expected by JUnit. + #[cfg(feature = "junit")] + Junit, } /// A representation of the kinds of messages inside a diagnostic. diff --git a/crates/ruff_db/src/diagnostic/render.rs b/crates/ruff_db/src/diagnostic/render.rs index f189a757e6..7dbfe650e2 100644 --- a/crates/ruff_db/src/diagnostic/render.rs +++ b/crates/ruff_db/src/diagnostic/render.rs @@ -30,6 +30,8 @@ mod azure; mod json; #[cfg(feature = "serde")] mod json_lines; +#[cfg(feature = "junit")] +mod junit; mod pylint; #[cfg(feature = "serde")] mod rdjson; @@ -196,6 +198,10 @@ impl std::fmt::Display for DisplayDiagnostics<'_> { DiagnosticFormat::Pylint => { PylintRenderer::new(self.resolver).render(f, self.diagnostics)?; } + #[cfg(feature = "junit")] + DiagnosticFormat::Junit => { + junit::JunitRenderer::new(self.resolver).render(f, self.diagnostics)?; + } } Ok(()) diff --git a/crates/ruff_db/src/diagnostic/render/junit.rs b/crates/ruff_db/src/diagnostic/render/junit.rs new file mode 100644 index 0000000000..2b30e3ec73 --- /dev/null +++ b/crates/ruff_db/src/diagnostic/render/junit.rs @@ -0,0 +1,195 @@ +use std::{collections::BTreeMap, ops::Deref, path::Path}; + +use quick_junit::{NonSuccessKind, Report, TestCase, TestCaseStatus, TestSuite, XmlString}; + +use ruff_source_file::LineColumn; + +use crate::diagnostic::{Diagnostic, SecondaryCode, render::FileResolver}; + +/// A renderer for diagnostics in the [JUnit] format. +/// +/// See [`junit.xsd`] for the specification in the JUnit repository and an annotated [version] +/// linked from the [`quick_junit`] docs. +/// +/// [JUnit]: https://junit.org/ +/// [`junit.xsd`]: https://github.com/junit-team/junit-framework/blob/2870b7d8fd5bf7c1efe489d3991d3ed3900e82bb/platform-tests/src/test/resources/jenkins-junit.xsd +/// [version]: https://llg.cubic.org/docs/junit/ +/// [`quick_junit`]: https://docs.rs/quick-junit/latest/quick_junit/ +pub struct JunitRenderer<'a> { + resolver: &'a dyn FileResolver, +} + +impl<'a> JunitRenderer<'a> { + pub fn new(resolver: &'a dyn FileResolver) -> Self { + Self { resolver } + } + + pub(super) fn render( + &self, + f: &mut std::fmt::Formatter, + diagnostics: &[Diagnostic], + ) -> std::fmt::Result { + let mut report = Report::new("ruff"); + + if diagnostics.is_empty() { + let mut test_suite = TestSuite::new("ruff"); + test_suite + .extra + .insert(XmlString::new("package"), XmlString::new("org.ruff")); + let mut case = TestCase::new("No errors found", TestCaseStatus::success()); + case.set_classname("ruff"); + test_suite.add_test_case(case); + report.add_test_suite(test_suite); + } else { + for (filename, diagnostics) in group_diagnostics_by_filename(diagnostics, self.resolver) + { + let mut test_suite = TestSuite::new(filename); + test_suite + .extra + .insert(XmlString::new("package"), XmlString::new("org.ruff")); + + let classname = Path::new(filename).with_extension(""); + + for diagnostic in diagnostics { + let DiagnosticWithLocation { + diagnostic, + start_location: location, + } = diagnostic; + let mut status = TestCaseStatus::non_success(NonSuccessKind::Failure); + status.set_message(diagnostic.body()); + + if let Some(location) = location { + status.set_description(format!( + "line {row}, col {col}, {body}", + row = location.line, + col = location.column, + body = diagnostic.body() + )); + } else { + status.set_description(diagnostic.body()); + } + + let code = diagnostic + .secondary_code() + .map_or_else(|| diagnostic.name(), SecondaryCode::as_str); + let mut case = TestCase::new(format!("org.ruff.{code}"), status); + case.set_classname(classname.to_str().unwrap()); + + if let Some(location) = location { + case.extra.insert( + XmlString::new("line"), + XmlString::new(location.line.to_string()), + ); + case.extra.insert( + XmlString::new("column"), + XmlString::new(location.column.to_string()), + ); + } + + test_suite.add_test_case(case); + } + report.add_test_suite(test_suite); + } + } + + let adapter = FmtAdapter { fmt: f }; + report.serialize(adapter).map_err(|_| std::fmt::Error) + } +} + +// TODO(brent) this and `group_diagnostics_by_filename` are also used by the `grouped` output +// format. I think they'd make more sense in that file, but I started here first. I'll move them to +// that module when adding the `grouped` output format. +struct DiagnosticWithLocation<'a> { + diagnostic: &'a Diagnostic, + start_location: Option, +} + +impl Deref for DiagnosticWithLocation<'_> { + type Target = Diagnostic; + + fn deref(&self) -> &Self::Target { + self.diagnostic + } +} + +fn group_diagnostics_by_filename<'a>( + diagnostics: &'a [Diagnostic], + resolver: &'a dyn FileResolver, +) -> BTreeMap<&'a str, Vec>> { + let mut grouped_diagnostics = BTreeMap::default(); + for diagnostic in diagnostics { + let (filename, start_location) = diagnostic + .primary_span_ref() + .map(|span| { + let file = span.file(); + let start_location = + span.range() + .filter(|_| !resolver.is_notebook(file)) + .map(|range| { + file.diagnostic_source(resolver) + .as_source_code() + .line_column(range.start()) + }); + + (span.file().path(resolver), start_location) + }) + .unwrap_or_default(); + + grouped_diagnostics + .entry(filename) + .or_insert_with(Vec::new) + .push(DiagnosticWithLocation { + diagnostic, + start_location, + }); + } + grouped_diagnostics +} + +struct FmtAdapter<'a> { + fmt: &'a mut dyn std::fmt::Write, +} + +impl std::io::Write for FmtAdapter<'_> { + fn write(&mut self, buf: &[u8]) -> std::io::Result { + self.fmt + .write_str(std::str::from_utf8(buf).map_err(|_| { + std::io::Error::new( + std::io::ErrorKind::InvalidData, + "Invalid UTF-8 in JUnit report", + ) + })?) + .map_err(std::io::Error::other)?; + + Ok(buf.len()) + } + + fn flush(&mut self) -> std::io::Result<()> { + Ok(()) + } + + fn write_fmt(&mut self, args: std::fmt::Arguments<'_>) -> std::io::Result<()> { + self.fmt.write_fmt(args).map_err(std::io::Error::other) + } +} + +#[cfg(test)] +mod tests { + use crate::diagnostic::{ + DiagnosticFormat, + render::tests::{create_diagnostics, create_syntax_error_diagnostics}, + }; + + #[test] + fn output() { + let (env, diagnostics) = create_diagnostics(DiagnosticFormat::Junit); + insta::assert_snapshot!(env.render_diagnostics(&diagnostics)); + } + + #[test] + fn syntax_errors() { + let (env, diagnostics) = create_syntax_error_diagnostics(DiagnosticFormat::Junit); + insta::assert_snapshot!(env.render_diagnostics(&diagnostics)); + } +} diff --git a/crates/ruff_linter/src/message/snapshots/ruff_linter__message__junit__tests__output.snap b/crates/ruff_db/src/diagnostic/render/snapshots/ruff_db__diagnostic__render__junit__tests__output.snap similarity index 90% rename from crates/ruff_linter/src/message/snapshots/ruff_linter__message__junit__tests__output.snap rename to crates/ruff_db/src/diagnostic/render/snapshots/ruff_db__diagnostic__render__junit__tests__output.snap index 5f86514093..8b40b484f8 100644 --- a/crates/ruff_linter/src/message/snapshots/ruff_linter__message__junit__tests__output.snap +++ b/crates/ruff_db/src/diagnostic/render/snapshots/ruff_db__diagnostic__render__junit__tests__output.snap @@ -1,7 +1,6 @@ --- -source: crates/ruff_linter/src/message/junit.rs -expression: content -snapshot_kind: text +source: crates/ruff_db/src/diagnostic/render/junit.rs +expression: env.render_diagnostics(&diagnostics) --- diff --git a/crates/ruff_linter/src/message/snapshots/ruff_linter__message__junit__tests__syntax_errors.snap b/crates/ruff_db/src/diagnostic/render/snapshots/ruff_db__diagnostic__render__junit__tests__syntax_errors.snap similarity index 67% rename from crates/ruff_linter/src/message/snapshots/ruff_linter__message__junit__tests__syntax_errors.snap rename to crates/ruff_db/src/diagnostic/render/snapshots/ruff_db__diagnostic__render__junit__tests__syntax_errors.snap index 6c01051cbd..e44145b0f3 100644 --- a/crates/ruff_linter/src/message/snapshots/ruff_linter__message__junit__tests__syntax_errors.snap +++ b/crates/ruff_db/src/diagnostic/render/snapshots/ruff_db__diagnostic__render__junit__tests__syntax_errors.snap @@ -1,15 +1,14 @@ --- -source: crates/ruff_linter/src/message/junit.rs -expression: content -snapshot_kind: text +source: crates/ruff_db/src/diagnostic/render/junit.rs +expression: env.render_diagnostics(&diagnostics) --- - + line 1, col 15, SyntaxError: Expected one or more symbol names after import - + line 3, col 12, SyntaxError: Expected ')', found newline diff --git a/crates/ruff_linter/Cargo.toml b/crates/ruff_linter/Cargo.toml index 835e24a6e4..588d2a33fe 100644 --- a/crates/ruff_linter/Cargo.toml +++ b/crates/ruff_linter/Cargo.toml @@ -15,7 +15,7 @@ license = { workspace = true } [dependencies] ruff_annotate_snippets = { workspace = true } ruff_cache = { workspace = true } -ruff_db = { workspace = true, features = ["serde"] } +ruff_db = { workspace = true, features = ["junit", "serde"] } ruff_diagnostics = { workspace = true, features = ["serde"] } ruff_notebook = { workspace = true } ruff_macros = { workspace = true } @@ -55,7 +55,6 @@ path-absolutize = { workspace = true, features = [ pathdiff = { workspace = true } pep440_rs = { workspace = true } pyproject-toml = { workspace = true } -quick-junit = { workspace = true } regex = { workspace = true } rustc-hash = { workspace = true } schemars = { workspace = true, optional = true } diff --git a/crates/ruff_linter/src/message/junit.rs b/crates/ruff_linter/src/message/junit.rs deleted file mode 100644 index bec60249d0..0000000000 --- a/crates/ruff_linter/src/message/junit.rs +++ /dev/null @@ -1,117 +0,0 @@ -use std::io::Write; -use std::path::Path; - -use quick_junit::{NonSuccessKind, Report, TestCase, TestCaseStatus, TestSuite, XmlString}; - -use ruff_db::diagnostic::Diagnostic; -use ruff_source_file::LineColumn; - -use crate::message::{Emitter, EmitterContext, MessageWithLocation, group_diagnostics_by_filename}; - -#[derive(Default)] -pub struct JunitEmitter; - -impl Emitter for JunitEmitter { - fn emit( - &mut self, - writer: &mut dyn Write, - diagnostics: &[Diagnostic], - context: &EmitterContext, - ) -> anyhow::Result<()> { - let mut report = Report::new("ruff"); - - if diagnostics.is_empty() { - let mut test_suite = TestSuite::new("ruff"); - test_suite - .extra - .insert(XmlString::new("package"), XmlString::new("org.ruff")); - let mut case = TestCase::new("No errors found", TestCaseStatus::success()); - case.set_classname("ruff"); - test_suite.add_test_case(case); - report.add_test_suite(test_suite); - } else { - for (filename, messages) in group_diagnostics_by_filename(diagnostics) { - let mut test_suite = TestSuite::new(&filename); - test_suite - .extra - .insert(XmlString::new("package"), XmlString::new("org.ruff")); - - for message in messages { - let MessageWithLocation { - message, - start_location, - } = message; - let mut status = TestCaseStatus::non_success(NonSuccessKind::Failure); - status.set_message(message.body()); - let location = if context.is_notebook(&message.expect_ruff_filename()) { - // We can't give a reasonable location for the structured formats, - // so we show one that's clearly a fallback - LineColumn::default() - } else { - start_location - }; - - status.set_description(format!( - "line {row}, col {col}, {body}", - row = location.line, - col = location.column, - body = message.body() - )); - let mut case = TestCase::new( - if let Some(code) = message.secondary_code() { - format!("org.ruff.{code}") - } else { - "org.ruff".to_string() - }, - status, - ); - let file_path = Path::new(&*filename); - let file_stem = file_path.file_stem().unwrap().to_str().unwrap(); - let classname = file_path.parent().unwrap().join(file_stem); - case.set_classname(classname.to_str().unwrap()); - case.extra.insert( - XmlString::new("line"), - XmlString::new(location.line.to_string()), - ); - case.extra.insert( - XmlString::new("column"), - XmlString::new(location.column.to_string()), - ); - - test_suite.add_test_case(case); - } - report.add_test_suite(test_suite); - } - } - - report.serialize(writer)?; - - Ok(()) - } -} - -#[cfg(test)] -mod tests { - use insta::assert_snapshot; - - use crate::message::JunitEmitter; - use crate::message::tests::{ - capture_emitter_output, create_diagnostics, create_syntax_error_diagnostics, - }; - - #[test] - fn output() { - let mut emitter = JunitEmitter; - let content = capture_emitter_output(&mut emitter, &create_diagnostics()); - - assert_snapshot!(content); - } - - #[test] - fn syntax_errors() { - let mut emitter = JunitEmitter; - let content = capture_emitter_output(&mut emitter, &create_syntax_error_diagnostics()); - - assert_snapshot!(content); - } -} diff --git a/crates/ruff_linter/src/message/mod.rs b/crates/ruff_linter/src/message/mod.rs index 2f0cb9414c..417ae1fbb5 100644 --- a/crates/ruff_linter/src/message/mod.rs +++ b/crates/ruff_linter/src/message/mod.rs @@ -14,7 +14,6 @@ use ruff_db::files::File; pub use github::GithubEmitter; pub use gitlab::GitlabEmitter; pub use grouped::GroupedEmitter; -pub use junit::JunitEmitter; use ruff_notebook::NotebookIndex; use ruff_source_file::{LineColumn, SourceFile}; use ruff_text_size::{Ranged, TextRange, TextSize}; @@ -28,7 +27,6 @@ mod diff; mod github; mod gitlab; mod grouped; -mod junit; mod sarif; mod text;