Add OsSystem support to mdtests (#16518)

## 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"`
This commit is contained in:
Micha Reiser 2025-03-06 09:41:40 +00:00 committed by GitHub
parent 48f906e06c
commit ce0018c3cb
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
31 changed files with 541 additions and 229 deletions

View file

@ -496,7 +496,7 @@ impl std::error::Error for FileError {}
mod tests {
use crate::file_revision::FileRevision;
use crate::files::{system_path_to_file, vendored_path_to_file, FileError};
use crate::system::DbWithTestSystem;
use crate::system::DbWithWritableSystem as _;
use crate::tests::TestDb;
use crate::vendored::VendoredFileSystemBuilder;
use zip::CompressionMethod;

View file

@ -85,7 +85,9 @@ impl Eq for ParsedModule {}
mod tests {
use crate::files::{system_path_to_file, vendored_path_to_file};
use crate::parsed::parsed_module;
use crate::system::{DbWithTestSystem, SystemPath, SystemVirtualPath};
use crate::system::{
DbWithTestSystem, DbWithWritableSystem as _, SystemPath, SystemVirtualPath,
};
use crate::tests::TestDb;
use crate::vendored::{VendoredFileSystemBuilder, VendoredPath};
use crate::Db;
@ -96,7 +98,7 @@ mod tests {
let mut db = TestDb::new();
let path = "test.py";
db.write_file(path, "x = 10".to_string())?;
db.write_file(path, "x = 10")?;
let file = system_path_to_file(&db, path).unwrap();
@ -112,7 +114,7 @@ mod tests {
let mut db = TestDb::new();
let path = SystemPath::new("test.ipynb");
db.write_file(path, "%timeit a = b".to_string())?;
db.write_file(path, "%timeit a = b")?;
let file = system_path_to_file(&db, path).unwrap();

View file

@ -176,7 +176,7 @@ mod tests {
use crate::files::system_path_to_file;
use crate::source::{line_index, source_text};
use crate::system::{DbWithTestSystem, SystemPath};
use crate::system::{DbWithWritableSystem as _, SystemPath};
use crate::tests::TestDb;
#[test]
@ -184,13 +184,13 @@ mod tests {
let mut db = TestDb::new();
let path = SystemPath::new("test.py");
db.write_file(path, "x = 10".to_string())?;
db.write_file(path, "x = 10")?;
let file = system_path_to_file(&db, path).unwrap();
assert_eq!(source_text(&db, file).as_str(), "x = 10");
db.write_file(path, "x = 20".to_string()).unwrap();
db.write_file(path, "x = 20").unwrap();
assert_eq!(source_text(&db, file).as_str(), "x = 20");
@ -202,7 +202,7 @@ mod tests {
let mut db = TestDb::new();
let path = SystemPath::new("test.py");
db.write_file(path, "x = 10".to_string())?;
db.write_file(path, "x = 10")?;
let file = system_path_to_file(&db, path).unwrap();
@ -228,7 +228,7 @@ mod tests {
let mut db = TestDb::new();
let path = SystemPath::new("test.py");
db.write_file(path, "x = 10\ny = 20".to_string())?;
db.write_file(path, "x = 10\ny = 20")?;
let file = system_path_to_file(&db, path).unwrap();
let index = line_index(&db, file);

View file

@ -12,7 +12,7 @@ use std::error::Error;
use std::fmt::Debug;
use std::path::{Path, PathBuf};
use std::{fmt, io};
pub use test::{DbWithTestSystem, InMemorySystem, TestSystem};
pub use test::{DbWithTestSystem, DbWithWritableSystem, InMemorySystem, TestSystem};
use walk_directory::WalkDirectoryBuilder;
use crate::file_revision::FileRevision;
@ -161,6 +161,15 @@ pub trait System: Debug {
fn as_any_mut(&mut self) -> &mut dyn std::any::Any;
}
/// System trait for non-readonly systems.
pub trait WritableSystem: System {
/// Writes the given content to the file at the given path.
fn write_file(&self, path: &SystemPath, content: &str) -> Result<()>;
/// Creates a directory at `path` as well as any intermediate directories.
fn create_directory_all(&self, path: &SystemPath) -> Result<()>;
}
#[derive(Clone, Debug, Eq, PartialEq)]
pub struct Metadata {
revision: FileRevision,

View file

@ -153,28 +153,9 @@ impl MemoryFileSystem {
virtual_files.contains_key(&path.to_path_buf())
}
/// Writes the files to the file system.
///
/// The operation overrides existing files with the same normalized path.
///
/// Enclosing directories are automatically created if they don't exist.
pub fn write_files<P, C>(&self, files: impl IntoIterator<Item = (P, C)>) -> Result<()>
where
P: AsRef<SystemPath>,
C: ToString,
{
for (path, content) in files {
self.write_file(path.as_ref(), content.to_string())?;
}
Ok(())
}
/// Stores a new file in the file system.
///
/// The operation overrides the content for an existing file with the same normalized `path`.
///
/// Enclosing directories are automatically created if they don't exist.
pub fn write_file(&self, path: impl AsRef<SystemPath>, content: impl ToString) -> Result<()> {
let mut by_path = self.inner.by_path.write().unwrap();
@ -187,6 +168,42 @@ impl MemoryFileSystem {
Ok(())
}
/// Writes the files to the file system.
///
/// The operation overrides existing files with the same normalized path.
///
/// Enclosing directories are automatically created if they don't exist.
pub fn write_files_all<P, C>(&self, files: impl IntoIterator<Item = (P, C)>) -> Result<()>
where
P: AsRef<SystemPath>,
C: ToString,
{
for (path, content) in files {
self.write_file_all(path.as_ref(), content.to_string())?;
}
Ok(())
}
/// Stores a new file in the file system.
///
/// The operation overrides the content for an existing file with the same normalized `path`.
///
/// Enclosing directories are automatically created if they don't exist.
pub fn write_file_all(
&self,
path: impl AsRef<SystemPath>,
content: impl ToString,
) -> Result<()> {
let path = path.as_ref();
if let Some(parent) = path.parent() {
self.create_directory_all(parent)?;
}
self.write_file(path, content)
}
/// Stores a new virtual file in the file system.
///
/// The operation overrides the content for an existing virtual file with the same `path`.
@ -486,7 +503,11 @@ fn get_or_create_file<'a>(
normalized: &Utf8Path,
) -> Result<&'a mut File> {
if let Some(parent) = normalized.parent() {
create_dir_all(paths, parent)?;
let parent_entry = paths.get(parent).ok_or_else(not_found)?;
if parent_entry.is_file() {
return Err(not_a_directory());
}
}
let entry = paths.entry(normalized.to_path_buf()).or_insert_with(|| {
@ -719,7 +740,7 @@ mod tests {
P: AsRef<SystemPath>,
{
let fs = MemoryFileSystem::new();
fs.write_files(files.into_iter().map(|path| (path, "")))
fs.write_files_all(files.into_iter().map(|path| (path, "")))
.unwrap();
fs
@ -822,29 +843,25 @@ mod tests {
}
#[test]
fn write_file_fails_if_a_component_is_a_file() {
let fs = with_files(["a/b.py"]);
fn write_file_fails_if_a_parent_directory_is_missing() {
let fs = with_files(["c.py"]);
let error = fs
.write_file(SystemPath::new("a/b.py/c"), "content".to_string())
.write_file(SystemPath::new("a/b.py"), "content".to_string())
.unwrap_err();
assert_eq!(error.kind(), ErrorKind::Other);
assert_eq!(error.kind(), ErrorKind::NotFound);
}
#[test]
fn write_file_fails_if_path_points_to_a_directory() -> Result<()> {
let fs = MemoryFileSystem::new();
fs.create_directory_all("a")?;
fn write_file_all_fails_if_a_component_is_a_file() {
let fs = with_files(["a/b.py"]);
let error = fs
.write_file(SystemPath::new("a"), "content".to_string())
.write_file_all(SystemPath::new("a/b.py/c"), "content".to_string())
.unwrap_err();
assert_eq!(error.kind(), ErrorKind::Other);
Ok(())
}
#[test]
@ -864,7 +881,7 @@ mod tests {
let fs = MemoryFileSystem::new();
let path = SystemPath::new("a.py");
fs.write_file(path, "Test content".to_string())?;
fs.write_file_all(path, "Test content".to_string())?;
assert_eq!(fs.read_to_string(path)?, "Test content");
@ -895,6 +912,21 @@ mod tests {
Ok(())
}
#[test]
fn write_file_fails_if_path_points_to_a_directory() -> Result<()> {
let fs = MemoryFileSystem::new();
fs.create_directory_all("a")?;
let error = fs
.write_file(SystemPath::new("a"), "content".to_string())
.unwrap_err();
assert_eq!(error.kind(), ErrorKind::Other);
Ok(())
}
#[test]
fn read_fails_if_virtual_path_doesnt_exit() {
let fs = MemoryFileSystem::new();
@ -1046,7 +1078,7 @@ mod tests {
let root = SystemPath::new("/src");
let system = MemoryFileSystem::with_current_directory(root);
system.write_files([
system.write_files_all([
(root.join("foo.py"), "print('foo')"),
(root.join("a/bar.py"), "print('bar')"),
(root.join("a/baz.py"), "print('baz')"),
@ -1105,7 +1137,7 @@ mod tests {
let root = SystemPath::new("/src");
let system = MemoryFileSystem::with_current_directory(root);
system.write_files([
system.write_files_all([
(root.join("foo.py"), "print('foo')"),
(root.join("a/bar.py"), "print('bar')"),
(root.join("a/.baz.py"), "print('baz')"),
@ -1151,7 +1183,7 @@ mod tests {
let root = SystemPath::new("/src");
let system = MemoryFileSystem::with_current_directory(root);
system.write_file(root.join("foo.py"), "print('foo')")?;
system.write_file_all(root.join("foo.py"), "print('foo')")?;
let writer = DirectoryEntryToString::new(root.to_path_buf());
@ -1181,7 +1213,7 @@ mod tests {
let root = SystemPath::new("/src");
let fs = MemoryFileSystem::with_current_directory(root);
fs.write_files([
fs.write_files_all([
(root.join("foo.py"), "print('foo')"),
(root.join("a/bar.py"), "print('bar')"),
(root.join("a/.baz.py"), "print('baz')"),

View file

@ -7,7 +7,7 @@ use ruff_notebook::{Notebook, NotebookError};
use crate::system::{
DirectoryEntry, FileType, GlobError, GlobErrorKind, Metadata, Result, System, SystemPath,
SystemPathBuf, SystemVirtualPath,
SystemPathBuf, SystemVirtualPath, WritableSystem,
};
use super::walk_directory::{
@ -191,6 +191,16 @@ impl System for OsSystem {
}
}
impl WritableSystem for OsSystem {
fn write_file(&self, path: &SystemPath, content: &str) -> Result<()> {
std::fs::write(path.as_std_path(), content)
}
fn create_directory_all(&self, path: &SystemPath) -> Result<()> {
std::fs::create_dir_all(path.as_std_path())
}
}
#[derive(Debug)]
struct OsDirectoryWalker;

View file

@ -1,6 +1,5 @@
use glob::PatternError;
use ruff_notebook::{Notebook, NotebookError};
use ruff_python_trivia::textwrap;
use std::panic::RefUnwindSafe;
use std::sync::{Arc, Mutex};
@ -12,6 +11,7 @@ use crate::system::{
use crate::Db;
use super::walk_directory::WalkDirectoryBuilder;
use super::WritableSystem;
/// System implementation intended for testing.
///
@ -22,10 +22,16 @@ use super::walk_directory::WalkDirectoryBuilder;
/// Don't use this system for production code. It's intended for testing only.
#[derive(Debug, Clone)]
pub struct TestSystem {
inner: Arc<dyn System + RefUnwindSafe + Send + Sync>,
inner: Arc<dyn WritableSystem + RefUnwindSafe + Send + Sync>,
}
impl TestSystem {
pub fn new(inner: impl WritableSystem + RefUnwindSafe + Send + Sync + 'static) -> Self {
Self {
inner: Arc::new(inner),
}
}
/// Returns the [`InMemorySystem`].
///
/// ## Panics
@ -50,12 +56,12 @@ impl TestSystem {
fn use_system<S>(&mut self, system: S)
where
S: System + Send + Sync + RefUnwindSafe + 'static,
S: WritableSystem + Send + Sync + RefUnwindSafe + 'static,
{
self.inner = Arc::new(system);
}
pub fn system(&self) -> &dyn System {
pub fn system(&self) -> &dyn WritableSystem {
&*self.inner
}
}
@ -134,6 +140,73 @@ impl Default for TestSystem {
}
}
impl WritableSystem for TestSystem {
fn write_file(&self, path: &SystemPath, content: &str) -> Result<()> {
self.system().write_file(path, content)
}
fn create_directory_all(&self, path: &SystemPath) -> Result<()> {
self.system().create_directory_all(path)
}
}
/// Extension trait for databases that use a [`WritableSystem`].
///
/// Provides various helper function that ease testing.
pub trait DbWithWritableSystem: Db + Sized {
type System: WritableSystem;
fn writable_system(&self) -> &Self::System;
/// Writes the content of the given file and notifies the Db about the change.
fn write_file(&mut self, path: impl AsRef<SystemPath>, content: impl AsRef<str>) -> Result<()> {
let path = path.as_ref();
match self.writable_system().write_file(path, content.as_ref()) {
Ok(()) => {
File::sync_path(self, path);
Ok(())
}
Err(error) if error.kind() == std::io::ErrorKind::NotFound => {
if let Some(parent) = path.parent() {
self.writable_system().create_directory_all(parent)?;
for ancestor in parent.ancestors() {
File::sync_path(self, ancestor);
}
self.writable_system().write_file(path, content.as_ref())?;
File::sync_path(self, path);
Ok(())
} else {
Err(error)
}
}
err => err,
}
}
/// Writes auto-dedented text to a file.
fn write_dedented(&mut self, path: &str, content: &str) -> Result<()> {
self.write_file(path, ruff_python_trivia::textwrap::dedent(content))?;
Ok(())
}
/// Writes the content of the given files and notifies the Db about the change.
fn write_files<P, C, I>(&mut self, files: I) -> Result<()>
where
I: IntoIterator<Item = (P, C)>,
P: AsRef<SystemPath>,
C: AsRef<str>,
{
for (path, content) in files {
self.write_file(path, content)?;
}
Ok(())
}
}
/// Extension trait for databases that use [`TestSystem`].
///
/// Provides various helper function that ease testing.
@ -142,35 +215,6 @@ pub trait DbWithTestSystem: Db + Sized {
fn test_system_mut(&mut self) -> &mut TestSystem;
/// Writes the content of the given file and notifies the Db about the change.
///
/// ## Panics
/// If the db isn't using the [`InMemorySystem`].
fn write_file(&mut self, path: impl AsRef<SystemPath>, content: impl ToString) -> Result<()> {
let path = path.as_ref();
let memory_fs = self.test_system().memory_file_system();
let sync_ancestors = path
.parent()
.is_some_and(|parent| !memory_fs.exists(parent));
let result = memory_fs.write_file(path, content);
if result.is_ok() {
File::sync_path(self, path);
// Sync the ancestor paths if the path's parent
// directory didn't exist before.
if sync_ancestors {
for ancestor in path.ancestors() {
File::sync_path(self, ancestor);
}
}
}
result
}
/// Writes the content of the given virtual file.
///
/// ## Panics
@ -182,32 +226,6 @@ pub trait DbWithTestSystem: Db + Sized {
.write_virtual_file(path, content);
}
/// Writes auto-dedented text to a file.
///
/// ## Panics
/// If the db isn't using the [`InMemorySystem`].
fn write_dedented(&mut self, path: &str, content: &str) -> crate::system::Result<()> {
self.write_file(path, textwrap::dedent(content))?;
Ok(())
}
/// Writes the content of the given files and notifies the Db about the change.
///
/// ## Panics
/// If the db isn't using the [`InMemorySystem`].
fn write_files<P, C, I>(&mut self, files: I) -> crate::system::Result<()>
where
I: IntoIterator<Item = (P, C)>,
P: AsRef<SystemPath>,
C: ToString,
{
for (path, content) in files {
self.write_file(path, content)?;
}
Ok(())
}
/// Uses the given system instead of the testing system.
///
/// This useful for testing advanced file system features like permissions, symlinks, etc.
@ -215,7 +233,7 @@ pub trait DbWithTestSystem: Db + Sized {
/// Note that any files written to the memory file system won't be copied over.
fn use_system<S>(&mut self, os: S)
where
S: System + Send + Sync + RefUnwindSafe + 'static,
S: WritableSystem + Send + Sync + RefUnwindSafe + 'static,
{
self.test_system_mut().use_system(os);
}
@ -229,6 +247,17 @@ pub trait DbWithTestSystem: Db + Sized {
}
}
impl<T> DbWithWritableSystem for T
where
T: DbWithTestSystem,
{
type System = TestSystem;
fn writable_system(&self) -> &Self::System {
self.test_system()
}
}
#[derive(Default, Debug)]
pub struct InMemorySystem {
user_config_directory: Mutex<Option<SystemPathBuf>>,
@ -236,6 +265,13 @@ pub struct InMemorySystem {
}
impl InMemorySystem {
pub fn new(cwd: SystemPathBuf) -> Self {
Self {
user_config_directory: Mutex::new(None),
memory_fs: MemoryFileSystem::with_current_directory(cwd),
}
}
pub fn fs(&self) -> &MemoryFileSystem {
&self.memory_fs
}
@ -314,3 +350,13 @@ impl System for InMemorySystem {
self
}
}
impl WritableSystem for InMemorySystem {
fn write_file(&self, path: &SystemPath, content: &str) -> Result<()> {
self.memory_fs.write_file(path, content)
}
fn create_directory_all(&self, path: &SystemPath) -> Result<()> {
self.memory_fs.create_directory_all(path)
}
}