Add OsSystem support to mdtests
#16518
Conversation
MichaReiser
force-pushed
the
micha/mdtest-os-system
branch
from
cf2929f to
ebb484a
Compare
| 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"), "")]) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
force-pushed
the
micha/mdtest-os-system
branch
from
ebb484a to
72c7c23
Compare
MichaReiser
force-pushed
the
micha/case-sensitive-module-resolver-tests
branch
4 times, most recently
from
aebf24a to
ad5e00e
Compare
MichaReiser
force-pushed
the
micha/mdtest-os-system
branch
6 times, most recently
from
a57bb2f to
edcb9e6
Compare
MichaReiser
requested review from
AlexWaygood,
carljm and
sharkdp
as code owners
|
| use crate::ProjectMetadata; | ||
| use ruff_db::files::system_path_to_file; | ||
| use ruff_db::system::{DbWithTestSystem, SystemPathBuf}; | ||
| use ruff_db::system::{DbWithWritableSystem as _, SystemPathBuf}; |
There was a problem hiding this comment.
Why is the as _ needed here?
There was a problem hiding this comment.
It's not technically needed but it only makes the trait available in the module without importing it by name. That means you can call all the extension methods without binding the name locally.
| /// ## 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<()> { |
There was a problem hiding this comment.
Given the naming adjustments throughout this PR, I'm confused why a method named write_file explicitly creates all parent directories here.
There was a problem hiding this comment.
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.
| 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"), "")]) |
There was a problem hiding this comment.
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
force-pushed
the
micha/mdtest-os-system
branch
from
edcb9e6 to
38e8267
Compare
Summary
This PR introduces a new mdtest option
systemthat can either bein-memoryoroswhere
in-memoryis the default.The motivation for supporting
osis so that we can write OS/system specific testswith 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"