Encapsulate test resources in TestSession struct (#1096)

## Description of change Currently we return tuples from session constructor functions. This makes adding new resources to testing session challenging. Wrapping all testing resources in `TestSession` will help us to add new resources easily and it will also simply the type signatures of the functions using the test session. ## Does this change impact existing behavior? No, just tests. ## Does this change need a changelog entry in any of the crates? 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: Burak Varli <[email protected]>
awslabs · Nov 4, 2024 · 88826e4 · 88826e4
1 parent fb3832b
commit 88826e4
Show file tree

Hide file tree

Showing 15 changed files with 275 additions and 300 deletions.
diff --git a/mountpoint-s3/tests/common/fuse.rs b/mountpoint-s3/tests/common/fuse.rs
@@ -75,6 +75,20 @@ impl TestSessionConfig {
     }
 }
 
+// Holds resources for the testing session and cleans them on drop.
+pub struct TestSession {
+    pub mount_dir: TempDir,
+    pub session: BackgroundSession,
+    pub test_client: TestClientBox,
+}
+
+pub trait TestSessionCreator: FnOnce(&str, TestSessionConfig) -> TestSession {}
+
+// Since trait aliases are not stable yet, we can't just `type TestSessionCreator = FnOnce(...)`.
+// As a workaround we can subtrait `FnOnce(...)` and have this blank impl to allow
+// `FnOnce(...)` in place of `impl TestSessionCreator`.
+impl<T> TestSessionCreator for T where T: FnOnce(&str, TestSessionConfig) -> TestSession {}
+
 fn create_fuse_session<Client, Prefetcher>(
     client: Client,
     prefetcher: Prefetcher,
@@ -116,7 +130,7 @@ pub mod mock_session {
     const BUCKET_NAME: &str = "test_bucket";
 
     /// Create a FUSE mount backed by a mock object client that does not talk to S3
-    pub fn new(test_name: &str, test_config: TestSessionConfig) -> (TempDir, BackgroundSession, TestClientBox) {
+    pub fn new(test_name: &str, test_config: TestSessionConfig) -> TestSession {
         let mount_dir = tempfile::tempdir().unwrap();
 
         let prefix = if test_name.is_empty() {
@@ -145,13 +159,15 @@ pub mod mock_session {
         );
         let test_client = create_test_client(client, &prefix);
 
-        (mount_dir, session, test_client)
+        TestSession {
+            mount_dir,
+            session,
+            test_client,
+        }
     }
 
     /// Create a FUSE mount backed by a mock object client, with caching, that does not talk to S3
-    pub fn new_with_cache<Cache>(
-        cache: Cache,
-    ) -> impl FnOnce(&str, TestSessionConfig) -> (TempDir, BackgroundSession, TestClientBox)
+    pub fn new_with_cache<Cache>(cache: Cache) -> impl TestSessionCreator
     where
         Cache: DataCache + Send + Sync + 'static,
     {
@@ -184,7 +200,11 @@ pub mod mock_session {
             );
             let test_client = create_test_client(client, &prefix);
 
-            (mount_dir, session, test_client)
+            TestSession {
+                mount_dir,
+                session,
+                test_client,
+            }
         }
     }
 
@@ -282,7 +302,7 @@ pub mod s3_session {
     use crate::common::s3::{get_test_bucket_and_prefix, get_test_region, get_test_sdk_client};
 
     /// Create a FUSE mount backed by a real S3 client
-    pub fn new(test_name: &str, test_config: TestSessionConfig) -> (TempDir, BackgroundSession, TestClientBox) {
+    pub fn new(test_name: &str, test_config: TestSessionConfig) -> TestSession {
         let mount_dir = tempfile::tempdir().unwrap();
 
         let (bucket, prefix) = get_test_bucket_and_prefix(test_name);
@@ -307,13 +327,15 @@ pub mod s3_session {
         );
         let test_client = create_test_client(&region, &bucket, &prefix);
 
-        (mount_dir, session, test_client)
+        TestSession {
+            mount_dir,
+            session,
+            test_client,
+        }
     }
 
     /// Create a FUSE mount backed by a real S3 client, with caching
-    pub fn new_with_cache<Cache>(
-        cache: Cache,
-    ) -> impl FnOnce(&str, TestSessionConfig) -> (TempDir, BackgroundSession, TestClientBox)
+    pub fn new_with_cache<Cache>(cache: Cache) -> impl TestSessionCreator
     where
         Cache: DataCache + Send + Sync + 'static,
     {
@@ -341,7 +363,11 @@ pub mod s3_session {
             );
             let test_client = create_test_client(&region, &bucket, &prefix);
 
-            (mount_dir, session, test_client)
+            TestSession {
+                mount_dir,
+                session,
+                test_client,
+            }
         }
     }
 

diff --git a/mountpoint-s3/tests/direct_io.rs b/mountpoint-s3/tests/direct_io.rs
@@ -11,18 +11,13 @@ use std::fs::File;
 use std::os::unix::fs::{FileExt, OpenOptionsExt};
 use std::{fs::OpenOptions, time::Duration};
 
-use crate::common::fuse::{self, TestClientBox, TestSessionConfig};
-use fuser::BackgroundSession;
+use crate::common::fuse::{self, TestSessionConfig, TestSessionCreator};
 use mountpoint_s3::data_cache::InMemoryDataCache;
 use mountpoint_s3::fs::{CacheConfig, S3FilesystemConfig};
 use serial_test::serial;
-use tempfile::TempDir;
 use test_case::test_case;
 
-fn cache_and_direct_io_test<F>(creator_fn: F, prefix: &str)
-where
-    F: FnOnce(&str, TestSessionConfig) -> (TempDir, BackgroundSession, TestClientBox),
-{
+fn cache_and_direct_io_test(creator_fn: impl TestSessionCreator, prefix: &str) {
     const OBJECT_SIZE: usize = 8;
 
     let test_session_conf = TestSessionConfig {
@@ -37,7 +32,9 @@ where
         },
         ..Default::default()
     };
-    let (mount_point, _session, mut test_client) = creator_fn(prefix, test_session_conf);
+    let test_session = creator_fn(prefix, test_session_conf);
+    let mount_point = test_session.mount_dir;
+    let mut test_client = test_session.test_client;
 
     let file_name = "file.bin";
 

diff --git a/mountpoint-s3/tests/fuse_tests/consistency_test.rs b/mountpoint-s3/tests/fuse_tests/consistency_test.rs
@@ -1,21 +1,18 @@
 use std::fs::File;
 use std::os::unix::prelude::FileExt;
 
-use fuser::BackgroundSession;
-use tempfile::TempDir;
 use test_case::test_case;
 
-use crate::common::fuse::{self, TestClientBox, TestSessionConfig};
+use crate::common::fuse::{self, TestSessionCreator};
 use mountpoint_s3::data_cache::InMemoryDataCache;
 
-fn page_cache_sharing_test<F>(creator_fn: F, prefix: &str)
-where
-    F: FnOnce(&str, TestSessionConfig) -> (TempDir, BackgroundSession, TestClientBox),
-{
+fn page_cache_sharing_test(creator_fn: impl TestSessionCreator, prefix: &str) {
     // Big enough to avoid readahead
     const OBJECT_SIZE: usize = 512 * 1024;
 
-    let (mount_point, _session, mut test_client) = creator_fn(prefix, Default::default());
+    let test_session = creator_fn(prefix, Default::default());
+    let mount_point = test_session.mount_dir;
+    let mut test_client = test_session.test_client;
 
     // Create the first version of the file
     let old_contents = vec![0xaau8; OBJECT_SIZE];

diff --git a/mountpoint-s3/tests/fuse_tests/lookup_test.rs b/mountpoint-s3/tests/fuse_tests/lookup_test.rs
@@ -5,19 +5,16 @@ use std::{
     time::Duration,
 };
 
-use fuser::BackgroundSession;
 use mountpoint_s3::{fs::CacheConfig, S3FilesystemConfig};
-use tempfile::TempDir;
 use test_case::test_case;
 
-use crate::common::fuse::{self, read_dir_to_entry_names, TestClientBox, TestSessionConfig};
+use crate::common::fuse::{self, read_dir_to_entry_names, TestSessionConfig, TestSessionCreator};
 
 /// See [mountpoint_s3::inode::tests::test_lookup_directory_overlap].
-fn lookup_directory_overlap_test<F>(creator_fn: F, prefix: &str, subdir: &str)
-where
-    F: FnOnce(&str, TestSessionConfig) -> (TempDir, BackgroundSession, TestClientBox),
-{
-    let (mount_point, _session, mut test_client) = creator_fn(prefix, Default::default());
+fn lookup_directory_overlap_test(creator_fn: impl TestSessionCreator, prefix: &str, subdir: &str) {
+    let test_session = creator_fn(prefix, Default::default());
+    let mount_point = test_session.mount_dir;
+    let mut test_client = test_session.test_client;
 
     test_client
         .put_object(&format!("dir/{subdir}hello.txt"), b"hello world")
@@ -51,11 +48,10 @@ fn lookup_directory_overlap_test_mock(prefix: &str, subdir: &str) {
     lookup_directory_overlap_test(fuse::mock_session::new, prefix, subdir);
 }
 
-fn lookup_weird_characters_test<F>(creator_fn: F, prefix: &str)
-where
-    F: FnOnce(&str, TestSessionConfig) -> (TempDir, BackgroundSession, TestClientBox),
-{
-    let (mount_point, _session, mut test_client) = creator_fn(prefix, Default::default());
+fn lookup_weird_characters_test(creator_fn: impl TestSessionCreator, prefix: &str) {
+    let test_session = creator_fn(prefix, Default::default());
+    let mount_point = test_session.mount_dir;
+    let mut test_client = test_session.test_client;
 
     let keys = &[
         "weird$dir name",
@@ -105,10 +101,7 @@ fn lookup_directory_weird_characters_mock() {
     lookup_weird_characters_test(fuse::mock_session::new, "lookup_weird_characters_test");
 }
 
-fn lookup_previously_shadowed_file_test<F>(creator_fn: F)
-where
-    F: FnOnce(&str, TestSessionConfig) -> (TempDir, BackgroundSession, TestClientBox),
-{
+fn lookup_previously_shadowed_file_test(creator_fn: impl TestSessionCreator) {
     let filesystem_config = S3FilesystemConfig {
         cache_config: CacheConfig {
             serve_lookup_from_cache: false,
@@ -118,13 +111,15 @@ where
         },
         ..Default::default()
     };
-    let (mount_point, _session, mut test_client) = creator_fn(
+    let test_session = creator_fn(
         Default::default(),
         TestSessionConfig {
             filesystem_config,
             ..Default::default()
         },
     );
+    let mount_point = test_session.mount_dir;
+    let mut test_client = test_session.test_client;
 
     let name = "foo";
     let nested = format!("{name}/bar");
@@ -152,11 +147,10 @@ fn lookup_previously_shadowed_file_test_mock() {
     lookup_previously_shadowed_file_test(fuse::mock_session::new);
 }
 
-fn lookup_unicode_keys_test<F>(creator_fn: F, prefix: &str)
-where
-    F: FnOnce(&str, TestSessionConfig) -> (TempDir, BackgroundSession, TestClientBox),
-{
-    let (mount_point, _session, mut test_client) = creator_fn(prefix, Default::default());
+fn lookup_unicode_keys_test(creator_fn: impl TestSessionCreator, prefix: &str) {
+    let test_session = creator_fn(prefix, Default::default());
+    let mount_point = test_session.mount_dir;
+    let mut test_client = test_session.test_client;
 
     let keys = &["مرحبًا", "こんにちは/", "🇦🇺", "🐈/🦀"];
 
@@ -200,10 +194,7 @@ fn lookup_unicode_keys_mock() {
     lookup_unicode_keys_test(fuse::mock_session::new, "lookup_unicode_keys_test");
 }
 
-fn lookup_with_negative_cache<F>(creator_fn: F)
-where
-    F: FnOnce(&str, TestSessionConfig) -> (TempDir, BackgroundSession, TestClientBox),
-{
+fn lookup_with_negative_cache(creator_fn: impl TestSessionCreator) {
     const FILE_NAME: &str = "hello.txt";
     let config = TestSessionConfig {
         filesystem_config: S3FilesystemConfig {
@@ -217,7 +208,9 @@ where
         },
         ..Default::default()
     };
-    let (mount_point, _session, mut test_client) = creator_fn("lookup_with_negative_cache", config);
+    let test_session = creator_fn("lookup_with_negative_cache", config);
+    let mount_point = test_session.mount_dir;
+    let mut test_client = test_session.test_client;
 
     // Check negative caching
     let file_path = mount_point.path().join(FILE_NAME);

diff --git a/mountpoint-s3/tests/fuse_tests/mkdir_test.rs b/mountpoint-s3/tests/fuse_tests/mkdir_test.rs
@@ -1,16 +1,13 @@
 use std::fs::{self, metadata, DirBuilder, File};
 
-use fuser::BackgroundSession;
-use tempfile::TempDir;
 use test_case::test_case;
 
-use crate::common::fuse::{self, read_dir_to_entry_names, TestClientBox, TestSessionConfig};
+use crate::common::fuse::{self, read_dir_to_entry_names, TestSessionCreator};
 
-fn mkdir_test<F>(creator_fn: F, prefix: &str)
-where
-    F: FnOnce(&str, TestSessionConfig) -> (TempDir, BackgroundSession, TestClientBox),
-{
-    let (mount_point, _session, mut test_client) = creator_fn(prefix, Default::default());
+fn mkdir_test(creator_fn: impl TestSessionCreator, prefix: &str) {
+    let test_session = creator_fn(prefix, Default::default());
+    let mount_point = test_session.mount_dir;
+    let mut test_client = test_session.test_client;
 
     // Create local directory
     let dirname = "local_dir";

diff --git a/mountpoint-s3/tests/fuse_tests/perm_test.rs b/mountpoint-s3/tests/fuse_tests/perm_test.rs
@@ -4,18 +4,19 @@ use std::{
     os::unix::prelude::{MetadataExt, PermissionsExt},
 };
 
-use fuser::BackgroundSession;
 use mountpoint_s3::S3FilesystemConfig;
 use nix::unistd::{getgid, getuid};
-use tempfile::TempDir;
 use test_case::test_case;
 
-use crate::common::fuse::{self, read_dir_to_entry_names, TestClientBox, TestSessionConfig};
+use crate::common::fuse::{self, read_dir_to_entry_names, TestSessionConfig, TestSessionCreator};
 
-fn perm_test<F>(creator_fn: F, uid: Option<u32>, gid: Option<u32>, dir_mode: Option<u16>, file_mode: Option<u16>)
-where
-    F: FnOnce(&str, TestSessionConfig) -> (TempDir, BackgroundSession, TestClientBox),
-{
+fn perm_test(
+    creator_fn: impl TestSessionCreator,
+    uid: Option<u32>,
+    gid: Option<u32>,
+    dir_mode: Option<u16>,
+    file_mode: Option<u16>,
+) {
     let mut config = S3FilesystemConfig::default();
     if let Some(id) = uid {
         config.uid = id;
@@ -30,13 +31,15 @@ where
         config.file_mode = mode;
     }
 
-    let (mount_point, _session, mut test_client) = creator_fn(
+    let test_session = creator_fn(
         "",
         TestSessionConfig {
             filesystem_config: config,
             ..Default::default()
         },
     );
+    let mount_point = test_session.mount_dir;
+    let mut test_client = test_session.test_client;
 
     // expected values
     let uid = uid.unwrap_or_else(|| getuid().into());
@@ -84,15 +87,13 @@ where
     assert_eq!(file_content, "hello world");
 }
 
-fn perm_test_negative<F>(
-    creator_fn: F,
+fn perm_test_negative(
+    creator_fn: impl TestSessionCreator,
     uid: Option<u32>,
     gid: Option<u32>,
     dir_mode: Option<u16>,
     file_mode: Option<u16>,
-) where
-    F: FnOnce(&str, TestSessionConfig) -> (TempDir, BackgroundSession, TestClientBox),
-{
+) {
     let mut config = S3FilesystemConfig::default();
     if let Some(id) = uid {
         config.uid = id;
@@ -107,13 +108,15 @@ fn perm_test_negative<F>(
         config.file_mode = mode;
     }
 
-    let (mount_point, _session, mut test_client) = creator_fn(
+    let test_session = creator_fn(
         "",
         TestSessionConfig {
             filesystem_config: config,
             ..Default::default()
         },
     );
+    let mount_point = test_session.mount_dir;
+    let mut test_client = test_session.test_client;
 
     // expected values
     let uid = uid.unwrap_or_else(|| getuid().into());