From 289bafa3e02271248a9abda5255e996656655128 Mon Sep 17 00:00:00 2001 From: blyxyas Date: Tue, 4 Apr 2023 00:13:47 +0200 Subject: [PATCH 1/4] New chapter: Writing tests Co-authored-by: Nahua --- book/src/SUMMARY.md | 1 + book/src/development/writing_tests.md | 220 ++++++++++++++++++++++++++ 2 files changed, 221 insertions(+) create mode 100644 book/src/development/writing_tests.md diff --git a/book/src/SUMMARY.md b/book/src/SUMMARY.md index daaefd06a97..9df393db8a0 100644 --- a/book/src/SUMMARY.md +++ b/book/src/SUMMARY.md @@ -14,6 +14,7 @@ - [Basics](development/basics.md) - [Adding Lints](development/adding_lints.md) - [Defining Lints](development/defining_lints.md) + - [Writing tests](development/writing_tests.md) - [Lint Passes](development/lint_passes.md) - [Type Checking](development/type_checking.md) - [Method Checking](development/method_checking.md) diff --git a/book/src/development/writing_tests.md b/book/src/development/writing_tests.md new file mode 100644 index 00000000000..57d7b99651c --- /dev/null +++ b/book/src/development/writing_tests.md @@ -0,0 +1,220 @@ +# Testing + +Developing lints for Clippy is a Test-Driven Development (TDD) process because +our first task before implementing any logic for a new lint is to write some test cases. + +## Develop Lints with Tests + +When we develop Clippy, we enter a complex and chaotic realm full of +programmatic issues, stylistic errors, illogical code and non-adherence to convention. +Tests are the first layer of order we can leverage to define when and where +we want a new lint to trigger or not. + +Moreover, writing tests first help Clippy developers to find a balance for +the first iteration of and further enhancements for a lint. +With test cases on our side, we will not have to worry about over-engineering +a lint on its first version nor missing out some obvious edge cases of the lint. +This approach empowers us to iteratively enhance each lint. + +## Clippy UI Tests + +We use **UI tests** for testing in Clippy. +These UI tests check that the output of Clippy is exactly as we expect it to be. +Each test is just a plain Rust file that contains the code we want to check. + +The output of Clippy is compared against a `.stderr` file. +Note that you don't have to create this file yourself. +We'll get to generating the `.stderr` files with the command [`cargo dev bless`](#cargo-dev-bless) (seen later on). + +### Write Test Cases + +Let us now think about some tests for our imaginary `foo_functions` lint, +We start by opening the test file `tests/ui/foo_functions.rs` that was created by `cargo dev new_lint`. + +Update the file with some examples to get started: + +```rust +#![warn(clippy::foo_functions)] +// Impl methods +struct A; +impl A { + pub fn fo(&self) {} + pub fn foo(&self) {} + pub fn food(&self) {} +} + +// Default trait methods +trait B { + fn fo(&self) {} + fn foo(&self) {} + fn food(&self) {} +} + +// Plain functions +fn fo() {} +fn foo() {} +fn food() {} + +fn main() { + // We also don't want to lint method calls + foo(); + let a = A; + a.foo(); +} +``` + +Without actual lint logic to emit the lint when we see a `foo` function name, +these tests are still quite meaningless. +However, we can now run the test with the following command: + +```sh +$ TESTNAME=foo_functions cargo uitest +``` + +Clippy will compile and it will conclude with an `ok` for the tests: + +``` +...Clippy warnings and test outputs... +test compile_test ... ok +test result: ok. 3 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.48s +``` + +This is normal. After all, we wrote a bunch of Rust code but we haven't really +implemented any logic for Clippy to detect `foo` functions and emit a lint. + +As we gradually implement our lint logic, we will keep running this UI test command. +Clippy will begin outputting information that allows us to check if the output is +turning into what we want it to be. + +### Example output + +As our `foo_functions` lint is tested, the output would look something like this: + +``` +failures: + +---- compile_test stdout ---- +normalized stderr: +error: function called "foo" + --> $DIR/foo_functions.rs:6:12 + | +LL | pub fn foo(&self) {} + | ^^^ + | + = note: `-D clippy::foo-functions` implied by `-D warnings` + +error: function called "foo" + --> $DIR/foo_functions.rs:13:8 + | +LL | fn foo(&self) {} + | ^^^ + +error: function called "foo" + --> $DIR/foo_functions.rs:19:4 + | +LL | fn foo() {} + | ^^^ + +error: aborting due to 3 previous errors +``` + +Note the *failures* label at the top of the fragment, we'll get rid of it (saving this output) in the next section. + +> _Note:_ You can run multiple test files by specifying a comma separated list: +> `TESTNAME=foo_functions,bar_methods,baz_structs`. +### `cargo dev bless` + +Once we are satisfied with the output, we need to run this command to +generate or update the `.stderr` file for our lint: + +```sh +$ TESTNAME=foo_functions cargo uitest +# (Output is as we want it to be) +$ cargo dev bless +``` + +This write the emitted lint suggestions and fixes to the `.stderr` file, +with the reason for the lint, suggested fixes, and line numbers, etc. + +> _Note:_ we need to run `TESTNAME=foo_functions cargo uitest` every time before we run +> `cargo dev bless`. + +Running `TESTNAME=foo_functions cargo uitest` should pass then. When we +commit our lint, we need to commit the generated `.stderr` files, too. + +In general, you should only commit files changed by `cargo dev bless` for the +specific lint you are creating/editing. + +> _Note:_ If the generated `.stderr`, and `.fixed` files are empty, +> they should be removed. + +## Cargo Lints + +The process of testing is different for Cargo lints in that now we are +interested in the `Cargo.toml` manifest file. +In this case, we also need a minimal crate associated with that manifest. + +Imagine we have a new example lint that is named `foo_categories`, we can run: + +```sh +$ cargo dev new_lint --name=foo_categories --pass=late --category=cargo +``` + +After running `cargo dev new_lint` we will find by default two new crates, +each with its manifest file: + +* `tests/ui-cargo/foo_categories/fail/Cargo.toml`: this file should cause the + new lint to raise an error. +* `tests/ui-cargo/foo_categories/pass/Cargo.toml`: this file should not trigger + the lint. + +If you need more cases, you can copy one of those crates (under `foo_categories`) and rename it. + +The process of generating the `.stderr` file is the same as for other lints +and prepending the `TESTNAME` variable to `cargo uitest` works for Cargo lints too. + +Overall, you should see the following changes when you generate a new Cargo lint: + +```sh +$ git status +On branch foo_categories +Changes not staged for commit: + (use "git add ..." to update what will be committed) + (use "git restore ..." to discard changes in working directory) + modified: CHANGELOG.md + modified: clippy_lints/src/cargo/mod.rs + modified: clippy_lints/src/lib.rs +Untracked files: + (use "git add ..." to include in what will be committed) + clippy_lints/src/cargo/foo_categories.rs + tests/ui-cargo/foo_categories/ +``` + +## Rustfix Tests + +If the lint you are working on is making use of structured suggestions, the test +file should include a `// run-rustfix` comment at the top. + +Structured suggestions tell a user how to fix or re-write certain code that has been linted, they are usually linted +with [`span_lint_and_sugg`](https://doc.rust-lang.org/beta/nightly-rustc/clippy_utils/diagnostics/fn.span_lint_and_sugg.html). + +The `// run-rustfix` comment will additionally run [rustfix] for our test. +Rustfix will apply the suggestions from the lint to the test file code and +compare that to the contents of a `.fixed` file. + +We'll talk about suggestions more in depth in a later chapter. + + +Use `cargo dev bless` to automatically generate the `.fixed` file after running the tests. + +[rustfix]: https://github.com/rust-lang/rustfix +## Testing Manually + +Manually testing against an example file can be useful if you have added some +`println!`s and the test suite output becomes unreadable. + +To try Clippy with your local modifications, run from the working copy root. + +```sh +$ cargo dev lint input.rs +``` From 864df49bd34d8082b085ec3da90a1c203ee927c6 Mon Sep 17 00:00:00 2001 From: Philipp Krones Date: Sun, 16 Apr 2023 22:48:10 +0200 Subject: [PATCH 2/4] Formatting, slimming and ui-toml tests --- book/src/development/writing_tests.md | 91 ++++++++++++++------------- 1 file changed, 49 insertions(+), 42 deletions(-) diff --git a/book/src/development/writing_tests.md b/book/src/development/writing_tests.md index 57d7b99651c..ebed25cb667 100644 --- a/book/src/development/writing_tests.md +++ b/book/src/development/writing_tests.md @@ -5,7 +5,7 @@ our first task before implementing any logic for a new lint is to write some tes ## Develop Lints with Tests -When we develop Clippy, we enter a complex and chaotic realm full of +When we develop Clippy, we enter a complex and chaotic realm full of programmatic issues, stylistic errors, illogical code and non-adherence to convention. Tests are the first layer of order we can leverage to define when and where we want a new lint to trigger or not. @@ -18,23 +18,25 @@ This approach empowers us to iteratively enhance each lint. ## Clippy UI Tests -We use **UI tests** for testing in Clippy. -These UI tests check that the output of Clippy is exactly as we expect it to be. -Each test is just a plain Rust file that contains the code we want to check. +We use **UI tests** for testing in Clippy. These UI tests check that the output +of Clippy is exactly as we expect it to be. Each test is just a plain Rust file +that contains the code we want to check. -The output of Clippy is compared against a `.stderr` file. -Note that you don't have to create this file yourself. -We'll get to generating the `.stderr` files with the command [`cargo dev bless`](#cargo-dev-bless) (seen later on). +The output of Clippy is compared against a `.stderr` file. Note that you don't +have to create this file yourself. We'll get to generating the `.stderr` files +with the command [`cargo dev bless`](#cargo-dev-bless) (seen later on). ### Write Test Cases -Let us now think about some tests for our imaginary `foo_functions` lint, -We start by opening the test file `tests/ui/foo_functions.rs` that was created by `cargo dev new_lint`. +Let us now think about some tests for our imaginary `foo_functions` lint. We +start by opening the test file `tests/ui/foo_functions.rs` that was created by +`cargo dev new_lint`. Update the file with some examples to get started: ```rust -#![warn(clippy::foo_functions)] +#![warn(clippy::foo_functions)] // < Add this, so the lint is guaranteed to be enabled in this file + // Impl methods struct A; impl A { @@ -64,8 +66,8 @@ fn main() { ``` Without actual lint logic to emit the lint when we see a `foo` function name, -these tests are still quite meaningless. -However, we can now run the test with the following command: +this test will just pass, because no lint will be emitted. However, we can now +run the test with the following command: ```sh $ TESTNAME=foo_functions cargo uitest @@ -118,10 +120,12 @@ LL | fn foo() {} error: aborting due to 3 previous errors ``` -Note the *failures* label at the top of the fragment, we'll get rid of it (saving this output) in the next section. +Note the *failures* label at the top of the fragment, we'll get rid of it +(saving this output) in the next section. > _Note:_ You can run multiple test files by specifying a comma separated list: > `TESTNAME=foo_functions,bar_methods,baz_structs`. + ### `cargo dev bless` Once we are satisfied with the output, we need to run this command to @@ -136,11 +140,11 @@ $ cargo dev bless This write the emitted lint suggestions and fixes to the `.stderr` file, with the reason for the lint, suggested fixes, and line numbers, etc. -> _Note:_ we need to run `TESTNAME=foo_functions cargo uitest` every time before we run -> `cargo dev bless`. +> _Note:_ we need to run `TESTNAME=foo_functions cargo uitest` every time before +> we run `cargo dev bless`. -Running `TESTNAME=foo_functions cargo uitest` should pass then. When we -commit our lint, we need to commit the generated `.stderr` files, too. +Running `TESTNAME=foo_functions cargo uitest` should pass then. When we commit +our lint, we need to commit the generated `.stderr` files, too. In general, you should only commit files changed by `cargo dev bless` for the specific lint you are creating/editing. @@ -148,11 +152,27 @@ specific lint you are creating/editing. > _Note:_ If the generated `.stderr`, and `.fixed` files are empty, > they should be removed. +## `toml` Tests + +Some lints can be configured through a `clippy.toml` file. Those configuration +values are tested in `tests/ui-toml`. + +To add a new test there, create a new directory and add the files: + +- `clippy.toml`: Put here the configuration value you want to test. +- `lint_name.rs`: A test file where you put the testing code, that should see a + different lint behavior according to the configuration set in the + `clippy.toml` file. + +The potential `.stderr` and `.fixed` files can again be generated with `cargo +dev bless`. + ## Cargo Lints The process of testing is different for Cargo lints in that now we are -interested in the `Cargo.toml` manifest file. -In this case, we also need a minimal crate associated with that manifest. +interested in the `Cargo.toml` manifest file. In this case, we also need a +minimal crate associated with that manifest. Those tests are generated in +`tests/ui-cargo`. Imagine we have a new example lint that is named `foo_categories`, we can run: @@ -168,46 +188,33 @@ each with its manifest file: * `tests/ui-cargo/foo_categories/pass/Cargo.toml`: this file should not trigger the lint. -If you need more cases, you can copy one of those crates (under `foo_categories`) and rename it. +If you need more cases, you can copy one of those crates (under +`foo_categories`) and rename it. -The process of generating the `.stderr` file is the same as for other lints +The process of generating the `.stderr` file is the same as for other lints and prepending the `TESTNAME` variable to `cargo uitest` works for Cargo lints too. -Overall, you should see the following changes when you generate a new Cargo lint: - -```sh -$ git status -On branch foo_categories -Changes not staged for commit: - (use "git add ..." to update what will be committed) - (use "git restore ..." to discard changes in working directory) - modified: CHANGELOG.md - modified: clippy_lints/src/cargo/mod.rs - modified: clippy_lints/src/lib.rs -Untracked files: - (use "git add ..." to include in what will be committed) - clippy_lints/src/cargo/foo_categories.rs - tests/ui-cargo/foo_categories/ -``` - ## Rustfix Tests If the lint you are working on is making use of structured suggestions, the test file should include a `// run-rustfix` comment at the top. -Structured suggestions tell a user how to fix or re-write certain code that has been linted, they are usually linted -with [`span_lint_and_sugg`](https://doc.rust-lang.org/beta/nightly-rustc/clippy_utils/diagnostics/fn.span_lint_and_sugg.html). +Structured suggestions tell a user how to fix or re-write certain code that has +been linted with [`span_lint_and_sugg`]. The `// run-rustfix` comment will additionally run [rustfix] for our test. Rustfix will apply the suggestions from the lint to the test file code and compare that to the contents of a `.fixed` file. We'll talk about suggestions more in depth in a later chapter. - + -Use `cargo dev bless` to automatically generate the `.fixed` file after running the tests. +Use `cargo dev bless` to automatically generate the `.fixed` file after running +the tests. [rustfix]: https://github.com/rust-lang/rustfix +[`span_lint_and_sugg`]: https://doc.rust-lang.org/beta/nightly-rustc/clippy_utils/diagnostics/fn.span_lint_and_sugg.html + ## Testing Manually Manually testing against an example file can be useful if you have added some From 32dc7c592d7911ad8d80ec3cc28dfd39c9c04f06 Mon Sep 17 00:00:00 2001 From: Philipp Krones Date: Fri, 18 Aug 2023 19:28:41 +0200 Subject: [PATCH 3/4] Applying review suggestions --- book/src/development/writing_tests.md | 27 +++++++++++---------------- 1 file changed, 11 insertions(+), 16 deletions(-) diff --git a/book/src/development/writing_tests.md b/book/src/development/writing_tests.md index ebed25cb667..c7f67dcae9f 100644 --- a/book/src/development/writing_tests.md +++ b/book/src/development/writing_tests.md @@ -24,7 +24,7 @@ that contains the code we want to check. The output of Clippy is compared against a `.stderr` file. Note that you don't have to create this file yourself. We'll get to generating the `.stderr` files -with the command [`cargo dev bless`](#cargo-dev-bless) (seen later on). +with the command [`cargo bless`](#cargo-bless) (seen later on). ### Write Test Cases @@ -41,20 +41,20 @@ Update the file with some examples to get started: struct A; impl A { pub fn fo(&self) {} - pub fn foo(&self) {} + pub fn foo(&self) {} //~ ERROR: function called "foo" pub fn food(&self) {} } // Default trait methods trait B { fn fo(&self) {} - fn foo(&self) {} + fn foo(&self) {} //~ ERROR: function called "foo" fn food(&self) {} } // Plain functions fn fo() {} -fn foo() {} +fn foo() {} //~ ERROR: function called "foo" fn food() {} fn main() { @@ -126,27 +126,22 @@ Note the *failures* label at the top of the fragment, we'll get rid of it > _Note:_ You can run multiple test files by specifying a comma separated list: > `TESTNAME=foo_functions,bar_methods,baz_structs`. -### `cargo dev bless` +### `cargo bless` Once we are satisfied with the output, we need to run this command to generate or update the `.stderr` file for our lint: ```sh -$ TESTNAME=foo_functions cargo uitest -# (Output is as we want it to be) -$ cargo dev bless +$ TESTNAME=foo_functions cargo uibless ``` -This write the emitted lint suggestions and fixes to the `.stderr` file, -with the reason for the lint, suggested fixes, and line numbers, etc. - -> _Note:_ we need to run `TESTNAME=foo_functions cargo uitest` every time before -> we run `cargo dev bless`. +This writes the emitted lint suggestions and fixes to the `.stderr` file, with +the reason for the lint, suggested fixes, and line numbers, etc. Running `TESTNAME=foo_functions cargo uitest` should pass then. When we commit our lint, we need to commit the generated `.stderr` files, too. -In general, you should only commit files changed by `cargo dev bless` for the +In general, you should only commit files changed by `cargo bless` for the specific lint you are creating/editing. > _Note:_ If the generated `.stderr`, and `.fixed` files are empty, @@ -165,7 +160,7 @@ To add a new test there, create a new directory and add the files: `clippy.toml` file. The potential `.stderr` and `.fixed` files can again be generated with `cargo -dev bless`. +bless`. ## Cargo Lints @@ -209,7 +204,7 @@ compare that to the contents of a `.fixed` file. We'll talk about suggestions more in depth in a later chapter. -Use `cargo dev bless` to automatically generate the `.fixed` file after running +Use `cargo bless` to automatically generate the `.fixed` file after running the tests. [rustfix]: https://github.com/rust-lang/rustfix From 8ee6ca0981e2087c24ecd8018bbb20437e8ce777 Mon Sep 17 00:00:00 2001 From: Philipp Krones Date: Fri, 18 Aug 2023 19:29:00 +0200 Subject: [PATCH 4/4] Update rustfix section to new ui-test crate --- book/src/development/writing_tests.md | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/book/src/development/writing_tests.md b/book/src/development/writing_tests.md index c7f67dcae9f..7cb530a3bb9 100644 --- a/book/src/development/writing_tests.md +++ b/book/src/development/writing_tests.md @@ -191,15 +191,16 @@ and prepending the `TESTNAME` variable to `cargo uitest` works for Cargo lints t ## Rustfix Tests -If the lint you are working on is making use of structured suggestions, the test -file should include a `// run-rustfix` comment at the top. +If the lint you are working on is making use of structured suggestions, +[`rustfix`] will apply the suggestions from the lint to the test file code and +compare that to the contents of a `.fixed` file. Structured suggestions tell a user how to fix or re-write certain code that has been linted with [`span_lint_and_sugg`]. -The `// run-rustfix` comment will additionally run [rustfix] for our test. -Rustfix will apply the suggestions from the lint to the test file code and -compare that to the contents of a `.fixed` file. +Should `span_lint_and_sugg` be used to generate a suggestion, but not all +suggestions lead to valid code, you can use the `//@no-rustfix` comment on top +of the test file, to not run `rustfix` on that file. We'll talk about suggestions more in depth in a later chapter. @@ -207,7 +208,7 @@ We'll talk about suggestions more in depth in a later chapter. Use `cargo bless` to automatically generate the `.fixed` file after running the tests. -[rustfix]: https://github.com/rust-lang/rustfix +[`rustfix`]: https://github.com/rust-lang/rustfix [`span_lint_and_sugg`]: https://doc.rust-lang.org/beta/nightly-rustc/clippy_utils/diagnostics/fn.span_lint_and_sugg.html ## Testing Manually