From f08a5f67eb99935f130ba6428aaa0ce889b0f833 Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Fri, 13 Oct 2023 06:24:12 +0530 Subject: [PATCH] Add test for Notebook text output (#7925) ## Summary This PR adds test cases for the Notebook output in text format. ## Test Plan Update test snapshots. --- crates/ruff_linter/src/message/diff.rs | 1 + crates/ruff_linter/src/message/mod.rs | 122 +++++++++++++++++- ...message__text__tests__notebook_output.snap | 32 +++++ crates/ruff_linter/src/message/text.rs | 17 ++- crates/ruff_notebook/src/index.rs | 7 + 5 files changed, 177 insertions(+), 2 deletions(-) create mode 100644 crates/ruff_linter/src/message/snapshots/ruff_linter__message__text__tests__notebook_output.snap diff --git a/crates/ruff_linter/src/message/diff.rs b/crates/ruff_linter/src/message/diff.rs index 4c487b1b2d0b4..6bfe8750c4a36 100644 --- a/crates/ruff_linter/src/message/diff.rs +++ b/crates/ruff_linter/src/message/diff.rs @@ -35,6 +35,7 @@ impl<'a> Diff<'a> { impl Display for Diff<'_> { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + // TODO(dhruvmanila): Add support for Notebook cells once it's user-facing let mut output = String::with_capacity(self.source_code.source_text().len()); let mut last_end = TextSize::default(); diff --git a/crates/ruff_linter/src/message/mod.rs b/crates/ruff_linter/src/message/mod.rs index 0c243871a40a6..0e5520b5a75ff 100644 --- a/crates/ruff_linter/src/message/mod.rs +++ b/crates/ruff_linter/src/message/mod.rs @@ -150,7 +150,8 @@ mod tests { use rustc_hash::FxHashMap; use ruff_diagnostics::{Diagnostic, DiagnosticKind, Edit, Fix}; - use ruff_source_file::SourceFileBuilder; + use ruff_notebook::NotebookIndex; + use ruff_source_file::{OneIndexed, SourceFileBuilder}; use ruff_text_size::{Ranged, TextRange, TextSize}; use crate::message::{Emitter, EmitterContext, Message}; @@ -221,6 +222,113 @@ def fibonacci(n): ] } + pub(super) fn create_notebook_messages() -> (Vec, FxHashMap) { + let notebook = r"# cell 1 +import os +# cell 2 +import math + +print('hello world') +# cell 3 +def foo(): + print() + x = 1 +"; + + let unused_import_os = Diagnostic::new( + DiagnosticKind { + name: "UnusedImport".to_string(), + body: "`os` imported but unused".to_string(), + suggestion: Some("Remove unused import: `os`".to_string()), + }, + TextRange::new(TextSize::from(16), TextSize::from(18)), + ) + .with_fix(Fix::safe_edit(Edit::range_deletion(TextRange::new( + TextSize::from(9), + TextSize::from(19), + )))); + + let unused_import_math = Diagnostic::new( + DiagnosticKind { + name: "UnusedImport".to_string(), + body: "`math` imported but unused".to_string(), + suggestion: Some("Remove unused import: `math`".to_string()), + }, + TextRange::new(TextSize::from(35), TextSize::from(39)), + ) + .with_fix(Fix::safe_edit(Edit::range_deletion(TextRange::new( + TextSize::from(28), + TextSize::from(40), + )))); + + let unused_variable = Diagnostic::new( + DiagnosticKind { + name: "UnusedVariable".to_string(), + body: "Local variable `x` is assigned to but never used".to_string(), + suggestion: Some("Remove assignment to unused variable `x`".to_string()), + }, + TextRange::new(TextSize::from(98), TextSize::from(99)), + ) + .with_fix(Fix::unsafe_edit(Edit::deletion( + TextSize::from(94), + TextSize::from(104), + ))); + + let notebook_source = SourceFileBuilder::new("notebook.ipynb", notebook).finish(); + + let mut notebook_indexes = FxHashMap::default(); + notebook_indexes.insert( + "notebook.ipynb".to_string(), + NotebookIndex::new( + vec![ + OneIndexed::from_zero_indexed(0), + OneIndexed::from_zero_indexed(0), + OneIndexed::from_zero_indexed(1), + OneIndexed::from_zero_indexed(1), + OneIndexed::from_zero_indexed(1), + OneIndexed::from_zero_indexed(1), + OneIndexed::from_zero_indexed(2), + OneIndexed::from_zero_indexed(2), + OneIndexed::from_zero_indexed(2), + OneIndexed::from_zero_indexed(2), + ], + vec![ + OneIndexed::from_zero_indexed(0), + OneIndexed::from_zero_indexed(1), + OneIndexed::from_zero_indexed(0), + OneIndexed::from_zero_indexed(1), + OneIndexed::from_zero_indexed(2), + OneIndexed::from_zero_indexed(3), + OneIndexed::from_zero_indexed(0), + OneIndexed::from_zero_indexed(1), + OneIndexed::from_zero_indexed(2), + OneIndexed::from_zero_indexed(3), + ], + ), + ); + + let unused_import_os_start = unused_import_os.start(); + let unused_import_math_start = unused_import_math.start(); + let unused_variable_start = unused_variable.start(); + + ( + vec![ + Message::from_diagnostic( + unused_import_os, + notebook_source.clone(), + unused_import_os_start, + ), + Message::from_diagnostic( + unused_import_math, + notebook_source.clone(), + unused_import_math_start, + ), + Message::from_diagnostic(unused_variable, notebook_source, unused_variable_start), + ], + notebook_indexes, + ) + } + pub(super) fn capture_emitter_output( emitter: &mut dyn Emitter, messages: &[Message], @@ -232,4 +340,16 @@ def fibonacci(n): String::from_utf8(output).expect("Output to be valid UTF-8") } + + pub(super) fn capture_emitter_notebook_output( + emitter: &mut dyn Emitter, + messages: &[Message], + notebook_indexes: &FxHashMap, + ) -> String { + let context = EmitterContext::new(notebook_indexes); + let mut output: Vec = Vec::new(); + emitter.emit(&mut output, messages, &context).unwrap(); + + String::from_utf8(output).expect("Output to be valid UTF-8") + } } diff --git a/crates/ruff_linter/src/message/snapshots/ruff_linter__message__text__tests__notebook_output.snap b/crates/ruff_linter/src/message/snapshots/ruff_linter__message__text__tests__notebook_output.snap new file mode 100644 index 0000000000000..5cd2f33939a8f --- /dev/null +++ b/crates/ruff_linter/src/message/snapshots/ruff_linter__message__text__tests__notebook_output.snap @@ -0,0 +1,32 @@ +--- +source: crates/ruff_linter/src/message/text.rs +expression: content +--- +notebook.ipynb:cell 1:2:8: F401 [*] `os` imported but unused + | +1 | # cell 1 +2 | import os + | ^^ F401 + | + = help: Remove unused import: `os` + +notebook.ipynb:cell 2:2:8: F401 [*] `math` imported but unused + | +1 | # cell 2 +2 | import math + | ^^^^ F401 +3 | +4 | print('hello world') + | + = help: Remove unused import: `math` + +notebook.ipynb:cell 3:4:5: F841 [*] Local variable `x` is assigned to but never used + | +2 | def foo(): +3 | print() +4 | x = 1 + | ^ F841 + | + = help: Remove assignment to unused variable `x` + + diff --git a/crates/ruff_linter/src/message/text.rs b/crates/ruff_linter/src/message/text.rs index 67bf97fe999e9..d43104d97dace 100644 --- a/crates/ruff_linter/src/message/text.rs +++ b/crates/ruff_linter/src/message/text.rs @@ -351,7 +351,10 @@ struct SourceCode<'a> { mod tests { use insta::assert_snapshot; - use crate::message::tests::{capture_emitter_output, create_messages}; + use crate::message::tests::{ + capture_emitter_notebook_output, capture_emitter_output, create_messages, + create_notebook_messages, + }; use crate::message::TextEmitter; use crate::settings::types::UnsafeFixes; @@ -383,4 +386,16 @@ mod tests { assert_snapshot!(content); } + + #[test] + fn notebook_output() { + let mut emitter = TextEmitter::default() + .with_show_fix_status(true) + .with_show_source(true) + .with_unsafe_fixes(UnsafeFixes::Enabled); + let (messages, notebook_indexes) = create_notebook_messages(); + let content = capture_emitter_notebook_output(&mut emitter, &messages, ¬ebook_indexes); + + assert_snapshot!(content); + } } diff --git a/crates/ruff_notebook/src/index.rs b/crates/ruff_notebook/src/index.rs index c6207ef27e69a..a001df375fd5d 100644 --- a/crates/ruff_notebook/src/index.rs +++ b/crates/ruff_notebook/src/index.rs @@ -15,6 +15,13 @@ pub struct NotebookIndex { } impl NotebookIndex { + pub fn new(row_to_cell: Vec, row_to_row_in_cell: Vec) -> Self { + Self { + row_to_cell, + row_to_row_in_cell, + } + } + /// Returns the cell number (1-based) for the given row (1-based). pub fn cell(&self, row: OneIndexed) -> Option { self.row_to_cell.get(row.to_zero_indexed()).copied()