From 7d6e8f9d26a451a155edcf8289f93781158fd3bb Mon Sep 17 00:00:00 2001 From: Daniel Carl Jones Date: Mon, 13 Jan 2025 10:13:22 +0000 Subject: [PATCH] Update reftest to always generate a directory at the root of the reference model (#1219) This change impacts only Mountpoint's reference tests. The current strategy for generating the tree allows for the root to be a file, which is not possible. This leads to us adding special cases to the reftest comparison logic as well as having bizarre test cases which are hard to understand. This change updates the strategy by ensuring that the root is always a directory, and simplifies some of the unused proptest layers. ### Does this change impact existing behavior? No change to existing Mountpoint behavior. This changes the type of trees generated by our reference tests, removing those that are not possible in Mountpoint. ### 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 --- mountpoint-s3/tests/reftests/generators.rs | 34 +++++++++++++++------- mountpoint-s3/tests/reftests/harness.rs | 16 +++++----- 2 files changed, 31 insertions(+), 19 deletions(-) diff --git a/mountpoint-s3/tests/reftests/generators.rs b/mountpoint-s3/tests/reftests/generators.rs index 763bdb476..13e053689 100644 --- a/mountpoint-s3/tests/reftests/generators.rs +++ b/mountpoint-s3/tests/reftests/generators.rs @@ -90,9 +90,16 @@ impl FileContent { } } +/// Represents a file system tree. +/// +/// The root should always be the [TreeNode::Directory] variant. #[derive(Clone)] pub enum TreeNode { + /// File node in a tree. + /// + /// A file node must never be at the root of the tree, and is given a name by its parent (a [TreeNode::Directory]). File(FileContent), + /// Directory node in the tree. Directory(BTreeMap), } @@ -115,19 +122,26 @@ impl Debug for TreeNode { } } +/// Generates a tree of directories and files. +/// +/// Leaves are always [TreeNode::File] or an empty [TreeNode::Directory]. +/// Parents are always [TreeNode::Directory]. pub fn gen_tree(depth: u32, max_size: u32, max_items: u32, max_width: usize) -> impl Strategy { - let leaf = prop_oneof![any::().prop_map(TreeNode::File),]; - leaf.prop_recursive( + let leaf = any::().prop_map(TreeNode::File); + + let strategy = leaf.prop_recursive( depth, // levels max_size, // max number of nodes max_items, // number of items per collection move |inner| { - prop_oneof![ - // Take the inner strategy and make the recursive cases. - prop::collection::btree_map(any::(), inner, 0..max_width).prop_map(TreeNode::Directory), - ] + // Take the inner strategy and make the recursive cases. + // Since the size of the tree could be zero, this also introduces directories as leaves. + prop::collection::btree_map(any::(), inner, 0..max_width).prop_map(TreeNode::Directory) }, - ) + ); + + // Ensure the root is always a directory by wrapping the inner strategy + prop::collection::btree_map(any::(), strategy, 0..max_width).prop_map(TreeNode::Directory) } /// Take a generated tree and create the corresponding S3 namespace (list of keys) @@ -135,10 +149,8 @@ pub fn flatten_tree(node: TreeNode) -> Vec<(String, MockObject)> { fn aux(node: TreeNode, path: String, acc: &mut Vec<(String, MockObject)>) { match node { TreeNode::File(content) => { - // Don't create an empty key if a [TreeNode::File] is the root of the tree - if !path.is_empty() { - acc.push((path, content.to_mock_object())); - } + assert!(!path.is_empty(), "file node should never be created at root"); + acc.push((path, content.to_mock_object())); } TreeNode::Directory(contents) => { for (name, child) in contents { diff --git a/mountpoint-s3/tests/reftests/harness.rs b/mountpoint-s3/tests/reftests/harness.rs index 045362024..a499ecd3e 100644 --- a/mountpoint-s3/tests/reftests/harness.rs +++ b/mountpoint-s3/tests/reftests/harness.rs @@ -947,7 +947,7 @@ mod mutations { #[test] fn regression_overwrite() { run_test( - TreeNode::File(FileContent(0, FileSize::Small(0))), + TreeNode::Directory(BTreeMap::from([])), vec![ Op::WriteFile("-a".into(), DirectoryIndex(0), FileContent(0, FileSize::Small(0))), Op::WriteFile("-a".into(), DirectoryIndex(0), FileContent(0, FileSize::Small(0))), @@ -959,7 +959,7 @@ mod mutations { #[test] fn regression_empty_file() { run_test( - TreeNode::File(FileContent(0, FileSize::Small(0))), + TreeNode::Directory(BTreeMap::from([])), vec![ Op::CreateFile("a".into(), DirectoryIndex(0), FileContent(0, FileSize::Small(0))), Op::FinishWrite(InflightWriteIndex(0)), @@ -988,7 +988,7 @@ mod mutations { #[test] fn regression_unlink_local_directory() { run_test( - TreeNode::File(FileContent(0, FileSize::Small(0))), + TreeNode::Directory(BTreeMap::from([])), vec![ Op::CreateDirectory(DirectoryIndex(0), "a".into()), Op::UnlinkFile(DirectoryIndex(0), ChildIndex(0)), @@ -1161,7 +1161,7 @@ mod mutations { #[test] fn regression_mkdir_put() { run_test( - TreeNode::File(FileContent(0, FileSize::Small(0))), + TreeNode::Directory(BTreeMap::from([])), vec![ Op::CreateDirectory(DirectoryIndex(0), "a".into()), Op::PutObject(DirectoryIndex(0), "a".into(), FileContent(0, FileSize::Small(0))), @@ -1173,7 +1173,7 @@ mod mutations { #[test] fn regression_put_over_open_file() { run_test( - TreeNode::File(FileContent(0, FileSize::Small(0))), + TreeNode::Directory(BTreeMap::from([])), vec![ Op::CreateFile("a".into(), DirectoryIndex(0), FileContent(0, FileSize::Small(0))), Op::PutObject(DirectoryIndex(0), "a".into(), FileContent(0, FileSize::Small(0))), @@ -1185,7 +1185,7 @@ mod mutations { #[test] fn regression_put_over_open_directory() { run_test( - TreeNode::File(FileContent(0, FileSize::Small(0))), + TreeNode::Directory(BTreeMap::from([])), vec![ Op::CreateDirectory(DirectoryIndex(0), "a".into()), Op::PutObject(DirectoryIndex(0), "a".into(), FileContent(0, FileSize::Small(0))), @@ -1236,7 +1236,7 @@ mod mutations { #[test] fn regression_put_into_directory1() { run_test( - TreeNode::File(FileContent(0, FileSize::Small(0))), + TreeNode::Directory(BTreeMap::from([])), vec![ Op::CreateDirectory(DirectoryIndex(0), "a".into()), Op::CreateDirectory(DirectoryIndex(0), "-".into()), @@ -1298,7 +1298,7 @@ mod mutations { #[test] fn regression_unlink_newly_put_object() { run_test( - TreeNode::File(FileContent(0, FileSize::Small(0))), + TreeNode::Directory(BTreeMap::from([])), vec![ Op::CreateDirectory(DirectoryIndex(0), "a".into()), Op::PutObject(DirectoryIndex(1), "a".into(), FileContent(0, FileSize::Small(0))),