-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Refactor Rust lint test structure to use RuffTestFixture #20689
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
Conversation
|
- Keep insta::with_settings filters as they are still needed - The filters replace temporary directory paths in warning messages - All refactored functions now pass: top_level_options, lint_options, mixed_levels, precedence
You were correct - RuffTestFixture functions don't need the tempdir filters because the warning messages show clean filenames (ruff.toml) rather than full temporary directory paths. - Removed insta::with_settings! wrappers - Use raw assert_cmd_snapshot! calls - Removed unused tempdir_filter import - All tests pass: top_level_options, lint_options, mixed_levels, precedence
|
Thanks for working on this. Yes, this is what I had in mind. |
…uffTestFixture - exclude: Uses RuffTestFixture.new() and write_file() for multiple files - deduplicate_directory_and_explicit_file: Simple refactoring with write_file() - Both functions now use clean assert_cmd_snapshot! calls without filters - Tests pass with identical behavior
…stFixture - exclude_stdin: Uses RuffTestFixture.with_file() for stdin-based testing - line_too_long_width_override: Another stdin-based function refactoring - Both functions now use clean assert_cmd_snapshot! calls without filters - Tests pass with identical behavior after snapshot updates
|
Hi @MichaReiser , not sure why I can't modify the reviewers label, PTAL |
|
Can't help on modify the windows insta snapshots very well, because I don't own a windows computer |
Refactor tempdir_filter to use strip_prefix for UNC paths
I fixed the filter problem, but for some tests, windows and linux have different behavior, the linux tests expect |
|
In the new pattern, there are four functions have different behavior between windows and linux
|
MichaReiser
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. This is great. I only have a few questions around using dunce/dedent
crates/ruff/tests/test_fixture.rs
Outdated
| //! The core concept is borrowed from ty/tests/cli/main.rs and can be extended | ||
| //! with more functionality from there in the future if needed. | ||
|
|
||
| #![allow(dead_code)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove the allow dead code and remove any unusede code instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current functions are actually all used, not sure why the clippy believe they are dead, add some unit tests to make sure they are used now.
crates/ruff/tests/test_fixture.rs
Outdated
| let normalized_path = if let Some(stripped) = path_str.strip_prefix(r"\\?\") { | ||
| stripped | ||
| } else { | ||
| path_str | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason why you aren't using dunce here (you can add it as a dev-only dependency)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice crate, I didn't know it exists before.
crates/ruff/tests/cli/main.rs
Outdated
| // Always add optional UNC prefix to match both \\?\C:\... and C:\... patterns | ||
| // This ensures compatibility regardless of whether input or output contains UNC prefix |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than handling UNC paths here, I suggest to do the same as ty and strip any UNC prefix in new
crates/ruff/tests/test_fixture.rs
Outdated
| let project_dir = temp_dir | ||
| .path() | ||
| .canonicalize() | ||
| .context("Failed to canonicalize project path")?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's strip the UNC prefix here using dunce (you can add it as a dev dependency)
crates/ruff/tests/test_fixture.rs
Outdated
|
|
||
| Self::ensure_parent_directory(&file_path)?; | ||
|
|
||
| let content = content.trim_start_matches('\n'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use dedent here instead of trimming start new lines only?
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
8438e78 to
336133b
Compare
Part of: #19016, for lint.rs