Skip to content

Commit

Permalink
Update reftest to always generate a directory at the root of the refe…
Browse files Browse the repository at this point in the history 
…rence 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 <[email protected]>
  • Loading branch information
@dannycjones
dannycjones authored Jan 13, 2025
1 parent 456c7de commit 7d6e8f9
Show file tree
Hide file tree
Showing 2 changed files with 31 additions and 19 deletions.
34 changes: 23 additions & 11 deletions mountpoint-s3/tests/reftests/generators.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<Name, TreeNode>),
}

Expand All @@ -115,30 +122,35 @@ 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<Value = TreeNode> {
let leaf = prop_oneof![any::<FileContent>().prop_map(TreeNode::File),];
leaf.prop_recursive(
let leaf = any::<FileContent>().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::<Name>(), 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::<Name>(), 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::<Name>(), strategy, 0..max_width).prop_map(TreeNode::Directory)
}

/// Take a generated tree and create the corresponding S3 namespace (list of keys)
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 {
Expand Down
16 changes: 8 additions & 8 deletions mountpoint-s3/tests/reftests/harness.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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))),
Expand All @@ -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)),
Expand Down Expand Up @@ -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)),
Expand Down Expand Up @@ -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))),
Expand All @@ -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))),
Expand All @@ -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))),
Expand Down Expand Up @@ -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()),
Expand Down Expand Up @@ -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))),
Expand Down

0 comments on commit 7d6e8f9

Please sign in to comment.