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

Jump to bottom

Add OsSystem support to mdtests #16518

Open
wants to merge 1 commit into
base: micha/case-sensitive-module-resolver-tests
Choose a base branch
from
Open

Add OsSystem support to mdtests #16518

wants to merge 1 commit into from

Conversation

MichaReiser
Copy link
Member

Summary

This PR introduces a new mdtest option system that can either be in-memory or os
where in-memory is the default.

The motivation for supporting os is so that we can write OS/system specific tests
with mdtests. Specifically, I want to write mdtests for the module resolver,
testing that module resolution is case sensitive.

Test Plan

I tested that the case-sensitive module resolver test start failing when setting system = "os"

@MichaReiser MichaReiser force-pushed the micha/mdtest-os-system branch from cf2929f to ebb484a Compare March 5, 2025 13:01
@MichaReiser MichaReiser added the red-knot Multi-file analysis & type inference label Mar 5, 2025
MichaReiser
MichaReiser commented Mar 5, 2025
View reviewed changes
crates/red_knot_project/src/metadata.rs
@@ -321,7 +321,7 @@ mod tests {

system
.memory_file_system()
.write_files([(root.join("foo.py"), ""), (root.join("bar.py"), "")])
.write_files_all([(root.join("foo.py"), ""), (root.join("bar.py"), "")])
Copy link
Member Author

@MichaReiser MichaReiser Mar 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not very fond of this name. The MemoryFileSystem::write_files and write_file methods used to create any missing directories. This is inconsistent with what other systems do but is very convenient in tests.

That's why I renamed the existing write_files and write_file methods to write_files_all and write_file_all (in resemblence of create_dir_all) and created a new write_file method that errors when the parent directory doesn't exist. However, I don't think it's a very good name.

Copy link
Contributor

Choose a reason for hiding this comment

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

The only other options that come to mind are significantly more verbose; write_files_mkdirs, write_files_with_parents... I think write_files_all is OK.

@MichaReiser MichaReiser force-pushed the micha/mdtest-os-system branch from ebb484a to 72c7c23 Compare March 5, 2025 13:30
@MichaReiser MichaReiser force-pushed the micha/case-sensitive-module-resolver-tests branch 4 times, most recently from aebf24a to ad5e00e Compare March 5, 2025 13:49
@MichaReiser MichaReiser force-pushed the micha/mdtest-os-system branch 5 times, most recently from a60c6a1 to a57bb2f Compare March 5, 2025 14:22
@MichaReiser MichaReiser force-pushed the micha/mdtest-os-system branch from a57bb2f to edcb9e6 Compare March 5, 2025 14:31
@MichaReiser MichaReiser marked this pull request as ready for review March 5, 2025 14:38
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Mar 5, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

carljm
carljm approved these changes Mar 5, 2025
View reviewed changes
Copy link
Contributor

@carljm carljm left a comment

Choose a reason for hiding this comment

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

Nice!

crates/red_knot_project/src/files.rs
@@ -255,7 +255,7 @@ mod tests {
use crate::files::Index;
use crate::ProjectMetadata;
use ruff_db::files::system_path_to_file;
use ruff_db::system::{DbWithTestSystem, SystemPathBuf};
use ruff_db::system::{DbWithWritableSystem as _, SystemPathBuf};
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the as _ needed here?

crates/ruff_db/src/system/test.rs
/// ## Panics
/// If the db isn't using the [`InMemorySystem`].
fn write_file(&mut self, path: impl AsRef<SystemPath>, content: impl ToString) -> Result<()> {
fn write_file(&mut self, path: impl AsRef<SystemPath>, content: impl AsRef<str>) -> Result<()> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Given the naming adjustments throughout this PR, I'm confused why a method named write_file explicitly creates all parent directories here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah that's fair. The thinking was that this trait is only used for testing where this seems the right default. But I can see how it is confusing in the bigger picture.

crates/red_knot_project/src/metadata.rs
@@ -321,7 +321,7 @@ mod tests {

system
.memory_file_system()
.write_files([(root.join("foo.py"), ""), (root.join("bar.py"), "")])
.write_files_all([(root.join("foo.py"), ""), (root.join("bar.py"), "")])
Copy link
Contributor

Choose a reason for hiding this comment

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

The only other options that come to mind are significantly more verbose; write_files_mkdirs, write_files_with_parents... I think write_files_all is OK.

crates/red_knot_test/src/db.rs
let db = Self {
project_root,
Self {
system: MdtestSystem::in_memory(SystemPathBuf::from("/")),
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we ever create an MdtestSystem with any cwd other than /? Should that just be built-in?

crates/red_knot_test/src/db.rs
in_memory.fs().remove_all();
}
MdtestSystemInner::Os { .. } => {
self.system = MdtestSystem::in_memory(SystemPathBuf::from("/"));
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems surprising at first that "resetting" a real fs just means throwing it away and replacing it with an in-memory fs. But I guess it makes sense.

crates/red_knot_test/src/db.rs
Comment on lines +52 to +53
pub(crate) fn with_tempdir_fs(&mut self, cwd: SystemPathBuf, temp_dir: TempDir) {
self.system.with_os(cwd, temp_dir);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the with_* naming is surprising for methods that mutate in-place; that name strongly suggests an immutable builder pattern where self is cloned and the updated one is returned. (Because in that case the method name describes the returned object.)

For in-place mutation I would expect e.g. set_tempdir_fs and set_os instead.

crates/red_knot_test/src/db.rs
pub(crate) fn project_root(&self) -> &SystemPath {
&self.project_root
pub(crate) fn reset_fs(&mut self) {
match &*self.system.0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: maybe delegate to a self.system.reset_fs() instead of digging so much into MdTestSystem internals from outside?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carljm carljm carljm approved these changes

@AlexWaygood AlexWaygood Awaiting requested review from AlexWaygood AlexWaygood is a code owner

@sharkdp sharkdp Awaiting requested review from sharkdp sharkdp is a code owner

Assignees
No one assigned
Labels
red-knot Multi-file analysis & type inference
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@MichaReiser @carljm