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.

Sign up for GitHub

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 internal hidden rules for testing #9747

Merged
merged 17 commits into from
Feb 1, 2024
Merged

Add internal hidden rules for testing #9747

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
fa3a8e6
Add internal hidden rules for testing
zanieb Jan 31, 2024
43ed884
Ignore internal rules in ecosystem checks
zanieb Jan 31, 2024
0fea08c
Remove uncessary clone
zanieb Jan 31, 2024
f5e44b6
Fix RUF900 comments
zanieb Jan 31, 2024
ca92070
Fix code for `check_no_hint_for_hidden_unsafe_fixes_with_no_safe_fixe…
zanieb Jan 31, 2024
7d80d95
Skip diagnostics if fix comment inserted
zanieb Jan 31, 2024
d05452f
Fix move of import
zanieb Jan 31, 2024
3419e5c
Fix `fix_only_unsafe_fixes_available`
zanieb Jan 31, 2024
072c52d
Fix ecosystem ignore
zanieb Jan 31, 2024
4a3a02d
Fix moved ruff imports
zanieb Jan 31, 2024
6d20a71
`continue`
zanieb Jan 31, 2024
052edbd
Clippy
zanieb Jan 31, 2024
8247b85
Update proc macro to preserve attributes that are common across all c…
zanieb Feb 1, 2024
2b31bbe
Remove schema strip
zanieb Feb 1, 2024
d89a888
Clippy
zanieb Feb 1, 2024
fafbc25
Fix implementation of `intersection_all`
zanieb Feb 1, 2024
8d9b6b5
Separate dev tests in CI
zanieb Feb 1, 2024
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
7 changes: 5 additions & 2 deletions .github/workflows/ci.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,10 @@ jobs:
tool: cargo-insta
- uses: Swatinem/rust-cache@v2
- name: "Run tests"
run: cargo insta test --all --all-features --unreferenced reject
run: cargo insta test --all --exclude ruff_dev --all-features --unreferenced reject
- name: "Run dev tests"
# e.g. generating the schema — these should not run with all features enabled
run: cargo insta test -p ruff_dev --unreferenced reject
# Check for broken links in the documentation.
- run: cargo doc --all --no-deps
env:
Expand Down Expand Up @@ -146,7 +149,7 @@ jobs:
- name: "Run tests"
shell: bash
# We can't reject unreferenced snapshots on windows because flake8_executable can't run on windows
run: cargo insta test --all --all-features
run: cargo insta test --all --exclude ruff_dev --all-features
Copy link
Member

@MichaReiser MichaReiser Feb 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Can we add another run command that runs the test of ruff_dev but with the default features only? We otherwise lose the schema test.

Although I'm seeing test failures locally even when not using --all-features, making that test rather annoying.

A workaround could be to export a are_testing_rules_enabled and conditionally implement it depending on the presence of the feature flag and not run the schema test when it's enabled. But that still means that the test won't fail anymore locally and still requires an explicit cargo test ruff_dev run :(


cargo-test-wasm:
name: "cargo test (wasm)"
Expand Down
2 changes: 2 additions & 0 deletions crates/ruff/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,8 @@ walkdir = { workspace = true }
wild = { workspace = true }

[dev-dependencies]
# Enable test rules during development
ruff_linter = { path = "../ruff_linter", features = ["clap", "test-rules"] }
Copy link
Member

Choose a reason for hiding this comment

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

@BurntSushi - Can you double-confirm for me that these won't enable in non-cargo test builds?

Copy link
Member Author

Choose a reason for hiding this comment

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

If I do cargo build --release --all-features they are present

❯ ./target/release/ruff check example.py --select RUF
example.py:1:1: RUF900 Hey this is a stable test rule.
example.py:1:1: RUF901 [*] Hey this is a stable test rule with a safe fix.
example.py:1:1: RUF902 Hey this is a stable test rule with an unsafe fix.
example.py:1:1: RUF903 Hey this is a stable test rule with a display only fix.
Found 4 errors.
[*] 1 fixable with the `--fix` option (1 hidden fix can be enabled with the `--unsafe-fixes` option).

but they are not present without --all-features in the release or debug targets.

Copy link
Member Author

Choose a reason for hiding this comment

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

Idk what features we include in releases, it's buried in maturin?

Copy link
Member

Choose a reason for hiding this comment

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

We don't specify anything, so we get the default features.

Copy link
Member

Choose a reason for hiding this comment

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

(Which should omit these.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh great so I think we're good then

Copy link
Member

Choose a reason for hiding this comment

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

Yeah features enabled in dev-dependencies should definitely not cross over into standard builds. But yeah as Zanie mentioned, if you have anything that's using --all-features, then they will get enabled in that case.

assert_cmd = { workspace = true }
# Avoid writing colored snapshots when running tests from the terminal
colored = { workspace = true, features = ["no-color"]}
Expand Down
Loading
Loading