Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add contributing docs specific to rule-testing patterns #4690

Merged
merged 2 commits into from
May 28, 2023
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 22 additions & 13 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ Welcome! We're happy to have you here. Thank you in advance for your contributio
- [Project Structure](#project-structure)
- [Example: Adding a new lint rule](#example-adding-a-new-lint-rule)
- [Rule naming convention](#rule-naming-convention)
- [Rule testing: fixtures and snapshots](#rule-testing-fixtures-and-snapshots)
- [Example: Adding a new configuration option](#example-adding-a-new-configuration-option)
- [MkDocs](#mkdocs)
- [Release Process](#release-process)
Expand Down Expand Up @@ -110,7 +111,7 @@ At a high level, the steps involved in adding a new lint rule are as follows:
checks), `crates/ruff/src/checkers/tokens.rs` (for token-based checks), `crates/ruff/src/checkers/lines.rs`
(for text-based checks), or `crates/ruff/src/checkers/filesystem.rs` (for filesystem-based
checks).
1. Add a test fixture.
1. Add proper [testing](#rule-testing-fixtures-and-snapshots) for your rule.
1. Update the generated files (documentation and generated code).

To define the violation, start by creating a dedicated file for your rule under the appropriate
Expand All @@ -125,18 +126,8 @@ collecting diagnostics as it goes.
If you need to inspect the AST, you can run `cargo dev print-ast` with a Python file. Grep
for the `Check::new` invocations to understand how other, similar rules are implemented.

To add a test fixture, create a file under `crates/ruff/resources/test/fixtures/[linter]`, named to match
the code you defined earlier (e.g., `crates/ruff/resources/test/fixtures/pycodestyle/E402.py`). This file should
contain a variety of violations and non-violations designed to evaluate and demonstrate the behavior
of your lint rule.

Run `cargo dev generate-all` to generate the code for your new fixture. Then run Ruff
locally with (e.g.) `cargo run -p ruff_cli -- check crates/ruff/resources/test/fixtures/pycodestyle/E402.py --no-cache --select E402`.

Once you're satisfied with the output, codify the behavior as a snapshot test by adding a new
`test_case` macro in the relevant `crates/ruff/src/rules/[linter]/mod.rs` file. Then, run `cargo test`.
Your test will fail, but you'll be prompted to follow-up with `cargo insta review`. Accept the
generated snapshot, then commit the snapshot file alongside the rest of your changes.
Once you're satisfied with your code, add tests for your rule. See [rule testing](#rule-testing-fixtures-and-snapshots)
for more details.

Finally, regenerate the documentation and generated code with `cargo dev generate-all`.

Expand All @@ -154,6 +145,24 @@ This implies that rule names:
When re-implementing rules from other linters, this convention is given more importance than
preserving the original rule name.

#### Rule testing: fixtures and snapshots
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Everything above is just drive-by formatting changes added by me after the initial PR.)


To test rules, Ruff uses snapshots of Ruff's output for a given file (fixture). Generally, there will be one file per rule (e.g., `E402.py`), and each file will contain all necessary examples of both violations and non-violations. `cargo insta review` will generate a snapshot file containing Ruff's output for each fixture, which you can then commit alongside your changes.

Once you've completed the code for the rule itself, you can define tests with the following steps:

1. Add a Python file to `crates/ruff/resources/test/fixtures/[linter]` that contains the code you want to test. The file name should match the rule name (e.g., `E402.py`), and it should include examples of both violations and non-violations.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you use the script add_rule.py the file is created automatically.


1. Run Ruff locally against your file and verify the output is as expected. For example, if you added a new rule named `E402`, you could run `cargo run -p ruff_cli -- check crates/ruff/resources/test/fixtures/pycodestyle/E402.py --no-cache --select E402`. Once you're satisfied with the output (you see the violations you expect, and no others), proceed to the next step.

1. Run `cargo dev generate-all` to generate the code for your new fixture.

1. Add the test to the relevant `crates/ruff/src/rules/[linter]/mod.rs` file. If you're contributing a rule to a pre-existing set, you should be able to find a similar example to pattern-match against. If you're adding a new linter, you'll need to create a new `mod.rs` file.

1. Run `cargo test`. Your test will fail, but you'll be prompted to follow-up with `cargo insta review`. Run `cargo insta review` and accept the generated snapshot, then commit the snapshot file alongside the rest of your changes.

1. Run `cargo test` again to ensure that your test now passes.

### Example: Adding a new configuration option

Ruff's user-facing settings live in a few different places.
Expand Down