From a84b27e679137697bdb994e9c5f79d366fb4a945 Mon Sep 17 00:00:00 2001 From: Andrew Gallant Date: Tue, 4 Feb 2025 14:51:54 -0500 Subject: [PATCH] red_knot_test: add support for diagnostic snapshotting This ties together everything from the previous commits. Some interesting bits here are how the snapshot is generated (where we include relevant info to make it easier to review the snapshots) and also a tweak to how inline assertions are processed. This commit also includes some example snapshots just to get a sense of what they look like. Follow-up work should add more of these I think. --- Cargo.lock | 1 + .../resources/mdtest/import/basic.md | 10 ++ ...ructures_-_Unresolvable_module_import.snap | 28 ++++ ...ures_-_Unresolvable_submodule_imports.snap | 51 ++++++++ .../red_knot_python_semantic/tests/mdtest.rs | 12 +- crates/red_knot_test/Cargo.toml | 1 + crates/red_knot_test/src/lib.rs | 103 +++++++++++++-- crates/red_knot_test/src/parser.rs | 120 ++++++++++++++++++ 8 files changed, 308 insertions(+), 18 deletions(-) create mode 100644 crates/red_knot_python_semantic/resources/mdtest/snapshots/basic.md_-_Structures_-_Unresolvable_module_import.snap create mode 100644 crates/red_knot_python_semantic/resources/mdtest/snapshots/basic.md_-_Structures_-_Unresolvable_submodule_imports.snap diff --git a/Cargo.lock b/Cargo.lock index ffaedcc63c..9f7463a7c6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2518,6 +2518,7 @@ dependencies = [ "anyhow", "camino", "colored 3.0.0", + "insta", "memchr", "red_knot_python_semantic", "red_knot_vendored", diff --git a/crates/red_knot_python_semantic/resources/mdtest/import/basic.md b/crates/red_knot_python_semantic/resources/mdtest/import/basic.md index 44f5f078bb..69061b71bc 100644 --- a/crates/red_knot_python_semantic/resources/mdtest/import/basic.md +++ b/crates/red_knot_python_semantic/resources/mdtest/import/basic.md @@ -116,8 +116,18 @@ reveal_type(c.C) # revealed: Literal[C] class C: ... ``` +## Unresolvable module import + + + +```py +import zqzqzqzqzqzqzq # error: [unresolved-import] "Cannot resolve import `zqzqzqzqzqzqzq`" +``` + ## Unresolvable submodule imports + + ```py # Topmost component resolvable, submodule not resolvable: import a.foo # error: [unresolved-import] "Cannot resolve import `a.foo`" diff --git a/crates/red_knot_python_semantic/resources/mdtest/snapshots/basic.md_-_Structures_-_Unresolvable_module_import.snap b/crates/red_knot_python_semantic/resources/mdtest/snapshots/basic.md_-_Structures_-_Unresolvable_module_import.snap new file mode 100644 index 0000000000..c363925a97 --- /dev/null +++ b/crates/red_knot_python_semantic/resources/mdtest/snapshots/basic.md_-_Structures_-_Unresolvable_module_import.snap @@ -0,0 +1,28 @@ +--- +source: crates/red_knot_test/src/lib.rs +expression: snapshot +--- +--- +mdtest name: basic.md - Structures - Unresolvable module import +mdtest path: crates/red_knot_python_semantic/resources/mdtest/import/basic.md +--- + +# Python source files + +## mdtest_snippet__1.py + +``` +1 | import zqzqzqzqzqzqzq # error: [unresolved-import] "Cannot resolve import `zqzqzqzqzqzqzq`" +``` + +# Diagnostics + +``` +error: lint:unresolved-import + --> /src/mdtest_snippet__1.py:1:8 + | +1 | import zqzqzqzqzqzqzq # error: [unresolved-import] "Cannot resolve import `zqzqzqzqzqzqzq`" + | ^^^^^^^^^^^^^^ Cannot resolve import `zqzqzqzqzqzqzq` + | + +``` diff --git a/crates/red_knot_python_semantic/resources/mdtest/snapshots/basic.md_-_Structures_-_Unresolvable_submodule_imports.snap b/crates/red_knot_python_semantic/resources/mdtest/snapshots/basic.md_-_Structures_-_Unresolvable_submodule_imports.snap new file mode 100644 index 0000000000..ee1454a3af --- /dev/null +++ b/crates/red_knot_python_semantic/resources/mdtest/snapshots/basic.md_-_Structures_-_Unresolvable_submodule_imports.snap @@ -0,0 +1,51 @@ +--- +source: crates/red_knot_test/src/lib.rs +expression: snapshot +--- +--- +mdtest name: basic.md - Structures - Unresolvable submodule imports +mdtest path: crates/red_knot_python_semantic/resources/mdtest/import/basic.md +--- + +# Python source files + +## mdtest_snippet__1.py + +``` +1 | # Topmost component resolvable, submodule not resolvable: +2 | import a.foo # error: [unresolved-import] "Cannot resolve import `a.foo`" +3 | +4 | # Topmost component unresolvable: +5 | import b.foo # error: [unresolved-import] "Cannot resolve import `b.foo`" +``` + +## a/__init__.py + +``` +``` + +# Diagnostics + +``` +error: lint:unresolved-import + --> /src/mdtest_snippet__1.py:2:8 + | +1 | # Topmost component resolvable, submodule not resolvable: +2 | import a.foo # error: [unresolved-import] "Cannot resolve import `a.foo`" + | ^^^^^ Cannot resolve import `a.foo` +3 | +4 | # Topmost component unresolvable: + | + +``` + +``` +error: lint:unresolved-import + --> /src/mdtest_snippet__1.py:5:8 + | +4 | # Topmost component unresolvable: +5 | import b.foo # error: [unresolved-import] "Cannot resolve import `b.foo`" + | ^^^^^ Cannot resolve import `b.foo` + | + +``` diff --git a/crates/red_knot_python_semantic/tests/mdtest.rs b/crates/red_knot_python_semantic/tests/mdtest.rs index ba03247669..9c21cc51b2 100644 --- a/crates/red_knot_python_semantic/tests/mdtest.rs +++ b/crates/red_knot_python_semantic/tests/mdtest.rs @@ -8,20 +8,20 @@ use dir_test::{dir_test, Fixture}; )] #[allow(clippy::needless_pass_by_value)] fn mdtest(fixture: Fixture<&str>) { - let fixture_path = Utf8Path::new(fixture.path()); + let absolute_fixture_path = Utf8Path::new(fixture.path()); let crate_dir = Utf8Path::new(env!("CARGO_MANIFEST_DIR")); let snapshot_path = crate_dir.join("resources").join("mdtest").join("snapshots"); let workspace_root = crate_dir.ancestors().nth(2).unwrap(); - let long_title = fixture_path.strip_prefix(workspace_root).unwrap(); - let short_title = fixture_path.file_name().unwrap(); + let relative_fixture_path = absolute_fixture_path.strip_prefix(workspace_root).unwrap(); + let short_title = absolute_fixture_path.file_name().unwrap(); - let test_name = test_name("mdtest", fixture_path); + let test_name = test_name("mdtest", absolute_fixture_path); red_knot_test::run( - fixture_path, + absolute_fixture_path, + relative_fixture_path, &snapshot_path, - long_title.as_str(), short_title, &test_name, ); diff --git a/crates/red_knot_test/Cargo.toml b/crates/red_knot_test/Cargo.toml index 4f974b5a52..b18c09c178 100644 --- a/crates/red_knot_test/Cargo.toml +++ b/crates/red_knot_test/Cargo.toml @@ -22,6 +22,7 @@ ruff_text_size = { workspace = true } anyhow = { workspace = true } camino = { workspace = true } colored = { workspace = true } +insta = { workspace = true, features = ["filters"] } memchr = { workspace = true } regex = { workspace = true } rustc-hash = { workspace = true } diff --git a/crates/red_knot_test/src/lib.rs b/crates/red_knot_test/src/lib.rs index bc0e5205a9..632209be95 100644 --- a/crates/red_knot_test/src/lib.rs +++ b/crates/red_knot_test/src/lib.rs @@ -28,17 +28,17 @@ const MDTEST_TEST_FILTER: &str = "MDTEST_TEST_FILTER"; /// Panic on test failure, and print failure details. #[allow(clippy::print_stdout)] pub fn run( - fixture_path: &Utf8Path, + absolute_fixture_path: &Utf8Path, + relative_fixture_path: &Utf8Path, snapshot_path: &Utf8Path, - long_title: &str, short_title: &str, test_name: &str, ) { - let source = std::fs::read_to_string(fixture_path).unwrap(); + let source = std::fs::read_to_string(absolute_fixture_path).unwrap(); let suite = match test_parser::parse(short_title, &source) { Ok(suite) => suite, Err(err) => { - panic!("Error parsing `{fixture_path}`: {err:?}") + panic!("Error parsing `{absolute_fixture_path}`: {err:?}") } }; @@ -60,7 +60,7 @@ pub fn run( db.memory_file_system().remove_all(); Files::sync_all(&mut db); - if let Err(failures) = run_test(&mut db, snapshot_path, &test) { + if let Err(failures) = run_test(&mut db, relative_fixture_path, snapshot_path, &test) { any_failures = true; println!("\n{}\n", test.name().bold().underline()); @@ -73,7 +73,8 @@ pub fn run( for failure in failures { let absolute_line_number = backtick_line.checked_add(relative_line_number).unwrap(); - let line_info = format!("{long_title}:{absolute_line_number}").cyan(); + let line_info = + format!("{relative_fixture_path}:{absolute_line_number}").cyan(); println!(" {line_info} {failure}"); } } @@ -97,6 +98,7 @@ pub fn run( fn run_test( db: &mut db::Db, + relative_fixture_path: &Utf8Path, snapshot_path: &Utf8Path, test: &parser::MarkdownTest, ) -> Result<(), Failures> { @@ -186,6 +188,10 @@ fn run_test( ) .expect("Failed to update Program settings in TestDb"); + // When snapshot testing is enabled, this is populated with + // all diagnostics. Otherwise it remains empty. + let mut snapshot_diagnostics = vec![]; + let failures: Failures = test_files .into_iter() .filter_map(|test_file| { @@ -234,16 +240,36 @@ fn run_test( diagnostic })); - match matcher::match_file(db, test_file.file, diagnostics) { - Ok(()) => None, - Err(line_failures) => Some(FileFailures { - backtick_offset: test_file.backtick_offset, - by_line: line_failures, - }), + let failure = + match matcher::match_file(db, test_file.file, diagnostics.iter().map(|d| &**d)) { + Ok(()) => None, + Err(line_failures) => Some(FileFailures { + backtick_offset: test_file.backtick_offset, + by_line: line_failures, + }), + }; + if test.should_snapshot_diagnostics() { + snapshot_diagnostics.extend(diagnostics); } + failure }) .collect(); + if !snapshot_diagnostics.is_empty() { + let snapshot = + create_diagnostic_snapshot(db, relative_fixture_path, test, snapshot_diagnostics); + let name = test.name().replace(' ', "_"); + insta::with_settings!( + { + snapshot_path => snapshot_path, + input_file => name.clone(), + filters => vec![(r"\\", "/")], + prepend_module_to_snapshot => false, + }, + { insta::assert_snapshot!(name, snapshot) } + ); + } + if failures.is_empty() { Ok(()) } else { @@ -254,6 +280,7 @@ fn run_test( type Failures = Vec; /// The failures for a single file in a test by line number. +#[derive(Debug)] struct FileFailures { /// The offset of the backticks that starts the code block in the Markdown file backtick_offset: TextSize, @@ -268,3 +295,55 @@ struct TestFile { // Offset of the backticks that starts the code block in the Markdown file backtick_offset: TextSize, } + +fn create_diagnostic_snapshot( + db: &mut db::Db, + relative_fixture_path: &Utf8Path, + test: &parser::MarkdownTest, + diagnostics: impl IntoIterator, +) -> String { + // TODO(ag): Do something better than requiring this + // global state to be twiddled everywhere. + colored::control::set_override(false); + + let mut snapshot = String::new(); + writeln!(snapshot).unwrap(); + writeln!(snapshot, "---").unwrap(); + writeln!(snapshot, "mdtest name: {}", test.name()).unwrap(); + writeln!(snapshot, "mdtest path: {relative_fixture_path}").unwrap(); + writeln!(snapshot, "---").unwrap(); + writeln!(snapshot).unwrap(); + + writeln!(snapshot, "# Python source files").unwrap(); + writeln!(snapshot).unwrap(); + for file in test.files() { + writeln!(snapshot, "## {}", file.path).unwrap(); + writeln!(snapshot).unwrap(); + // Note that we don't use ```py here because the line numbering + // we add makes it invalid Python. This sacrifices syntax + // highlighting when you look at the snapshot on GitHub, + // but the line numbers are extremely useful for analyzing + // snapshots. So we keep them. + writeln!(snapshot, "```").unwrap(); + + let line_number_width = file.code.lines().count().to_string().len(); + for (i, line) in file.code.lines().enumerate() { + let line_number = i + 1; + writeln!(snapshot, "{line_number:>line_number_width$} | {line}").unwrap(); + } + writeln!(snapshot, "```").unwrap(); + writeln!(snapshot).unwrap(); + } + + writeln!(snapshot, "# Diagnostics").unwrap(); + writeln!(snapshot).unwrap(); + for (i, diag) in diagnostics.into_iter().enumerate() { + if i > 0 { + writeln!(snapshot).unwrap(); + } + writeln!(snapshot, "```").unwrap(); + writeln!(snapshot, "{}", diag.display(db)).unwrap(); + writeln!(snapshot, "```").unwrap(); + } + snapshot +} diff --git a/crates/red_knot_test/src/parser.rs b/crates/red_knot_test/src/parser.rs index 8463616506..872abbd5b0 100644 --- a/crates/red_knot_test/src/parser.rs +++ b/crates/red_knot_test/src/parser.rs @@ -73,6 +73,10 @@ impl<'m, 's> MarkdownTest<'m, 's> { pub(crate) fn configuration(&self) -> &MarkdownTestConfig { &self.section.config } + + pub(super) fn should_snapshot_diagnostics(&self) -> bool { + self.section.snapshot_diagnostics + } } /// Iterator yielding all [`MarkdownTest`]s in a [`MarkdownTestSuite`]. @@ -122,6 +126,7 @@ struct Section<'s> { level: u8, parent_id: Option, config: MarkdownTestConfig, + snapshot_diagnostics: bool, } #[newtype_index] @@ -226,6 +231,7 @@ impl<'s> Parser<'s> { level: 0, parent_id: None, config: MarkdownTestConfig::default(), + snapshot_diagnostics: false, }); Self { sections, @@ -284,10 +290,27 @@ impl<'s> Parser<'s> { } fn parse_impl(&mut self) -> anyhow::Result<()> { + const SECTION_CONFIG_SNAPSHOT: &str = ""; const CODE_BLOCK_END: &[u8] = b"```"; while let Some(first) = self.cursor.bump() { match first { + '<' => { + self.explicit_path = None; + self.preceding_blank_lines = 0; + // If we want to support more comment directives, then we should + // probably just parse the directive generically first. But it's + // not clear if we'll want to add more, since comments are hidden + // from GitHub Markdown rendering. + if self + .cursor + .as_str() + .starts_with(&SECTION_CONFIG_SNAPSHOT[1..]) + { + self.cursor.skip_bytes(SECTION_CONFIG_SNAPSHOT.len() - 1); + self.process_snapshot_diagnostics()?; + } + } '#' => { self.explicit_path = None; self.preceding_blank_lines = 0; @@ -402,6 +425,7 @@ impl<'s> Parser<'s> { level: header_level.try_into()?, parent_id: Some(parent), config: self.sections[parent].config.clone(), + snapshot_diagnostics: self.sections[parent].snapshot_diagnostics, }; if self.current_section_files.is_some() { @@ -500,6 +524,32 @@ impl<'s> Parser<'s> { Ok(()) } + fn process_snapshot_diagnostics(&mut self) -> anyhow::Result<()> { + if self.current_section_has_config { + bail!( + "Section config to enable snapshotting diagnostics must come before \ + everything else (including TOML configuration blocks).", + ); + } + if self.current_section_files.is_some() { + bail!( + "Section config to enable snapshotting diagnostics must come before \ + everything else (including embedded files).", + ); + } + + let current_section = &mut self.sections[self.stack.top()]; + if current_section.snapshot_diagnostics { + bail!( + "Section config to enable snapshotting diagnostics should appear \ + at most once.", + ); + } + current_section.snapshot_diagnostics = true; + + Ok(()) + } + fn pop_sections_to_level(&mut self, level: usize) { while level <= self.sections[self.stack.top()].level.into() { self.stack.pop(); @@ -1295,4 +1345,74 @@ mod tests { let err = super::parse("file.md", &source).expect_err("Should fail to parse"); assert_eq!(err.to_string(), "Trailing code-block metadata is not supported. Only the code block language can be specified."); } + + #[test] + fn duplicate_section_directive_not_allowed() { + let source = dedent( + " + # Some header + + + + + ```py + x = 1 + ``` + ", + ); + let err = super::parse("file.md", &source).expect_err("Should fail to parse"); + assert_eq!( + err.to_string(), + "Section config to enable snapshotting diagnostics should appear at most once.", + ); + } + + #[test] + fn section_directive_must_appear_before_config() { + let source = dedent( + " + # Some header + + ```toml + [environment] + typeshed = \"/typeshed\" + ``` + + + + ```py + x = 1 + ``` + ", + ); + let err = super::parse("file.md", &source).expect_err("Should fail to parse"); + assert_eq!( + err.to_string(), + "Section config to enable snapshotting diagnostics must \ + come before everything else \ + (including TOML configuration blocks).", + ); + } + + #[test] + fn section_directive_must_appear_before_embedded_files() { + let source = dedent( + " + # Some header + + ```py + x = 1 + ``` + + + ", + ); + let err = super::parse("file.md", &source).expect_err("Should fail to parse"); + assert_eq!( + err.to_string(), + "Section config to enable snapshotting diagnostics must \ + come before everything else \ + (including embedded files).", + ); + } }