Update reftests with small refactor and renames for clarity (#1225)

This change makes minor updates to improve clarity in the reference
tests.

### Does this change impact existing behavior?

No, refactors reftests only.

### Does this change need a changelog entry?

No.

---

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license and I agree to the terms of
the [Developer Certificate of Origin
(DCO)](https://developercertificate.org/).

Signed-off-by: Daniel Carl Jones <[email protected]>

Author: dannycjones

mountpoint-s3/tests/reftests/harness.rs

@@ -282,26 +282,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()
+            .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);
@@ -643,8 +657,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();
@@ -686,7 +700,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!(
@@ -710,7 +724,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",
                             );
                         }
@@ -722,8 +736,8 @@ 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();
@@ -733,10 +747,10 @@ impl Harness {
 
     /// Compare the contents of a given reference file to the system under test (SUT),
     /// by opening and reading the file from the SUT.
-    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;
@@ -747,7 +761,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");

mountpoint-s3/tests/reftests/reference.rs

@@ -76,7 +76,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
@@ -121,25 +121,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;
                     }
@@ -151,15 +151,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;
@@ -176,7 +176,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,
@@ -189,10 +189,10 @@ impl Reference {
     /// and produce a result which should maintain our promised semantics.
     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 {
@@ -279,12 +279,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();
     }
 
@@ -328,7 +328,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())
     }
 }