Add internal hidden rules for testing #9747

zanieb · 2024-01-31T19:12:26Z

Updated implementation of #7369 which was left out in the cold.

This was motivated again following changes in #9691 and #9689 where we could not test the changes without actually deprecating or removing rules.

Follow-up to discussion in #7210

Moves integration tests from using rules that are transitively in nursery / preview groups to dedicated test rules that only exist during development. These rules always raise violations (they do not require specific file behavior). The rules are not available in production or in the documentation.

Uses features instead of cfg(test) for cross-crate support per rust-lang/cargo#8379

crates/ruff_linter/src/linter.rs

crates/ruff/tests/integration_test.rs

BurntSushi

Yeah I think I buy it!

crates/ruff_linter/src/linter.rs

…s_when_disabled`

crates/ruff_linter/src/linter.rs

zanieb · 2024-01-31T22:48:26Z

The ecosystem checks won't succeed until this is main because RUF9XX does not exist on the baseline binary.

charliermarsh · 2024-01-31T23:13:12Z

crates/ruff/Cargo.toml

@@ -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"] }


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

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.

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

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

(Which should omit these.)

Oh great so I think we're good then

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.

charliermarsh · 2024-01-31T23:14:19Z

crates/ruff_linter/src/rule_selector.rs

@@ -289,6 +289,8 @@ mod schema {
                                (!prefix.is_empty()).then(|| prefix.to_string())
                            })),
                    )
+                    // Strip the test rules from the schema
+                    .filter(|code| !code.starts_with("RUF9"))


Wish there were something better here (e.g., what if we could use the rule source, and have a special lint_source() called test), but might be difficult.

We could add something to the macro but it seems like more trouble than it's worth at this point

Should we just put them all in a separate lint group e.g. RFT for "Ruff test"? Seems safer and easier to filter in general?

Might end up being more work to implement overall though?

I'm actually wondering how these even up getting included in the schema, do we run with --all-features or something?

Anyway, I'm cool with whatever here.

Huh this raises a preview warning

❯ ./target/debug/ruff check example.py --select RUF9 warning: Selection `RUF9` has no effect because the `--preview` flag was not included.

but doesn't do anything else.. RUF8 errors

❯ ./target/debug/ruff check example.py --select RUF8 error: invalid value 'RUF8' for '--select <RULE_CODE>'

I guess I'll fix that next..

Yeah this is actually confusing... why are they included in the schema in the first place?

They shouldn't even exist when we run cargo dev generate-all, right?

Okay this was a bug in the proc macro 🎉
8247b85 fixes it (but has a mediocre implementation I'm fixing up)

charliermarsh · 2024-01-31T23:14:38Z

python/ruff-ecosystem/ruff_ecosystem/projects.py

@@ -206,6 +206,9 @@ def to_ruff_args(self) -> list[str]:
            "check",
            "--no-cache",
            "--exit-zero",
+            # Ignore internal test rules
+            "--ignore",
+            "RUF9",


Why / how do these get enabled here? Why is this necessary?

Like how does the feature end up being enabled?

I believe we get our ecosystem binaries from the test builds, which have the development features enabled? I think they have to be turned on in the ruff binary because we use that for integration tests.

It'd be nice if they were not present here, but do we want to have a separate build for the ecosystem checks?

…hild codes

zanieb · 2024-02-01T04:34:05Z

The schema generation tests use --all-features even though the real schema generation does not 😮‍💨

charliermarsh · 2024-02-01T04:36:56Z

Noooooo

) Updates #9689 and #9691 to use rule testing infrastructure from #9747

MichaReiser · 2024-02-02T08:53:22Z

.github/workflows/ci.yaml

@@ -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


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 :(

zanieb mentioned this pull request Jan 31, 2024

Add internal rules for testing #7369

Closed

zanieb force-pushed the zb/test-rules-new branch 3 times, most recently from 03363f1 to 306a263 Compare January 31, 2024 19:21

zanieb requested a review from MichaReiser January 31, 2024 19:24

zanieb commented Jan 31, 2024

View reviewed changes

crates/ruff_linter/src/linter.rs Outdated Show resolved Hide resolved

zanieb force-pushed the zb/test-rules-new branch 2 times, most recently from 142f261 to 2367e7a Compare January 31, 2024 19:33

zanieb commented Jan 31, 2024

View reviewed changes

crates/ruff/tests/integration_test.rs Outdated Show resolved Hide resolved

zanieb commented Jan 31, 2024

View reviewed changes

crates/ruff/tests/integration_test.rs Outdated Show resolved Hide resolved

BurntSushi approved these changes Jan 31, 2024

View reviewed changes

crates/ruff_linter/src/linter.rs Outdated Show resolved Hide resolved

This comment was marked as outdated.

Sign in to view

Add internal hidden rules for testing

fa3a8e6

zanieb force-pushed the zb/test-rules-new branch from 2367e7a to fa3a8e6 Compare January 31, 2024 21:04

zanieb added 4 commits January 31, 2024 15:06

Ignore internal rules in ecosystem checks

43ed884

Remove uncessary clone

0fea08c

Fix RUF900 comments

f5e44b6

Fix code for `check_no_hint_for_hidden_unsafe_fixes_with_no_safe_fixe…

ca92070

…s_when_disabled`

zanieb commented Jan 31, 2024

View reviewed changes

crates/ruff_linter/src/linter.rs Outdated Show resolved Hide resolved

zanieb added 2 commits January 31, 2024 16:23

Skip diagnostics if fix comment inserted

7d80d95

Fix move of import

d05452f

zanieb commented Jan 31, 2024

View reviewed changes

crates/ruff_linter/src/linter.rs Outdated Show resolved Hide resolved

zanieb added 5 commits January 31, 2024 16:26

Fix fix_only_unsafe_fixes_available

3419e5c

Fix ecosystem ignore

072c52d

Fix moved ruff imports

4a3a02d

continue

6d20a71

Clippy

052edbd

charliermarsh approved these changes Jan 31, 2024

View reviewed changes

Update proc macro to preserve attributes that are common across all c…

8247b85

…hild codes

zanieb added 3 commits January 31, 2024 22:20

Remove schema strip

2b31bbe

Clippy

d89a888

Fix implementation of intersection_all

fafbc25

Separate dev tests in CI

8d9b6b5

zanieb merged commit f18e7d4 into main Feb 1, 2024
17 checks passed

zanieb deleted the zb/test-rules-new branch February 1, 2024 14:44

zanieb mentioned this pull request Feb 1, 2024

Use fake rules for testing deprecation and removal infrastructure #9752

Merged

zanieb added a commit that referenced this pull request Feb 1, 2024

Use fake rules for testing deprecation and removal infrastructure (#9752

dcc69f2

) Updates #9689 and #9691 to use rule testing infrastructure from #9747

zanieb added a commit that referenced this pull request Feb 1, 2024

Use fake rules for testing deprecation and removal infrastructure (#9752

46c0937

) Updates #9689 and #9691 to use rule testing infrastructure from #9747

MichaReiser reviewed Feb 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add internal hidden rules for testing #9747

Add internal hidden rules for testing #9747

zanieb commented Jan 31, 2024 •

edited

BurntSushi left a comment

This comment was marked as outdated.

zanieb commented Jan 31, 2024

charliermarsh Jan 31, 2024

zanieb Jan 31, 2024

zanieb Jan 31, 2024

charliermarsh Feb 1, 2024

charliermarsh Feb 1, 2024

zanieb Feb 1, 2024

BurntSushi Feb 1, 2024

charliermarsh Jan 31, 2024

zanieb Jan 31, 2024

zanieb Feb 1, 2024

zanieb Feb 1, 2024

charliermarsh Feb 1, 2024

charliermarsh Feb 1, 2024

zanieb Feb 1, 2024

zanieb Feb 1, 2024

charliermarsh Feb 1, 2024

zanieb Feb 1, 2024 •

edited

charliermarsh Jan 31, 2024

charliermarsh Jan 31, 2024

zanieb Jan 31, 2024

zanieb commented Feb 1, 2024 •

edited

charliermarsh commented Feb 1, 2024

MichaReiser Feb 2, 2024 •

edited

Add internal hidden rules for testing #9747

Add internal hidden rules for testing #9747

Conversation

zanieb commented Jan 31, 2024 • edited

BurntSushi left a comment

Choose a reason for hiding this comment

This comment was marked as outdated.

zanieb commented Jan 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zanieb Feb 1, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zanieb commented Feb 1, 2024 • edited

charliermarsh commented Feb 1, 2024

MichaReiser Feb 2, 2024 • edited

Choose a reason for hiding this comment

zanieb commented Jan 31, 2024 •

edited

zanieb Feb 1, 2024 •

edited

zanieb commented Feb 1, 2024 •

edited

MichaReiser Feb 2, 2024 •

edited