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).", + ); + } }