Merge branch 'tests-20220525' into 'main'

Add a few coverage-driven tests

See merge request tpo/core/arti!536

crates/tor-persist/src/fs.rs

@@ -113,8 +113,8 @@ impl FsStateMgr {
     /// Remove old and/or obsolete items from this storage manager.
     ///
     /// Requires that we hold the lock.
-    fn clean(&self) {
-        for fname in clean::files_to_delete(self.inner.statepath.as_path(), SystemTime::now()) {
+    fn clean(&self, now: SystemTime) {
+        for fname in clean::files_to_delete(self.inner.statepath.as_path(), now) {
             info!("Deleting obsolete file {}", fname.display());
             if let Err(e) = std::fs::remove_file(&fname) {
                 warn!("Unable to delete {}: {}", fname.display(), e);
@@ -141,7 +141,7 @@ impl StateMgr for FsStateMgr {
         if lockfile.owns_lock() {
             Ok(LockStatus::AlreadyHeld)
         } else if lockfile.try_lock()? {
-            self.clean();
+            self.clean(SystemTime::now());
             Ok(LockStatus::NewlyAcquired)
         } else {
             Ok(LockStatus::NoLock)
@@ -167,6 +167,7 @@ impl StateMgr for FsStateMgr {
         let string = match self.inner.statepath.read_to_string(rel_fname) {
             Ok(string) => string,
             Err(fs_mistrust::Error::NotFound(_)) => return Ok(None),
+            Err(fs_mistrust::Error::Io(_, e)) => return Err(Error::IoError(e)),
             Err(e) => return Err(e.into()),
         };
 
@@ -195,7 +196,7 @@ impl StateMgr for FsStateMgr {
 mod test {
     #![allow(clippy::unwrap_used)]
     use super::*;
-    use std::collections::HashMap;
+    use std::{collections::HashMap, time::Duration};
 
     #[test]
     fn simple() -> Result<()> {
@@ -235,4 +236,97 @@ mod test {
 
         Ok(())
     }
+
+    #[test]
+    fn clean_successful() -> Result<()> {
+        let dir = tempfile::TempDir::new().unwrap();
+        let statedir = dir.path().join("state");
+        let store = FsStateMgr::from_path(dir.path())?;
+
+        assert_eq!(store.try_lock()?, LockStatus::NewlyAcquired);
+        let fname = statedir.join("numbat.toml");
+        let fname2 = statedir.join("quoll.json");
+        std::fs::write(fname, "we no longer use toml files.").unwrap();
+        std::fs::write(fname2, "{}").unwrap();
+
+        let count = statedir.read_dir().unwrap().count();
+        assert_eq!(count, 3); // two files, one lock.
+
+        // Now we can make sure that "clean" actually removes the right file.
+        store.clean(SystemTime::now() + Duration::from_secs(365 * 86400));
+        let lst: Vec<_> = statedir.read_dir().unwrap().collect();
+        assert_eq!(lst.len(), 2); // one file, one lock.
+        assert!(lst
+            .iter()
+            .any(|ent| ent.as_ref().unwrap().file_name() == "quoll.json"));
+
+        Ok(())
+    }
+
+    #[cfg(target_family = "unix")]
+    #[test]
+    fn permissions() -> Result<()> {
+        use std::fs::Permissions;
+        use std::os::unix::fs::PermissionsExt;
+
+        let ro_dir = Permissions::from_mode(0o500);
+        let rw_dir = Permissions::from_mode(0o700);
+        let unusable = Permissions::from_mode(0o000);
+
+        let dir = tempfile::TempDir::new().unwrap();
+        let statedir = dir.path().join("state");
+        let store = FsStateMgr::from_path(dir.path())?;
+
+        assert_eq!(store.try_lock()?, LockStatus::NewlyAcquired);
+        let fname = statedir.join("numbat.toml");
+        let fname2 = statedir.join("quoll.json");
+        std::fs::write(&fname, "we no longer use toml files.").unwrap();
+        std::fs::write(&fname2, "{}").unwrap();
+
+        // Make the store directory read-only and make sure that we can't delete from it.
+        std::fs::set_permissions(&statedir, ro_dir)?;
+        store.clean(SystemTime::now() + Duration::from_secs(365 * 86400));
+        let lst: Vec<_> = statedir.read_dir().unwrap().collect();
+        if lst.len() == 2 {
+            // We must be root.  Don't do any more tests here.
+            return Ok(());
+        }
+        assert_eq!(lst.len(), 3); // We can't remove the file, but we didn't freak out. Great!
+                                  // Try failing to read a mode-0 file.
+        std::fs::set_permissions(&statedir, rw_dir)?;
+        std::fs::set_permissions(&fname2, unusable)?;
+
+        let h: Result<Option<HashMap<String, u32>>> = store.load("quoll");
+        assert!(h.is_err());
+        assert!(matches!(h, Err(Error::IoError(_))));
+
+        Ok(())
+    }
+
+    #[test]
+    fn locking() {
+        let dir = tempfile::TempDir::new().unwrap();
+        let store1 = FsStateMgr::from_path(dir.path()).unwrap();
+        let store2 = FsStateMgr::from_path(dir.path()).unwrap();
+
+        // Nobody has the lock; store1 will take it.
+        assert_eq!(store1.try_lock().unwrap(), LockStatus::NewlyAcquired);
+        assert_eq!(store1.try_lock().unwrap(), LockStatus::AlreadyHeld);
+        assert!(store1.can_store());
+
+        // store1 has the lock; store2 will try to get it and fail.
+        assert!(!store2.can_store());
+        assert_eq!(store2.try_lock().unwrap(), LockStatus::NoLock);
+        assert!(!store2.can_store());
+
+        // Store 1 will drop the lock.
+        store1.unlock().unwrap();
+        assert!(!store1.can_store());
+        assert!(!store2.can_store());
+
+        // Now store2 can get the lock.
+        assert_eq!(store2.try_lock().unwrap(), LockStatus::NewlyAcquired);
+        assert!(store2.can_store());
+        assert!(!store1.can_store());
+    }
 }

crates/tor-persist/src/fs/clean.rs

@@ -150,5 +150,17 @@ mod test {
         let removable_later = files_to_delete(dir.path(), now + CUTOFF * 2);
         assert_eq!(removable_later.len(), 1);
         assert_eq!(removable_later[0].file_stem().unwrap(), "quokka");
+
+        // Make sure we tolerate files written "in the future"
+        let removable_earlier = files_to_delete(dir.path(), now - CUTOFF * 2);
+        assert!(removable_earlier.is_empty());
+    }
+
+    #[test]
+    fn absent() {
+        let dir = tempfile::TempDir::new().unwrap();
+        let dir2 = dir.path().join("subdir_that_doesnt_exist");
+        let r = files_to_delete(&dir2, SystemTime::now());
+        assert!(r.is_empty());
     }
 }

crates/tor-proto/src/util/skew.rs

@@ -145,6 +145,8 @@ impl PartialOrd for ClockSkew {
 
 #[cfg(test)]
 mod test {
+    #![allow(clippy::unwrap_used)]
+
     use super::*;
 
     #[test]
@@ -175,4 +177,42 @@ mod test {
         );
         assert_eq!(skew, ClockSkew::None);
     }
+
+    #[test]
+    fn from_f64() {
+        use ClockSkew as CS;
+        use Duration as D;
+
+        assert_eq!(CS::from_secs_f64(0.0), Some(CS::None));
+        assert_eq!(CS::from_secs_f64(f64::MIN_POSITIVE / 2.0), Some(CS::None)); // subnormal
+        assert_eq!(CS::from_secs_f64(1.0), Some(CS::None));
+        assert_eq!(CS::from_secs_f64(-1.0), Some(CS::None));
+        assert_eq!(CS::from_secs_f64(3.0), Some(CS::Fast(D::from_secs(3))));
+        assert_eq!(CS::from_secs_f64(-3.0), Some(CS::Slow(D::from_secs(3))));
+
+        assert_eq!(CS::from_secs_f64(1.0e100), Some(CS::Fast(D::MAX)));
+        assert_eq!(CS::from_secs_f64(-1.0e100), Some(CS::Slow(D::MAX)));
+
+        assert_eq!(CS::from_secs_f64(f64::NAN), None);
+        assert_eq!(CS::from_secs_f64(f64::INFINITY), Some(CS::Fast(D::MAX)));
+        assert_eq!(CS::from_secs_f64(f64::NEG_INFINITY), Some(CS::Slow(D::MAX)));
+    }
+
+    #[test]
+    fn order() {
+        use rand::seq::SliceRandom as _;
+        use ClockSkew as CS;
+        let sorted: Vec<ClockSkew> = vec![-10.0, -5.0, 0.0, 0.0, 10.0, 20.0]
+            .into_iter()
+            .map(|n| CS::from_secs_f64(n).unwrap())
+            .collect();
+
+        let mut rng = rand::thread_rng();
+        let mut v = sorted.clone();
+        for _ in 0..100 {
+            v.shuffle(&mut rng);
+            v.sort();
+            assert_eq!(v, sorted);
+        }
+    }
 }