test(ads-client): Time dependent tests now use a fake clock (#7059)

inline not needed

cleanup

wording

fixed

reorg

Author: luc-lisi

components/ads-client/src/http_cache.rs

@@ -5,6 +5,7 @@
 mod builder;
 mod bytesize;
 mod cache_control;
+mod clock;
 mod connection_initializer;
 mod request_hash;
 mod store;
@@ -216,7 +217,7 @@ mod tests {
         HttpCache::builder("ignored_in_tests.db")
             .default_ttl(Duration::from_secs(secs))
             .max_size(ByteSize::mib(4))
-            .build()
+            .build_for_time_dependent_tests()
             .expect("cache build should succeed")
     }
 
@@ -409,7 +410,7 @@ mod tests {
         assert!(matches!(out.cache_outcome, CacheOutcome::MissStored));
 
         // After ~>1s, cleanup should remove it
-        std::thread::sleep(std::time::Duration::from_secs(2));
+        cache.store.get_clock().advance(2);
 
         cache.store.delete_expired_entries().unwrap();
 
@@ -439,12 +440,12 @@ mod tests {
         assert!(matches!(out.cache_outcome, CacheOutcome::MissStored));
 
         // Not expired yet at ~1s
-        std::thread::sleep(std::time::Duration::from_secs(1));
+        cache.store.get_clock().advance(1);
         cache.store.delete_expired_entries().unwrap();
         assert!(cache.store.lookup(&req).unwrap().is_some());
 
         // Expired after ~2s
-        std::thread::sleep(std::time::Duration::from_secs(2));
+        cache.store.get_clock().advance(2);
         cache.store.delete_expired_entries().unwrap();
         assert!(cache.store.lookup(&req).unwrap().is_none());
     }
@@ -469,12 +470,12 @@ mod tests {
         assert!(matches!(out.cache_outcome, CacheOutcome::MissStored));
 
         // Not expired at ~1s
-        std::thread::sleep(std::time::Duration::from_secs(1));
+        cache.store.get_clock().advance(1);
         cache.store.delete_expired_entries().unwrap();
         assert!(cache.store.lookup(&req).unwrap().is_some());
 
         // Expired after ~3s
-        std::thread::sleep(std::time::Duration::from_secs(3));
+        cache.store.get_clock().advance(3);
         cache.store.delete_expired_entries().unwrap();
         assert!(cache.store.lookup(&req).unwrap().is_none());
     }

components/ads-client/src/http_cache/builder.rs

@@ -7,6 +7,7 @@ use crate::http_cache::HttpCache;
 use super::bytesize::ByteSize;
 use super::connection_initializer::HttpCacheConnectionInitializer;
 use super::store::HttpCacheStore;
+use rusqlite::Connection;
 use sql_support::open_database;
 use std::path::PathBuf;
 use std::time::Duration;
@@ -57,6 +58,15 @@ impl HttpCacheBuilder {
         }
     }
 
+    #[cfg(test)]
+    pub fn new_for_tests(db_path: impl Into<PathBuf>) -> Self {
+        Self {
+            db_path: db_path.into(),
+            max_size: None,
+            default_ttl: None,
+        }
+    }
+
     pub fn max_size(mut self, max_size: ByteSize) -> Self {
         self.max_size = Some(max_size);
         self
@@ -95,16 +105,20 @@ impl HttpCacheBuilder {
         Ok(())
     }
 
-    pub fn build(self) -> Result<HttpCache, Error> {
-        self.validate()?;
-
+    fn open_connection(&self) -> Result<Connection, Error> {
         let initializer = HttpCacheConnectionInitializer {};
         let conn = if cfg!(test) {
             open_database::open_memory_database(&initializer)?
         } else {
             open_database::open_database(&self.db_path, &initializer)?
         };
+        Ok(conn)
+    }
+
+    pub fn build(&self) -> Result<HttpCache, Error> {
+        self.validate()?;
 
+        let conn = self.open_connection()?;
         let max_size = self.max_size.unwrap_or(DEFAULT_MAX_SIZE);
         let store = HttpCacheStore::new(conn);
         let default_ttl = self.default_ttl.unwrap_or(DEFAULT_TTL);
@@ -115,6 +129,22 @@ impl HttpCacheBuilder {
             default_ttl,
         })
     }
+
+    #[cfg(test)]
+    pub fn build_for_time_dependent_tests(&self) -> Result<HttpCache, Error> {
+        self.validate()?;
+
+        let conn = self.open_connection()?;
+        let max_size = self.max_size.unwrap_or(DEFAULT_MAX_SIZE);
+        let store = HttpCacheStore::new_with_test_clock(conn);
+        let default_ttl = self.default_ttl.unwrap_or(DEFAULT_TTL);
+
+        Ok(HttpCache {
+            max_size,
+            store,
+            default_ttl,
+        })
+    }
 }
 
 #[cfg(test)]

components/ads-client/src/http_cache/clock.rs

@@ -0,0 +1,47 @@
+pub trait Clock: Send + Sync + 'static {
+    fn now_epoch_seconds(&self) -> i64;
+    #[cfg(test)]
+    fn advance(&self, secs: i64);
+}
+
+pub struct CacheClock;
+
+impl Clock for CacheClock {
+    fn now_epoch_seconds(&self) -> i64 {
+        chrono::Utc::now().timestamp()
+    }
+    #[cfg(test)]
+    fn advance(&self, _secs: i64) {
+        panic!(
+            "
+        You cannot advance a non-test clock.
+        Be sure to build the cache or store with the test clock for time-dependent tests.
+    "
+        )
+    }
+}
+
+#[cfg(test)]
+pub struct TestClock {
+    now: std::sync::atomic::AtomicI64,
+}
+
+#[cfg(test)]
+impl TestClock {
+    pub fn new(start: i64) -> Self {
+        Self {
+            now: std::sync::atomic::AtomicI64::new(start),
+        }
+    }
+}
+
+#[cfg(test)]
+impl Clock for TestClock {
+    fn now_epoch_seconds(&self) -> i64 {
+        self.now.load(std::sync::atomic::Ordering::Relaxed)
+    }
+    fn advance(&self, secs: i64) {
+        self.now
+            .fetch_add(secs, std::sync::atomic::Ordering::Relaxed);
+    }
+}

components/ads-client/src/http_cache/store.rs

@@ -2,9 +2,13 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
-use std::{collections::HashMap, time::Duration};
+use std::{collections::HashMap, sync::Arc, time::Duration};
 
-use crate::http_cache::{request_hash::RequestHash, ByteSize};
+use crate::http_cache::{
+    clock::{CacheClock, Clock},
+    request_hash::RequestHash,
+    ByteSize,
+};
 use parking_lot::Mutex;
 use rusqlite::{params, Connection, OptionalExtension, Result as SqliteResult};
 use viaduct::{Header, Request, Response};
@@ -21,6 +25,7 @@ pub enum FaultKind {
 
 pub struct HttpCacheStore {
     conn: Mutex<Connection>,
+    clock: Arc<dyn Clock>,
     #[cfg(test)]
     fault: parking_lot::Mutex<FaultKind>,
 }
@@ -29,11 +34,29 @@ impl HttpCacheStore {
     pub fn new(conn: Connection) -> Self {
         Self {
             conn: Mutex::new(conn),
+            clock: Arc::new(CacheClock),
             #[cfg(test)]
             fault: parking_lot::Mutex::new(FaultKind::None),
         }
     }
 
+    #[cfg(test)]
+    pub fn new_with_test_clock(conn: Connection) -> Self {
+        use crate::http_cache::clock::TestClock;
+
+        Self {
+            conn: Mutex::new(conn),
+            clock: Arc::new(TestClock::new(chrono::Utc::now().timestamp())),
+            #[cfg(test)]
+            fault: parking_lot::Mutex::new(FaultKind::None),
+        }
+    }
+
+    #[cfg(test)]
+    pub fn get_clock(&self) -> &(dyn Clock) {
+        &*self.clock
+    }
+
     /// Removes all entries from cache.
     pub fn clear_all(&self) -> SqliteResult<usize> {
         let conn = self.conn.lock();
@@ -60,7 +83,7 @@ impl HttpCacheStore {
         let conn = self.conn.lock();
         conn.execute(
             "DELETE FROM http_cache WHERE expires_at < ?1",
-            params![chrono::Utc::now().timestamp()],
+            params![self.clock.now_epoch_seconds()],
         )
     }
 
@@ -118,7 +141,7 @@ impl HttpCacheStore {
         let headers_map: HashMap<String, String> = response.headers.clone().into();
         let response_headers = serde_json::to_vec(&headers_map).unwrap_or_default();
         let size_bytes = (response_headers.len() + response.body.len()) as i64;
-        let now = chrono::Utc::now().timestamp();
+        let now = self.clock.now_epoch_seconds();
         let ttl_seconds = ttl.as_secs();
         let expires_at = now + ttl_seconds as i64;
 
@@ -252,7 +275,7 @@ mod tests {
         let initializer = HttpCacheConnectionInitializer {};
         let conn = open_database::open_memory_database(&initializer)
             .expect("failed to open memory cache db");
-        HttpCacheStore::new(conn)
+        HttpCacheStore::new_with_test_clock(conn)
     }
 
     #[test]
@@ -358,16 +381,16 @@ mod tests {
         let (c1, e1, t1) = fetch_timestamps(&store, &req);
         assert_eq!(t1, 300);
 
-        // Change TTL to 1s and upsert; wait a tick so cached_at likely changes
-        std::thread::sleep(std::time::Duration::from_millis(50));
+        store.get_clock().advance(3);
+
         store
             .store_with_ttl(&req, &resp, &Duration::new(1, 0))
             .unwrap();
         let (c2, e2, t2) = fetch_timestamps(&store, &req);
         assert_eq!(t2, 1);
         // cached_at should be >= previous cached_at; expires_at should move accordingly
-        assert!(c2 >= c1);
-        assert!(e2 <= e1, "expires_at should move earlier when TTL shrinks");
+        assert!(c2 > c1);
+        assert!(e2 < e1, "expires_at should move earlier when TTL shrinks");
     }
 
     #[test]
@@ -390,7 +413,7 @@ mod tests {
         assert!(store.lookup(&req_fresh).unwrap().is_some());
 
         // Let first one expire; then cleanup
-        std::thread::sleep(std::time::Duration::from_secs(2));
+        store.clock.advance(2);
         let removed = store.delete_expired_entries().unwrap();
         assert!(
             removed >= 1,
@@ -412,7 +435,7 @@ mod tests {
             .store_with_ttl(&req, &resp, &Duration::new(1, 0))
             .unwrap();
         // Check that lookup still returns (store is policy-agnostic).
-        std::thread::sleep(std::time::Duration::from_secs(2));
+        store.clock.advance(2);
         assert!(store.lookup(&req).unwrap().is_some());
 
         // Test cleanup still removes it
@@ -434,7 +457,7 @@ mod tests {
         assert!(store.lookup(&req).unwrap().is_some());
 
         // Advance a second so now > expires_at
-        std::thread::sleep(std::time::Duration::from_secs(1));
+        store.clock.advance(2);
         let removed = store.delete_expired_entries().unwrap();
         assert!(removed >= 1);
         assert!(store.lookup(&req).unwrap().is_none());
@@ -458,10 +481,7 @@ mod tests {
 
     #[test]
     fn test_ttl_expiration() {
-        let initializer = HttpCacheConnectionInitializer {};
-        let conn = open_database::open_memory_database(&initializer)
-            .expect("failed to open memory cache db");
-        let store = HttpCacheStore::new(conn);
+        let store = create_test_store();
 
         let request = create_test_request("https://example.com/api", b"test body");
         let response = create_test_response(200, b"test response");
@@ -473,7 +493,7 @@ mod tests {
         let retrieved = store.lookup(&request).unwrap().unwrap();
         assert_eq!(retrieved.0.body, b"test response");
 
-        std::thread::sleep(Duration::from_secs(2));
+        store.clock.advance(2);
 
         let retrieved_after_expiry = store.lookup(&request).unwrap();
         assert!(retrieved_after_expiry.is_some());

components/ads-client/src/lib.rs

@@ -25,7 +25,7 @@ use crate::http_cache::{ByteSize, CacheMode, HttpCacheError, RequestCachePolicy}
 use crate::models::AdPlacementRequest;
 
 mod error;
-mod http_cache;
+pub mod http_cache;
 mod instrument;
 mod mars;
 mod models;