From d8cd78c94f2d1b68e3c9d00367b9a9b5aa662d02 Mon Sep 17 00:00:00 2001 From: Dhruv Manilawala Date: Sat, 6 Apr 2024 11:03:20 +0530 Subject: [PATCH] Use `test_ok` for inline tests, merge test report (#10801) ## Summary This PR makes two quality of life improvements for parser's inline test implementation: 1. Rename `test` to `test_ok` to explicitly specify that this is a valid syntax test 2. Merge the reporting of "ok" and "err" test cases and unreferenced test cases The output of (2) now looks like: ``` ---- generate_inline_tests stdout ---- Error: Unreferenced test files found for which no comment exists: crates/ruff_python_parser/resources/inline/err/something_else.py Please delete these files manually Following files were not up-to date and has been updated: crates/ruff_python_parser/resources/inline/err/renamed_it.py Re-run the tests with `cargo test` to update the test snapshots ``` ## Test Plan 1. Add two random test cases: ```rs // test_ok something // x = 1 // test_err something_else // [1, 2 ``` 2. Run the generation step: ```console cargo test --package ruff_python_parser --test generate_inline_tests ``` 3. Rename `something_else` to `renamed_it` and run the generation step again to see the above output --- .gitattributes | 2 + crates/ruff_python_parser/CONTRIBUTING.md | 6 +- .../src/parser/expression.rs | 4 +- crates/ruff_python_parser/src/parser/mod.rs | 2 +- .../src/parser/statement.rs | 8 +- .../tests/generate_inline_tests.rs | 122 ++++++++++++------ 6 files changed, 95 insertions(+), 49 deletions(-) diff --git a/.gitattributes b/.gitattributes index d7d5267dea592..610c6b39ba8dd 100644 --- a/.gitattributes +++ b/.gitattributes @@ -8,5 +8,7 @@ crates/ruff_linter/resources/test/fixtures/pycodestyle/W391_3.py text eol=crlf crates/ruff_python_formatter/resources/test/fixtures/ruff/docstring_code_examples_crlf.py text eol=crlf crates/ruff_python_formatter/tests/snapshots/format@docstring_code_examples_crlf.py.snap text eol=crlf +crates/ruff_python_parser/resources/inline linguist-generated=true + ruff.schema.json linguist-generated=true text=auto eol=lf *.md.snap linguist-language=Markdown diff --git a/crates/ruff_python_parser/CONTRIBUTING.md b/crates/ruff_python_parser/CONTRIBUTING.md index defbf943285b1..ec8c0ccfddb6a 100644 --- a/crates/ruff_python_parser/CONTRIBUTING.md +++ b/crates/ruff_python_parser/CONTRIBUTING.md @@ -13,7 +13,7 @@ Test that the parser successfully parses the input with no syntax errors. They'r written in the following format: ```rs -// test this_is_the_test_name +// test_ok this_is_the_test_name // def foo(): // pass println!("some rust code"); @@ -28,14 +28,14 @@ in the following format: println!("some rust code"); ``` -Note that the difference between the two is the `test` and `test_err` keywords. +Note that the difference between the two is the `test_ok` and `test_err` keywords. The comment block must be independent of any other comment blocks. For example, the following is not extracted: ```rs // Some random comment // -// test this_is_the_test_name +// test_ok this_is_the_test_name // def foo(): // pass println!("some rust code"); diff --git a/crates/ruff_python_parser/src/parser/expression.rs b/crates/ruff_python_parser/src/parser/expression.rs index e601acd6d95a1..ce365fd8092a2 100644 --- a/crates/ruff_python_parser/src/parser/expression.rs +++ b/crates/ruff_python_parser/src/parser/expression.rs @@ -2294,7 +2294,7 @@ impl<'src> Parser<'src> { self.bump(TokenKind::Lambda); let parameters = if self.at(TokenKind::Colon) { - // test lambda_with_no_parameters + // test_ok lambda_with_no_parameters // lambda: 1 None } else { @@ -2303,7 +2303,7 @@ impl<'src> Parser<'src> { self.expect(TokenKind::Colon); - // test lambda_with_valid_body + // test_ok lambda_with_valid_body // lambda x: x // lambda x: x if True else y // lambda x: await x diff --git a/crates/ruff_python_parser/src/parser/mod.rs b/crates/ruff_python_parser/src/parser/mod.rs index ef195fbab6e7f..f89fa3f8291d6 100644 --- a/crates/ruff_python_parser/src/parser/mod.rs +++ b/crates/ruff_python_parser/src/parser/mod.rs @@ -958,7 +958,7 @@ impl RecoveryContextKind { // When the parser is parsing f-string elements inside format spec, // the terminator would be `}`. - // test fstring_format_spec_terminator + // test_ok fstring_format_spec_terminator // f"hello {x:} world" // f"hello {x:.3f} world" TokenKind::Rbrace, diff --git a/crates/ruff_python_parser/src/parser/statement.rs b/crates/ruff_python_parser/src/parser/statement.rs index d87fbf9e8ae76..f73929d9bedc4 100644 --- a/crates/ruff_python_parser/src/parser/statement.rs +++ b/crates/ruff_python_parser/src/parser/statement.rs @@ -456,7 +456,7 @@ impl<'src> Parser<'src> { None }; - // test from_import_no_space + // test_ok from_import_no_space // from.import x // from...import x self.expect(TokenKind::Import); @@ -580,7 +580,7 @@ impl<'src> Parser<'src> { dotted_name.push_str(&self.parse_identifier()); } - // test dotted_name_normalized_spaces + // test_ok dotted_name_normalized_spaces // import a.b.c // import a . b . c ast::Identifier { @@ -714,7 +714,7 @@ impl<'src> Parser<'src> { self.add_error(ParseErrorType::EmptyGlobalNames, self.current_token_range()); } - // test global_stmt + // test_ok global_stmt // global x // global x, y, z ast::StmtGlobal { @@ -755,7 +755,7 @@ impl<'src> Parser<'src> { ); } - // test nonlocal_stmt + // test_ok nonlocal_stmt // nonlocal x // nonlocal x, y, z ast::StmtNonlocal { diff --git a/crates/ruff_python_parser/tests/generate_inline_tests.rs b/crates/ruff_python_parser/tests/generate_inline_tests.rs index c1bb59a0eb051..7f71d74717e3b 100644 --- a/crates/ruff_python_parser/tests/generate_inline_tests.rs +++ b/crates/ruff_python_parser/tests/generate_inline_tests.rs @@ -6,12 +6,12 @@ //! - //! - use std::collections::HashMap; +use std::fmt; use std::fs; -use std::ops::{Deref, DerefMut}; +use std::ops::{AddAssign, Deref, DerefMut}; use std::path::{Path, PathBuf}; use anyhow::{Context, Result}; -use itertools::Itertools; fn project_root() -> PathBuf { PathBuf::from(env!("CARGO_MANIFEST_DIR")) @@ -25,13 +25,80 @@ fn generate_inline_tests() -> Result<()> { let parser_dir = project_root().join("crates/ruff_python_parser/src/parser"); let tests = TestCollection::try_from(parser_dir.as_path())?; - install_tests(&tests.ok, "crates/ruff_python_parser/resources/inline/ok")?; - install_tests(&tests.err, "crates/ruff_python_parser/resources/inline/err")?; + let mut test_files = TestFiles::default(); + test_files += install_tests(&tests.ok, "crates/ruff_python_parser/resources/inline/ok")?; + test_files += install_tests(&tests.err, "crates/ruff_python_parser/resources/inline/err")?; + + if !test_files.is_empty() { + anyhow::bail!("{}", test_files); + } Ok(()) } -fn install_tests(tests: &HashMap, target_dir: &str) -> Result<()> { +#[derive(Debug, Default)] +struct TestFiles { + unreferenced: Vec, + updated: Vec, +} + +impl TestFiles { + fn is_empty(&self) -> bool { + self.unreferenced.is_empty() && self.updated.is_empty() + } +} + +impl AddAssign for TestFiles { + fn add_assign(&mut self, other: TestFiles) { + self.unreferenced.extend(other.unreferenced); + self.updated.extend(other.updated); + } +} + +impl fmt::Display for TestFiles { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + if self.is_empty() { + writeln!(f, "No unreferenced or updated test files found") + } else { + let root_dir = project_root(); + if !self.unreferenced.is_empty() { + writeln!( + f, + "Unreferenced test files found for which no comment exists:", + )?; + for path in &self.unreferenced { + writeln!(f, " {}", path.strip_prefix(&root_dir).unwrap().display())?; + } + writeln!(f, "Please delete these files manually")?; + } + if !self.updated.is_empty() { + if !self.unreferenced.is_empty() { + writeln!(f)?; + } + writeln!( + f, + "Following files were not up-to date and has been updated:", + )?; + for path in &self.updated { + writeln!(f, " {}", path.strip_prefix(&root_dir).unwrap().display())?; + } + writeln!( + f, + "Re-run the tests with `cargo test` to update the test snapshots" + )?; + if std::env::var("CI").is_ok() { + writeln!( + f, + "NOTE: Run the tests locally and commit the updated files" + )?; + } + } + Ok(()) + } + } +} + +fn install_tests(tests: &HashMap, target_dir: &str) -> Result { let root_dir = project_root(); let tests_dir = root_dir.join(target_dir); if !tests_dir.is_dir() { @@ -41,22 +108,6 @@ fn install_tests(tests: &HashMap, target_dir: &str) -> Result<()> // Test kind is irrelevant for existing test cases. let existing = existing_tests(&tests_dir)?; - let unreferenced = existing - .iter() - .filter(|&(name, _)| !tests.contains_key(name)) - .map(|(_, path)| path) - .collect::>(); - - if !unreferenced.is_empty() { - anyhow::bail!( - "Unreferenced test files found for which no comment exists:\n {}\nPlease delete these files manually\n", - unreferenced - .iter() - .map(|path| path.strip_prefix(&root_dir).unwrap().display()) - .join("\n ") - ); - } - let mut updated_files = vec![]; for (name, test) in tests { @@ -73,21 +124,14 @@ fn install_tests(tests: &HashMap, target_dir: &str) -> Result<()> updated_files.push(path); } - if !updated_files.is_empty() { - let mut message = format!( - "Following files were not up-to date and has been updated:\n {}\nRe-run the tests with `cargo test` to update the test snapshots\n", - updated_files - .iter() - .map(|path| path.strip_prefix(&root_dir).unwrap().display()) - .join("\n ") - ); - if std::env::var("CI").is_ok() { - message.push_str("NOTE: Run the tests locally and commit the updated files\n"); - } - anyhow::bail!(message); - } - - Ok(()) + Ok(TestFiles { + unreferenced: existing + .into_iter() + .filter(|(name, _)| !tests.contains_key(name)) + .map(|(_, path)| path) + .collect::>(), + updated: updated_files, + }) } #[derive(Default, Debug)] @@ -115,7 +159,7 @@ impl TryFrom<&Path> for TestCollection { if test.is_ok() { if let Some(old_test) = tests.ok.insert(test.name.clone(), test) { anyhow::bail!( - "Duplicate test found: {name:?} (search '// test {name}' for the location)\n", + "Duplicate test found: {name:?} (search '// test_ok {name}' for the location)\n", name = old_test.name ); } @@ -141,7 +185,7 @@ enum TestKind { /// A test of the following form: /// /// ```text -/// // (test|test_err) name +/// // (test_ok|test_err) name /// // /// ``` #[derive(Debug)] @@ -165,7 +209,7 @@ fn collect_tests(text: &str) -> Vec { let first_line = &comment_block[0]; let (kind, name) = match first_line.split_once(' ') { - Some(("test", suffix)) => (TestKind::Ok, suffix), + Some(("test_ok", suffix)) => (TestKind::Ok, suffix), Some(("test_err", suffix)) => (TestKind::Err, suffix), _ => continue, };