Skip to content

Commit 7d6e8f9

Browse files
dannycjonesdannycjones
authored
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 <[email protected]>
1 parent 456c7de commit 7d6e8f9

File tree

2 files changed

+31
-19
lines changed

2 files changed

+31
-19
lines changed

‎mountpoint-s3/tests/reftests/generators.rs‎

Lines changed: 23 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -90,9 +90,16 @@ impl FileContent {
9090
}
9191
}
9292

93+
/// Represents a file system tree.
94+
///
95+
/// The root should always be the [TreeNode::Directory] variant.
9396
#[derive(Clone)]
9497
pub enum TreeNode {
98+
/// File node in a tree.
99+
///
100+
/// A file node must never be at the root of the tree, and is given a name by its parent (a [TreeNode::Directory]).
95101
File(FileContent),
102+
/// Directory node in the tree.
96103
Directory(BTreeMap<Name, TreeNode>),
97104
}
98105

@@ -115,30 +122,35 @@ impl Debug for TreeNode {
115122
}
116123
}
117124

125+
/// Generates a tree of directories and files.
126+
///
127+
/// Leaves are always [TreeNode::File] or an empty [TreeNode::Directory].
128+
/// Parents are always [TreeNode::Directory].
118129
pub fn gen_tree(depth: u32, max_size: u32, max_items: u32, max_width: usize) -> impl Strategy<Value = TreeNode> {
119-
let leaf = prop_oneof![any::<FileContent>().prop_map(TreeNode::File),];
120-
leaf.prop_recursive(
130+
let leaf = any::<FileContent>().prop_map(TreeNode::File);
131+
132+
let strategy = leaf.prop_recursive(
121133
depth, // levels
122134
max_size, // max number of nodes
123135
max_items, // number of items per collection
124136
move |inner| {
125-
prop_oneof![
126-
// Take the inner strategy and make the recursive cases.
127-
prop::collection::btree_map(any::<Name>(), inner, 0..max_width).prop_map(TreeNode::Directory),
128-
]
137+
// Take the inner strategy and make the recursive cases.
138+
// Since the size of the tree could be zero, this also introduces directories as leaves.
139+
prop::collection::btree_map(any::<Name>(), inner, 0..max_width).prop_map(TreeNode::Directory)
129140
},
130-
)
141+
);
142+
143+
// Ensure the root is always a directory by wrapping the inner strategy
144+
prop::collection::btree_map(any::<Name>(), strategy, 0..max_width).prop_map(TreeNode::Directory)
131145
}
132146

133147
/// Take a generated tree and create the corresponding S3 namespace (list of keys)
134148
pub fn flatten_tree(node: TreeNode) -> Vec<(String, MockObject)> {
135149
fn aux(node: TreeNode, path: String, acc: &mut Vec<(String, MockObject)>) {
136150
match node {
137151
TreeNode::File(content) => {
138-
// Don't create an empty key if a [TreeNode::File] is the root of the tree
139-
if !path.is_empty() {
140-
acc.push((path, content.to_mock_object()));
141-
}
152+
assert!(!path.is_empty(), "file node should never be created at root");
153+
acc.push((path, content.to_mock_object()));
142154
}
143155
TreeNode::Directory(contents) => {
144156
for (name, child) in contents {

‎mountpoint-s3/tests/reftests/harness.rs‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -947,7 +947,7 @@ mod mutations {
947947
#[test]
948948
fn regression_overwrite() {
949949
run_test(
950-
TreeNode::File(FileContent(0, FileSize::Small(0))),
950+
TreeNode::Directory(BTreeMap::from([])),
951951
vec![
952952
Op::WriteFile("-a".into(), DirectoryIndex(0), FileContent(0, FileSize::Small(0))),
953953
Op::WriteFile("-a".into(), DirectoryIndex(0), FileContent(0, FileSize::Small(0))),
@@ -959,7 +959,7 @@ mod mutations {
959959
#[test]
960960
fn regression_empty_file() {
961961
run_test(
962-
TreeNode::File(FileContent(0, FileSize::Small(0))),
962+
TreeNode::Directory(BTreeMap::from([])),
963963
vec![
964964
Op::CreateFile("a".into(), DirectoryIndex(0), FileContent(0, FileSize::Small(0))),
965965
Op::FinishWrite(InflightWriteIndex(0)),
@@ -988,7 +988,7 @@ mod mutations {
988988
#[test]
989989
fn regression_unlink_local_directory() {
990990
run_test(
991-
TreeNode::File(FileContent(0, FileSize::Small(0))),
991+
TreeNode::Directory(BTreeMap::from([])),
992992
vec![
993993
Op::CreateDirectory(DirectoryIndex(0), "a".into()),
994994
Op::UnlinkFile(DirectoryIndex(0), ChildIndex(0)),
@@ -1161,7 +1161,7 @@ mod mutations {
11611161
#[test]
11621162
fn regression_mkdir_put() {
11631163
run_test(
1164-
TreeNode::File(FileContent(0, FileSize::Small(0))),
1164+
TreeNode::Directory(BTreeMap::from([])),
11651165
vec![
11661166
Op::CreateDirectory(DirectoryIndex(0), "a".into()),
11671167
Op::PutObject(DirectoryIndex(0), "a".into(), FileContent(0, FileSize::Small(0))),
@@ -1173,7 +1173,7 @@ mod mutations {
11731173
#[test]
11741174
fn regression_put_over_open_file() {
11751175
run_test(
1176-
TreeNode::File(FileContent(0, FileSize::Small(0))),
1176+
TreeNode::Directory(BTreeMap::from([])),
11771177
vec![
11781178
Op::CreateFile("a".into(), DirectoryIndex(0), FileContent(0, FileSize::Small(0))),
11791179
Op::PutObject(DirectoryIndex(0), "a".into(), FileContent(0, FileSize::Small(0))),
@@ -1185,7 +1185,7 @@ mod mutations {
11851185
#[test]
11861186
fn regression_put_over_open_directory() {
11871187
run_test(
1188-
TreeNode::File(FileContent(0, FileSize::Small(0))),
1188+
TreeNode::Directory(BTreeMap::from([])),
11891189
vec![
11901190
Op::CreateDirectory(DirectoryIndex(0), "a".into()),
11911191
Op::PutObject(DirectoryIndex(0), "a".into(), FileContent(0, FileSize::Small(0))),
@@ -1236,7 +1236,7 @@ mod mutations {
12361236
#[test]
12371237
fn regression_put_into_directory1() {
12381238
run_test(
1239-
TreeNode::File(FileContent(0, FileSize::Small(0))),
1239+
TreeNode::Directory(BTreeMap::from([])),
12401240
vec![
12411241
Op::CreateDirectory(DirectoryIndex(0), "a".into()),
12421242
Op::CreateDirectory(DirectoryIndex(0), "-".into()),
@@ -1298,7 +1298,7 @@ mod mutations {
12981298
#[test]
12991299
fn regression_unlink_newly_put_object() {
13001300
run_test(
1301-
TreeNode::File(FileContent(0, FileSize::Small(0))),
1301+
TreeNode::Directory(BTreeMap::from([])),
13021302
vec![
13031303
Op::CreateDirectory(DirectoryIndex(0), "a".into()),
13041304
Op::PutObject(DirectoryIndex(1), "a".into(), FileContent(0, FileSize::Small(0))),

0 commit comments

Comments
 (0)