Conversation
zanieb
force-pushed
the
zb/test-rules-new
branch
3 times, most recently
from
03363f1 to
306a263
Compare
zanieb
force-pushed
the
zb/test-rules-new
branch
2 times, most recently
from
142f261 to
2367e7a
Compare
This comment was marked as outdated.
This comment was marked as outdated.
zanieb
force-pushed
the
zb/test-rules-new
branch
from
2367e7a to
fa3a8e6
Compare
|
The ecosystem checks won't succeed until this is main because |
|
|
||
| [dev-dependencies] | ||
| # Enable test rules during development | ||
| ruff_linter = { path = "../ruff_linter", features = ["clap", "test-rules"] } |
There was a problem hiding this comment.
@BurntSushi - Can you double-confirm for me that these won't enable in non-cargo test builds?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Idk what features we include in releases, it's buried in maturin?
There was a problem hiding this comment.
We don't specify anything, so we get the default features.
There was a problem hiding this comment.
(Which should omit these.)
There was a problem hiding this comment.
Oh great so I think we're good then
There was a problem hiding this comment.
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.
| })), | ||
| ) | ||
| // Strip the test rules from the schema | ||
| .filter(|code| !code.starts_with("RUF9")) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
We could add something to the macro but it seems like more trouble than it's worth at this point
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Might end up being more work to implement overall though?
There was a problem hiding this comment.
I'm actually wondering how these even up getting included in the schema, do we run with --all-features or something?
There was a problem hiding this comment.
Anyway, I'm cool with whatever here.
There was a problem hiding this comment.
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..
There was a problem hiding this comment.
Yeah this is actually confusing... why are they included in the schema in the first place?
There was a problem hiding this comment.
They shouldn't even exist when we run cargo dev generate-all, right?
There was a problem hiding this comment.
Okay this was a bug in the proc macro 🎉
8247b85 fixes it (but has a mediocre implementation I'm fixing up)
| "--exit-zero", | ||
| # Ignore internal test rules | ||
| "--ignore", | ||
| "RUF9", |
There was a problem hiding this comment.
Why / how do these get enabled here? Why is this necessary?
There was a problem hiding this comment.
Like how does the feature end up being enabled?
There was a problem hiding this comment.
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?
|
The schema generation tests use |
|
Noooooo |
| 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 |
There was a problem hiding this comment.
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 :(
4 participants
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