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

Update reftests with small refactor and renames for clarity #1225

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+47 −33
Open

Update reftests with small refactor and renames for clarity #1225

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 34 additions & 20 deletions mountpoint-s3/tests/reftests/harness.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,26 +253,40 @@ impl Harness {
/// Compared to [compare_contents], this avoids doing a `readdir` before `lookup`, and so tests
/// a different path through the inode code.
pub async fn compare_single_path(&self, idx: usize) {
let inodes = self.reference.list_recursive();
if inodes.is_empty() {
let ref_inodes = self.reference.list_recursive();
if ref_inodes.is_empty() {
return;
}
let (path, node) = &inodes[idx % inodes.len()];
let (path_components, node) = &ref_inodes[idx % ref_inodes.len()];

let mut parent = FUSE_ROOT_INODE;
let mut seen_inos = HashSet::from([FUSE_ROOT_INODE]);
for name in path.iter().take(path.len().saturating_sub(1)) {
let lookup = self.fs.lookup(parent, name.as_ref()).await.unwrap();

let (leaf, ancestors) = path_components
.split_last()
dannycjones marked this conversation as resolved.
Show resolved Hide resolved
.expect("there should always be at least one path component");
for name in ancestors {
let lookup = self
.fs
.lookup(parent, name.as_ref())
.await
.expect("ancestor must exist in MP fs");
assert_eq!(lookup.attr.kind, FileType::Directory);
let ino = lookup.attr.ino;
assert!(
seen_inos.insert(lookup.attr.ino),
"should not have seen ancestor before"
seen_inos.insert(ino),
"should not have seen ancestor ino {ino} in MP fs before"
);
parent = lookup.attr.ino;
parent = ino;
}

let lookup = self.fs.lookup(parent, path.last().unwrap().as_ref()).await.unwrap();
assert!(seen_inos.insert(lookup.attr.ino), "should not have seen inode before");
let lookup = self
.fs
.lookup(parent, leaf.as_ref())
.await
.expect("directory entry at path should exist in MP fs");
let ino = lookup.attr.ino;
assert!(seen_inos.insert(ino), "should not have seen ino {ino} before");
match node {
Node::Directory { .. } => {
assert_eq!(lookup.attr.kind, FileType::Directory);
Expand Down Expand Up @@ -605,8 +619,8 @@ impl Harness {
) -> BoxFuture<'a, ()> {
async move {
let dir_handle = self.fs.opendir(fs_dir, 0).await.unwrap().fh;
let children = ref_dir.children();
let mut keys = children.keys().cloned().collect::<HashSet<_>>();
let ref_children = ref_dir.children();
let mut unvisited_ref_children = ref_children.keys().cloned().collect::<HashSet<_>>();

let mut reply = DirectoryReply::new(self.readdir_limit);
self.fs.readdir(fs_dir, dir_handle, 0, &mut reply).await.unwrap();
Expand Down Expand Up @@ -648,7 +662,7 @@ impl Harness {
let lkup = self.fs.lookup(fs_dir, &reply.name).await.unwrap();
let attr = lkup.attr;

match children.get(name) {
match ref_children.get(name) {
Some(node) => {
let ref_kind = node.node_type().into();
assert_eq!(
Expand All @@ -672,7 +686,7 @@ impl Harness {
self.compare_contents_recursive(fs_dir, reply.ino, node).await;
}
assert!(
keys.remove(name),
unvisited_ref_children.remove(name),
"file name must be in the set of child names in the reference fs",
);
}
Expand All @@ -684,19 +698,19 @@ impl Harness {
}

assert!(
keys.is_empty(),
"reference contained elements not in the filesystem in dir inode {fs_dir}: {keys:?}"
unvisited_ref_children.is_empty(),
"reference contained elements not in the filesystem in dir inode {fs_dir}: {unvisited_ref_children:?}"
);

self.fs.releasedir(fs_dir, dir_handle, 0).await.unwrap();
}
.boxed()
}

async fn compare_file<'a>(&'a self, fs_file: InodeNo, ref_file: &'a MockObject) {
let fh = match self.fs.open(fs_file, OpenFlags::empty(), 0).await {
async fn compare_file<'a>(&'a self, fs_ino: InodeNo, ref_file: &'a MockObject) {
let fh = match self.fs.open(fs_ino, OpenFlags::empty(), 0).await {
Ok(ret) => ret.fh,
Err(e) => panic!("failed to open ino {fs_file} for content comparison: {e:?}"),
Err(e) => panic!("failed to open ino {fs_ino} for content comparison: {e:?}"),
};
let mut offset = 0;
const MAX_READ_SIZE: usize = 128 * 1024;
Expand All @@ -707,7 +721,7 @@ impl Harness {
assert_eq!(ref_bytes.len(), num_bytes, "number of bytes did not match");
let bytes_from_read = self
.fs
.read(fs_file, fh, offset as i64, num_bytes as u32, 0, None)
.read(fs_ino, fh, offset as i64, num_bytes as u32, 0, None)
.await
.expect("read should succeed");
assert_eq!(&ref_bytes[..], &bytes_from_read, "read bytes did not match");
Expand Down
26 changes: 13 additions & 13 deletions mountpoint-s3/tests/reftests/reference.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ impl Node {
#[derive(Debug)]
pub struct Reference {
/// Contents of our S3 bucket
remote_keys: BTreeMap<String, MockObject>,
remote_objects: BTreeMap<String, MockObject>,
/// Local files
local_files: Vec<PathBuf>,
/// Local directories
Expand Down Expand Up @@ -98,25 +98,25 @@ impl MaterializedReference {
);

let mut parent_node = &mut self.root;
while let Some(dir) = components.next() {
while let Some(name) = components.next() {
let Node::Directory { children, .. } = parent_node else {
return false;
// TODO: see above -- implicit directories are allowed to disappear
// panic!("unexpected internal file node while adding {:?}", path.as_ref());
};
let dir = dir.as_os_str().to_str().unwrap();
let name = name.as_os_str().to_str().unwrap();
if components.peek().is_none() {
// If both a local and a remote directory exist, don't overwrite the remote one's
// contents, as they will be visible even though the directory is local. But
// remember the directory is still local.
if typ == NodeType::Directory {
if let Some(Node::Directory { is_local, .. }) = children.get_mut(dir) {
if let Some(Node::Directory { is_local, .. }) = children.get_mut(name) {
*is_local = true;
break;
}
}
// If a directory of this name exists, ignore any local file
if let Some(node) = children.get(dir) {
if let Some(node) = children.get(name) {
if node.node_type() == NodeType::Directory {
return false;
}
Expand All @@ -128,15 +128,15 @@ impl MaterializedReference {
},
NodeType::File => Node::File(File::Local),
};
children.insert(dir.to_owned(), new_node);
children.insert(name.to_owned(), new_node);
break;
} else {
// TODO: see above -- implicit directories are allowed to disappear
// parent_node = children.entry(dir.to_owned()).or_insert_with(|| Node::Directory {
// children: BTreeMap::new(),
// is_local: true,
// })
let Some(child_node) = children.get_mut(dir) else {
let Some(child_node) = children.get_mut(name) else {
return false;
};
parent_node = child_node;
Expand All @@ -153,7 +153,7 @@ impl Reference {
let local_directories = vec![];
let materialized = build_reference(remote_keys.iter().map(|(k, o): &(_, _)| (k, o)));
Self {
remote_keys: remote_keys.into_iter().collect(),
remote_objects: remote_keys.into_iter().collect(),
local_files,
local_directories,
materialized,
Expand All @@ -162,10 +162,10 @@ impl Reference {

fn rematerialize(&self) -> MaterializedReference {
tracing::debug!(
remote_keys=?self.remote_keys, local_files=?self.local_files, local_directories=?self.local_directories,
remote_keys=?self.remote_objects.keys(), local_files=?self.local_files, local_directories=?self.local_directories,
"rematerialize",
);
let mut materialized = build_reference(self.remote_keys.iter());
let mut materialized = build_reference(self.remote_objects.iter());
for local_dir in self.local_directories.iter() {
let added = materialized.add_local_node(local_dir, NodeType::Directory);
if added {
Expand Down Expand Up @@ -252,12 +252,12 @@ impl Reference {
}

pub fn add_remote_key(&mut self, key: &str, object: MockObject) {
self.remote_keys.insert(key.to_owned(), object);
self.remote_objects.insert(key.to_owned(), object);
self.materialized = self.rematerialize();
}

pub fn remove_remote_key(&mut self, key: &str) {
self.remote_keys.remove(key);
self.remote_objects.remove(key);
self.materialized = self.rematerialize();
}

Expand Down Expand Up @@ -301,7 +301,7 @@ impl Reference {

/// A list of objects in the bucket
pub fn remote_keys(&self) -> impl ExactSizeIterator<Item = &str> {
self.remote_keys.keys().map(|key| key.as_str())
self.remote_objects.keys().map(|key| key.as_str())
}
}

Expand Down
Loading